fix(devtools): scope storage watchers to avoid EMFILE#934
Open
fix(devtools): scope storage watchers to avoid EMFILE#934
Conversation
![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: b5bb2ab807
ℹ️ 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".
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/devtools/src/server-rpc/storage-watch.ts (1)
9-16: Optional: preservedrivercontext when invokingwatch.
const watch = mount?.driver.watchdetaches the method from its receiver. If any driver implementswatchas a regular function referencingthis(e.g.,this.client,this.options), calling it unbound will throw at runtime.♻️ Proposed fix
- const watch = mount?.driver.watch + const watch = mount?.driver.watch?.bind(mount.driver)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools/src/server-rpc/storage-watch.ts` around lines 9 - 16, The code currently detaches the watch method by assigning const watch = mount?.driver.watch which can break drivers that use this; instead obtain a bound function from mount.driver (e.g., bind the method to mount.driver) or call it as a method on the driver so the driver context is preserved when invoking watch; update the call site that returns await watch(...) to use the bound method or call mount.driver.watch with the same arguments so onChange(event, normalizeKey(`${mountKey}${key}`)) still runs but with the correct receiver.packages/devtools/src/server-rpc/storage.ts (1)
24-39: Registeringcloseandreadyhooks insidenitro:initis inconsistent and fragile.Both sibling files (
server-routes.tsandserver-tasks.ts) registerreadyandcloseat the function's top level, guardingreadywithif (!nitro) return. Nesting these insidenitro:initmeans each invocation ofnitro:initadds a new pair of listeners. While the existingunwatch-before-rewatch logic makes this safe today, it results in redundant hook executions and diverges from the pattern established by the rest of this PR.♻️ Proposed refactor (aligns with server-routes.ts / server-tasks.ts pattern)
export function setupStorageRPC({ nuxt, rpc, ensureDevAuthToken, }: NuxtDevtoolsServerContext) { const storageMounts: StorageMounts = {} let storage: Storage | undefined let unwatchStorageMounts: Array<() => Promise<void> | void> = [] nuxt.hook('nitro:init', (nitro) => { storage = nitro.storage - nuxt.hook('close', async () => { - await Promise.all(unwatchStorageMounts.map(unwatch => unwatch())) - unwatchStorageMounts = [] - }) - - nuxt.hook('ready', async () => { - if (!storage) - return - await Promise.all(unwatchStorageMounts.map(unwatch => unwatch())) - unwatchStorageMounts = await Promise.all(Object.keys(storageMounts).map(mountName => - watchStorageMount(storage, mountName, (event, key) => { - if (shouldIgnoreStorageKey(key)) - return - rpc.broadcast.callHook.asEvent('storage:key:update', key, event) - }))) - }) - // Taken from https://github.com/unjs/nitro/blob/... const mounts = { ...nitro.options.storage, ...nitro.options.devStorage, } for (const name of Object.keys(mounts)) { if (shouldIgnoreStorageKey(name)) continue storageMounts[name] = mounts[name]! } }) + nuxt.hook('ready', async () => { + if (!storage) + return + const _storage = storage + await Promise.all(unwatchStorageMounts.map(unwatch => unwatch())) + unwatchStorageMounts = await Promise.all(Object.keys(storageMounts).map(mountName => + watchStorageMount(_storage, mountName, (event, key) => { + if (shouldIgnoreStorageKey(key)) + return + rpc.broadcast.callHook.asEvent('storage:key:update', key, event) + }))) + }) + + nuxt.hook('close', async () => { + await Promise.all(unwatchStorageMounts.map(unwatch => unwatch())) + unwatchStorageMounts = [] + }) + return { ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools/src/server-rpc/storage.ts` around lines 24 - 39, The hooks for storage mounts are registered inside a nitro:init listener which causes duplicate registrations each time nitro:init fires; move the nuxt.hook('ready', ...) and nuxt.hook('close', ...) registrations out of the nitro:init handler to the module top-level, guarding the ready hook with the same pattern used elsewhere (e.g. if (!nitro) return) so they are only registered once. Preserve the existing behavior: on ready, clear prior unwatchs, set unwatchStorageMounts = await Promise.all(Object.keys(storageMounts).map(mountName => watchStorageMount(storage, mountName, (event, key) => { if (shouldIgnoreStorageKey(key)) return; rpc.broadcast.callHook.asEvent('storage:key:update', key, event) }))); and on close await Promise.all(unwatchStorageMounts.map(unwatch => unwatch())) and reset unwatchStorageMounts = []; reference functions/vars watchStorageMount, shouldIgnoreStorageKey, rpc.broadcast.callHook.asEvent, storageMounts, storage, and unwatchStorageMounts when moving the hooks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/devtools/src/server-rpc/storage-watch.ts`:
- Around line 1-2: The code imports WatchCallback and WatchEvent from
'unstorage' which are not guaranteed public types; remove these from the import
and add local type aliases instead: define type WatchEvent = "update" | "remove"
and type WatchCallback = (event: WatchEvent, key: string) => void, keep
importing Storage, normalizeBaseKey and normalizeKey as before, and update any
references to WatchCallback/WatchEvent in the file to use the new local types
(e.g., where watch handlers or event parameters are typed).
In `@packages/devtools/src/server-rpc/storage.ts`:
- Around line 33-38: The closure passed to Promise.all may see a non-narrowed
let variable `storage`, causing a type error; fix by capturing the narrowed
value into a const before mapping (e.g. after `if (!storage) return` do `const
_storage = storage`) and then use `_storage` in the call to `watchStorageMount`
(update references in the `unwatchStorageMounts = await
Promise.all(Object.keys(storageMounts).map(...)` callback to
`watchStorageMount(_storage, mountName, ...)`), leaving the rest (including
`shouldIgnoreStorageKey` and `rpc.broadcast.callHook.asEvent`) unchanged.
---
Nitpick comments:
In `@packages/devtools/src/server-rpc/storage-watch.ts`:
- Around line 9-16: The code currently detaches the watch method by assigning
const watch = mount?.driver.watch which can break drivers that use this; instead
obtain a bound function from mount.driver (e.g., bind the method to
mount.driver) or call it as a method on the driver so the driver context is
preserved when invoking watch; update the call site that returns await
watch(...) to use the bound method or call mount.driver.watch with the same
arguments so onChange(event, normalizeKey(`${mountKey}${key}`)) still runs but
with the correct receiver.
In `@packages/devtools/src/server-rpc/storage.ts`:
- Around line 24-39: The hooks for storage mounts are registered inside a
nitro:init listener which causes duplicate registrations each time nitro:init
fires; move the nuxt.hook('ready', ...) and nuxt.hook('close', ...)
registrations out of the nitro:init handler to the module top-level, guarding
the ready hook with the same pattern used elsewhere (e.g. if (!nitro) return) so
they are only registered once. Preserve the existing behavior: on ready, clear
prior unwatchs, set unwatchStorageMounts = await
Promise.all(Object.keys(storageMounts).map(mountName =>
watchStorageMount(storage, mountName, (event, key) => { if
(shouldIgnoreStorageKey(key)) return;
rpc.broadcast.callHook.asEvent('storage:key:update', key, event) }))); and on
close await Promise.all(unwatchStorageMounts.map(unwatch => unwatch())) and
reset unwatchStorageMounts = []; reference functions/vars watchStorageMount,
shouldIgnoreStorageKey, rpc.broadcast.callHook.asEvent, storageMounts, storage,
and unwatchStorageMounts when moving the hooks.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/devtools/src/server-rpc/server-routes.tspackages/devtools/src/server-rpc/server-tasks.tspackages/devtools/src/server-rpc/storage-watch.tspackages/devtools/src/server-rpc/storage.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/devtools/src/server-rpc/storage-watch.ts (1)
1-2:⚠️ Potential issue | 🟠 Major
WatchCallbackandWatchEventmay not be part ofunstorage's stable public type surface.A previous review raised this exact concern and it remains unaddressed.
WatchEventlives in unstorage's internaltypes.tsandWatchCallbackis the inline type(event: WatchEvent, key: string) => void. Neither is confirmed to be a named export in the publicdist/index.d.ts— they may not survive a minor version bump.Replace with locally-defined aliases:
🛡️ Proposed fix
-import type { Storage, WatchCallback, WatchEvent } from 'unstorage' +import type { Storage } from 'unstorage' import { normalizeBaseKey, normalizeKey } from 'unstorage' +type WatchEvent = 'update' | 'remove' +type WatchCallback = (event: WatchEvent, key: string) => voidRun the following script to confirm whether
WatchCallbackandWatchEventare named exports in the installed unstorage declaration files:#!/bin/bash # Find unstorage dist declaration files and check for WatchCallback / WatchEvent exports fd -e d.ts . -p unstorage --exec grep -l "export.*Watch" {} \; 2>/dev/null | head -10 echo "--- Named exports ---" fd "index.d.ts" -p unstorage --exec grep -n "export.*WatchCallback\|export.*WatchEvent" {} \; 2>/dev/null echo "--- unstorage version ---" fd "package.json" -p "node_modules/unstorage" --max-depth 1 --exec grep '"version"' {} \; 2>/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools/src/server-rpc/storage-watch.ts` around lines 1 - 2, The code imports WatchCallback and WatchEvent from 'unstorage' which are internal and may break; remove WatchCallback and WatchEvent from the import and instead declare local types in this module: add a local type alias "type WatchEvent = { type: string; [key: string]: any }" (or a stricter shape matching how you use it) and "type WatchCallback = (event: WatchEvent, key: string) => void", export them if needed by other modules, then update any references to use these local types; keep the existing import of Storage, normalizeBaseKey, normalizeKey unchanged. Also run the provided verification script to confirm exports in your installed unstorage if you want to double-check.
🧹 Nitpick comments (1)
packages/devtools/src/server-rpc/storage.ts (1)
24-39: Theconst activeStoragenarrowing fix is well-applied, and the close-hook cleanup is clean.One optional tidying: the
shouldIgnoreStorageKey(key)guard at Line 36 is redundant. ThestorageMountsobject is already built with only non-ignored mounts (Lines 49–53), andwatchStorageMountalways prefixes emitted keys with the mount name, so the key prefix is guaranteed to be non-ignored.♻️ Optional simplification
unwatchStorageMounts = await Promise.all(Object.keys(storageMounts).map(mountName => watchStorageMount(activeStorage, mountName, (event, key) => { - if (shouldIgnoreStorageKey(key)) - return rpc.broadcast.callHook.asEvent('storage:key:update', key, event) })))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools/src/server-rpc/storage.ts` around lines 24 - 39, Remove the redundant key-ignore guard inside the nuxt.hook('ready') callback: since storageMounts is constructed to exclude ignored mounts and watchStorageMount prefixes emitted keys with the mount name, the shouldIgnoreStorageKey(key) check before calling rpc.broadcast.callHook.asEvent('storage:key:update', key, event) can be deleted; update the ready hook (where activeStorage, unwatchStorageMounts and watchStorageMount are used) to directly broadcast the event without that guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/devtools/src/server-rpc/storage-watch.ts`:
- Around line 10-15: The current call to mount.driver.watch may resolve to
undefined (Driver.watch returns MaybePromise<Unwatch | void>), which causes a
TypeError when cleanup later calls the stored unwatch; wrap the result of await
mount.driver.watch(...) and only push a real function into unwatchStorageMounts:
call mount.driver.watch with the callback (as in the existing return) and check
the returned value (e.g., const unwatch = await mount.driver.watch(...)); if
unwatch is a function, store it in unwatchStorageMounts (or return it),
otherwise store a no-op function or skip storing; update references to
mount.driver.watch and unwatchStorageMounts in this module and storage.ts to
ensure cleanup always calls a valid function.
---
Duplicate comments:
In `@packages/devtools/src/server-rpc/storage-watch.ts`:
- Around line 1-2: The code imports WatchCallback and WatchEvent from
'unstorage' which are internal and may break; remove WatchCallback and
WatchEvent from the import and instead declare local types in this module: add a
local type alias "type WatchEvent = { type: string; [key: string]: any }" (or a
stricter shape matching how you use it) and "type WatchCallback = (event:
WatchEvent, key: string) => void", export them if needed by other modules, then
update any references to use these local types; keep the existing import of
Storage, normalizeBaseKey, normalizeKey unchanged. Also run the provided
verification script to confirm exports in your installed unstorage if you want
to double-check.
---
Nitpick comments:
In `@packages/devtools/src/server-rpc/storage.ts`:
- Around line 24-39: Remove the redundant key-ignore guard inside the
nuxt.hook('ready') callback: since storageMounts is constructed to exclude
ignored mounts and watchStorageMount prefixes emitted keys with the mount name,
the shouldIgnoreStorageKey(key) check before calling
rpc.broadcast.callHook.asEvent('storage:key:update', key, event) can be deleted;
update the ready hook (where activeStorage, unwatchStorageMounts and
watchStorageMount are used) to directly broadcast the event without that guard.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/devtools/src/server-rpc/storage-watch.tspackages/devtools/src/server-rpc/storage.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/devtools/src/server-rpc/storage.ts (1)
41-50:⚠️ Potential issue | 🟡 MinorCleanup is skipped when
activeStorageis falsyThe early-return on line 43 short-circuits before
unwatchStorageMountsis drained. Ifreadyever fires a second time whilestoragehappens to beundefined(e.g., after a failed HMR reinit), any previously registered watchers will be orphaned untilclose.The fix is trivial — drain first, then guard:
🛡️ Proposed fix
nuxt.hook('ready', async () => { + await Promise.all(unwatchStorageMounts.map(unwatch => unwatch())) + unwatchStorageMounts = [] const activeStorage = storage if (!activeStorage) return - await Promise.all(unwatchStorageMounts.map(unwatch => unwatch())) unwatchStorageMounts = await Promise.all(Object.keys(storageMounts).map(mountName => watchStorageMount(activeStorage, mountName, (event, key) => { rpc.broadcast.callHook.asEvent('storage:key:update', key, event) }))) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools/src/server-rpc/storage.ts` around lines 41 - 50, The ready hook currently returns early when activeStorage (storage) is falsy, skipping cleanup of existing watchers; change the order in the nuxt.hook('ready', ...) handler to always first drain/unwatch existing watchers by awaiting Promise.all(unwatchStorageMounts.map(unwatch => unwatch())) (or await Promise.all(unwatchStorageMounts) if they are already promises), then reset unwatchStorageMounts and only after that check if storage/activeStorage is truthy before re-registering new watchers with watchStorageMount and rpc.broadcast.callHook.asEvent('storage:key:update', ...); ensure references to storage, activeStorage, unwatchStorageMounts and watchStorageMount are used exactly as in the diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/devtools/src/server-rpc/storage.ts`:
- Around line 41-50: The ready hook currently returns early when activeStorage
(storage) is falsy, skipping cleanup of existing watchers; change the order in
the nuxt.hook('ready', ...) handler to always first drain/unwatch existing
watchers by awaiting Promise.all(unwatchStorageMounts.map(unwatch => unwatch()))
(or await Promise.all(unwatchStorageMounts) if they are already promises), then
reset unwatchStorageMounts and only after that check if storage/activeStorage is
truthy before re-registering new watchers with watchStorageMount and
rpc.broadcast.callHook.asEvent('storage:key:update', ...); ensure references to
storage, activeStorage, unwatchStorageMounts and watchStorageMount are used
exactly as in the diff.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/devtools/src/server-rpc/storage-watch.tspackages/devtools/src/server-rpc/storage.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/devtools/src/server-rpc/storage-watch.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/devtools/src/server-rpc/storage-watch.ts`:
- Around line 10-14: The current call to storage.watch(...) opens watchers for
every mount (even though you filter by mountKey), causing EMFILE growth; change
watchStorageMount to call storage.getMount(mountKey) (or equivalent) to retrieve
the specific driver and invoke that driver's watch(...) method instead of
storage.watch, so only the target mount creates watchers; handle the case where
getMount returns null/undefined by returning a no-op/unwatch function and
preserve the original callback signature (event: WatchEvent, key: string) while
normalizing the key before calling onChange.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/devtools/src/server-rpc/storage-watch.ts (1)
1-19: All previous issues addressed — LGTM.The implementation correctly:
- Uses driver-scoped
mount.driver.watchto avoid the EMFILE regression.- Guards the void return from
Driver.watchwith?? (() => {}).- Defines
WatchEvent/WatchCallbacklocally instead of relying on unstorage internals.- Uses
normalizeBaseKey(mount.base) !== mountKeyas the critical guard preventing fallback to the root mount when no exact match exists.One optional cleanup on line 11:
storage.getMount()is typed as returning{ base: string; driver: Driver }(non-nullable), so the!mountcheck can never be true and can be removed.Optional cleanup
- if (!mount || normalizeBaseKey(mount.base) !== mountKey || !mount.driver?.watch) + if (normalizeBaseKey(mount.base) !== mountKey || !mount.driver?.watch)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools/src/server-rpc/storage-watch.ts` around lines 1 - 19, The mount null-check is redundant because storage.getMount() is typed as non-nullable; in watchStorageMount remove the unnecessary `if (!mount || ...)` branch and rely only on the existing guard `normalizeBaseKey(mount.base) !== mountKey || !mount.driver?.watch` (or equivalent) to decide early return, keeping the rest of the function (calling mount.driver.watch, normalizing keys, and returning unwatch) unchanged; this targets the mount variable and the watchStorageMount function to simplify the condition without altering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/devtools/src/server-rpc/storage-watch.ts`:
- Around line 1-19: The mount null-check is redundant because storage.getMount()
is typed as non-nullable; in watchStorageMount remove the unnecessary `if
(!mount || ...)` branch and rely only on the existing guard
`normalizeBaseKey(mount.base) !== mountKey || !mount.driver?.watch` (or
equivalent) to decide early return, keeping the rest of the function (calling
mount.driver.watch, normalizing keys, and returning unwatch) unchanged; this
targets the mount variable and the watchStorageMount function to simplify the
condition without altering behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #933
nitro.storage.watch()usage with mount-scoped watchers.srclive updates for server routes/tasks and custom-mount live updates for the Storage tab.close.