perf(memory): coalesce + cache session-file listings to cut NFS READDIR load#91081
perf(memory): coalesce + cache session-file listings to cut NFS READDIR load#91081amknight wants to merge 5 commits into
Conversation
…DDIR load Add an in-flight dedupe + short-TTL (5s) snapshot cache around listSessionFilesForAgent, keyed by the resolved sessions directory. Concurrent scans (startup catch-up, incremental sync, dreaming, qmd export) now share a single READDIR, and snapshots are invalidated via the canonical onSessionTranscriptUpdate event (covers append, compaction, rewrite, inject, command exec, and archive rotation). The TTL is only a backstop for out-of-process writers on a shared NFS dir.
|
Codex review: needs maintainer review before merge. Reviewed June 7, 2026, 10:49 PM ET / 02:49 UTC. Summary PR surface: Source +124, Tests +133. Total +257 across 2 files. Reproducibility: yes. at source level, but not with live NFS proof: current main and v2026.6.1 call Review metrics: 2 noteworthy metrics.
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 this shape only if memory owners accept the bounded 1s shared-storage freshness tradeoff; otherwise keep the zero-staleness in-flight coalescing and defer TTL reuse or require stronger storage-level proof. Do we have a high-confidence way to reproduce the issue? Yes at source level, but not with live NFS proof: current main and v2026.6.1 call Is this the best way to solve the issue? Acceptable but not purely mechanical: in-flight coalescing is the clean zero-staleness win, while the 1s snapshot cache is a maintainer-owned freshness tradeoff for shared-storage setups. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 4780546c124d. Label changesLabel justifications:
Evidence reviewedPR surface: Source +124, Tests +133. Total +257 across 2 files. View PR surface stats
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
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
clawsweeper
Bot
added
rating: 🦐 gold shrimp
f9941ff to
cd31200
Compare
A transient fs.readdir failure (e.g. NFS EIO/ESTALE) was caught as [] and published into the TTL snapshot, so one blip could be coalesced across callers and reused as a false-empty session set for the whole TTL window - making sync/export/dreaming see no session files and risk pruning. Read results are now a discriminated union: a failed scan is returned to the caller as empty but never cached, so the next call retries; a legitimately absent dir (ENOENT) is still cached as empty. Adds regression tests for the failed-scan retry and the absent-dir caching paths. Addresses Codex/ClawSweeper review on PR #91081.
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
/clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Maintainer Follow-Up Draft for PR #91081PR: #91081 SummaryThe overall direction in #91081 looks reasonable: coalescing concurrent For this to be release-grade, though, the PR should make the cache correctness Main ConcernThe current PR has strong helper-level proof, but it does not yet show a full The PR body acknowledges this risk by treating the 1s TTL as a backstop for I would like the release-grade version of this change to include an explicit The audit should distinguish:
Why #91091 Matters#91091 is conceptually important because it handles the consumer-side failure #91081 prevents amplification: a transient #91091 prevents destructive interpretation: memory sync should not treat a Those are different layers. Even after #91081, the compatibility API still
I would treat #91091, or an equivalent consumer-side guard, as part of the Ongoing Maintenance RiskEven if the current writer/pruner audit passes, this remains an ongoing A future change can easily add a new Suggested Structured Guardrails
Suggested Maintainer FeedbackThis looks like a good performance direction, especially the in-flight Please add an explicit audit of every in-process writer/pruner that can mutate I also think we should make this structural rather than relying on a one-time Longer term, I would prefer session-directory mutations to go through a small Release-Grade Acceptance BarI would be comfortable with this direction if the final PR or companion PRs
Additional Things to Consider
|
A full session sync builds `activePaths` from `listSessionFilesForAgent` and then deletes every indexed session row (files/chunks/vectors/FTS) whose path is not in that listing. The listing swallowed all readdir errors and returned [], so a single transient sessions-dir scan failure (e.g. NFS EIO/ESTALE) was indistinguishable from an empty directory and pruned the entire session memory index, forcing an expensive full re-embed on the next successful scan.
Make the enumeration authoritative for the destructive prune: add `scanSessionFilesForAgent` returning { ok, files } (listSessionFilesForAgent now delegates and is unchanged for callers). The full-enumeration sync passes `scanOk` into `resolveMemorySessionSyncPlan`, which only sets `pruneStaleRows` when a full scan actually read the directory. A failed scan skips the prune (and logs) and leaves the index intact for the next successful enumeration; an authoritatively empty directory still prunes, so legitimately removed sessions (e.g. disk-budget removing the last archive) do not leave orphaned rows.
Latent on main; addresses the consumer-side hazard noted in review of #91081 (which fixed the listing cache from amplifying it).
|
Declining for now, reconsidering the approach |
A full session sync builds `activePaths` from `listSessionFilesForAgent` and then deletes every indexed session row (files/chunks/vectors/FTS) whose path is not in that listing. The listing swallowed all readdir errors and returned [], so a single transient sessions-dir scan failure (e.g. NFS EIO/ESTALE) was indistinguishable from an empty directory and pruned the entire session memory index, forcing an expensive full re-embed on the next successful scan.
Make the enumeration authoritative for the destructive prune: add `scanSessionFilesForAgent` returning { ok, files } (listSessionFilesForAgent now delegates and is unchanged for callers). The full-enumeration sync passes `scanOk` into `resolveMemorySessionSyncPlan`, which only sets `pruneStaleRows` when a full scan actually read the directory. A failed scan skips the prune (and logs) and leaves the index intact for the next successful enumeration; an authoritatively empty directory still prunes, so legitimately removed sessions (e.g. disk-budget removing the last archive) do not leave orphaned rows.
Latent on main; addresses the consumer-side hazard noted in review of #91081 (which fixed the listing cache from amplifying it).
A full session sync builds `activePaths` from `listSessionFilesForAgent` and then deletes every indexed session row (files/chunks/vectors/FTS) whose path is not in that listing. The listing swallowed all readdir errors and returned [], so a single transient sessions-dir scan failure (e.g. NFS EIO/ESTALE) was indistinguishable from an empty directory and pruned the entire session memory index, forcing an expensive full re-embed on the next successful scan.
Make the enumeration authoritative for the destructive prune: add `scanSessionFilesForAgent` returning { ok, files } (listSessionFilesForAgent now delegates and is unchanged for callers). The full-enumeration sync passes `scanOk` into `resolveMemorySessionSyncPlan`, which only sets `pruneStaleRows` when a full scan actually read the directory. A failed scan skips the prune (and logs) and leaves the index intact for the next successful enumeration; an authoritatively empty directory still prunes, so legitimately removed sessions (e.g. disk-budget removing the last archive) do not leave orphaned rows.
Latent on main; addresses the consumer-side hazard noted in review of #91081 (which fixed the listing cache from amplifying it).
2 participants
Summary
Repeated session-directory scans (
listSessionFilesForAgent) issue afs.readdir(dir, { withFileTypes: true })per call with zero caching or coalescing across call sites. On networked filesystems (NFS) this is expensive: resolving each entry's type turns one logical scan into roughly one attribute fetch per session file, and several subsystems (startup catch-up, incremental sync, dreaming, qmd export) scan the same agent sessions dir in close succession — producing redundant READDIR/GETATTR load.This adds an in-flight dedupe + short-TTL snapshot cache around
listSessionFilesForAgent:Promise, so a burst of scans triggers one READDIR instead of N. Each caller still sees a result that reflects the directory at read time; there is no staleness window for the concurrent case.OPENCLAW_STATE_DIR, and lines up withpath.dirname(update.sessionFile)for precise invalidation)..slice()of the snapshot, so the cached array is never shared by reference. A caller mutating its result can't corrupt the cache or another caller's view; the coalescing/caching is invisible to callers.onSessionTranscriptUpdateevent drops the affected directory's snapshot. Every in-process mutation to a session-owned path (append, compaction, rewrite, chat inject, command exec) emits this event, and archive rotation (archiveFileOnDisk) already emits through it too, so one canonical hook covers all in-process write paths without scattered per-path invalidation.SESSION_FILES_LISTING_TTL_MS = 1_000(1s) bounds staleness for writers that never reach our event bus (e.g. another node sharing the NFS sessions dir, or low-frequency archive prunes). Correctness for in-process writes comes from the event invalidation above, not the TTL; 1s keeps the eventual-consistency window for out-of-band writers tight.{ ok: true; files } | { ok: false }). A transientfs.readdirfailure (NFSEIO/ESTALE/EACCES/timeout) is returned to the one caller as empty but not published into the snapshot, so the next call retries instead of serving a false-empty session set for the TTL window. A legitimately absent dir (ENOENT, the normal "no sessions yet" state) is still cached as empty, since the first session write emits an invalidation event.The cache is bounded (64 dirs), module-lifecycle-owned, and race-safe: a completed read only publishes its snapshot if its entry is still the active one, so an invalidation or newer read during the await wins. A
resetSessionFilesListingCache()helper exists for test isolation and is intentionally kept out of the package's public re-export surface.withFileTypesremoval (to cut the per-entry NFSlstatamplification) is deferred to a focused follow-up.Scope — what reads are covered
The cache wraps a single physical read: the
fs.readdir(dir, { withFileTypes: true })insidelistSessionFilesForAgent, wherediris the resolved per-agent sessions directory (…/agents/<agentId>/sessions). ThatwithFileTypesscan is the expensive NFS operation — one logical scan fans out into roughly one attribute fetch per session file.Four in-process memory-engine call sites repeatedly scan the same sessions dir in close succession (the "repeated scans" pattern from the analysis); all now route through the coalesce/cache layer:
manager-sync-ops.ts:1188markSessionStartupCatchupDirtyFiles()sessionssource enabled.manager-sync-ops.ts:1629targetSessionFilesare passed, it lists the whole dir to build the reindex plan.dreaming-phases.ts:850agentIds, listing each agent's sessions to batch messages by day.qmd-manager.ts:2383exportSessions()These run on overlapping schedules and can fire concurrently, so they previously produced N independent READDIR storms over a single directory. With the cache:
onSessionTranscriptUpdateevent drops the snapshot, so the next call re-scans.Out of scope (intentional — focused change): other sessions-dir reads that are one-shot / CLI / diagnostic rather than hot-loop, and are left for a follow-up:
dreaming-narrative.ts:901(separate narrative readdir; the most natural follow-up candidate),cli.runtime.ts:527(scanSessionFilesCLI diagnostic),session-cost-usage.ts:378(cost/usage subsystem),dreaming-repair.ts:78(repair corpus pass). Unrelated scans of the parentagents/directory (session-utils.ts,agent-list.ts, gatewaydoctor.ts) use a different key and are untouched.Verification
Committed regression tests
node scripts/run-vitest.mjs packages/memory-host-sdk/src/host/session-files.test.ts— 19/19 (12 pre-existing + 7 new). The cache tests count real scans by spying onfsPromises.readdir:readdircalled exactly once.vi.useFakeTimers({ toFake: ["Date"] }), advancingDatepast the 1s TTL forces a re-scan ⇒ 2 scans, fresh listing.emitSessionTranscriptUpdate({ sessionFile })drops the snapshot ⇒ next call re-scans, fresh listing.readdirrejected once withEIO⇒ first call returns[], a second call within the TTL re-scans (2 scans) and recovers the real listing.ENOENT) is cached as empty ⇒ 1 scan across two calls..length = 0,.push); a later read still returns the correct listing ⇒ caller mutation can't corrupt the snapshot, all from 1 scan.Other proof:
manager-sync-ops.startup-catchup,manager-targeted-sync,manager-session-sync-state,qmd-manager,manager.fts-only-reindex,manager-source-state,dreaming-phases,manager-sync-yield,session-files-yield.pnpm tsgo:coreandpnpm tsgo:test:packagesexit 0.node scripts/run-oxlint.mjsclean;oxfmt --checkclean.Concurrency chaos / fuzz hammering (dev-time stress; not committed)
A seeded invariant-based harness drove randomized interleavings of concurrent read bursts + real file create/delete +
onSessionTranscriptUpdateevents against the live cache, asserting:fsPromises.readdirspy confirms a synchronous burst of N concurrent reads issues ≤1 physical scan (a fresh read, or 0 when served from a live snapshot), never one per caller, and every caller observes the same content (compared by value, since each now receives its own array copy);Ran against the current design (1s TTL, per-caller copies, READDIR-count coalescing proof) at 20,000 iterations × burst 32 × 300 agents across 5 seeds — all green. (Kept as a local stress tool rather than a committed suite to avoid a long-running test in CI; the committed unit tests above are the durable regression guard.)
Mutation testing (dev-time; confirms the tests actually catch breakage)
Each cache invariant was corrupted one at a time in
session-files.ts; the unit + chaos suites were re-run to confirm they kill the mutant, then the source was restored (verified clean viagit diff):.slice()on in-flight originator return.slice()on coalesced joiner return.slice()on cached-snapshot return9/9 correctness mutants killed, 0 unexpected survivors. The surviving
drop-race-guardis expected: reads replace the map entry rather than mutating it, so a stale completion writing into an orphaned entry is harmless — the guard is documented defensive intent, not load-bearing.max-dirsis a perf knob, not a correctness property.