feat(core,connector-core): report transport liveness separately from bind readiness - #981
Conversation
The union merge driver on the suite registry resolved both sides correctly but placed this branch's two entries above the three main added while this was open. Shard membership is positional, so that moved main's three suites two places each and changed the shard of all three, which is the one thing both comment blocks promise does not happen. Measured against main's ordering: before this commit, 3 of main's 411 suites changed index, moving shard 0 to 2, 1 to 3, and 2 to 0. After it, 0 of 411 change. This branch's two entries sit last, and no suite is lost or duplicated.
A session's connection is three facts, not one: whether the Cotal bind is up, whether the socket under it is live, and whether the session was stopped on purpose. Collapsing them into a single boolean gets two situations exactly backwards. The state that needs attention is a live bind over a dead socket, where sends queue or fail while the client reconnects. A boolean derived from bind readiness reports that as fine. The state that needs no attention at all is a deliberate stop, and since stop() clears readiness and transport together, it was indistinguishable from a lost connection; the retained failure then read as a current fault rather than as the post-mortem it is. So the tool reports a state derived once, in the agent, where every consumer sees the same reading: ready, degraded, connecting, disconnected, stopped. It reports the raw facts beside it, so a caller that reads the combination differently is not stuck with ours. The retained issue is reported as `connectionIssue` while it is the current reason and as `lastConnectionIssue` once stopped, because the key name is what a reader acts on. MeshAgent gains `stopping` and `connectionState`. The private `stopping` field is renamed to `_stopping` to match the file's existing convention for a field with a public getter. Every one of the five states is reachable rather than aspirational. The endpoint emits transport true when connect() returns while the bind below is still in progress, which is the connecting window; the degraded and disconnected edges are the real disconnect and rebuild paths. The suite grades all five through a real MCP client and server rather than by calling the tool helper. Six mutations pin the derivation: one per state collapse, one on the issue scoping, and two on the liveness getters, so the reported facts are proved to come from live state rather than being back-derived from the state field. The suite states plainly what it does not claim: it stages the combinations by writing private fields, so it proves reporting rather than reachability, and the reachability argument lives with the transport suites that drive a real broker.
The mutation-fixtures gate rejects a find anchor that spans a comment, and six anchors in this fixture did. The rule is right: an anchor on prose is disarmed by anyone tidying prose, and a comment-only commit cannot announce that it disabled a guard. Four anchors are retargeted onto multi-line code-only windows that are unique on their own. Two trailing comments are removed from emit lines whose ordering rationale is already stated in the block above them, and two handler comments are lifted just outside their blocks so the guard they describe can be anchored on code alone. No behavior changes.
… into conn/phase1-connection-status-v2 # Conflicts: # extensions/connector-core/smoke/fixtures/transport-liveness.mutations.json # extensions/connector-core/src/agent.ts
The union merge driver kept both sides of the ci-suites.txt append but placed this branch's entries before main's, which displaced smoke:sys-injection-evict into a different shard. Measured with check:shard-stability: 1 of 412 pre-existing suites changing shard, naming a suite this branch never touched. Moving the block to the true tail restores it.
|
Head moved to Why it had to move. This PR was reported Refs verified identical to the remote first, so the disagreement was not a stale checkout. The cause is So the fix was to merge What changed, in full:
No line of this branch's own code changed. The diff of The second commit is not cosmetic, and it is #983 for the fourth time. Union kept both sides of the append but placed this branch's two entries before main's, displacing a suite this branch never touched: After moving the block to the tail: Re-verified at the new head, since the merge brought in 69 files including
|
|
I pushed the merge that unstuck this PR without having read its diff, which was the wrong order. Doing that read now, and recording it here so the review has a durable home rather than living on a mesh channel that is currently down. The core change holds up
// A rebuild can replace `this.nc` before the old iterator finishes. Late disconnect/close
// from that old epoch says nothing about the replacement and must not flip its liveness.
if (this.nc !== nc) continue;Capturing One gap, and it is a missing assertion rather than a defectThe terminal-close cell asserts readiness only: check(
"a REAL terminal close marks readiness false before exposing its user-visible reason",
await until(() => !agent.connected && /mesh connection closed/.test(terminalIssueAtError ?? ""), 30_000),
{ ready: agent.connected, terminalIssueAtError, latestIssue: agent.connectionIssue, transport },
);
That matters because of what consumes it. Phase 1 derives: return this._transportConnected ? "connecting" : "disconnected";so Measured, not assumed. I added So this is not a blocker and nothing needs fixing. It is one clause missing from an existing assertion, protecting a fact a downstream phase renders to a model. Worth adding before phase 1 lands, since phase 1 is what makes the fact user-visible. Scope of this readOpen-brief, not a lens. I read the |
The terminal-close cell asserted readiness only and passed the transport array into its diagnostic payload without checking it. The one cell that did assert transportConnected === false ran after stop(), so it proved stop() clears the flag rather than that a terminal close does. cotal_connection_status renders connected:false with transportConnected:true as "connecting", so a terminal close that left transport true would report a permanently dead session as one that is coming up, and nothing would fail.
The fixture said it graded the exact nats.js lifecycle contract, but two of the edges this branch introduces sat outside the ledger. The close status branch emits a transport-false line that is byte-identical to the one in the rebuild path, and the only mutation covering that text anchors the rebuild site through its trailing connection emit. Nothing broke the close branch, so its guard was never load-bearing in the proof. MeshAgent.stop clears readiness and transport together, but the mutation there names the transport line only as disambiguating context and deletes the readiness assignment alone, so the transport half was asserted and never entered.
The unit cell drives this edge by pushing a close status onto a hand-built queue, which shows the branch works but not that a real close reaches it. The broker companion already drives a genuine terminal close through nc.closed(); it just asserted nothing about transport there. Folding the transport clause into the same until() predicate rather than spot-checking after it, because the flag can arrive after readiness and a single read at that instant would be a race.
…sport" The added clause is not lethal. Deleting the close branch emit from watchStatus and running this suite against the mutant leaves it at 7 of 7, so the clause passes for a reason other than the code it names. The broker is killed to reach a terminal close, so nats.js emits disconnect first and transport is already false by the time the close arrives. There is no window in this scenario where the close branch is what clears it, which is what the review said and what the experiment confirms. An assertion that cannot fail reads as coverage, so it is worse here than its absence.
connectAndBind emitted `connection: true` as its last act, before either caller could run tearDownIfStopped. A stop() arriving during the bind therefore left every endpoint listener holding a connected edge that nothing followed or corrected, on a connection the caller had already discarded. The state teardown was already correct on both the initial-start and rebuild paths, so only the event was unfenced. Guard the emit on `stopped`. MeshAgent was never the exposed consumer: it carries its own `stopping` guard and drops the late edge, which is why the fix belongs at the emitter rather than in a consumer. Any other listener on the endpoint had no such guard. The unit suite cannot reach this. All three of its stop-versus-bind cells replace connectAndBind wholesale, so nothing inside the method is under test there. The broker suite gets the cell instead: it gates armPlane3, the last await connectAndBind makes before reporting the endpoint live, which holds a real bind open at its final step while stop() lands. Removing the guard turns that cell red with the late edge in its diagnostic, recorded as a mutation fixture so it stays proven.
The cell waits on a real dial and bind rather than a local poll, so it gets the same 30s budget the terminal-close cell uses. It returns as soon as the bind arrives, so the larger budget costs nothing on the happy path and only changes what happens when a slow runner would otherwise have failed a correct suite.
The correctness block is addressed at 99c8414
The state teardown was already right on both paths, so only the event was unfenced. The emit is now guarded on Two things the fix turned up that change the finding's shapeMeshAgent was never the exposed consumer. It carries its own The unit suite structurally cannot prove this. All three of its stop-versus-bind cells replace How it is provenThe broker suite gets the cell. It gates Removing the guard turns that cell red with the late edge in its own diagnostic:
Limits worth statingThe timing is forced rather than observed. The gate makes the race deterministic instead of waiting for it to happen, which is the only way to test it repeatably; the race itself is not hypothetical, and The entry point is real: Verified at 99c8414: broker suite 8/8, unit suite 20/20, |
The union merge kept both appends but placed this branch block ahead of the suites main added, shifting three of them into other shards. Moving the block to the end restores every pre-existing assignment.
Review pointed out the fixture said gating armPlane3 was the only place the guard is reachable. Any gated await inside connectAndBind would reach it. armPlane3 is the one chosen because it is the last, so the whole bind is real up to the decision point, and because it returns immediately unless the endpoint hosts Plane 3, which a MeshAgent endpoint never does.
…hat expires The mid-bind cell measures the start() caller. doRebuild is covered by the guard being one shared unbranched statement both callers await. Note at the guard that this reasoning expires if the tail becomes caller-aware or the emit splits per path, since that is when the rebuild race needs its own cell.
Review found the sibling of the readiness race and it reaches further back. watchStatus seeds transport as soon as the dial returns, and connectAndBind calls it right after the dial, long before the bind completes. A stop() landing while the dial is still in flight therefore had that seed fire on an endpoint already stopped: reproduced through a real pending dial, the listener saw transport true after stop, followed by false. Guard the seed on stopped, as the readiness emit already is. MeshAgent masked both edges through its own stopping guard, so direct endpoint listeners were the exposed ones in both cases. The cell holds a real dial pending behind a TCP proxy and replaces nothing in the endpoint, so the whole path runs unmodified.
|
Repaired against current Exact head: The real local merge was conflict-free, matching Validation at start load
Stacked PR #1002: its four shared files were not manually resolved or behavior-edited during this repair. A tree merge of #1002's exact approved head |
|
Repair update at exact head |
feat(connector-core): report connection state through cotal_connection_status
|
Final stack state after PR #1002 merged into this branch: exact head is now |
davidfarah2003
left a comment
davidfarah2003
left a comment
There was a problem hiding this comment.
LANDING-QUEUE REVIEW: BLOCKERS
- The body no longer describes what this branch ships. #1002 was merged into
conn/phase1a-transport-liveness(46acf2cbb, 2026-08-30T03:17), so this PR now carries an entire second feature the body never mentions: thecotal_connection_statustool atextensions/connector-core/src/tool-specs.ts:551, new public APIConnectionState(extensions/connector-core/src/agent.ts:211) plus thetransportConnected/lastInboxDrainedAt/stopping/connectionStategetters (agent.ts:361,371,378,385), a_lastInboxDrainedAtwrite on both drain paths,docs/mcp-tools.md,scripts/generate-tool-docs.mjs, a 15-cell smoke with its own 7-mutation fixture, and a secondminorchangeset.changeset/connection-status-tool.md. #1002's own body says "It does not merge before #981"; the stack collapsed the other way and this body was not updated to follow. For a landing queue the body is the record of what lands, and right now it under-describes the diff by a whole feature. - Testing section counts disagree with the shipped artifacts. "Two suites, both appended to the end of the CI suite list" — three are appended:
smoke:transport-liveness,smoke:transport-liveness:broker,smoke:connection-status. "smoke:transport-liveness:broker(7 cells)" — the shipped fixture pins it at"completionMarker": "SUITE COMPLETE: 9 cells"(extensions/connector-core/smoke/fixtures/transport-liveness-broker.mutations.json:6), and the suite has 9check(calls. "The broker suite carries its own single-mutation fixture" — that fixture ships two mutations (same file,mutationsarray at line 21), the second being the transport-seed guard your own "Behavior changes worth calling out" section describes. Under-claiming, but these are the numbers a reviewer reconciles against and none of the three match. - No reported evidence run. There is no Evidence block: no pass counts for the three suites on the tree that actually ships, no kill line for the 18-mutation, 2-mutation and 7-mutation fixtures, no typecheck/build, no
check:shard-stability. The suites and fixtures are described, never reported as executed. #1002 carries those numbers, but for a merged-in child, not for this PR's current head — and rail (b) wants the mutation proof named on what lands. - Verified good, and it is genuinely good work: the epoch guard in
watchStatuscapturesconst nc = this.ncand re-checks it both in-loop and in the catch; the two stop-race fences (endpoint.tsif (this.stopped) return;before theconnection:trueemit, and before thetransportseed) are each pinned by a broker-suite mutation on real code;disconnect/reconnect/closeare handled andreconnectingdeliberately is not; the terminal-close emit ordering is asserted as a contract rather than left implicit. The catch-handler guard is honestly declared unreachable-as-measured rather than dressed up as proven — that is the right way to write that down. - Rails and hygiene are clean: three suite names at the absolute tail of
bin/smoke/ci-suites.txt(the only mid-file edits are two blank-line deletions, whichbin/smoke/ci-suites.mjsstrips beforeshard.mjsindexes, so shard assignments hold); no fallbacks — theserver?field is omitted when the runtime supplies none rather than invented, and status comes straight offnc.status()with no polling substitute;connectionIssuenarrowing to pre-bind is documented indocs/connectors.md; both changesets areminor; no AI/tool attribution anywhere. - (i) Interactions: #1002 is inside this branch, so the two land together or not at all — its review is posted there.
bin/smoke/ci-suites.txttail conflicts with #921, #880, #1053 and #1054, and #1052/#1034/#1033 restructure that file andshard.mjsunderneath all of them.
Resolves the ci-suites.txt !merge conflict by preserving main's ordering and re-appending this branch's transport-liveness and connection-status suite entries, with their comment blocks, at the tail.
Re-appends this branch's suite entries at the tail of main's registry after the observer-watch-stall merge, preserving main's ordering.
Re-appends this branch's suite entries at the tail of main's registry after the sparse-history-walk merge, preserving main's ordering.
1 participant
A connector session had one notion of "connected": the endpoint's
connectionevent, which means afull Cotal bind has completed. That is the right signal for readiness gates and for the self-heal
supervisor, and it is the wrong signal for "is the socket up right now". A transient NATS
disconnect and reconnect never moved it, so a session could sit with a dead socket while every
consumer read it as connected.
This adds raw transport liveness as a separate, additive signal and leaves
connectionalone. Thereadiness gates and the self-heal supervisor consume
connection, and making it track socketliveness would flap them on every transient reconnect.
What changed
CotalEndpointgains atransportevent carrying{ connected, server? }, driven from theclient's status stream. The status union was checked against the pinned client rather than assumed:
disconnectandreconnectare the authoritative pair, andreconnectingis telemetry that mustnot move state.
watchStatusneeded the epoch guardsuperviseConnectionalready had. An old connection's statusiterator keeps running after a rebuild replaces it, so without the guard a dying epoch can overwrite
the state of the healthy connection that replaced it.
MeshAgenttracks the new state alongside readiness, suppresses duplicate edges so a repeatedconfirmation cannot look like a fresh outage, and ignores late endpoint events after
stop(), sincean in-flight bind can complete after shutdown and must not resurrect a stopped session.
Behavior changes worth calling out
stop()now clears both states locally rather than waiting on an endpoint event that deliberatelyignores its own stopped close, and it fences the initial
connectAndBindthe way the rebuild pathalready did. Without that fence, a
stop()racing the initial bind left a live connection,heartbeat, consumers and presence behind. That is #975, and only the rebuild path had the check.
The same race had a second half that the state fence did not cover.
connectAndBindemitted itsconnection: trueas its last act, before either caller could reachtearDownIfStopped, so astop()landing mid-bind left every endpoint listener holding a connected edge on a connectionthe caller had already discarded, with nothing following to correct it. The emit is now guarded on
stoppedas well.MeshAgentwas never the exposed consumer, since it carries its ownstoppingguard and drops the late edge; that is why the guard belongs at the emitter, where one check covers
every listener, rather than in a consumer.
Review then found the sibling edge on the same race, and it reaches further back.
watchStatusseeds
transport: trueas soon as the dial returns, andconnectAndBindcalls it right after thedial, long before the bind completes. A
stop()landing while the dial was still in flight hadthat seed fire on an endpoint already stopped, so the listener saw transport true after stop,
followed by false. Reproduced through a real pending dial before it was fixed. Both edges are now
guarded on
stopped.stop()no longer clears the last connection issue. A cleanly stopped session keeps it forpost-mortem diagnosis instead of discarding the reason it stopped. Post-stop errors cannot overwrite
that retained value. The issue remains scoped to pre-bind readiness failures and still clears on a
successful bind.
Testing
Two suites, both appended to the end of the CI suite list so every existing shard assignment is
unchanged.
smoke:transport-liveness(20 cells) grades the state contracts deterministically with controlledstatus queues: epoch staleness, disconnect and reconnect edges, duplicate suppression, the stop
races, terminal-close diagnostic ordering, and issue scoping.
smoke:transport-liveness:broker(9 cells) owns a throwaway broker on an OS-assigned port andproves the real paths: initial transport before full-bind readiness, a real broker loss lowering
transport without flapping readiness, a real client reconnect restoring it, and a real terminal
close through
nc.closed()reaching the diagnostic ordering rather than a hand-built double.An 18-mutation fixture pins the deterministic suite and each mutation names the cell expected to
fail. The broker suite carries its own two-mutation fixture, one for the readiness-event fence and
one for the stopped guard on the transport seed that fires first, because those guards are
unreachable from the deterministic suite: all three of its stop-versus-bind
cells replace
connectAndBindwholesale, so nothing inside the method is under test there. Thebroker cell gates
armPlane3instead, the last awaitconnectAndBindmakes before reporting theendpoint live and a no-op for an endpoint hosting no Plane 3, which holds a real bind open at its
final step while
stop()lands and leaves the method itself real.What the proof does not claim
One mutation covers the epoch guard on
watchStatus's catch handler, and its cell manufactures therejection rather than reaching it from a real client. Rather than dress that up, it was measured:
temporary instrumentation around the real catch, across five complete broker-companion runs covering
loss, reconnect, manual epoch replacement and terminal close, recorded zero fires. Every status
iterator ended normally.
The guard is kept, because removing it would leave the catch path disagreeing with the reachable
in-loop epoch guard beside it, and the claim is bounded to what was measured. The cell is named as a
controlled throw, and the suite header, the fixture and the endpoint comment all state that real
reachability is intentionally unclaimed on the pinned client. This is deliberately not a claim of
impossibility across other runtimes or future client versions.
The stopped guard is measured for the
start()caller only.connectAndBindhas a second caller,doRebuild, and no cell races a stop against a rebuild's bind. That path is covered by the guardbeing one shared unbranched statement both callers await, with
tearDownIfStoppedbeside it inboth places, rather than by measurement. A cell there would re-prove control flow at the cost of a
timing-heavy terminal-close race on a CI shard. The reasoning is written at the guard itself along
with the condition that voids it: if that tail ever becomes caller-aware, or the emit splits per
path, the rebuild race needs its own cell.
Closes #975