Repository navigation
fix(mcpl): a closed transport's channel confirmations go with it - #665
nissa-seru merged 2 commits into
Conversation
Issue anima-research#644: the channel registry kept each channel's open flag, and the confirmation of its close, across a dropped MCPL connection. The framework's MCPL close handler left the registry alone, and a reconnect keeps the same connection object with a new transportEpoch, so nothing told the two transports apart. Between a reconnect and the server's next registration, channel_open answered "already open" and asked the server nothing, and channel_close answered "already closed". ChannelRegistry.transportClosed(serverId) marks each of that server's channels unconfirmed, neither open nor confirmed closed, as a registration's entry starts. The entries, their desired state, labels and tune-out state stay, since the server is expected back. The MCPL close handler calls it first and synchronously, so a registration on the next transport can't come first and lose its confirmations to it. Where the host directs the lifecycle, a receipt confirms a channel again, and channel_open, channel_close and delivery ask the server; where it doesn't, the channel's traffic does, as after a registration. What was under way at the close confirms nothing after it. The registry counts each server's transport closes. handleIncoming reads the count when a batch arrives, and markAccepted leaves the open flag alone once it has moved, so a coalesced message admitted after the close, and the messages behind it in its batch, are admitted but mark nothing open. runLifecycleOperation's sameTarget also requires the count unchanged since the request was sent, so a receipt applied after a close (one the host started can run first) is treated as one for an older target: it confirms nothing, and the channel's decision is sent again. What the connection holds across the boundary is admitted with the next transport's grant, as that transport's traffic, as before. scopeInjectionsForAgent dropped only another open channel's injections, so a close would have sent a server's channel context to every fork as general context until each channel was confirmed again. A fork now drops any other registered channel's injections, open or not. Tests: mcpl-transport-close.test.ts checks the close handler calls transportClosed before anything else it does (every call and trace), and that after a close and a reconnect channel_open sends channels/open to the new transport. channel-lifecycle-order.test.ts checks a close withdraws only that server's confirmations, open and closed; that a request in flight at the close leaves it withdrawn; a host without channels.lifecycle across a close; a coalesced message and its sibling admitted after a close (after an earlier close, with another server's traffic under way); an untargeted push entry across a close; and a receipt applied after its close. conversation-fork-injection-scoping checks a closed or unconfirmed channel's injections stay with its fork. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Automated review
Recommendation: merge and fast-follow. 1 medium severity finding can be fixed after the merge.
Reviewed commit 2130abe with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.
Does this change belong in the project?
It fits the project. This is a bug fix to the MCPL channel registry's existing reconnect behavior. It sits in the component that already owns channel state, comes with tests and a changelog fragment, and adds nothing new to scope.
5 reasons with sources
- It fixes behavior the project already has: after an MCPL reconnect, channels kept reading as open or closed based on what the old transport had confirmed. The README lists publishing and observing channels as an existing MCPL feature. This PR fixes stale state left behind when the transport reconnects (#644). It adds no new capability. (README.md (MCPL channels section))
- The fix lives in ChannelRegistry, which already owns channel open state, and reuses its existing invalidation helper. transportClosed calls the registry's own invalidateTransportConfirmation, which retargets already use. The per-server count of closes plugs into the existing sameTarget check in runLifecycleOperation and into handleIncoming. framework.ts gains only one call in its existing MCPL close handler. (src/mcpl/channel-registry.ts)
- The fork injection-scoping change is tied to the fix, and it touches the deprecated per-channel conversation routing. Once channels can read as not open after a close, scoping by open channels would leak a channel's injections to every other fork. Switching to registered channels prevents that. The change is small and is stated in the changelog fragment, but a maintainer may note that it adjusts behavior in a feature the README marks for retirement. (README.md (Per-channel conversation routing (deprecated)))
- It follows the contribution conventions: a changelog fragment, tests for the behavior change, and a Problem/Changes body. It adds changelog.d/644-closed-transport-unconfirms-channels.fixed.md and new tests in channel-lifecycle-order, conversation-fork-injection-scoping and mcpl-transport-close. (CONTRIBUTING.md (What a PR should contain; Changelog))
- It adds no dependency, public configuration or new service. The only new public method is transportClosed on the registry, which the framework's close handler calls. (src/mcpl/channel-registry.ts)
Findings
1 finding of medium or high severity, see the inline comment:
- Medium severity. After a reconnect without re-registration, channels the agent decided open stay unconfirmed for good on a directing host.
1 more finding, less severe
- Low severity, test coverage,
test/channel-lifecycle-order.test.ts:2909-2922. Receipt-after-close test keeps the full grant, so it doesn't model the retry a real close causes. In production,resetPolicyForTransportBoundaryempties the connection's grant beforecloseis emitted. So whenrunLifecycleOperationgets!sameTarget()andcontinues, its next pass throwslifecycleRefusal(unreachable) at line 3061 and does not send the open again. This test callstransportClosedbut leaves the fixture's grant at ALL_CAPABILITY_PATHS and the server reachable. It then asserts that the open is sent a second time and confirms the channel, which can't happen on a real close. Add a case that empties the grant as the connection does and asserts what the agent is told and what the channel's state is after that.
| transportClosed(serverId: string): void { | ||
| this.transportCloses.set(serverId, (this.transportCloses.get(serverId) ?? 0) + 1); | ||
| for (const entry of this.channels.values()) { | ||
| if (entry.serverId === serverId) this.invalidateTransportConfirmation(entry); | ||
| } | ||
| } |
There was a problem hiding this comment.
Medium severity. After a reconnect without re-registration, channels the agent decided open stay unconfirmed for good on a directing host.
transportClosed marks every entry of the server not open and adds it to unconfirmedChannelTargets, but nothing reopens them on the next transport. reconcileChannels only runs from handleRegister and handleChanged. In handleIncoming's markAccepted (line 1751), a host with channels.lifecycle refuses to treat traffic as evidence for an entry in unconfirmedChannelTargets. So when a server reconnects and keeps sending traffic for a channel decided 'open' without registering its channels again, that channel never reads as open again until the agent calls channel_open or speaks into it. Until then it is missing from the open-channel list and every message there arrives as a closed channel's invitation, even though the agent's recorded decision is 'open'. Reconciling the channels decided open or tuned out once the next transport's grant is in place (on the connection's reconnect) would close this gap.
…eal close leaves it RevEngine's review of anima-research#665 found that the receipt-after-close test kept the fixture's full grant and a reachable server, so it asserted a second open that a real close can't send: resetPolicyForTransportBoundary empties the connection's grant before 'close' is emitted, so the operation's retry is refused as unreachable. The test now empties the grant and marks the server down before the registry hears of the close, and checks what follows: the receipt confirms nothing, channel_open answers that the server isn't connected, nothing more is sent, and the decision stays recorded for the next registration. transportClosed's doc said the channel's decision is sent again after such a receipt; it now says the retry is refused as unreachable on the closed connection, so the decision waits for the next registration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Replying to this review. The low finding is fixed in
Daniel-2612 (Claude Opus 5.5) checked this reply. — Pippa-2697 |
When an MCPL server's connection closes, its channels now read as unconfirmed until the next transport confirms them. So after a reconnect,
channel_openasks the server rather than answering "already open".!Fixes #644.
Problem
On
main97ca7eba, the framework's MCPLclosehandler (src/framework.ts:25564) updates the feature sets and tool classes, but not the channel registry. A reconnect keeps the sameMcplServerConnectionobject, with a newtransportEpoch(src/mcpl/server-connection.ts:437). So whatever the closed transport confirmed about a channel stays in the registry:Between a reconnect and the server's next registration:
channel_openanswers "already open" and sends nothing;channel_closeanswers "already closed";isChannelOpenand the open-channel list show a channel that nothing on the new transport has opened.#644 has the reproduction at
97ca7eba. #632's tune-out reads the same flag at its head.Changes
ChannelRegistry.transportClosed(serverId)(src/mcpl/channel-registry.ts:2294) marks each of that server's channels unconfirmed: not open, and not confirmed closed. That's how a registration's entry starts (registerEntry,:1311onmain).handleIncomingnow notes the count when a batch arrives, and its traffic marks nothing open once the count has moved. The message itself is still admitted.runLifecycleOperation'ssameTarget): it confirms nothing. The operation tries again, and on the closed connection that's refused as unreachable, sochannel_openanswers that the server isn't connected and the decision waits for the next registration.closehandler callstransportClosedfirst and synchronously, before anything else the handler does. So a registration on the next transport, whose entries are new, can't come first and lose its confirmations to the close.scopeInjectionsForAgent). A fork keeps injections namespaced by its own channel or by no channel. Until now it dropped only those namespaced by another open channel. Once a close leaves a server's channels unconfirmed, their injections would otherwise pass as general context and reach every other fork, until each is confirmed again. Now any registered channel's injections stay with its own fork, open or not.changelog.d/644-closed-transport-unconfirms-channels.fixed.mdsays what callers will notice.What callers will notice
channel_openandchannel_closeanswer that it isn't connected and that the decision is recorded, for a channel this change left unconfirmed too.channels.lifecyclegrant): until the channel is confirmed again,channel_openandchannel_closeeach send their request to the server. Delivery opens the channel before it publishes. Each costs one more open or close request in that window, as it would after a registration.mainthey stayed open across the drop, and now the agent can't speak first into a quiet channel until its traffic shows it open.What each part is for, and what was left out
The change is for one thing: nothing the registry says about a channel's transport should outlive the transport that said it.
transportClosed. One place withdraws the confirmations, so every reader of a channel's open flag reads it right after a close:channel_open's andchannel_close's "already" answers;isChannelOpenand the open-channel list;It reuses the registry's own
invalidateTransportConfirmation, which a retarget, an answer for an older connection or target, and an answer for a different channel already use.The count of closes. Without it, the coalescer's await lets the closed transport's traffic confirm a channel again, and so can a receipt applied after a close the host started: the A channel confirmed open before a reconnect still reads as confirmed open until the server registers it again #644 symptom by other ways. It's one counter per server, read once per batch, once per admission and twice per lifecycle request. It counts the registry's own withdrawals, so the check reads exactly "has a close withdrawn this since". What the connection holds across the boundary isn't covered, and isn't meant to be: it reaches
handleIncomingafter the next transport's grant, as that transport's traffic (resetPolicyForTransportBoundary), as after a registration.The call, first and synchronous. This is the condition the fix's design was agreed on. A registration arrives only on the next transport, which is installed (
server-connection.ts:1969onmain) after the old one's close is emitted (:2627for a drop,:1801and:1829for the host's own close). A close handled after the next registration would withdraw that registration's own confirmations.Scoping by registered channels. Without it, this change would widen a leak: a dropped server's channels keep their open flag on
main, so their injections stay with their own fork, and here they'd reach every fork until confirmed. Reading registration rather than the open flag costs nothing, since the same lookup the invitation uses (getDescriptor) answers it. The open-only set came with the fork routing, with no reason given for it.The tests and the fragment. The tests are below. CONTRIBUTING asks for a fragment with a behaviour change.
Cost: one pass over the registry's channels per transport close, and a map read per incoming batch, per admitted message and per lifecycle answer.
Left out:
transportEpochat each receipt and in the traffic path. The open flag is read as a plain field in many places, so each would need a getter or a check, while the close is one place.removeServerdoes for a server that's gone.channel_openwould then answer that the channel isn't registered until the server registers again, and the agent's view of the server's channels would empty on every drop.Tests
Full check: at
2130abe6, this PR's first commit, onmain97ca7eba, I ran CI's commands on Linux with Node 24.22.0:npm install,tsc --noEmit,npm run build,npm test's test command, then the three files CI runs without--test-force-exit.d241b07echanges one test and one doc comment, answering RevEngine's review. At it, the typecheck passes, this change's three test files pass 178 of 178, and the removals below still fail as named. CI runs the whole suite on it.main: CI's run 39609929106 at97ca7eba(ubuntu-latest, Node 24) has 5,966 tests, 5,921 passing and 45 skipped. The 9 more tests here are this change's. One test CI's Node leg skips, "under Bun, a failed dial's error leaves no credential…" (test/mcpl-ws-transport.test.ts), ran here because Bun was on the run'sPATH.New tests, each protecting a contract #644 or this change's review showed:
test/mcpl-transport-close.test.ts:ChannelRegistry.mainat their assertions.test/channel-lifecycle-order.test.ts:channel_closeasks again. Both the withdrawal and the count give that outcome, so no single removal below fails it.channels.lifecycle: delivery is refused and nothing is sent until the next transport's traffic shows the channel open.channel_closeafterwards asks the server. Another server's message, held across the same close, still shows its channel open.test/conversation-fork-injection-scoping.test.ts: "keeps a closed or unconfirmed channel's injections with its own fork…" covers a channel never opened and channels whose server's transport closed.mainthe registry and scoping tests fail only becausetransportCloseddoesn't exist there. The removals below show what each one protects.Removals and mutations, each run on its own at this change's code, each failing the test meant for it, at the assertion named:
removeServer: the ordering test;open = false, without the unconfirmed mark: the contract test, atchannel_closeanswering "already closed";markAccepteddropped: the coalesced and untargeted-entry tests, at "the held message's admission doesn't confirm x" and "the closed transport's traffic doesn't confirm it";sameTarget: the receipt test, at "the closed transport's receipt confirms nothing";With #598, which changes the same files:
mainwith #598 (at046c89e1) and this branch merge without conflict. On that merge, the typecheck and build pass, and this change's three test files with #598'schannel-lifecycle-provenanceandroute-failure-targetingpass 263 of 263.Internal review: it read the first version and then its fix round, and this head answers both. A second colleague read each round.
Existing tests: none turned round. The suite passes whole, the reconnect files (
mcpl-reconnect-registration,mcpl-reconnect-reregistration,mcpl-reconnect-events) included. No existing test changed: the diff to the three test files only adds lines.What this leaves
channel_decline, which refuses it. A run shows that onmainafter a failed open. With this change, it also shows after a transport close, until the channel is confirmed again. That's The closed-channel invitation says "a channel you haven't opened" for a channel the agent opened #664.channel_declineon a tuned-out channel whose transport closed passes its guard, which reads the open flag, and records a decline over the tune-out: in a run at this change's code, the decline succeeds and the channel's decision becomesclosed. fix(tune-out): tune_out opens a closed channel's transport, and channel_decline refuses a tuned-out channel #632 adds the guard that refuses a decline on a tuned-out channel, whatever its transport.Not verified
main.changelog.d/644-closed-transport-unconfirms-channels.fixed.md.Pippa-2697 (Claude Opus 5.5) wrote this change, to a design Ada-1017 (Claude Opus 5.5) chose. Greptile's review of #632 found it, and so did Anarchid's. Nigel-2654 (Claude Opus 5.5) carried it out of #632 as #644.
Description checked and posted by Daniel-2612 (Claude Opus 5.5).
🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling, and posted from the shared
nissa-seruaccount.