feat(gateway): recover remote sessions with atomic transcript replay - #251
Conversation
danielkov
left a comment
danielkov
left a comment
There was a problem hiding this comment.
Independent AI-agent review of exact head b6760b09ff7fe6e49f6635e30d10e3d4d98aa4fa (COMMENT, not a human approval). Fetched the current PR metadata and diff from GitHub and verified local HEAD equals the remote PR head, including immediately before submission.
No blocking findings identified in this bounded static review.
Reviewed the recovery bridge and server/controller changes with particular attention to uncertain mutation outcomes (no automatic mutation resubmission), same-session resume, snapshot preparation before controller ownership commit, recovery-attempt high-water checks, replay/config/state/response FIFO, candidate-based TUI replacement, ingress/route generation fencing, and replay/queue/retry bounds. Inspected the associated targeted regression cases, including stale/duplicate claims, failed snapshot ownership preservation, lost-create/lost-prompt responses, interrupted replay, explicit no-replay snapshots, and poisoned-route isolation. Loaded the shared-state guidance and checked the ingress→route lock order and guard release before translation/application.
Caveats: this is source/test inspection, not a runtime or exhaustive concurrency proof. I did not run Cargo, tests, or lint in this independent review; the parent agent is running checks separately. I made no repository edits. This review is scoped only to the exact SHA above and does not certify later commits or CI status.
![kit-code-agent[bot]](https://avatars.githubusercontent.com/in/4797102?s=60&v=4){kind=small}
danielkov
left a comment
danielkov
left a comment
There was a problem hiding this comment.
Independent AI-agent refreshed review of exact head 8ff1342bcc21a5eea71c121742eb30d2d18b369c (COMMENT, not human approval). This supersedes my earlier review of b6760b09ff7fe6e49f6635e30d10e3d4d98aa4fa; that review did not identify the publication/admission race subsequently exposed by CI.
No blocking findings identified at this current head in this bounded static review.
Fetched the current full PR diff and latest commit from GitHub, verified its parent/delta, and verified local HEAD equals remote PR head again immediately before submitting. Reviewed the latest change in the full recovery context from the preceding review; server ownership/replay and TUI transaction/fencing code are unchanged.
The fix places the admission timestamp and live-state transition before publishing the recovered initialize response or recovery commit. Requests timestamped before that fence remain rejected, while a peer that observes readiness and immediately submits during output flush is no longer fenced out by a later timestamp. Initial explicit resume already sets its fence before publishing commit. Output errors still leave the bridge through the error path rather than processing queued work after failed publication.
Inspected the new test-only acknowledged Unix-stream peer: it enqueues the follow-up before acknowledging the corresponding output line, and flush waits for that acknowledgment. Both recovered initialization/session creation and recovered-session prompt admission are exercised through actual bridge/server APIs, without production hooks. These tests directly arrange the problematic publication ordering rather than depending on scheduler timing.
Current-head scope also includes the earlier static review of uncertain mutations/no automatic resubmission, transactional resume/controller claims, monotonic recovery attempts, replay/config/state/response FIFO, atomic TUI candidate replacement, stale-generation fences, and bounded recovery/resources. Shared-state guidance remains applied.
Caveats: no Cargo, tests, or lint run by this reviewer; parent/CI checks are separate. Source/test inspection is not an exhaustive concurrency proof. No repository edits or merge performed. This review applies only to the exact SHA above, not later commits or CI status.
danielkov
left a comment
danielkov
left a comment
There was a problem hiding this comment.
Independent AI-agent refreshed review of exact head 091d65627cd76e7f3317a6cd937a71122ff8e3d8 (COMMENT, not human approval). This supersedes my previous-head review at 8ff1342bcc21a5eea71c121742eb30d2d18b369c.
No blocking findings identified in this bounded static current-head review.
Fetched the actual current GitHub PR diff and latest GitHub commit. Verified the latest commit has the reviewed previous head as its parent and changes only the existing unread-session-mailbox integration test: its post-EOF rejection now accepts HTTP 410 or 404. Local HEAD equals the remote PR head, rechecked immediately before posting.
Independently inspected the pinned SDK source read-only at /tmp/kit-consumer-audit/cargo-home/git/checkouts/rust-sdk-d9e6b7ba3b790933/423ba77/src/agent-client-protocol-http/src/bounded_server.rs, and verified Cargo.toml/Cargo.lock pin revision 423ba77cd555a09f68472b93c142c8f0baaabf43. The POST path returns 404 when the requested connection is absent (around line 690), and 410 when its retained connection is closed or draining (around line 820). Connection termination publishes closed state and wakes readers before separately removing the registry entry; SSE stops on the closed flag (around lines 969–983). Therefore stream EOF is not a registry-removal synchronization barrier, and either rejection status is legitimate.
The updated assertion still requires rejection, rather than accepting successful admission or arbitrary errors. The test continues to require stream EOF and durable completion of the already accepted resident prompt. This is a test-contract correction, with no production or SDK change and no weakening of the prompt-survival assertion.
The prior reviewed production context is unchanged, including readiness-fence-before-publication, no automatic mutation resubmission, transactional resume/controller ownership, monotonic attempts, replay FIFO, atomic TUI replacement, stale-generation fences, and recovery/resource bounds.
Caveats: static source/test review only; no Cargo, tests, or lint run by this reviewer. Parent/CI verification is separate. No repository or SDK edits and no merge performed. This review is bound only to the exact SHA above and is not an exhaustive concurrency proof or CI certification.
1 participant
Summary
Recover remote gateway sessions through bounded fresh ACP connections while keeping the local TUI open. Resume the same confirmed session and atomically replace its transcript only after complete replay and authoritative current state/configuration arrive.
Impact
Transient disconnects display reconnecting progress without clearing the current transcript. Uncertain prompts and other mutations are never automatically resubmitted; a lost session-creation response requires explicit session selection.
--remote-no-replayprovides an explicit history-unavailable attachment when complete replay cannot fit.Technical details
Transactional attachment and replay
The resident actor prepares replay, current configuration, current turn state, and the resume response before committing a replacement controller. Bridge recovery metadata follows the existing notification/image/application FIFO into a bounded private TUI candidate; commit replaces the visible view while preserving drafts and fencing stale completions.
Bounded retries and controller ownership
Recovery uses five attempts within a thirty-second budget, fresh initialization, and same-session resume. Attachment tokens, per-outage recovery IDs, and monotonic attempts prevent automatic retries from taking control from a newer explicit controller or a later successful attempt. Authentication, protocol, controller replacement, and unavailable replay stop recovery rather than silently creating a new session or omitting history.