fix(memory): warn after startup watcher pressure check#89244
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 3, 2026, 2:55 PM ET / 18:55 UTC. Summary PR surface: Source +113, Tests +72. Total +185 across 6 files. Reproducibility: yes. from source inspection for the missing warning path: current main starts the relevant watchers without a high-pressure warning, and the linked FD-pressure issue documents large watched memory trees. I did not run a live large-tree gateway reproduction in this read-only review. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the runtime warning if maintainers accept warning-only mitigation, and track deeper adaptive watcher or FD-reduction work separately if needed. Do we have a high-confidence way to reproduce the issue? Yes from source inspection for the missing warning path: current main starts the relevant watchers without a high-pressure warning, and the linked FD-pressure issue documents large watched memory trees. I did not run a live large-tree gateway reproduction in this read-only review. Is this the best way to solve the issue? Yes as a bounded mitigation, but not as a complete FD-pressure fix: the PR moves warning policy to actual watcher state and leaves config/default behavior intact. Maintainers still need to decide whether warning-only behavior is sufficient for this FD-pressure family. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 892602eaba07. Label changesLabel justifications:
Evidence reviewedPR surface: Source +113, Tests +72. Total +185 across 6 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
a36756a to
18e300b
Compare
18e300b to
70e32c1
Compare
|
Bounded scan makes sense, but I wanted to flag one edge case: returning Maybe that tradeoff is intentional, but if so I'd love a short comment near the early return explaining that this is a best-effort heuristic and can under-warn on huge trees. That would help future readers avoid treating the warning as exhaustive. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
b774472 to
0942d34
Compare
94e8013 to
ec1eb08
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
1bcfc66 to
539f4f2
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Summary
How the warning works
ready, after Chokidar has populated its initial watched state.ready; extending an already-ready watcher is not continuously polled.readycallback.Related
Verification
node scripts/run-vitest.mjs extensions/memory-core/src/memory/manager.watcher-config.test.ts extensions/memory-core/src/memory/qmd-manager.test.tspnpm exec oxlint extensions/memory-core/src/memory/manager-sync-ops.ts extensions/memory-core/src/memory/qmd-manager.ts extensions/memory-core/src/memory/watch-pressure.ts extensions/memory-core/src/memory/manager.watcher-config.test.ts extensions/memory-core/src/memory/qmd-manager.test.ts --deny-warningsgit diff --check origin/main...HEAD