fix(coding-agent): snapshot transfer ids from the materialized cursor; mismatches settle the transfer, not the worker channel - #2044
Open
snimu wants to merge 4 commits into
Open
Conversation
…nd settle transfer mismatches transfer-scoped Worker side, the chunked-snapshot transfer id was computed from the live session cursor at a different time than the message-array capture it labels, so two transfers could carry the same id for different bytes whenever events arrived during snapshot materialization. Derive the id from the snapshot's own lastEventCursor at all three call sites (attach, replacement, catchup) so the id always names the captured cut. Supervisor side, a duplicate-transfer disagreement (reentrant begin, mismatched duplicate envelope, mismatched duplicate chunk bytes, or mismatched duplicate end metadata) was punished with closeWorkerChannel=true, bouncing a healthy worker and every session on it. Settle these anomalies with the transfer as the blast radius instead: fail the one transfer and queue attached clients for a resync from a fresh snapshot, mirroring the existing session_snapshot_failed handling, which now shares the same helper. Malformed-frame handling (undecodable payloads) still closes the channel.
…drop, not the failing frame's purpose A failed published transfer can be serving any attached client's catch-up wait: drainClientCatchups clears its queue entry before draining, so when duplicate validation rejects the waiter it was logged and dropped, and an attach-purpose failure never requeued it — the client stayed attached with a stale transcript while the worker stayed ready. Queue the resync whenever the published cache is dropped; the frame purpose only picks the replacement/resync flavor.
…omments One workerAttachHarness builder serves both worker-side attach pins, and multi-line comment blocks collapse toward one-line invariant guards. No behavior or coverage change.
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
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.
Part of the worker-state single-truth program (Linear RES-1270); squashes discussion #1662 (both halves).
Purpose
Two halves of one duplicate-truth defect in chunked snapshot transfers:
${activeSessionId}-${eventGeneration}-${lastEventSequence}was computed from the live session cursor at a different time than the message-array capture it labels (aftercreateAttachResult's awaits on attach/catchup, before them on replacement). Events arriving during materialization let two different byte streams legitimately share one snapshot id.closeWorkerChannel=true:handleWorkerClose+client.close()on a healthy worker, failing every in-flight request for every session on that worker. Since fix(coding-agent): harden daemon startup and recovery ownership #1929 this is a control-plane bounce rather than a terminal brick, but it can recur every time the race re-fires on a big busy transcript.Change
daemon-mode.ts: newsnapshotTransferId(snapshot)derives the id fromsnapshot.lastEventCursor— the cursor captured synchronously with the message array increateSessionSnapshot— and all three call sites use it. For the replacement path the id computation moves after materialization (parameter deleted).daemon-supervisor.ts: newfailSnapshotTransfersettles a transfer anomaly with the transfer as the blast radius: fail that one generation (existingfailWorkerSnapshotCachewithcloseWorkerChannel=false) and queue attached clients for a resync from a fresh snapshot. The four mismatch sites use it, and the existingsession_snapshot_failedhandler plus the end-frame catchup loop are consolidated through the same helpers (they previously duplicated the loop inline). Malformed-frame handling (undecodable payloads) still closes the channel.Net src LOC: +33/-27 in daemon-supervisor (mostly the extracted helpers replacing two inline copies), +14/-5 in daemon-mode. No wire-shape change: snapshot ids are opaque strings and the mismatch handling is supervisor-internal, so this is backward-compatible with old workers and clients.
Tests
In
4602-snapshot-transfer-idempotency.test.ts(the existing file for exactly this machinery):snapshotStream.idnames the materialized snapshot cursor even when the live session cursor has advanced (fail-unfixed verified: reverting the worker-side hunk yields the live-cursor id).ready, no recovery run, caches dropped — and for a published replacement-purpose mismatch, that a freshattachreload is requested from the worker (the queued resync).Ran locally: 4602, 4601, 4677, snapshot-transcript-cache, agent-connection-daemon, daemon-supervisor-monitor — 224/224 pass;
npm run checkgreen.Note
Medium Risk
Changes daemon worker/supervisor snapshot protocol behavior; incorrect resync paths could leave clients stale, but blast radius is intentionally narrowed from whole-worker bounce to per-transfer recovery.
Overview
Chunked session snapshot transfers now use a transfer id tied to the materialized snapshot cursor (
snapshot.lastEventCursor) instead of the live session generation/sequence, so late-arriving events cannot label two different byte streams with the same id. Replacement snapshots compute that id only aftercreateAttachResultmaterializes the snapshot.On the supervisor, duplicate or invalid snapshot transfers (restarted begin, envelope mismatch, bad chunks/end, worker-reported failure) are handled via
failSnapshotTransfer/queueSnapshotResync: drop the affected cache generation, keep the worker channel open, and queue attached clients for a fresh attach/catch-up when the failed transfer was published. Malformed frame handling that still closes the channel is unchanged.Tests in ENG-4602 pin the materialized-cursor id and expect resync instead of worker channel close for those anomaly paths.
Reviewed by Cursor Bugbot for commit 5af3bbe. Bugbot is set up for automated code reviews on this repo. Configure here.
LOC
Total src: +61/−39 (net +22); tests: +61/−26 (net +35).
Note
Fix snapshot transfer IDs to use materialized cursor and scope transfer failures away from worker channel
snapshotTransferIdhelper in daemon-mode.ts, instead of the live session's current generation and sequence.DaemonSupervisor.failSnapshotTransferhelper in daemon-supervisor.ts contains anomalies to the affected transfer: it discards the matching snapshot cache and queues a fresh catch-up or replacement for attached clients.handleWorkerFramenow routes restart, completed-transfer mismatch, chunk-byte validation, end-frame validation, and worker-reported snapshot failures throughfailSnapshotTransferinstead of worker-level recovery; any snapshot protocol path not covered by this routing will still trigger the old worker-close path.Macroscope summarized 5af3bbe.