fix(session): isolate managed cold-history sidecar cache - #4183
Conversation
Managed cold-history state created a second i-* resident-cache instance, which made one manager appear to own two active resident stores. Give the sidecar a deterministic per-session namespace and retain lease-aware cleanup without changing the canonical cache. Lore-id: dev-resident-sidecar-namespace Constraint: managed sidecars must not share the resident-cache instance namespace Constraint: pre-mutation header append failures retain their original error code Confidence: high Scope-risk: session persistence lifecycle Reversibility: revert-safe Tested: session-memory integration, resident-cache GC, title-source persistence, SDK startup suites, coding-agent check
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5695a62c8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const candidate = path.join(root, instanceName); | ||
| fs.mkdirSync(candidate, { mode: BLOB_DIR_MODE }); | ||
| instanceDir = candidate; |
There was a problem hiding this comment.
When a process crashes after creating this deterministic s-<hash> directory, reopening the same managed session reaches mkdirSync and fails with EEXIST; the sidecar-root sweep is scheduled only after successful creation, while the explicit sweep in SessionManager targets the legacy resident root. A valid dead-owner lease therefore remains unusable until some different session happens to trigger a sidecar-root sweep, and a crash before owner.json is completed leaves debris that the sweep cannot recognize at all, permanently forcing this session onto the eager fallback. Reconcile/reap a stale matching lease before treating the collision as active.
Useful? React with 👍 / 👎.
| getResidentCacheRootDir(this.#residentCacheProfileAgentDir()), | ||
| // Sweep abandoned pre-namespace sidecars from the resident root. The sweep | ||
| // only reaps stale owner leases, so the canonical resident store is untouched. | ||
| void sweepResidentCacheRoot(getResidentCacheRootDir(this.#residentCacheProfileAgentDir())); |
There was a problem hiding this comment.
If this best-effort sweep encounters an error outside the explicitly allowlisted filesystem failures, sweepResidentCacheRoot rejects, but this newly added fire-and-forget call has no rejection handler. That produces an unhandled rejection during managed sidecar initialization and can terminate runtimes configured to treat unhandled rejections as fatal; use the existing scheduled-sweep wrapper pattern or attach a logging .catch(...) here.
Useful? React with 👍 / 👎.
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — crash-stale deterministic sidecar blocks session reopen (lifecycle regression)
Verdict: REQUEST_CHANGES
Exact head: 5695a62c8e9159b980bb8142efcea733f691520b (base 7858b0ff63db6a3fc0cce93639a8d329ca474b2c)
Scope reviewed: sidecar-cache root isolation, deterministic s-<session-hash> leases, lifecycle disposal, bounded GC, legacy sweep, header error precedence.
Claimed suites re-ran at exact head and pass: session-memory-integration + resident-cache-gc + title-source-persistence = 94 pass; sdk-host-wiring + sdk-broker-lifecycle-e2e + sdk-session-readiness-lifecycle = 165 pass. Exact-head CI terminal green (26 success, 0 failed, 0 pending). The blocker below is a lifecycle gap the current tests do not cover: they only exercise clean close→reopen, never a crash artifact.
Blocker: a crashed session's stale s-* sidecar makes the first reopen fail hard
The deterministic instance open in openVerifiedCacheInstanceDir (packages/coding-agent/src/session/blob-store.ts:594-652) does an unconditional fs.mkdirSync(candidate, { mode: BLOB_DIR_MODE }) (line 611). If the s-<sha256(sessionFile).slice(0,32)> dir already exists — which is exactly what a crash leaves behind (owner token with a dead PID, plus all .session-memory.spill.* contents) — the mkdir throws EEXIST and the whole open fails with Resident cache trust validation failed (instance_create_failed): <agent-dir>/sidecar-cache. There is no stale-lease adoption, no reap-and-retry, and no fallback.
The failure path schedules no sidecar-root sweep: scheduleResidentCacheRootSweep(root) runs only after a successful creation (blob-store.ts:629), and the acquisition path #managedSidecarRoot sweeps only the resident root (session-manager.ts:9970, void sweepResidentCacheRoot(getResidentCacheRootDir(...))). So after a crash, the first reopen of that same managed session throws, and recovery depends on opening a different session first (which schedules the async sidecar-root sweep) — and a retry racing that async sweep can fail again.
Reproduction (proven at exact head, Linux, sessionMemoryMode: "enabled"):
- Build a managed destination (
ManagedSessionDescendantStore+nestedManagedDestination) and a session withsetSessionMemoryMode("enabled"), an assistant message, 200 user messages, thenappendCompactionso cold retirement is active (coldRetirementActive === true). Thesidecar-cache/s-<hash>dir is now resident. - Simulate a crash: rewrite the dir's
owner.jsonto a dead PID (the exact on-disk state a killed process leaves;dispose()never ran). await SessionManager.openNestedManaged(sessionFile, destination, nestedStore, new FileSessionStorage(), cwd, "enabled")→ throwsResident cache trust validation failed (instance_create_failed): .../sidecar-cache. Stack:openVerifiedCacheInstanceDir(blob-store.ts:615) ←#managedSidecarRoot(session-manager.ts:9972) ←#resetSidecarRuntime(10231) ←#tryInitSessionFileFromSidecar(7105).#resetSidecarRuntime()at 7105 is outside the try block, so the trust error propagates and fails the open.- The stale dir remains; the failed open scheduled no sidecar-root sweep.
Also reproduced: opening the same session file concurrently from a second SessionManager in one process fails identically (instance_create_failed) while the first holder is live — the deterministic name converts concurrent same-file opens from "each gets its own cache dir" (the #4151 mkdtemp behavior) into a hard failure for the second opener.
Why this is a regression, not intended behavior: under #4151 the sidecar was a random mkdtemp i-* dir, so a stale/crashed sidecar could never collide with the next open; the new deterministic s-<hash> name makes the crashed dir a permanent (until swept) EEXIST barrier for that exact session. The PR's own unit test (resident-cache-gc.test.ts) encodes the hard throw as intended collision behavior, but it never covers the crash→reopen lifecycle.
Required fix
- On
EEXISTof a deterministic sidecar dir, read its owner lease: if stale (dead PID / PID-reused), reap it (or adopt-and-rebuild) and retry; if live, degrade instead of hard-failing the open (or coordinate). At minimum, schedule/await a sidecar-root sweep on the acquisition and failure paths (not only the resident root at session-manager.ts:9970), with a.catch()on the fire-and-forget call (the existingscheduleResidentCacheRootSweepwrapper already does this; the new call site does not).
Required regression tests
- Crash-stale sidecar (dead owner + spill contents) → first reopen of the same managed session succeeds (or rebuilds the sidecar), with
enabledmode. - Default
shadow-mode reopen with the same stale artifact: the shared#managedSidecarRootacquisition is reached from both modes' sidecar build paths, so this must be covered separately (and the sidecar build failure must degrade to the eager path, not fail the open). - Concurrent open of the same session file by two managers/processes: second opener must not hard-fail.
Header-error precedence (header_patch_write_failed → plain error in managedAppendFailure, managed-session-storage.ts:103-111) is consistent with the fault-injection contract today: the code is only produced pre-mutation by the test injector, so "definitely not committed" is truthful. Flag for the record: if a future native append ever emits this code after committing, this classification would lie; keep it strictly pre-mutation.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
No source mutation, push, merge, release.
# Conflicts: # packages/coding-agent/src/session/internal/managed-session-storage.ts
A process crash leaves the deterministic managed sidecar directory behind.\nReap only a verified stale owner lease before retrying the same directory, so\nmanaged cold-history sessions can reopen without accepting active sidecars.\n\nLore-id: 4183-stale-sidecar-recovery\nConfidence: high\nScope-risk: narrow\nReversibility: simple-revert\nTested: resident-cache GC, managed cold-history reopen, title-source persistence, coding-agent check
|
Repaired the crash-stale deterministic managed-sidecar reopen blocker at exact head
Verified: @codex review — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ab9313cd6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /** Get the managed cold-history sidecar cache root for a profile agent directory. */ | ||
| export function getSidecarCacheRootDir(profileAgentDir: string): string { | ||
| return dirs.agentSubdir(profileAgentDir, "sidecar-cache", "cache"); |
There was a problem hiding this comment.
This adds an exported utility and changes coding-agent session behavior, but neither packages/utils/CHANGELOG.md nor packages/coding-agent/CHANGELOG.md is updated. Add entries under each package's ## [Unreleased] section so the cache-layout and crash-recovery changes are included in release notes.
AGENTS.md reference: AGENTS.md:L188-L188
Useful? React with 👍 / 👎.
| fs.writeFileSync( | ||
| path.join(staleSidecar, "owner.json"), | ||
| JSON.stringify({ pid: 2_147_483_647, startTimeMs: 0, nonce: "crashed-sidecar", createdAt: 0 }), | ||
| { mode: 0o600 }, | ||
| ); |
There was a problem hiding this comment.
The new crash fixture uses fs.writeFileSync here and for the spill debris, then later reads the lease with fs.readFileSync, despite the repository contract requiring Bun.write() and Bun.file() for file reads and writes. Since this test is already async, await the Bun APIs and retain chmodSync only for the metadata operation.
AGENTS.md reference: AGENTS.md:L126-L126
Useful? React with 👍 / 👎.
|
MERGE_READY — exact head No merge was performed from this repair lane. — |
1 similar comment
|
MERGE_READY — exact head No merge was performed from this repair lane. — |
1 participant
Summary
sidecar-cacheroot with deterministics-<session-hash>leasesi-*debris from the resident rootheader_patch_write_failedrather than converting it into an uncertain committed append resultRegression coverage
i-*instance and one sidecars-*instanceValidation
bun test packages/coding-agent/test/session/session-memory-integration.test.ts packages/coding-agent/test/session/resident-cache-gc.test.ts packages/coding-agent/test/session-manager/title-source-persistence.test.ts— 94 passedbun test packages/coding-agent/test/sdk-host-wiring.test.ts packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts packages/coding-agent/test/sdk-session-readiness-lifecycle.test.ts— 165 passedbun --cwd=packages/coding-agent run check— passedAffected: #4098, #4108, #4153, #4173. Prior dev evidence: CI job 93471033802.
Signed public verdict: GJC — repair verified locally; ready for dev CI.