Skip to content

perf(sessions): share one enumeration across archive retention sweeps#91957

Merged
amknight merged 1 commit into
mainfrom
ak/session-disk-budget-sweep
Jun 15, 2026
Merged

perf(sessions): share one enumeration across archive retention sweeps#91957
amknight merged 1 commit into
mainfrom
ak/session-disk-budget-sweep

Conversation

@amknight

@amknight amknight commented Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

Every non-warn session-store save runs archived-transcript retention cleanup, and reset-archive retention defaults on (resolveResetArchiveRetentionMs falls back to pruneAfterMs when unset). So saveSessionStoreUnlocked invoked cleanupArchivedSessionTranscripts twice back-to-back — reason "deleted", then reason "reset" — and each call performed its own full readdir of the per-agent sessions directory. Session-store saves happen per agent turn, which means active agents paid two full directory enumerations per turn by default.

A production NFSv4.1 profile (Node trace of an active agent) captured exactly this signature: scans of agents/main/sessions in back-to-back pairs (inter-scan gaps of +0.00s), 3.4–3.8s per scan (~100 entries, with the NFS server amplifying each READDIR into per-entry metadata reads), each pair immediately preceding the sessions.json store write — ~21.7s of directory enumeration inside a 27-second active window. The pairs also serialize against transcript-append lock traffic on the same directory (lock open/unlink stalls of 2.6–3.5s while a scan is in flight).

The fix makes retention rules share one listing:

  • cleanupArchivedSessionTranscripts now accepts rules: Array<{ reason, olderThanMs }> and applies every rule from a single readdir per directory. An archive filename carries exactly one .{reason}.{timestamp} suffix, so per-entry matching is first-match.
  • Store maintenance passes both reasons in one call (one enumeration per save instead of two). The cron session reaper migrates to the same shape.
  • Invalid rules are dropped up front; a call with no valid rules never lists.
  • Rider in the same hot-path family: the disk-budget sweep's readSessionsDirFiles stat'd every session file serially; stats now run with bounded concurrency (8), so a budget sweep no longer serializes ~N per-file latency round trips on networked filesystems.

Behavior is otherwise preserved: identical removal semantics per reason/threshold, same removed/scanned accounting, all callers migrated in this change.

Related work on the other enumeration sources the same investigation surfaced: #91091 (destructive sweeps must not act on failed enumerations) and #91841 (memory delta syncs stop full-scanning). This PR removes the store-save scan-pair source the trace actually captured.

Verification

  • node scripts/run-vitest.mjs src/gateway/session-transcript-files.fs.archive-cleanup.test.ts src/gateway/session-transcript-files.fs.archive-events.test.ts src/config/sessions/disk-budget.test.ts src/config/sessions/store.pruning.integration.test.ts src/commands/sessions-cleanup.test.ts — all 3 Vitest shards passed.
  • pnpm tsgo:core, pnpm tsgo:core:test, node scripts/run-oxlint.mjs on touched files — clean.
  • New tests prove: multiple rules served by a single readdir (spy-counted); per-rule age thresholds applied independently; archives whose reason has no rule are preserved; invalid rules dropped with no listing at all.
  • Not run here: a live NFS re-trace. The profile's own verification protocol applies directly — the back-to-back scan pairs preceding each sessions.json write should become single scans.

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: M maintainer Maintainer-authored PR labels Jun 10, 2026
@clawsweeper

clawsweeper Bot commented Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 15, 2026, 7:07 AM ET / 11:07 UTC.

Summary
The PR changes archived-session cleanup to accept multiple retention rules per directory listing and uses bounded concurrency for session-directory file stats in disk-budget sweeps.

PR surface: Source +20, Tests +104. Total +124 across 5 files.

Reproducibility: yes. Current main shows save-time maintenance calls cleanupArchivedSessionTranscripts once for deleted archives and again for reset archives when reset retention is enabled by default, which source-reproduces the paired directory enumeration.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: serialized state: src/config/sessions/disk-budget.ts, serialized state: src/config/sessions/store.ts, serialized state: src/cron/session-reaper.ts, serialized state: src/gateway/session-transcript-files.fs.archive-cleanup.test.ts, serialized state: src/gateway/session-transcript-files.fs.ts, unknown-data-model-change: src/config/sessions/disk-budget.ts, and 2 more. Confirm migration or upgrade compatibility proof 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:

  • none.

Risk before merge

  • [P1] A live NFS before/after trace was not rerun in this read-only review; the performance improvement is source- and regression-test-backed rather than remeasured on the reported production profile.

Maintainer options:

  1. Decide the mitigation before merge
    Land the focused internal coalescing change after normal maintainer review, while leaving broader session-listing cache and reset-retention cleanup-service work to their separate PRs.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No automated repair is needed; this maintainer-labeled PR mainly needs normal owner review and landing judgment.

Security
Cleared: No security or supply-chain concern found; the diff stays in TypeScript session cleanup code/tests and imports an existing local concurrency helper.

Review details

Best possible solution:

Land the focused internal coalescing change after normal maintainer review, while leaving broader session-listing cache and reset-retention cleanup-service work to their separate PRs.

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

Yes. Current main shows save-time maintenance calls cleanupArchivedSessionTranscripts once for deleted archives and again for reset archives when reset retention is enabled by default, which source-reproduces the paired directory enumeration.

Is this the best way to solve the issue?

Yes. Coalescing rules at cleanupArchivedSessionTranscripts is the narrowest owner-boundary fix for the duplicated listing, and broader listing caches or CLI cleanup retention semantics are separate surfaces already represented by related PRs.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against e7ee1c55b433.

Label changes

Label changes:

  • add P2: The PR targets a session maintenance hot-path performance cost with limited blast radius and no public config or plugin API change.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The maintainer-authored/labeled PR is not subject to the external contributor proof gate; the body reports focused unit, type, and lint verification but no live NFS re-trace.
  • remove rating: 🌊 off-meta tidepool: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

  • P2: The PR targets a session maintenance hot-path performance cost with limited blast radius and no public config or plugin API change.
  • 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 maintainer-authored/labeled PR is not subject to the external contributor proof gate; the body reports focused unit, type, and lint verification but no live NFS re-trace.
Evidence reviewed

PR surface:

Source +20, Tests +104. Total +124 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 4 67 47 +20
Tests 1 104 0 +104
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 171 47 +124

What I checked:

  • Root policy read: Root AGENTS.md was read fully and requires deep PR review across current source, callers, tests, history, and scoped policy before verdicts. (AGENTS.md:5, e7ee1c55b433)
  • Scoped gateway policy read: Gateway scoped guidance was read fully; the PR touches gateway transcript helpers and a save-time hot path. (src/gateway/AGENTS.md:1, e7ee1c55b433)
  • Current duplicated cleanup behavior: Current main calls archived transcript cleanup once for deleted archives and then again for reset archives from the same save-time maintenance branch. (src/config/sessions/store.ts:710, e7ee1c55b433)
  • Current callee enumerates per call: Current main's cleanup helper performs one readdir per helper invocation, so the two save-time calls imply two directory listings when both retention reasons run. (src/gateway/session-transcript-files.fs.ts:520, e7ee1c55b433)
  • Merge-result cleanup coalesces rules: The GitHub potential merge commit changes cleanup to accept rules, filters invalid rules before listing, and applies all matching reasons from one directory listing. (src/gateway/session-transcript-files.fs.ts:505, 0c01d33187db)
  • Merge-result caller uses one cleanup call: The save-time store maintenance branch passes deleted and reset retention rules together, preserving independent thresholds while avoiding the second listing. (src/config/sessions/store.ts:714, 0c01d33187db)

Likely related people:

  • steipete: GitHub path history shows repeated recent work on session store and disk-budget maintenance, including prompt-blob/disk-budget cleanup and session store performance changes. (role: recent area contributor; confidence: high; commits: edd4c62da1c2, 467b068fdc7b, 59997d868942; files: src/config/sessions/store.ts, src/config/sessions/disk-budget.ts)
  • buyitsydney: GitHub path history shows recent archive-session work on archived transcript update indexing in the same gateway transcript helper area. (role: adjacent feature contributor; confidence: medium; commits: aba97a4c7cff; files: src/gateway/session-transcript-files.fs.ts)
  • amknight: Beyond authoring this branch, they have merged/open related NFS session-enumeration work that the PR body links as the same investigation thread. (role: adjacent performance investigator; confidence: medium; commits: ed11c8527608; files: extensions/memory-core/src/memory/manager-sync-ops.ts, packages/memory-host-sdk/src/host/session-files.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 the rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. label Jun 10, 2026
@amknight amknight mentioned this pull request Jun 10, 2026
Merged
@amknight

amknight commented Jun 15, 2026

Copy link
Copy Markdown
Member Author

/clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 15, 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. P2 Normal backlog priority with limited blast radius. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 15, 2026
@amknight
amknight merged commit 8682d07 into main Jun 15, 2026
213 of 222 checks passed
@amknight
amknight deleted the ak/session-disk-budget-sweep branch June 15, 2026 11:11
@clawsweeper clawsweeper Bot mentioned this pull request Jun 15, 2026
Closed
@github-actions github-actions Bot mentioned this pull request Jun 15, 2026
Open
@clawsweeper clawsweeper Bot mentioned this pull request Jun 15, 2026
Open
@amknight

amknight commented Jun 15, 2026

Copy link
Copy Markdown
Member Author

Verification

Tested rebased onto current main (e7ee1c55b4) — head 26ff3ebbe3 cherry-picks cleanly and the net diff is exactly the 5 changed files. Run locally (macOS) and on a Linux Node 24 Crabbox Testbox; results match.

Affected suites — 24 files / 213 tests passed (Testbox tbx_01kv5qgw2nhehqkrcr88a0q1rv, vitest v4.1.7, 6 shards ~56s): gateway 16/72, runtime-config 3/61, infra 1/51, cron 2/20, commands 1/6, utils 1/3. Covers session-transcript-files.fs.archive-cleanup, …archive-events, session-archive.imports, disk-budget, store.pruning, store.pruning.integration, session-reaper, service.session-reaper-in-finally, sessions-cleanup, session-cost-usage, run-with-concurrency, drain-active-sessions-for-shutdown.

Behavioral E2E (real saveSessionStore path, Linux) — drove an enforced-maintenance save over a seeded sessions dir with a fs.promises.readdir counter:

  • sessions dir enumerated exactly once for retention. Pre-fix, saveSessionStoreUnlocked issued two back-to-back cleanupArchivedSessionTranscripts calls (deleted then reset), each doing its own readdir.
  • removals correct from that single listing: old deleted (9d > 7d) and old reset (5d > 3d, distinct threshold) removed; fresh deleted/reset, bak, and the live transcript all kept.

Checks (Testbox tbx_01kv5g2tdbg69pc5c7r2h1py5r, pnpm check:changed, exit 0): tsgo typecheck incl. test types, oxlint 0/0 (core + extensions + scripts), 0 import cycles. PR CI green.

Not run: live NFS re-trace (as noted in the description) — the readdir-count proof is the in-process equivalent of the captured two-scans-per-save signature.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 16, 2026
@amknight
perf(sessions): share one enumeration across archive retention sweeps (…
35551f5 
…openclaw#91957)

Co-authored-by: Alex Knight <[email protected]>
crh-code pushed a commit to crh-code/openclaw that referenced this pull request Jun 18, 2026
@amknight @crh-code
perf(sessions): share one enumeration across archive retention sweeps (…
0424a5a 
…openclaw#91957)

Co-authored-by: Alex Knight <[email protected]>
@clawsweeper clawsweeper Bot mentioned this pull request Jun 26, 2026
Open
@clawsweeper clawsweeper Bot mentioned this pull request Jul 5, 2026
Closed
badgerbees pushed a commit to badgerbees/openclaw that referenced this pull request Jul 8, 2026
@amknight @badgerbees
perf(sessions): share one enumeration across archive retention sweeps (…
65197e8 
…openclaw#91957)

Co-authored-by: Alex Knight <[email protected]>
@clawsweeper clawsweeper Bot mentioned this pull request Jul 12, 2026
Merged
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

gateway Gateway runtime maintainer Maintainer-authored PR 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.

1 participant

@amknight