Skip to content

reconnect: replace connection-wide watermark with per-channel replay cursors - #494

Draft
Connor Peet (connor4312) wants to merge 1 commit into
mainfrom
connor4312/per-channel-replay-cursors
Draft

Connor Peet (connor4312) wants to merge 1 commit into
mainfrom
connor4312/per-channel-replay-cursors

Conversation

@connor4312

Copy link
Copy Markdown
Member

Summary

Replaces 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 cause a slower channel's undelivered actions to be skipped on reconnect, because the old
watermark advanced to the highest serverSeq observed across any subscribed channel and was
echoed back as a single global cutoff on the next reconnect call.

Example of the bug this fixes: channel A has an undelivered action at serverSeq=100, channel B
has delivered through serverSeq=101. With the old single watermark, the client reconnects with
lastSeenServerSeq=101 (B's progress) and the host silently skips replaying A's action 100 — it's
gone 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[], each
    carrying { channel, serverSeq } — the last serverSeq that channel actually applied.
  • ReconnectResult: single top-level recovery → channels: ChannelRecovery[], one entry per
    requested channel
    , each independently one of:
    • ChannelReplayRecovery (kind: "replay") — the missed action envelopes for that channel, in
      serverSeq order.
    • ChannelSnapshotRecovery (kind: "snapshot") — too far behind to replay; a fresh Snapshot
      with a fromSeq baseline the channel's cursor is reset to.
    • ChannelMissingRecovery (kind: "missing") — the channel no longer exists / was
      unsubscribed; the client drops it and never re-requests it.
  • The global, authoritative serverSeq (action/log identity and ordering) is unchanged and
    remains 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:

Client Change
TypeScript Full rewrite of HostShared/subscribe/unsubscribe/reconnect cursor tracking in clients/typescript/src/client/hosts/runtime.ts. Reference implementation; 4 new regression tests.
Rust clients/rust/crates/ahp/src/hosts/{runtime.rs,types.rs} ported to a channel_cursors map; 4 new regression tests.
Swift Hosts/{HostShared,HostRuntime}.swift ported; new PerChannelReplayCursorTests.swift (4 tests).
.NET MultiHostClient.cs rewritten with _channelCursors + BuildReplayCursors(); 4 new regression tests.
Go Low-level Client.Reconnect wire call updated to []ChannelReplayCursor; 2 new tests. MultiHostClient supervisor needed no change — it already does a full re-Initialize on any drop today, so it never had the cross-channel-skip bug.
Kotlin No behavioral changes — the Kotlin client ships no host-runtime/reconnect layer at all (wire types + reducers only, per 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 generated MarshalJSON dropped the "kind" discriminant on
re-encode (scripts/generate-go.ts was missing injectDiscriminantOnMarshal: true for that
union, since its variants use omitDiscriminants: true).

Migration

This is a breaking change to the reconnect RPC shape, with no protocol version bump
(per the task's constraints — no automatic version negotiation fallback was requested). There is
no silent legacy fallback:

  • Callers of typed clients must update reconnect() call sites to pass ChannelReplayCursor[]
    instead of a single lastSeenServerSeq.
  • Callers must handle ReconnectResult.channels (array, one entry per channel) instead of a
    single top-level recovery shape.
  • Hosts implementing the server side of reconnect must track and reply with per-channel
    recovery 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-stream
channel 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:

  • TCP channels would always resolve to ChannelReplayRecovery on reconnect — a byte stream needs
    a complete, order-preserving replay against the same retained socket/consumer state, never a
    ChannelSnapshotRecovery (snapshots can't represent in-flight byte-stream position).
  • A ChannelMissingRecovery for a TCP channel must be treated as connection-terminating by the
    TCP 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.
  • No code or types from feat: add TCP connection protocol #488 were imported, and its branch was not modified, per the task's
    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).
  • TypeScript (clients/typescript): npm test — 81/81 pass (incl. 4 new regression tests).
  • Rust (clients/rust): cargo test --all-features — all green (unit + integration + doc tests).
  • Go (clients/go): go test ./... -count=1 — all green.
  • Swift (clients/swift/AgentHostProtocol): swift test — 100/100 pass (96 pre-existing + 4 new).
  • Kotlin (clients/kotlin): ./gradlew clean test (JDK 17) — all green (438 tests).
  • .NET (clients/dotnet): dotnet test in the mcr.microsoft.com/dotnet/sdk:8.0 container
    (per clients/dotnet/AGENTS.md, no local SDK available) — 607/607 pass, including the live
    RealSocketTypeScriptConformanceTests test that spawns a real TypeScript host subprocess (this
    requires Node installed in the container and the TypeScript client's node_modules built for
    the 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)

…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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant