Skip to content

Cache subagent read snapshots on gateway hot paths#88166

Closed
efpiva wants to merge 1 commit into
openclaw:mainfrom
efpiva:edpiva/codeclaw-pr-subagent-read-snapshot
Closed

Cache subagent read snapshots on gateway hot paths#88166
efpiva wants to merge 1 commit into
openclaw:mainfrom
efpiva:edpiva/codeclaw-pr-subagent-read-snapshot

Conversation

@efpiva

@efpiva efpiva commented May 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Cache immutable subagent registry read snapshots by disk signature and in-memory version so hot read paths stop rebuilding the same merged registry state repeatedly.
  • Reuse operation-scoped subagent read indexes/session row context for sessions.list, sessions.get, and gateway row loading while preserving existing child-session filtering semantics.
  • Keep single-row session loads on the cheap candidate-cache path for small child sets and invalidate in-memory snapshots before lifecycle completion awaits can expose stale terminal state.

Verification

  • node scripts/run-vitest.mjs src/gateway/session-utils.perf.test.ts src/gateway/session-utils.single-row-cache.test.ts src/gateway/session-utils.subagent.test.ts src/agents/subagent-registry-read-context.test.ts src/agents/subagent-registry.persistence.test.ts
    • Passed before the Codex-review fix: 12 files / 156 tests.
  • node scripts/run-vitest.mjs src/agents/subagent-registry-lifecycle.test.ts src/agents/subagent-registry.persistence.test.ts src/agents/subagent-registry-read-context.test.ts src/gateway/session-utils.single-row-cache.test.ts src/gateway/session-utils.perf.test.ts src/gateway/session-utils.subagent.test.ts
    • Passed after the Codex-review fix: 14 files / 214 tests.
  • pnpm check:changed
    • Passed after rebase and after the Codex-review fix.
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main --prompt-file /tmp/openclaw-codeclaw-pr-review-notes.md
    • Initial Codex review found a stale in-memory snapshot window around in-place lifecycle mutations before awaited completion capture.
    • Fixed by marking in-memory snapshots dirty before the await and adding regression coverage.
    • Final Codex review after latest origin/main fetch: clean, no accepted/actionable findings.

Real behavior proof

Behavior addressed: Repeated gateway/session hot-path reads rebuilt subagent registry snapshots, cloned persisted run maps, and rebuilt full subagent read indexes; the patch caches immutable read snapshots and shares row-scoped read context while preserving subagent/session row behavior.

Real environment tested: CodeClaw codeclaw-triage Docker deployment with the triage named volumes preserved. Evidence used a pinned comparison base plus identical gated hotpath instrumentation for the before and after builds.

Exact steps or command run after this patch: Built OpenClaw tarballs for before/after evidence branches, rebuilt and force-recreated codeclaw-triage for each build, ran a 10-minute warmup, enabled gated hotpath profiling for a 1-hour measurement window, then disabled profiling. The patched container was left running afterward for continued validation.

Evidence after fix: Sanitized before/after evidence showed:

  • subagent.registry.readIndex total profiled time: 103.488ms before -> 45.641ms after (-55.9%).
  • gateway.session.childIndex total profiled time: 178.942ms before -> 82.271ms after (-54.0%).
  • subagent.registry.cloneMap total profiled time: 3682.616ms before -> 2253.583ms after (-38.8%).
  • No OOM, fatal-error, uncaught-exception, or gateway-failed log matches in either 1-hour measurement window.
  • After-window cgroup memory remained stable: 2453.1 MiB -> 2454.3 MiB; primary gateway RSS moved 712.3 MiB -> 601.5 MiB.

Observed result after fix: The patched CodeClaw container stayed running with restart count 0 and the patched OpenClaw build installed. Profiling was disabled after measurement so the container could continue running without accumulating profile logs.

What was not tested: Full CI was not run locally. Heap snapshots and CPU profiles were intentionally not collected. The 1-hour CodeClaw evidence was captured before the final Codex-review stale-cache fix; that follow-up fix only invalidates in-memory snapshots before an await and was covered by focused tests plus pnpm check:changed, not by rerunning another 1-hour CodeClaw evidence window.

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime agents Agent runtime and tooling size: XL proof: supplied External PR includes structured after-fix real behavior proof. labels May 29, 2026
@clawsweeper

clawsweeper Bot commented May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed May 29, 2026, 7:59 PM ET / 23:59 UTC.

Summary
The PR caches immutable subagent registry read snapshots and operation-scoped gateway session row contexts for subagent-heavy sessions.list, sessions.get, and row-loading paths, with focused regression and performance coverage.

PR surface: Source +358, Tests +681. Total +1039 across 15 files.

Reproducibility: Do we have a high-confidence way to reproduce the issue? Partly: the PR body gives a concrete CodeClaw Docker profiling path and before/after metrics, and the source path is clear, but I did not rerun the live profiler during this read-only review.

Review metrics: 2 noteworthy metrics.

  • Snapshot invalidation signals: 2 signals: disk signature and memory version. Session-state freshness now depends on both persisted-registry and in-memory mutation invalidation matching the real lifecycle paths.
  • Final-head proof split: 1-hour live measurement plus focused tests after final tweak. Maintainers should distinguish the live performance proof from the later stale-cache invalidation regression coverage.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
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] Caching immutable subagent/session read state can surface stale child-session or terminal-run state if any in-place subagent run mutation path misses a memory-version bump.
  • [P1] The one-hour CodeClaw performance window predates the final dirty-before-await invalidation tweak; that last tweak is covered by focused tests and changed checks rather than a second live profiler run.

Maintainer options:

  1. Land after invalidation review (recommended)
    A maintainer can accept the session-state risk if CI is green and the version bumps around set/delete/clear, persist, orphan recovery, and lifecycle completion are judged complete.
  2. Request final-head live proof
    Maintainers can ask for another CodeClaw profiler/log window on the exact final head if they want live proof after the dirty-before-await fix.
  3. Pause for a missed mutation path
    If review finds any in-place mutation path without memory-version invalidation, pause for a focused fix and regression test before merge.

Next step before merge

  • No automated repair is identified; maintainers should review the cache invalidation risk and required CI before deciding whether to merge.

Security
Cleared: No concrete security or supply-chain concern was found; the diff touches TypeScript runtime/tests only and does not change workflows, dependencies, lockfiles, permissions, or secret handling.

Review details

Best possible solution:

Land this cache after maintainer review accepts the invalidation coverage and required CI, preserving the focused snapshot, lifecycle, and gateway row-context tests.

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

Do we have a high-confidence way to reproduce the issue? Partly: the PR body gives a concrete CodeClaw Docker profiling path and before/after metrics, and the source path is clear, but I did not rerun the live profiler during this read-only review.

Is this the best way to solve the issue?

Is this the best way to solve the issue? Yes, with maintainer review: the implementation moves repeated gateway/session reads to operation-scoped immutable snapshots rather than adding scattered per-call caches, but the invalidation paths deserve human scrutiny before merge.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority gateway/agents performance improvement with limited surface but meaningful cache-invalidation review risk.
  • merge-risk: 🚨 session-state: The PR caches subagent registry/session row state, so a missed invalidation could show stale child-session or terminal-run state after merge.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix CodeClaw Docker profiling and stability evidence for the central cache behavior; the final stale-cache tweak was validated by focused tests rather than another live window.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix CodeClaw Docker profiling and stability evidence for the central cache behavior; the final stale-cache tweak was validated by focused tests rather than another live window.
Evidence reviewed

PR surface:

Source +358, Tests +681. Total +1039 across 15 files.

View PR surface stats
Area Files Added Removed Net
Source 10 465 107 +358
Tests 5 683 2 +681
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 15 1148 109 +1039

What I checked:

  • Repository hot-path policy applied: Root policy prefers carrying prepared hot-path facts forward and using lifecycle-owned caches with tests, which is directly relevant to this gateway/subagent registry cache review. (AGENTS.md:76, 582aa1ceb2b1)
  • Scoped Gateway policy applied: The Gateway scoped guide flags gateway hot paths and test performance as a specific ownership concern for this PR surface. (src/gateway/AGENTS.md:1, 582aa1ceb2b1)
  • Branch is not implemented on main: The PR head is not an ancestor of origin/main, so the requested cache work is still an open PR rather than already implemented on current main. (cee38220f4f2)
  • Registry snapshot cache implementation: The branch adds getSubagentRegistryReadSnapshot() keyed by persisted disk signature and in-memory registry version, then clones and freezes merged disk and memory run records for read use. (src/agents/subagent-registry-state.ts:70, cee38220f4f2)
  • Gateway row context reuse: The branch builds one subagent read index per session-list row context and shares it through gateway row construction. (src/gateway/session-utils.ts:676, cee38220f4f2)
  • Lifecycle invalidation safeguard: The branch marks in-memory snapshots dirty before awaiting terminal reply capture, addressing the stale-cache window called out by the earlier review. (src/agents/subagent-registry-lifecycle.ts:1107, cee38220f4f2)

Likely related people:

  • Peter Steinberger: Local current-main log/blame for the gateway session row and subagent registry surfaces points to commit 8f2e520 across the central files touched by this PR. (role: recent area contributor; confidence: medium; commits: 8f2e520abb50; files: src/gateway/session-utils.ts, src/agents/subagent-registry-state.ts, src/agents/subagent-registry.store.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 proof: sufficient ClawSweeper judged the real behavior proof convincing. 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. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels May 29, 2026
@galiniliev galiniliev self-assigned this May 30, 2026
galiniliev
galiniliev reviewed May 30, 2026
View reviewed changes

@galiniliev galiniliev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR caches immutable subagent registry read snapshots and reuses operation-scoped gateway session row context so sessions.list, sessions.get, and row loading stop rebuilding the same subagent indexes on hot paths. It matters because the touched surface is session-state sensitive: missed invalidation could show stale child-session or terminal-run state.

LOC: +1148/-109 (15 files)
By: Eduardo Piva (@efpiva, acct 2014-01-13) | OpenClaw: 22 PRs, 1 issue, 12 commits/12mo | GitHub: 14 commits, 22 PRs, 1 issue, 0 reviews
Assignment: assigned to @galiniliev at 2026-05-30 05:25 UTC; fresh at review time.

Findings: no code findings.

One merge blocker remains outside the patch review: live CI is UNSTABLE because checks-node-core-fast failed with timeouts in src/security/audit-sandbox-browser.test.ts:79 and src/security/audit-sandbox-browser.test.ts:105. That file is outside the touched surface, so I would rerun or otherwise clear that check before landing, but I do not see evidence that this PR caused it.

Review notes:

  • The cache shape looks consistent with the hot-path policy: disk snapshots are keyed by registry file signature, memory snapshots by subagentRegistryMemoryVersion, and the merged read snapshot is frozen before reuse (src/agents/subagent-registry-state.ts:70, src/agents/subagent-registry.store.ts:309).
  • In-place mutation invalidation is covered by persistSubagentRuns() version bumps and the explicit dirty-before-await lifecycle path (src/agents/subagent-registry.ts:264, src/agents/subagent-registry-lifecycle.ts:1105).
  • The gateway side keeps the optimization operation-scoped: list rows share one SessionListRowContext, single-row loads only build the full subagent read index past the child-candidate threshold, and fallback paths preserve existing child-session semantics (src/gateway/session-utils.ts:676, src/gateway/session-utils.ts:711, src/gateway/session-utils.ts:2279, src/gateway/session-utils.ts:2560).

Best-fix verdict: this is the right fix shape for the reported hot path. It moves prepared read facts earlier and reuses immutable snapshots instead of adding scattered per-call caches.

Provenance: N/A for a performance/cache improvement; recent related main history includes prior gateway/subagent read-cache work (fix(gateway): cache single session row child indexes, fix(agents): reuse cached subagent registry reads), but I did not identify a discrete regression-introducing PR.

Proof checked: PR body's focused Vitest/check evidence, live CI status, ClawSweeper review, scoped AGENTS.md for src/agents and src/gateway, gitcrawl thread lookup, git diff --check origin/main...origin/pr/88166 passed. I did not rerun the Vitest suite locally.

@steipete steipete mentioned this pull request May 30, 2026
Merged
@steipete

steipete commented May 30, 2026

Copy link
Copy Markdown
Contributor

Closing this as superseded by #88260, which landed in 5374c7a.

That replacement moves the subagent registry off the JSON/disk-signature path and into SQLite-backed state, so caching legacy disk snapshots is no longer the direction we want to maintain. If we revisit this hot path, the follow-up should optimize the SQLite/per-agent-state query path rather than add another file-registry cache layer.

Thanks @efpiva for the detailed profiling and tests here; the performance evidence was useful context for the replacement work.

@steipete steipete closed this May 30, 2026
@anyech anyech mentioned this pull request Jun 12, 2026
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@galiniliev galiniliev galiniliev left review comments

Assignees

@galiniliev galiniliev

Labels

agents Agent runtime and tooling gateway Gateway runtime merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XL 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.

3 participants

@efpiva @steipete @galiniliev