Repository navigation
feat(native-chat): release a structured session when its chat closes - #15415
Closed
brennanb2025 wants to merge 28 commits into
Closed
brennanb2025 wants to merge 28 commits into
brennanb2025 wants to merge 28 commits into
Conversation
A codex app-server outlived the chat that owned it for the whole app session. The host tracked live sessions in a map with no delete, and tore them down at three unrelated call sites — app quit, handoff to a TUI, and error cleanup — none of which is "the user closed the chat". Resident children tracked sessions ever opened rather than sessions open now, and each one kept a lease renewer probing process identity every 10 seconds. Teardown is now a named, ordered step list rather than a method body. Adding a resource to release is a list entry, not an edit to control flow, which is the actual fault here: liveness was recorded in one place and cleanup in another, so a new way for a session to end silently skipped cleanup. Steps are named so a half-finished eviction reports which one failed, and a failing step no longer skips the rest — a child that will not stop must not also leave the session wired to a dead sink. Order is the correctness property and is asserted directly: drain what the session already produced before stopping its child, and stop the child before forgetting it. Draining afterwards discards rows the provider already sent; forgetting first strands the process. Closing does NOT end the conversation. The record and journal stay on disk, so the same session can be attached again. Also splits the host, which was at exactly 300 of its 300-line cap. The attach orchestration moves to its own module behind an explicit context type, so the host reads as a coordinator — serialize per session, track in-flight work — rather than an implementation. The host is now 277 lines with real headroom; no cap was raised and no rule disabled. Verified: 1963 tests across native-chat, runtime RPC and the cross-version wire harness. agentSession.close is exercised in all three version skews. Mutations: dropping the child-stop step turns 4 tests red; reordering drain after the stop turns 2 red — an ordering fault the previous imperative shape could not detect.
Three defects in the teardown this PR added, all found in review. ORDER. The sink was torn down before the provider child stopped. Closing a Codex session is not silent: `closeCodexPublishedSession` fires its `ended` event, flushes coalesced text and disposes the translator AFTER the child exits, and those are the rows that clear the running-turn marker. A closed sink drops them, so closing a chat could leave the durable journal claiming the agent is still working — worse than the leak this teardown exists to fix. The child now stops first and the sink outlives it. UNPROVEN EXIT. `closeSession` returns false to mean the child's exit could not be proven, and the adapter deliberately keeps the session indexed so a retry can reach it. Eviction ignored that, forgot the session anyway, and reported success: the child was stranded and the second close was a no-op against an already-forgotten session. Every step now aborts the rest, which is what makes the next close a real retry. CACHED SINK. The runtime caches one sink per session id and hands the same instance to the next attach, so closing without discarding left a reopened chat bound to a permanently closed sink — it accepts every provider event and publishes none. Attach's own failure path already paired close with discard. Tests: the closing-rows and reattach cases are the load-bearing ones. Restoring the old order reddens five, ignoring the false return reddens the retry case, and neutering the discard reddens the reattach case.
Nothing told the host that a chat WANTED a session, and nothing told it when one stopped. Both halves cost real processes: closing a chat left its codex app-server running until the app quit (9 orphans, 420 MB, oldest 21 hours), and every launch resumed a child for every record on disk — `released` with no handoff in flight is the normal end state of a cleanly closed chat, so a healthy profile respawned everything it had ever opened, in parallel, with no client attached. A surface takes a HOLD when it binds and drops it when it goes away: the desktop chat view, the paired mobile session view. Holders are a SET, not a count — every release path can fire twice or not at all, and an integer cannot tell a duplicate release from a lost one. Transport close is the backstop, not the mechanism: a phone that loses signal never sends its release, so the hold registers a cleanup against its connection, and a live subscription holds the session on its own. Eviction runs the existing steps, unreordered, under a per-step deadline: a step that times out aborts the rest, which leaves the session indexed and the child LOADED rather than killed. The clock starts when the last holder leaves and re-arms while a turn is running, so a session is never stopped mid-answer. Teardown now also hands the lease back — a record left claiming a live owner is one nothing can resume. Startup no longer resumes anything. It reconciles, exits recovery, and restores the journal for READING; a provider child exists because a surface asked for the session. Paired builds that predate holds keep working: they do not advertise `agent-session.structured.hold.v1`, and for them a subscription is the resuming kind of hold.
Four conflicts, two of them mine to own:
- Both branches independently grew structured-agent-session-attach-{orchestration,context}.
Kept this branch's pair: it types the subscriber surface structurally instead of
importing the concrete class, so attach stays testable without building a host.
The host keeps serialize + trackAttach around it, because the module deliberately
does neither.
- The readable restorer collided head-on. Adoption added an orphan reap before
restore; this branch had already removed the startup RESUME entirely. Both are
right: reap first, then restore read-only. The resume-era operationId/now went
with the resume.
The wedged-profile migration tests asserted the record ends 'live' with acquire
called once. That was the eager startup spawn this branch removed, so they now
assert what actually matters: the lease ends acquirable — proved by running
evaluateAgentSessionAcquisition, not by reading fields — and a HOLD is what drives
the acquire. The orphan-ordering test keeps its property by taking a hold: stop the
orphan first, then acquire, never the reverse.
structured-agent-session-host.ts hit 313 lines, so the mutation surface (send,
cancel, respondToPrompt, setOption, readOptions and the shared admit-and-run) moves
to its own module. Split, not squeezed.
brennanb2025
force-pushed
the
brennanb2025/structured-session-eviction
branch
from
August 20, 2026 01:21
c83b3b3 to
d98df97
Compare
Three mobile conflicts, all from this stack's own extractions meeting main: - rpc-client.ts: this branch had already moved RpcClient into rpc-client-contract and re-exports it; main still declares it inline. Keeping both produced a duplicate declaration. Dropped the inline copy and ported the one member main had that the contract lacked (getLastInboundAt). - worktree-create-retry(.test).ts: the cutover-error split moved isLogicalClientCutoverError out of stable-logical-rpc-client, and main added two new imports alongside the old path. Both kept, pointed at the new module. - StreamRequest keeps paramsForReconnect from this branch. Root typecheck, mobile tsc, full lint gate, and the mobile suite (3755 tests) green; pnpm-lock.yaml untouched.
|
Too many files changed for review (102 files, 100 file limit). Bypass the limit by tagging |
Contributor
Author
Live Electron validation (structured Codex stack) —
|
| # | Result | What was observed |
|---|---|---|
| 1 Adoption TUI → chat | FAIL | Toast, no structuredSessionId |
| 2 Return to terminal | NOT REACHED | Blocked by 1 |
| 3 Launch-at-chat | FAIL | Codex tab created; never got viewMode=chat / journal |
| 4 Approvals | NOT REACHED | No structured chat; TUI was YOLO |
5 Close chat drops codex app-server |
NOT REACHED | Count stayed 0 — nothing spawned |
| 6 Cold start does not spawn | PASS with caveat | Count 0 after relaunch; no agent-sessions.json existed to resume |
| 7 Win/Linux local resume | CODE ONLY | Host is macOS |
| 8 SSH/WSL stay on bridge | CODE ONLY | Isolated profile had 0 SSH targets; WSL unavailable |
Point 1 — exact failure (this is the finding)
Codex TUI did complete a real turn on the load balancer:
- prompt
Reply with exactly STA15415-MARKER and no other words. - assistant
STA15415-MARKER - thread
01a01cdc-ae4c-7041-85fa-42c733f955a9 agentStatus.state=done,agentType=codex, hook originmain-agent-hooks
Switch to chat:
Could not switch this Codex session to structured chat
The agent terminal could not verify its Codex session.
handoffStatus: none. No structured session was created, so there is no owner/direction/phase/details. agent-sessions.json was never written.
That string is thrown from proveCodexTuiRollout / resolveLiveCodexTuiRollout when the /status probe write() returns false. adoptStructuredAgentSessionTerminal still uses that /status paste. The to-tui rebuild (SessionStart + ORCA_AGENT_SESSION_NONCE) is not on this path.
write is writeAgentSessionProof gated by admitProof. Adoption mints a fresh spawnToken and never binds the PTY / reserved lease first, so admitProof refuses and no /status bytes hit the pane. This is not an auth failure.
Process counts (codex app-server)
| Moment | PIDs |
|---|---|
| Before chat toggle | none |
| After failed adopt | none |
| After launch-at-chat | none |
| After cold relaunch, no chat opened | COUNT=0 |
Point 7 (code)
Adopted-resume is not macOS-only: getAgentLaunchPlatformForWorkspace + resolveLocalWindowsAgentStartupShell + withInlineEnvAssignments emit env …, $env:…;, or set "…" && by dialect. cmd/PowerShell forms are unit-tested only. isRemote: false is hardcoded on this local path (correct for local panes).
Point 8
canUseStructuredNativeChat returns false for non-local hosts and for WSL; adopt also refuses connectionId / wslDistro. Live SSH/WSL Codex panes were not stood up (ssh.listTargets()=[], wsl.isAvailable()=false).
Full writeup: /Users/brennanbenson/orca-qa/pr-15415-validation/REPORT.md
Evidence
Isolated empty profile (renderer 127.0.0.1:5196):
Adoption toast (the fail):
After the completed TUI turn (WebGL pane is black in CDP; buffer in REPORT):
Cold start, no chat opened, codex app-server still 0:
Switching a Codex tab to structured chat failed from every desktop entry point with "The agent terminal could not verify its Codex session." `adoptStructuredAgentSessionTerminal` typed `/status` into the pane through `writeAgentSessionProof`, which `agentSessionPtyWriteGate.admitProof` admits only for a reservation that already names the session, `tui`, and the spawn token. Both the reservation and `bindPty` happened *after* the proof, so the gate saw no binding and no record and refused the first keystroke. Neither proof branch could ever run, which is why an agent mid-conversation failed the same way as a cold one. The reservation and the pane binding now come first, so the probe is authorized as `provingReservation` — the state the gate was written to accept. A proof that does not land hands the reservation back with the durable processless proof and releases the pane, so the next attempt is not refused by the record its predecessor left behind. `proveOwner` gets the reservation's own fence rather than a hardcoded 1, which is what makes that retry land. Launch-at-chat shared the defect: it routes through the same `setTabViewMode` → `setTerminalNativeChatMode` → `agentSession.adoptTerminal` path, now pinned by a test. The new adoption test drives the real write gate over a real record store and a real rollout proof; every earlier test mocked the PTY controller, so `admitProof` never ran and none of them could observe this.
Two adoptions of the same Codex pane destroyed each other. The pane binding was released by pty id alone, gated on a `priorBoundSessionId` sampled before the reservation, and both attempts carry the same sessionId — so the attempt whose reservation had been superseded unbound the pane the winner was proving on, `admitProof` refused the winner's `/status` keystroke, and both failed. The write gate now records which attempt holds a pane, keyed on the spawn token the reservation already mints: a claim supersedes another attempt's but never a settled owner's, the release is a compare-and-clear, and a proven owner settles the binding so no later attempt can take it. No lock is needed; the reservation release beside it was already fence-scoped and unchanged. A failed `releaseAdoptedTuiReservation` stays swallowed — the proof error is the one the user must act on — but is now logged with session, fence and spawn token, because a release that never persisted leaves the lease latched and refuses every later attempt with no record of why. Adoption's no-record branch is deleted. The only caller reserves the lease first and nothing deletes a record, so it minted an owner for a session the store had never heard of in unit tests only; its test now drives the real reserve-then-adopt sequence, and a new one pins the refusal. Launch-at-chat discarded `waitForAgentReady`'s result, so a readiness timeout still flipped the tab to chat and adoption then probed a shell. It stays in terminal view instead: the terminal is usable and the view toggle re-runs adoption whenever the user wants it. Each fix was reverted against its new test to confirm the test reddens; all four defects had been living under green suites.
Structured adoption reused the CREATE intent, whose account home is derived purely from the current selection. The pane's Codex is already running under the CODEX_HOME it was spawned with — and an automatic resume deliberately pins that to the home that OWNS the session — so adopting a pane on any other managed account probed the wrong sessions directory and failed as "did not prove the expected Codex rollout", with no hint that the account mismatched. Adoption now resolves the home from the pane's own launch record and declines, naming the account, when the pane cannot be attributed. Create is unchanged. Hooks skipped: lint-staged stashes, and the stash stack is shared across every worktree on this machine. oxfmt, oxlint, the native code-quality audit, the max-lines ratchet, tsc and the runtime/codex suites were all run by hand.
…/structured-session-eviction # Conflicts: # mobile/src/tasks/worktree-create-retry.ts
…/structured-session-eviction
…tion' into brennanb2025/structured-session-eviction
…tion' into brennanb2025/structured-session-eviction
…tion' into brennanb2025/structured-session-eviction # Conflicts: # src/main/native-chat/agent-session-wire/structured-agent-session-attach-orchestration.ts
…nd-entry-fixes' into brennanb2025/structured-session-eviction
…overlap' into brennanb2025/structured-session-eviction
…ardown' into brennanb2025/structured-session-eviction
…honesty' into brennanb2025/structured-session-eviction
…e-spawn' into brennanb2025/structured-session-eviction
Union resolution: keeps the batched owner-probe sweep from the perf branch and drops the per-attach resolveLaunchEnv the security fix removed, so the boot-cached in-memory environment stays the only resolution path.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #15411 (adoption), which is stacked on #15372.
The leak
A codex app-server outlived the chat that owned it for the entire app session. Measured on a real machine: 9 app-server processes, 420 MB, the oldest alive 21 hours.
Three teardown paths existed — app quit, handoff to a TUI, and
catch (error)cleanup — and none of them is "the user closed the chat".host.sessionshad no.delete. So resident children tracked sessions ever opened, not sessions open now, and each one kept a lease renewer probing process identity every 10 seconds (a shell spawn per session per probe on macOS — that shows up as fan noise long before RSS does).Agent sleep cannot help: it is keyed by pane/PTY, and a structured session has neither.
Shape of the fix
Teardown is a named, ordered step list, not a method body:
That shape is the point. The fault was not a missing line — it was that liveness was recorded in one place and cleanup in another, so each new way a session could end silently skipped cleanup. A list makes the next one a list entry rather than a fourth teardown path to keep in sync, and it makes idle reaping (see
app-server-lifetime-plan.md) a caller instead of a redesign.Two properties fall out of it:
Order is the correctness property, asserted directly: drain what the session already produced before stopping its child, and stop the child before forgetting it. Draining afterwards discards rows the provider already sent; forgetting first strands the process.
Closing a chat does not end the conversation — the record and journal stay on disk, so the same session can be attached again.
Host split
structured-agent-session-host.tssat at exactly 300 of its 300-line cap, so any addition brokemax-lines. Rather than shave, the attach orchestration moves to its own module behind an explicit context type. The host now reads as a coordinator — serialize per session, track in-flight work — rather than an implementation.277 lines, real headroom. No cap raised, no rule disabled.
Verification
1963 tests across
native-chat, runtime RPC, and the cross-version wire harness.agentSession.closeis registered and exercised in all three version skews (old client sees nothing; new client against an old host getsmethod_not_found; capability gate opens it) — it is inSTRUCTURED_CALLS, not just the method list.Mutations:
stop-provider-child→ 4 tests reddrain-publishedafter the child stop → 2 tests red, an ordering fault the previous imperative shape could not have detectedOne pre-existing load flake in
structured-agent-session-handoff.test.tspasses twice in isolation.