fix(session): stop managed sidecar disposal from crashing close

[probepark]

What

SessionManager.close() could crash the whole CLI with an unhandled ResidentCacheTrustError. Two changes:

1. disposeVerifiedResidentCacheInstanceDir() now treats an instance directory that no longer exists as an already-completed disposal (ENOENT short-circuit) instead of raising directory_unverifiable. A present-but-untrusted directory still fails closed.
2. SessionManager.#releaseManagedSidecarCache() clears its sidecar state before disposing and downgrades a disposal failure to a logger.warn, matching the three other resident-store disposal sites in the same file.

Why

Observed crash from the 0.12.21 binary:

[Unhandled Rejection] ResidentCacheTrustError: Resident cache trust validation failed (directory_unverifiable): ~/.gjc/agent/resident-cache/i-ZaHXbi
    at dispose (blob-store)
    at #releaseManagedSidecarCache -> #releaseClosedSessionState -> close (session-manager)

Every other store disposal in session-manager.ts (lines 344, 6708, 6813) is wrapped in try/catch + warn; the managed sidecar cache release at line 9947 was not, so any trust failure during teardown escaped close() as an unhandled rejection and killed the process. On top of that, disposal of a directory that is already gone has nothing left to distrust — treating that race as a trust violation is what turned a benign cleanup race into a fatal crash. The unguarded throw also skipped the state resets on the following lines, stranding a half-released sidecar whose next release would throw again.

Testing

• bun test packages/coding-agent/test/session/managed-sidecar-cache-release.test.ts packages/coding-agent/test/session/resident-cache-gc.test.ts — 13 pass. Reverting only the two source files makes 3 of them fail, reproducing the exact reported directory_unverifiable rejection.
• bun test across the 9 resident-cache/session suites (session-resident-*, session/resident-cache-gc, ultragoal-redteam-resident-cache, session/session-memory-integration) — 89 + 83 pass, 0 fail.
• bun --cwd=packages/coding-agent run check — clean.

New coverage:

• resident-cache-gc: dispose of an already-removed instance dir is a no-op; a present dir that lost owner-only mode still throws ResidentCacheTrustError and is left in place.
• managed-sidecar-cache-release: a managed cold-history session closes cleanly both when its sidecar cache dir vanished and when disposal rejects outright.

GJC verdict

gajae.pr-review-verdict.v1 needs-human sha256:e72cabbcf026ba986b9500a42166f4528697aa73 reviewer:human evidence:local bun test + bun --cwd=packages/coding-agent run check

---

• Target branch dev
• bun check passes
• Tested locally
• CHANGELOG updated (if user-facing)
• Verdict matches exact PR head, not earlier commit

[Yeachan-Heo]

Review of exact head e72cabbcf026ba986b9500a42166f4528697aa73 against 79e4e0a097aeb54cfe94f79b8919af778dbc5c68.

The focused suites and package check are green, the present wrong-mode directory remains fail-closed/unremoved, and SessionManager.close() no longer throws for the covered failure shapes. Two cleanup-safety blockers remain.

HIGH — the ENOENT preflight masks parent-path substitution and permanently drops cleanup authority (packages/coding-agent/src/session/blob-store.ts:390-404, EphemeralBlobStore.dispose() at :1239-1246).

Deterministic exact-head reproduction:

const instanceDir = openVerifiedResidentCacheInstanceDir(cacheRoot);
const store = EphemeralBlobStore.adoptVerifiedDir(instanceDir);
const parkedRoot = `${cacheRoot}-parked`;
renameSync(cacheRoot, parkedRoot);
mkdirSync(replacementRoot, { mode: 0o700 });
symlinkSync(replacementRoot, cacheRoot, "dir");

store.dispose(); // returns success: lstat(instanceDir) sees ENOENT through the substituted parent

unlinkSync(cacheRoot);
renameSync(parkedRoot, cacheRoot);
existsSync(instanceDir); // true: the owned directory was never removed
store.dispose();         // no-op because #disposed was set true
existsSync(instanceDir); // still true

This passed as an adversarial test on the exact head. ENOENT proves only that the current pathname does not resolve; it does not prove that the owned target disappeared. A renamed/replaced parent therefore converts a path-trust failure into successful disposal, deletes the ownership-set entry, and makes the store permanently non-retryable while the original directory remains present.

HIGH — managed-sidecar release clears the only retry handle before disposal and logs the absolute cache path (packages/coding-agent/src/session/session-manager.ts:9942-9961).

Deterministic exact-head reproduction using the PR's managed-sidecar fixture:

const dispose = vi.spyOn(EphemeralBlobStore.prototype, "dispose")
  .mockImplementation(function () {
    throw new ResidentCacheTrustError("directory_untrusted", this.dir);
  });
const warn = vi.spyOn(logger, "warn").mockImplementation(() => {});

await manager.close(); // resolves
// warn metadata.error contains the complete absolute sidecar directory path

dispose.mockRestore();
await manager.close(); // resolves but performs no disposal retry
// the sidecar directory still exists

This also passed as an adversarial exact-head test. Because #managedSidecarCacheStore is cleared before dispose(), a retryable trust/permission failure becomes warning-only with no in-process cleanup authority. Repeated close is idempotent only by forgetting the orphan. The owned-directory set causes current-process sweeps to skip it; cleanup is deferred until a later process can classify the stale owner. The structured warning includes ResidentCacheTrustError.message, which embeds the absolute path.

Required change: distinguish target absence from parent/path substitution using retained/verified parent authority (and cover the substitution/race), preserve a retryable cleanup receipt/handle after failed sidecar disposal without letting close() throw, and log a bounded reason without the absolute path. Add regression coverage for parent substitution, repeated close after failure recovery, and warning payload confidentiality.

Remote state verified before submission: exact head/base, mergeable clean, 20 successful checks, 5 skipped, 0 failed/pending.

Signed-off-by: Bellman (@Yeachan-Heo)
VERDICT: REQUEST_CHANGES

[Yeachan-Heo]

REQUEST_CHANGES — exact head e72cabbcf026ba986b9500a42166f4528697aa73

Formal review 4899986204 found two deterministic cleanup-safety blockers despite green focused tests and CI:

1. An ENOENT preflight can mask parent-path substitution, mark disposal successful, and permanently drop cleanup authority while the originally owned directory remains.
2. Managed-sidecar release clears its retry handle before disposal; a trust/permission failure becomes non-retryable, and the warning can include an absolute cache path.

Required: retain verified parent/target authority across substitution races, preserve a retryable cleanup handle after non-throwing close(), emit bounded path-free diagnostics, and add regression coverage for substitution, retry-after-recovery, and warning confidentiality.

This comment records the signed terminal verdict and required footer for the exact-head review. No source mutation, merge, or release was performed.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]

[probepark]

Both HIGH blockers are addressed at head 66d2169dd. Your two reproductions were exact; each one is now a test.

HIGH 1 — ENOENT preflight masks parent-path substitution

d3eb56fd0 — fix(session): retain resident disposal authority (blob-store.ts, +54/-6)

You were right that the preflight proved the wrong thing. ENOENT establishes only that the current pathname does not resolve; through a substituted parent it was being read as "the owned target is gone", which retired ownership permanently while the original instance survived.

The store now retains a verified parent descriptor captured at adoption and revalidates it across disposal, so a substituted or raced parent fails closed and the store stays retryable instead of being marked disposed. Adoption itself now fails with parent_authority_unavailable rather than proceeding without that authority. Trusting ENOENT alone is recorded as rejected.

Regression coverage in resident-cache-gc.test.ts:

• retains disposal authority when the cache parent pathname is substituted — your park/symlink reproduction
• detects parent substitution racing the target absence check — the race variant

HIGH 2 — release clears the retry handle before disposal, and logs the absolute path

66d2169dd — fix(session): retain failed sidecar cleanups (session-manager.ts, +21/-…)

#releaseManagedSidecarCache() no longer drops the store before disposing it. Failed stores are held in a bounded in-process #managedSidecarCleanupStores retry set and re-attempted on the next release, so a retryable trust/permission failure keeps in-process cleanup authority instead of being forgotten. close() remains best-effort and non-throwing — that constraint is recorded on the commit.

The warning payload no longer carries ResidentCacheTrustError.message. It emits only a fixed reason, validated against /^[a-z0-9_]{1,64}$/ and falling back to cleanup_failed, so no absolute path can reach the log.

Regression coverage in managed-sidecar-cache-release.test.ts asserts all three properties you asked for:

• warning payload is exactly { reason: "directory_untrusted" } and contains none of the sidecar cache directories
• the directories still exist after the failing close — the orphan is not forgotten
• after dispose recovers, a repeated close makes more disposal attempts than the first and the directories are gone — retry-after-recovery actually retries

Verification

• bun test packages/coding-agent/test/session/resident-cache-gc.test.ts
• bun test packages/coding-agent/test/session/managed-sidecar-cache-release.test.ts packages/coding-agent/test/session/session-memory-sidecar.test.ts
• bun --cwd=packages/coding-agent run check
• Exact-head CI at 66d2169dd: 20 successful, 5 skipped, 0 failed.

Re-review requested.

[probepark]

Re-requesting review at rebased head def522ba8c. This was a base-only rebase onto current dev (f4b6811e3); the two reviewed fixes remain unchanged in substance.

• ENOENT/parent substitution authority: now 0fbf64677 (fix(session): retain resident disposal authority), rebased from d3eb56fd0. Covered by resident-cache-gc.test.ts.
• Retry handle and path-free warning: now def522ba8 (fix(session): retain failed sidecar cleanups), rebased from 66d2169dd. Covered by managed-sidecar-cache-release.test.ts and session-manager-resident-cache.test.ts.

Local exact-head verification: 19 pass / 0 fail / 54 assertions across those three suites; bun --cwd=packages/coding-agent run check passed. GitHub currently reports 19 successful, 6 skipped, 0 failed, 0 pending checks, and the PR is MERGEABLE. Please re-review this head so the stale CHANGES_REQUESTED decision against e72cabbcf can be cleared.