fix: too many file descriptors with thousands of notifications#1887
fix: too many file descriptors with thousands of notifications#1887
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
WalkthroughAdds a debounced StoreSyncService that schedules serialized store writes to ConfigService and avoids redundant writes; refactors NotificationsService to read its path from ConfigService and updates tests and plugin instantiation accordingly. Changes
Sequence DiagramsequenceDiagram
actor Store
participant StoreSyncService
participant DebounceTimer as Timer
participant ConfigService
Store->>StoreSyncService: notify state change
StoreSyncService->>StoreSyncService: clear existing timer
StoreSyncService->>DebounceTimer: schedule timer (STORE_SYNC_DEBOUNCE_MS)
Store->>StoreSyncService: rapid state change
StoreSyncService->>StoreSyncService: clear & reschedule timer
DebounceTimer->>StoreSyncService: timer fires
StoreSyncService->>StoreSyncService: serialize state & compare lastSyncedState
alt state changed
StoreSyncService->>ConfigService: set('store', serialized)
ConfigService-->>StoreSyncService: ack
StoreSyncService->>StoreSyncService: update lastSyncedState
else state unchanged
StoreSyncService-->>StoreSyncService: skip write
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
elibosley
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15c3fee023
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| constructor(private readonly configService: ConfigService) { | ||
| this.path = this.getConfiguredPath(); |
There was a problem hiding this comment.
Making ConfigService a required constructor dependency breaks existing non-DI instantiation paths: PluginService.importPlugins still calls new NotificationsService() when plugin loading fails, so this.configService is undefined and getConfiguredPath() throws before createNotification runs. In that failure path, the original plugin-validation error gets masked and the alert notification is never created, which regresses diagnostics for invalid plugins.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/unraid-api/graph/resolvers/notifications/loadNotificationsFile.test.ts (1)
29-43:⚠️ Potential issue | 🟡 MinorRemove the unused
notify.pathproperty from the mock; thedisplaysettings are still needed but untested.The
notify.pathproperty in the mock at line 32-34 is dead code—NotificationsServicenow reads the notification path fromConfigServicevia the injected config object. However, thedisplaysettings are still required: they're used informatDatetime()(line 864 of notifications.service.ts) to format timestamps according to user preferences.The test caveat is real: although
getters.dynamix()is called for display settings, no test actually verifies thatformattedTimestampis formatted correctly using these settings. The valid notification test omits theformattedTimestampassertion, and only edge-case tests check it.Simplify the mock to remove the unused
notify.pathproperty:Suggested mock adjustment
vi.mock('@app/store/index.js', () => ({ getters: { dynamix: vi.fn().mockReturnValue({ display: { date: 'Y-m-d', time: 'H:i:s', }, }), }, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/unraid-api/graph/resolvers/notifications/loadNotificationsFile.test.ts` around lines 29 - 43, The mock in the test uses an unused notify.path property; update the vi.mock for '@app/store/index.js' to remove notify.path while keeping getters.dynamix returning the display settings used by formatDatetime (date/time), since NotificationsService now reads the notification path from ConfigService; ensure the mock only includes getters.dynamix with the display object so tests still exercise formattedTimestamp behavior referenced by NotificationsService/formatDatetime.
🧹 Nitpick comments (1)
api/src/unraid-api/config/store-sync.service.ts (1)
8-8: Consider making debounce configurable or documenting the choice.The 1-second debounce is reasonable for reducing file descriptor pressure. Consider adding a brief comment explaining why 1000ms was chosen, or making it configurable via environment variable for different deployment scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/unraid-api/config/store-sync.service.ts` at line 8, The constant STORE_SYNC_DEBOUNCE_MS (currently 1000) should be made configurable or documented: either add a brief inline comment above STORE_SYNC_DEBOUNCE_MS explaining the 1000ms choice (to reduce file descriptor pressure and coalesce rapid updates), or replace the hardcoded value with an environment-backed setting (e.g., read process.env.STORE_SYNC_DEBOUNCE_MS, parseInt with a safe default of 1000) and use that value wherever STORE_SYNC_DEBOUNCE_MS is referenced so deployments can tune debounce behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/unraid-api/plugin/plugin.service.ts`:
- Line 54: The static importPlugins method instantiates ConfigService directly
which bypasses NestJS DI so NotificationsService(new ConfigService()) will use
defaults (e.g., /tmp/notifications); fix by either (A) changing importPlugins
signature to accept a properly injected ConfigService and pass that into
NotificationsService, or (B) replace the direct new with a module-level
singleton/lazy getter that is initialized from the DI context at startup, or (C)
if using the default path is intentional, add a clear comment in importPlugins
next to NotificationsService(new ConfigService()) stating this is deliberate and
that the notification path will fall back to the default.
---
Outside diff comments:
In
`@api/src/unraid-api/graph/resolvers/notifications/loadNotificationsFile.test.ts`:
- Around line 29-43: The mock in the test uses an unused notify.path property;
update the vi.mock for '@app/store/index.js' to remove notify.path while keeping
getters.dynamix returning the display settings used by formatDatetime
(date/time), since NotificationsService now reads the notification path from
ConfigService; ensure the mock only includes getters.dynamix with the display
object so tests still exercise formattedTimestamp behavior referenced by
NotificationsService/formatDatetime.
---
Nitpick comments:
In `@api/src/unraid-api/config/store-sync.service.ts`:
- Line 8: The constant STORE_SYNC_DEBOUNCE_MS (currently 1000) should be made
configurable or documented: either add a brief inline comment above
STORE_SYNC_DEBOUNCE_MS explaining the 1000ms choice (to reduce file descriptor
pressure and coalesce rapid updates), or replace the hardcoded value with an
environment-backed setting (e.g., read process.env.STORE_SYNC_DEBOUNCE_MS,
parseInt with a safe default of 1000) and use that value wherever
STORE_SYNC_DEBOUNCE_MS is referenced so deployments can tune debounce behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5f004d54-6d8b-4c40-811f-e5c5dc1da050
📒 Files selected for processing (6)
api/src/unraid-api/config/store-sync.service.spec.tsapi/src/unraid-api/config/store-sync.service.tsapi/src/unraid-api/graph/resolvers/notifications/loadNotificationsFile.test.tsapi/src/unraid-api/graph/resolvers/notifications/notifications.service.spec.tsapi/src/unraid-api/graph/resolvers/notifications/notifications.service.tsapi/src/unraid-api/plugin/plugin.service.ts
| } catch (error) { | ||
| PluginService.logger.error(`Plugin from ${pkgName} is invalid: %o`, error as object); | ||
| const notificationService = new NotificationsService(); | ||
| const notificationService = new NotificationsService(new ConfigService()); |
There was a problem hiding this comment.
Potential issue: manually instantiated ConfigService lacks configuration context.
Creating new ConfigService() directly bypasses NestJS dependency injection, meaning this ConfigService instance won't have access to the actual application configuration. The notification will use the default path (/tmp/notifications) rather than any configured path.
Since this is in a static method (importPlugins), proper DI is challenging. Consider one of these approaches:
- Accept the default path behavior for error notifications (simplest, may be acceptable)
- Pass ConfigService into the static method from a properly injected context
- Use a module-level singleton or lazy initialization pattern
If the default path is acceptable for plugin error notifications, consider adding a comment explaining this intentional behavior.
💡 Optional: Add clarifying comment
- const notificationService = new NotificationsService(new ConfigService());
+ // Note: Uses default config path since we're outside DI context during plugin load
+ const notificationService = new NotificationsService(new ConfigService());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/unraid-api/plugin/plugin.service.ts` at line 54, The static
importPlugins method instantiates ConfigService directly which bypasses NestJS
DI so NotificationsService(new ConfigService()) will use defaults (e.g.,
/tmp/notifications); fix by either (A) changing importPlugins signature to
accept a properly injected ConfigService and pass that into
NotificationsService, or (B) replace the direct new with a module-level
singleton/lazy getter that is initialized from the DI context at startup, or (C)
if using the default path is intentional, add a clear comment in importPlugins
next to NotificationsService(new ConfigService()) stating this is deliberate and
that the notification path will fall back to the default.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1887 +/- ##
==========================================
+ Coverage 48.62% 48.64% +0.01%
==========================================
Files 1013 1013
Lines 67606 67635 +29
Branches 6925 6940 +15
==========================================
+ Hits 32872 32898 +26
- Misses 34613 34616 +3
Partials 121 121 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
1 participant
Motivation
ConfigServiceby debouncing syncs and skipping unchanged state writes.ConfigServiceinstead of relying on store getters so tests can inject a deterministic path.Description
STORE_SYNC_DEBOUNCE_MS,syncTimer,lastSyncedState, andscheduleSync()inStoreSyncServiceto debounce updates and skip writes when serialized state is unchanged, and clear timers ononModuleDestroy().api/src/unraid-api/config/store-sync.service.spec.tswith mockedstoreandConfigServiceusing Vitest fake timers to verify debouncing, deduplication, and cleanup behavior.NotificationsServiceto acceptConfigServicein the constructor, addgetConfiguredPath()and use it inpaths()and constructor initialization rather thangetters.dynamix().notifications.service.spec.tsto provide a mockedConfigServiceto the testing module in both test suites and remove the previous dynamicgettersmocking approach so the tests use the injected path.Testing
store-sync.service.spec.tswhich verifies debounced writes, unchanged-state skipping, and destroy cleanup, and these tests passed.NotificationsServiceintegration-style tests after injecting a mockedConfigService, and those tests passed.Codex Task
Summary by CodeRabbit
New Features
Improvements
Tests