[Juliusolsson05]

Item 4 of #910 — the deferred medium/low findings from the OpenCode Terminal round-2 verification. Item 1 is the package half, opened as Juliusolsson05/opencode-terminal-headless#5; this branch will bump the submodule once that lands. Items 2 and 3 are #1081.

The bug

openOpencodeStore keys its shared connection on path plus device/inode, precisely so that an opencode.db replaced at the same path opens a new generation rather than keeping a lease on the unlinked file (review R1-F10). The host never reached that mechanism: createOpencodeDatabase resolved the path once and cached the handle for the process lifetime.

So after a replacement — a reinstall, a restore from backup, a rebuilt file — every main-process reader kept reading the deleted inode: history for parked and reloaded panes, the agent-transcript MCP tools, Agent Management's transcript locator. New OpenCode sessions simply never appeared until Agent Code restarted.

The fix

store() re-stats the file, at most once per REVALIDATE_INTERVAL_MS (5 s), and reopens when the identity changed. Bounded because store() is on the path of every history chunk and every transcript tool call: a statSync is microseconds, but per call it is still a syscall in a hot loop for an event that needs someone to replace a file by hand.

Two rules the tests pin, both deliberate:

• A stat that fails keeps the handle. A missing or momentarily unreadable path is not evidence of a replacement, and swapping on it trades a handle that still reads for an open that cannot succeed.
• The superseded handle is released on a timer, not inline. Callers borrow the shared handle and read from it synchronously after await store(), and a promise continuation is a microtask — all queued microtasks drain before the next timer callback, so a continuation that is already queued cannot have the handle pulled out from under it (the package makes a released handle throw).

What this deliberately does not cover

The database moving. That answer comes from resolveOpencodeDbPath, which memoises for the process lifetime on purpose — one call costs a Bun process start, paid before every pane's TUI can spawn — and its own invalidation policy already says relocation is picked up on the host's next start. Re-resolving here would pay that cost on every history call to catch a reinstall. The comment in opencodeDatabase.ts states the split.

Tests

opencodeDatabase.system.test.ts builds two real SQLite databases from recorded sessions (createProjectionDatabase) and renames one over the other — the verifier's V2 reproduction. It carries a positive control: a store acquired directly from the package already sees the replacement, so whatever the facade still misses is the facade's doing and not the package's generation selection. A second test covers the stat-failure rule.

Mutation results

mutation	result
never revalidate (the behaviour before this fix)	1 failed
revalidate on every call (drop the window)	1 failed
swap on a stat failure too	1 failed
record a constant instead of the real identity	1 failed
leave pending set across a swap	1 failed

No survivors.

npx tsc -b clean on Node 24. --project unit src/providers/opencode/ src/main/agentTranscripts/: 67 passed.

🤖 Generated with Claude Code

[Juliusolsson05]

Adversarial review returned REQUEST CHANGES with a blocker. All five findings addressed in 6c5873a4.

1. The macrotask release is a rationalisation (HIGH) — valid, and the whole design changed

You are right, and the reproduction is the kind that settles it. My comment asserted "nothing in the app holds the handle across a further await" and AgentTranscriptReader.transcriptRecords does precisely that — it yields with setImmediate between pages by design, with its own WHY comment three lines up that I did not read carefully enough. And the phase argument is not merely unproven, it is backwards: a timer callback runs in the timers phase, ahead of the check phase where setImmediate resumes, so the release reliably won.

The fix is the second option you named: the facade counts holders.

• lease() returns a borrow valid across awaits; a handle retired while leases are open is closed by the last release(), never before.
• store() keeps its old meaning — valid until the caller's next await — because every other consumer reads synchronously and that is exactly what it wants. The type says so now.
• prepareTranscript takes a lease and every entry point drops it in a finally (not per-return, so a return added later cannot skip it).

Two facade-level tests pin the invariant directly, because the phase race is not deterministic across machines — I could not make it lose on mine, which is itself the argument for not testing the race: with no lease the superseded handle is closed synchronously; with a lease it still reads until the lease drops, and the swap still happens. The end-to-end proof is a real walk over a 60-message session with the database replaced inside its first setImmediate gap; it fails under an inline release that ignores leases.

Counting holders also disposes of your M15 (.unref?.()) — there is no timer left.

2. Two permanent wedges (MEDIUM) — both valid, both fixed

2a. Exactly right, and the worse half is that it re-creates the bug this file exists to fix, silently. The openedIdentity === null early return is gone; the first successful stat after an unknown identity counts as a change, costing one spurious reopen that then records a real one. Your M13 was a repair, and it is now the code.

2b. Also right, and the comment was plainly false — I wrote "the identity the package validated its connection against" while taking a third independent stat. It is observed before the open now, so the same race records a stale identity, which the next interval corrects. Wrong in the direction that self-heals.

Staging 2b took a specific fixture: the seam opens the handle on the old file and renames the replacement in immediately after, which is the only arrangement where stat-before and stat-after disagree. My first attempt renamed before the open and did not discriminate at all.

3. The release decision was pinned by nothing (MEDIUM) — valid

Your table is the useful half of this review: "no survivors" was true only of the five mutations I chose, which is not a mutation result, it is a restatement of the tests. Twelve now, no survivors:

mutation	result
macrotask release (the shipped version)	4 failed
inline release ignoring leases	2 failed
never release (your M16)	3 failed
retired handle never closed on the last drop	1 failed
re-add the 2a early return (your M13)	1 failed
stat after the open (2b)	1 failed
surface a reopen failure instead of keeping the handle (4)	1 failed
retire the old handle before the reopen	2 failed
never revalidate	5 failed
swap on a stat failure too (your M5)	1 failed
lease release not idempotent	1 failed
revalidate on every call	1 failed

Your M17 (checkedAt after the reopen) and M14 (release() clearing the path/identity) I agree are equivalent and left alone; M2 (statement order) likewise.

On "swap on a stat failure": it now takes an assertion that openStore was not called a second time, because finding 4's fallback rescues the outcome either way — the difference is burning an open attempt every interval for as long as the path stays unreadable. Worth pinning the intent rather than the accident.

4. The changed-identity path broke the PR's own principle (LOW) — valid, fixed

A fair catch. The old handle keeps serving until the new one is in hand; a reopen that throws leaves it in place and the next interval retries. The test builds a new inode that is not yet a database (renamed into place, never written over the inode still in use — my first attempt corrupted the file the handle was reading) and asserts both halves: the reader keeps working, and it self-heals one interval later.

5. Comment inaccuracy in the scope rationale (LOW) — valid, corrected

Re-resolving is free; invalidating the memo is what costs a Bun start. The paragraph says that now, and says what it used to say.

On what held up

The inode check, the positive control, the no-churn assertion, the generation-counter walkthrough, the statSync-follows-symlinks confirmation and the submodule note are all evidence the PR should have carried. Thank you in particular for measuring the connection count across the swap and for noting that the system test ends before the drain — that is why M16 lived, and the new tests assert the close rather than the intent.

---

npx tsc -b clean on Node 24. --project unit --project system src/providers/opencode/ src/main/agentTranscripts/: 219 passed (was 212; +7 regression tests).