Skip to content

fix(memory): warn on gateway watcher FD risk#89185

Merged
RomneyDa merged 7 commits into
mainfrom
codex/fd-gateway-memory-watch-default
Jun 1, 2026
Merged

fix(memory): warn on gateway watcher FD risk#89185
RomneyDa merged 7 commits into
mainfrom
codex/fd-gateway-memory-watch-default

Conversation

@RomneyDa

@RomneyDa RomneyDa commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • Preserve the existing memorySearch.sync.watch default (true) in gateway mode so small and typical gateway workspaces keep live memory freshness.
  • Add config validation warnings for gateway configs that leave memory watching enabled on FD-pressure-prone surfaces such as large memory trees, extraPaths, QMD collections, session-memory indexing, or multi-agent gateways.
  • Keep sync.watch: false as the explicit mitigation, and make QMD watcher error events log instead of crashing the gateway.

Fixes #71335.

Why This Shape

The first commit on this branch tried to make unset gateway memorySearch.sync.watch default to false. That avoids long-lived watcher FD pressure, but it can make the average gateway user's memory UX worse: file edits may stay stale indefinitely unless they restart, manually reindex, configure an interval, or opt back into watching.

This updated approach keeps the current default-on behavior and adds operator-facing warnings instead. The warning points users with higher-risk gateway configs toward sync.watch: false without surprising small or several-month memory users who rely on live indexing.

A smarter future fix could make watching adaptive, for example by watching only recent files, applying thresholds, or choosing a lower-FD backend per platform. This PR does not add a new config option.

Related Issues And PRs

Behavior

  • Unset sync.watch: still resolves to true, including gateway mode.
  • Explicit sync.watch: true: still enables watching.
  • Explicit sync.watch: false: still disables watching and remains the recommended mitigation for FD pressure.
  • Gateway configs with enabled watch and FD-prone memory surfaces emit non-fatal config warnings.
  • QMD watcher error events log a warning instead of surfacing as unhandled EventEmitter errors.

Verification

  • node scripts/run-vitest.mjs src/agents/memory-search.test.ts src/config/validation.policy.test.ts extensions/memory-core/src/memory/qmd-manager.test.ts
  • node scripts/run-vitest.mjs src/config/validation.policy.test.ts
  • pnpm exec oxfmt --check --threads=1 docs/concepts/memory-builtin.md docs/reference/memory-config.md src/agents/memory-search.ts src/agents/memory-search.test.ts src/config/schema.help.ts src/config/types.tools.ts src/config/validation.policy.test.ts src/config/validation.ts extensions/memory-core/src/memory/qmd-manager.ts extensions/memory-core/src/memory/qmd-manager.test.ts
  • pnpm exec oxfmt --check --threads=1 src/config/validation.ts
  • pnpm docs:check-mdx
  • pnpm exec oxfmt --check --threads=1 docs/concepts/memory-builtin.md docs/reference/memory-config.md extensions/memory-core/src/memory/qmd-manager.ts src/agents/memory-search.ts src/config/schema.help.ts src/config/types.tools.ts
  • git diff --check origin/main..HEAD && git diff --check
  • pnpm tsgo:core
  • pnpm tsgo:core:test
  • pnpm tsgo:prod
  • NODE_OPTIONS=--max-old-space-size=8192 pnpm build:plugin-sdk:strict-smoke
  • pnpm lint --threads=8
  • .agents/skills/autoreview/scripts/autoreview --mode local
  • CI: 26779072956 passed on d4cc83b6075395f76a951bd295035422a56b5ab9

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation extensions: memory-core Extension: memory-core agents Agent runtime and tooling size: S maintainer Maintainer-authored PR labels Jun 1, 2026
@clawsweeper

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 1, 2026, 4:28 PM ET / 20:28 UTC.

Summary
The PR preserves default-on memory watching, adds gateway config warnings for FD-prone memory watch surfaces, documents the mitigation, and logs QMD watcher error events instead of leaving them unhandled.

PR surface: Source +96, Tests +129, Docs +6. Total +231 across 10 files.

Reproducibility: yes. for the QMD watcher error path from source: current main has no QMD watcher error listener, and Node EventEmitter throws unhandled error events without one. The gateway FD-pressure warning policy is supported by related reports and current watcher code, but I did not run a live long-running FD reproduction here.

Review metrics: 1 noteworthy metric.

  • Memory Watch Policy Surface: 0 defaults changed, 1 warning family added. The runtime default remains compatible, but existing gateway configs can now see a new validation warning that maintainers should approve before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Maintainers should explicitly accept warning-first resolution for the linked gateway default-off request or ask for warning-trigger/closing-syntax changes before merge.

Risk before merge

  • [P1] Existing gateway configs with memory watching enabled can begin emitting non-fatal config warnings during validation; maintainers should accept the warning trigger breadth and wording before merge.
  • [P1] This PR is a warning-first mitigation, not a new live FD-reduction mechanism; actual FD behavior still depends on prior watcher fixes and operators using sync.watch: false when needed.
  • [P1] The PR uses closing syntax for Feature: sync.watch should default to false in gateway mode #71335 even though it intentionally keeps the gateway default enabled, so merging it will close the default-off request as a policy decision.

Maintainer options:

  1. Accept Warning-First Policy
    Maintainers can merge as-is if they intentionally want to preserve default live freshness and treat the new gateway warnings as the resolution for FD-prone deployments.
  2. Narrow Warning Triggers
    Before merge, require the warning helper to trigger only on explicitly high-risk surfaces if broad warnings on any memorySearch config are too noisy for existing operators.
  3. Keep Default-Off Work Open
    If warning-only is not intended to resolve the linked default-off request, remove closing syntax and keep that issue as the canonical product decision.

Next step before merge

  • [P2] Human maintainer review should decide whether warning-first FD-pressure handling and closing the default-off issue are the intended product outcome; there is no narrow automated repair indicated from this review.

Security
Cleared: The diff touches docs, config validation warnings, tests, and watcher error logging; it does not add dependencies, workflow changes, secret handling, or new code-execution surfaces.

Review details

Best possible solution:

Land only after maintainers explicitly accept warning-first resolution for gateway memory-watch FD pressure, including the warning breadth and whether that should close the existing default-off request.

Do we have a high-confidence way to reproduce the issue?

Yes for the QMD watcher error path from source: current main has no QMD watcher error listener, and Node EventEmitter throws unhandled error events without one. The gateway FD-pressure warning policy is supported by related reports and current watcher code, but I did not run a live long-running FD reproduction here.

Is this the best way to solve the issue?

Unclear as a final product decision: the patch is a compatible mitigation and the QMD error listener is the right sibling-aligned repair, but warning-only handling is not the only possible answer to gateway FD pressure. Maintainers should choose between accepting the warning-first policy, narrowing the warnings, or leaving the default-off issue open.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against fe97c6000c5c.

Label changes

Label justifications:

  • P2: This is a normal-priority memory/gateway reliability improvement with limited blast radius and an explicit compatibility decision.
  • merge-risk: 🚨 compatibility: Merging can change existing gateway operator experience by adding config warnings and by closing a default-policy issue without changing the default.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The contributor proof gate is not applicable to this MEMBER-authored PR; the body and comments provide targeted test, lint, typecheck, build-smoke, autoreview, and passing CI evidence for maintainer review.
Evidence reviewed

PR surface:

Source +96, Tests +129, Docs +6. Total +231 across 10 files.

View PR surface stats
Area Files Added Removed Net
Source 5 99 3 +96
Tests 3 130 1 +129
Docs 2 9 3 +6
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 10 238 7 +231

What I checked:

  • Repository policy applied: Root AGENTS.md and scoped guides for src/agents, extensions, and docs were read; OpenClaw policy treats config/default warnings and gateway behavior as compatibility-sensitive PR review surfaces. (AGENTS.md:1, fe97c6000c5c)
  • Current main watch default: Current main resolves unset memorySearch.sync.watch to true, which is the compatibility behavior this PR says it preserves. (src/agents/memory-search.ts:443, fe97c6000c5c)
  • PR preserves runtime default: The final PR head keeps watch defaulting to true; the added resolver options are accepted as a no-op compatibility parameter rather than changing default policy. (src/agents/memory-search.ts:448, beac3c9ccf64)
  • Gateway warning implementation: The PR adds gateway-only memory watch warnings when memory search and watch are enabled and configured/default surfaces look FD-prone, then appends those warnings to plugin config validation results. (src/config/validation.ts:875, beac3c9ccf64)
  • Warning regression tests: The PR adds config validation coverage for warning on gateway extra paths, no warning when sync.watch is false, and per-agent warning paths in multi-agent gateways. (src/config/validation.policy.test.ts:44, beac3c9ccf64)
  • QMD watcher error handling: The PR adds an error listener to the QMD chokidar watcher that logs the message instead of leaving the watcher with no error listener. (extensions/memory-core/src/memory/qmd-manager.ts:1619, beac3c9ccf64)

Likely related people:

  • Peter Steinberger: Local blame attributes the current main memory resolver, config validation wrapper, and QMD watcher region sampled for this review to commit 6668eb82258c9847d38d841349a4d5937e0735a3. (role: recent area contributor; confidence: medium; commits: 6668eb82258c; files: src/agents/memory-search.ts, src/config/validation.ts, extensions/memory-core/src/memory/qmd-manager.ts)
  • Dallin Romney: Recent current-main history shows commit 5a67c5c5566ec29c0a5c1985a117a57854eb1da6 reducing Linux watcher fan-out in manager-sync-ops.ts, the closest sibling runtime watcher surface. (role: recent adjacent owner; confidence: high; commits: 5a67c5c5566e; files: extensions/memory-core/src/memory/manager-sync-ops.ts, extensions/memory-core/src/memory/manager.watcher-config.test.ts)
  • lukeboyett: The related merged FD fan-out fix in the provided GitHub context changed the memory watcher backend and is directly adjacent to this warning-first mitigation. (role: feature-history contributor; confidence: medium; commits: 9e43d0327fc8; files: extensions/memory-core/src/memory/manager-sync-ops.ts, extensions/memory-core/src/memory/manager.ts, extensions/memory-core/src/memory/manager.watcher-config.test.ts)
  • frankekn: The related merged watcher write-polling fix in the provided GitHub context removed awaitWriteFinish pressure from the same memory/QMD watcher family. (role: feature-history contributor; confidence: medium; commits: b04e42812eca; files: extensions/memory-core/src/memory/manager-sync-ops.ts, extensions/memory-core/src/memory/qmd-manager.ts, extensions/memory-core/src/memory/watch-settle.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. labels Jun 1, 2026
@RomneyDa
RomneyDa force-pushed the codex/fd-gateway-memory-watch-default branch from 8239ff2 to eaa0b8f Compare June 1, 2026 19:04
@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. label Jun 1, 2026
@RomneyDa
RomneyDa force-pushed the codex/fd-gateway-memory-watch-default branch from eaa0b8f to 624702e Compare June 1, 2026 19:16
@RomneyDa

RomneyDa commented Jun 1, 2026

Copy link
Copy Markdown
Member Author

Addressed the ClawSweeper feedback.

  • Rebased onto current origin/main; the unrelated OpenAI web-search Docker E2E files are no longer in this PR diff.
  • Added explicit PR-body release-note context for the gateway default compatibility change.
  • Fixed the QMD status-borrowing regression found during follow-up review: gateway status probes now compare against the default/full QMD cache identity while separate transient status manager creation still uses status runtime settings.
  • Added regression coverage for gateway status-only QMD borrow behavior.

Verification:

  • node scripts/run-vitest.mjs src/agents/memory-search.test.ts extensions/memory-core/src/memory/manager.watcher-config.test.ts extensions/memory-core/src/memory/search-manager.test.ts extensions/memory-core/src/memory/qmd-manager.test.ts
  • pnpm exec oxfmt --check --threads=1 docs/concepts/memory-builtin.md docs/reference/memory-config.md extensions/memory-core/src/memory/manager.ts extensions/memory-core/src/memory/manager.watcher-config.test.ts extensions/memory-core/src/memory/qmd-manager.test.ts extensions/memory-core/src/memory/qmd-manager.ts extensions/memory-core/src/memory/search-manager.test.ts extensions/memory-core/src/memory/search-manager.ts src/agents/memory-search.test.ts src/agents/memory-search.ts src/config/schema.help.ts src/config/types.tools.ts
  • pnpm docs:check-mdx
  • git diff --check origin/main..HEAD && git diff --check
  • .agents/skills/autoreview/scripts/autoreview --mode local
  • .agents/skills/autoreview/scripts/autoreview --mode commit --commit HEAD

Final pushed head: 624702efe75.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 1, 2026
@RomneyDa RomneyDa changed the title fix(memory): default gateway memory watch off fix(memory): warn on gateway watcher FD risk Jun 1, 2026
@RomneyDa

RomneyDa commented Jun 1, 2026

Copy link
Copy Markdown
Member Author

Updated this PR to keep memory watching enabled by default and move the FD-pressure handling to warnings.

  • Left the original default-off implementation as the first commit for review history.
  • Added a second commit that restores default-on behavior, adds gateway config warnings for FD-prone memory watch surfaces, and keeps sync.watch: false as the explicit mitigation.
  • Added QMD watcher error handling so watcher resource errors log instead of crashing via unhandled EventEmitter error.
  • Updated the PR title/body to explain the tradeoff and future smarter watcher options.

Verification:

  • node scripts/run-vitest.mjs src/agents/memory-search.test.ts src/config/validation.policy.test.ts extensions/memory-core/src/memory/qmd-manager.test.ts
  • pnpm exec oxfmt --check --threads=1 docs/concepts/memory-builtin.md docs/reference/memory-config.md src/agents/memory-search.ts src/agents/memory-search.test.ts src/config/schema.help.ts src/config/types.tools.ts src/config/validation.policy.test.ts src/config/validation.ts extensions/memory-core/src/memory/qmd-manager.ts extensions/memory-core/src/memory/qmd-manager.test.ts
  • pnpm docs:check-mdx
  • git diff --check origin/main..HEAD && git diff --check
  • .agents/skills/autoreview/scripts/autoreview --mode local

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@RomneyDa

RomneyDa commented Jun 1, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@RomneyDa

RomneyDa commented Jun 1, 2026

Copy link
Copy Markdown
Member Author

Fixed the CI failures from the warning-helper follow-up.

Root cause: the first helper shape let TypeScript narrow optional MemorySearchConfig values to never inside a single Boolean(...) OR chain. The follow-up commits split those checks into named booleans and removed the redundant final boolean cast that oxlint rejected.

Local verification added after the CI failures:

  • pnpm tsgo:core
  • pnpm tsgo:core:test
  • pnpm tsgo:prod
  • NODE_OPTIONS=--max-old-space-size=8192 pnpm build:plugin-sdk:strict-smoke
  • pnpm lint --threads=8
  • node scripts/run-vitest.mjs src/config/validation.policy.test.ts
  • pnpm exec oxfmt --check --threads=1 src/config/validation.ts

Latest CI run 26779072956 passed on d4cc83b6075395f76a951bd295035422a56b5ab9.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

Re-review progress:

@RomneyDa

RomneyDa commented Jun 1, 2026

Copy link
Copy Markdown
Member Author

Cleaned up the docs/comment wording in beac3c9ccf6.

  • Replaced PR-history phrasing like “stays enabled” with direct current-behavior wording.
  • Made the memory/QMD FD warning text read as user-facing configuration guidance.
  • Removed the verbose QMD watcher error-handler comment and shortened the resolver compatibility comment.

Verification for this cleanup:

  • pnpm exec oxfmt --check --threads=1 docs/concepts/memory-builtin.md docs/reference/memory-config.md extensions/memory-core/src/memory/qmd-manager.ts src/agents/memory-search.ts src/config/schema.help.ts src/config/types.tools.ts
  • pnpm docs:check-mdx
  • git diff --check

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@RomneyDa RomneyDa mentioned this pull request Jun 1, 2026
Open
@RomneyDa
RomneyDa merged commit 2405bbc into main Jun 1, 2026
159 checks passed
@RomneyDa
RomneyDa deleted the codex/fd-gateway-memory-watch-default branch June 1, 2026 21:23
This was referenced Jun 1, 2026
Merged
Merged
@github-actions github-actions Bot mentioned this pull request Jun 1, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 2, 2026
@RomneyDa
fix(memory): warn on gateway watcher FD risk (openclaw#89185)
cd56b39 
* fix(memory): default gateway memory watch off

* fix(memory): warn on gateway watcher fd risk

* fix(config): avoid warning helper narrowing

* fix(config): remove redundant warning boolean cast

* docs(memory): clarify watcher default wording

* docs(memory): simplify watcher warning copy

* fix(config): scope watcher warning to local gateway
SYU8384 pushed a commit to SYU8384/openclaw that referenced this pull request Jun 3, 2026
@RomneyDa
fix(memory): warn on gateway watcher FD risk (openclaw#89185)
e7589dc 
* fix(memory): default gateway memory watch off

* fix(memory): warn on gateway watcher fd risk

* fix(config): avoid warning helper narrowing

* fix(config): remove redundant warning boolean cast

* docs(memory): clarify watcher default wording

* docs(memory): simplify watcher warning copy

* fix(config): scope watcher warning to local gateway
sablehead pushed a commit to sablehead/openclaw that referenced this pull request Jun 10, 2026
@RomneyDa
fix(memory): warn on gateway watcher FD risk (openclaw#89185)
d2c35c9 
* fix(memory): default gateway memory watch off

* fix(memory): warn on gateway watcher fd risk

* fix(config): avoid warning helper narrowing

* fix(config): remove redundant warning boolean cast

* docs(memory): clarify watcher default wording

* docs(memory): simplify watcher warning copy

* fix(config): scope watcher warning to local gateway
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling docs Improvements or additions to documentation extensions: memory-core Extension: memory-core maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature: sync.watch should default to false in gateway mode

1 participant

@RomneyDa