reconnect: replace connection-wide watermark with per-channel replay cursors - #494
Draft
Connor Peet (connor4312) wants to merge 1 commit into
Draft
Connor Peet (connor4312) wants to merge 1 commit into
Connor Peet (connor4312) wants to merge 1 commit into
Conversation
…cursors Replace the single connection-wide `lastSeenServerSeq` reconnect watermark with independent per-channel replay cursors, fixing a real correctness bug: a fast channel's progress could silently skip a slower channel's undelivered actions, since the old watermark advanced to the highest `serverSeq` seen across *any* channel and was echoed back on reconnect as a single global cutoff. - `ReconnectParams.subscriptions` now carries one `ChannelReplayCursor` (channel URI + last-applied `serverSeq`) per subscribed channel, instead of a single client-wide `lastSeenServerSeq`. - `ReconnectResult.channels` returns one `ChannelRecovery` per requested channel, each independently `Replay` (missed actions in order), `Snapshot` (state reset to a `fromSeq` baseline), or `Missing` (channel no longer exists/unsubscribed). - Global `serverSeq` remains the authoritative action/log identity and ordering key; it is never used to decide what a channel needs to replay. Channel cursors advance only from that channel's own actions or its own snapshot baseline, never from another channel's progress. - Ported the new reconnect flow to every host-runtime implementation that has one: TypeScript, Rust, Swift, and .NET `MultiHostClient`/`AhpClient`, plus the low-level Go `Client.Reconnect` wire call. Kotlin and the Go `MultiHostClient` supervisor needed no behavioral changes (Kotlin ships no host-runtime layer; Go's supervisor always does a full `Initialize` on reconnect today). - Regenerated all client type mirrors from `types/common/commands.ts` via `npm run generate`; fixed a Go codegen bug where `ChannelRecovery`'s generated `MarshalJSON` dropped the `"kind"` discriminant on re-encode. - Added shared round-trip fixtures (divergent per-channel cursors, mixed replay/snapshot/missing recovery) wired into all six language test harnesses, plus targeted regression tests per runtime proving a fast channel never advances a slower one, snapshot/missing handling, and replay exhaustion. Migration: callers of the typed clients must update `reconnect()` call sites to pass `ChannelReplayCursor[]` instead of a single `lastSeenServerSeq`, and handle `ReconnectResult.channels` instead of a single top-level recovery. There is no silent legacy fallback; this is a breaking change to the `reconnect` RPC shape without a protocol version bump. (Commit message generated by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This branch has not been deployed
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.
Summary
Replaces the single connection-wide
lastSeenServerSeqreconnect watermark with independentper-channel replay cursors, fixing a real correctness bug: a fast channel's progress could
silently cause a slower channel's undelivered actions to be skipped on reconnect, because the old
watermark advanced to the highest
serverSeqobserved across any subscribed channel and wasechoed back as a single global cutoff on the next
reconnectcall.Example of the bug this fixes: channel A has an undelivered action at
serverSeq=100, channel Bhas delivered through
serverSeq=101. With the old single watermark, the client reconnects withlastSeenServerSeq=101(B's progress) and the host silently skips replaying A's action 100 — it'sgone forever. With per-channel cursors, A reconnects with its own cursor (whatever it last
actually applied) and gets A's action 100 replayed, independent of B.
Protocol / wire changes (no version bump)
ReconnectParams.subscriptions:string[]of channel URIs →ChannelReplayCursor[], eachcarrying
{ channel, serverSeq }— the lastserverSeqthat channel actually applied.ReconnectResult: single top-level recovery →channels: ChannelRecovery[], one entry perrequested channel, each independently one of:
ChannelReplayRecovery(kind: "replay") — the missed action envelopes for that channel, inserverSeqorder.ChannelSnapshotRecovery(kind: "snapshot") — too far behind to replay; a freshSnapshotwith a
fromSeqbaseline the channel's cursor is reset to.ChannelMissingRecovery(kind: "missing") — the channel no longer exists / wasunsubscribed; the client drops it and never re-requests it.
serverSeq(action/log identity and ordering) is unchanged andremains the single source of truth for ordering and dedup within a channel. It is never used to
decide what a channel needs replayed, and a channel's cursor is never advanced by another
channel's progress or used to seed the global
serverSeq.Implementation scope
Canonical types (
types/common/commands.ts), all 5 generators(Swift/Kotlin/Rust/Go/.NET) + regenerated output, JSON Schema, 3 docs
(
docs/specification/lifecycle.md,docs/guide/ahp-and-acp.md,docs/guide/reconciliation.md),and every host-runtime implementation that has a reconnect fast path:
HostShared/subscribe/unsubscribe/reconnect cursor tracking inclients/typescript/src/client/hosts/runtime.ts. Reference implementation; 4 new regression tests.clients/rust/crates/ahp/src/hosts/{runtime.rs,types.rs}ported to achannel_cursorsmap; 4 new regression tests.Hosts/{HostShared,HostRuntime}.swiftported; newPerChannelReplayCursorTests.swift(4 tests).MultiHostClient.csrewritten with_channelCursors+BuildReplayCursors(); 4 new regression tests.Client.Reconnectwire call updated to[]ChannelReplayCursor; 2 new tests.MultiHostClientsupervisor needed no change — it already does a full re-Initializeon any drop today, so it never had the cross-channel-skip bug.clients/kotlin/AGENTS.md). Covered by the new round-trip fixtures only.Shared conformance fixtures added in
types/test-cases/round-trips/:054-reconnect-params-per-channel-cursors.json— divergent cursors (A=100, B=101, C=0).055-reconnect-result-mixed-channel-recovery.json— mixed Replay/Snapshot/Missing in one result.Both wired into all 6 language round-trip harnesses. Fixed a real Go codegen bug found while
adding these:
ChannelRecovery's generatedMarshalJSONdropped the"kind"discriminant onre-encode (
scripts/generate-go.tswas missinginjectDiscriminantOnMarshal: truefor thatunion, since its variants use
omitDiscriminants: true).Migration
This is a breaking change to the
reconnectRPC shape, with no protocol version bump(per the task's constraints — no automatic version negotiation fallback was requested). There is
no silent legacy fallback:
reconnect()call sites to passChannelReplayCursor[]instead of a single
lastSeenServerSeq.ReconnectResult.channels(array, one entry per channel) instead of asingle top-level recovery shape.
reconnectmust track and reply with per-channelrecovery instead of a single watermark-based decision.
TCP channels (#488) integration implications
Read open, not-yet-merged PR #488 (
kycutler/tcp, not touched) for the planned TCP byte-streamchannel feature. This PR's design is compatible with and anticipated by #488's own stated
design: its description already says "Ordinary-channel replay remains independently checkpointed
to avoid applying existing state twice" and that snapshots cannot restore byte streams, with
missing/unavailable replay terminating the connection rather than silently reopening a socket.
Concretely, once #488 lands on top of this:
ChannelReplayRecoveryon reconnect — a byte stream needsa complete, order-preserving replay against the same retained socket/consumer state, never a
ChannelSnapshotRecovery(snapshots can't represent in-flight byte-stream position).ChannelMissingRecoveryfor a TCP channel must be treated as connection-terminating by theTCP consumer (per feat: add TCP connection protocol #488's own stated semantics), not as "drop and move on" the way an ordinary
pub/sub channel is treated here.
explicit constraints.
Validation
npm run generate— clean, regenerates all 6 artifacts + schema + docs with no drift.npm run test(root) — 515/515 pass, 100% statement/branch/function/line coverage maintained.npm run verify:change-fragments— passes (27 fragments incl. the new one).clients/typescript):npm test— 81/81 pass (incl. 4 new regression tests).clients/rust):cargo test --all-features— all green (unit + integration + doc tests).clients/go):go test ./... -count=1— all green.clients/swift/AgentHostProtocol):swift test— 100/100 pass (96 pre-existing + 4 new).clients/kotlin):./gradlew clean test(JDK 17) — all green (438 tests).clients/dotnet):dotnet testin themcr.microsoft.com/dotnet/sdk:8.0container(per
clients/dotnet/AGENTS.md, no local SDK available) — 607/607 pass, including the liveRealSocketTypeScriptConformanceTeststest that spawns a real TypeScript host subprocess (thisrequires Node installed in the container and the TypeScript client's
node_modulesbuilt forthe container's platform; it is excluded from the environment's bare-container default because
Node isn't preinstalled there).
No blockers. Branch was rebased cleanly onto latest
origin/main(one unrelated docs-only commit,#485) immediately before this push; no conflicts.
Issue tracking
No existing GitHub issue was found that matches this bug/feature (searched for
reconnect/lastSeenServerSeq/per-channel keywords); the closest related issues are #125(closed, different bug) and #492 (open, but explicitly out of scope — channel-level flow control
and fair delivery, not recovery/checkpoint semantics). This PR intentionally does not claim or
fabricate a "Fixes #N" line.
(Pull request body generated by Copilot)