Repository navigation
fix(native-chat): drain the reaped child's journal writes before an attach reports - #17160
brennanb2025 wants to merge 1 commit into
Conversation
Greptile SummaryThe PR adds the missing exit-side journal barrier so attach and native-handoff results are emitted only after deferred writes from a reaped provider child have landed.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts | Makes post-attachment publication awaitable so hydration cannot overtake deferred journal writes. |
| src/main/native-chat/agent-session-wire/structured-agent-session-attach-orchestration.ts | Drains buffered writes after rebinding the event sink and before attach completion. |
| src/main/native-chat/agent-session-wire/structured-agent-session-host-handoff.ts | Applies the same drain-before-snapshot ordering to native handoff recovery. |
| src/main/native-chat/agent-session-wire/structured-agent-session-attach-drain.test.ts | Adds a gated regression test proving attach waits for a buffered reap-time write. |
| src/main/runtime/structured-agent-session-integration.test.ts | Verifies takeover hydration no longer retains the superseded child’s active turn-lifecycle row. |
Sequence Diagram
sequenceDiagram
participant A as Attach or handoff
participant S as Deferred event sink
participant P as Superseded provider child
participant J as Replacement journal
participant C as Client or subscriber
A->>S: unbind()
A->>P: acquire replacement and reap child
P->>S: buffer final journal writes
A->>S: bind(J)
S->>J: replay buffered writes
A->>S: await drained()
S-->>A: writes settled
A->>C: hydration page or snapshot
Reviews (2): Last reviewed commit: "fix(native-chat): drain the reaped child..." | Re-trigger Greptile
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe attach flow now supports asynchronous Merge Risk: 🔵 Low · up to This change prevents stale turn-status rows from being reported during session attach and handoff, improving session state consistency. It is mergeable with explicit owner awareness that journal-write failures may still leave stale lifecycle state and should have a defined recovery policy. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides strong technical detail about the failure, mechanism, fix, and verification. However, it omits required template sections, including the always-required Linked Issue, Visual Proof or an explicit N/A explanation, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist. Resolution Add the missing template sections. Provide a linked issue after “Fixes #”. For this non-UI behavior change, include “N/A” with a brief reason under Visual Proof. Complete or explicitly mark the remaining sections and checklist items, including testing and cross-platform considerations.
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ttach reports A takeover reaps the superseded provider child from inside `adapter.acquire`, and the reap journals its end-of-turn bookkeeping — for Codex, the tombstone that ends the `turn-lifecycle` row. The sink is unbound by then, so those writes only buffer; `bind()` re-queues them onto the sink's async chain and nothing awaited it. The attach then read its hydration page, published the new fence and released the session lock while the tombstone was still in flight, so `agentSession.ensure` could hand back — and `agentSession.history` could serve — a page still showing the turn that same attach had just ended. The attach already fences the way in (`unbind()` + `await drained()` before the journal opens); this adds the missing fence on the way out, in both the attach and handoff-restart paths.
3af3f64 to
47c7664
Compare
|
Closing as obsolete: main no longer has the code this PR changes, or already behaves as it intended. Main already drains the event sink before an attach publishes: structured-agent-session-attach-orchestration.ts:305 awaits eventSink.drained() in beforePublish (from #16729/#17707). The handoff file it patches was deleted by #22783 (Stale-PR cleanup 2026-10-20, approved by Brennan. Reopen if something here is still missing on main.) |
The failure
structured-agent-session-integration.test.tsintermittently reported seven journal rows wheresix were expected, on
test / tests node 24 2/8:The extra element is the Codex turn-lifecycle row —
legacy:codex:<session>:turn-lifecycle:turn-1,body text
"Codex is working…"— still present in history after the takeover that should have ended it.Mechanism
turn/startedjournals aturn-lifecyclestatus row; the matching tombstone is written when theprovider child is closed. On a takeover (
agentSession.ensure), that close happens insideadapter.acquire— at which point the attach has already calledeventSink.unbind(), so thetombstone is only buffered on the deferred sink.
eventSink.bind()inonAttachedre-queues the buffered writes onto the sink's async chain, butnothing awaited that chain. The attach then read its hydration page, published the new fence and
released the session lock while the tombstone was still in flight. Whether a reader saw the stale
row came down to an unrelated race between the tombstone's journal write and the attach's own
store.recordOperationOutcomewrite.The attach already establishes the symmetric barrier on the way in
(
beforeJournalOpen: unbind() + await drained()). The barrier on the way out was missing.This is not cosmetic. The stale row is what
activeStructuredAgentSessionTurnId()reads, so asession whose child was just reaped could hydrate as "Codex is working…" — and in the main process
isTurnActiveuses the same signal to gate eviction.Fix
Await the rebound sink before the attach reports:
performAttachnow awaitsonAttached, and theorchestration drains the sink after
bind(). The same rule is applied to the handoff restart path,which binds and then immediately fans out a snapshot.
Not the PR it failed on
All the implicated files are byte-identical to
main, and the same failure occurred independently on#17115 (an unrelated Windows PowerShell polling PR). Rate across enumerated attempts: 2 / 85.
Verification
Ablation, 12 runs per arm (local load 10–15):
await onAttachedawait eventSink.drained()The failure was also reproduced byte-for-byte against the CI diff by delaying the
tombstone 150ms on pristine
main— extra"status"at index 1, rowlegacy:codex:<session>:turn-lifecycle%3Aturn-1, text"Codex is working…"—and that same probe passes with the fix.
Gates:
pnpm tc(all projects) clean;oxlint, both--deny-warningspluginaudits, the max-lines ratchet and
check-changed-code-quality.mjsall clean;git diff --checkclean; formatted withnpx oxfmt --writeon changed paths only.