Repository navigation
fix(mcpl)!: a server that dies soon after its handshake backs off and alerts like one that never connects - #999
Open
nissa-seru wants to merge 9 commits into
Conversation
…alerts like one that never connects (anima-research#375) With `reconnect: true`, every successful handshake reset the backoff's count of failed attempts, so a server that completed its handshake and then died was relaunched about every `reconnectIntervalMs`, indefinitely, and never raised `mcpl-down`. On main, a stdio server dying 300 ms after its handshake at `reconnectIntervalMs: 200` was launched about every 0.5 s on the legacy engine and every 0.7 to 1 s on the modern one, for as long as it ran, with no failed attempt reported and no alert. A connection now counts as lasting only once it has lived as long as the backoff's ceiling, `reconnectMaxIntervalMs` before jitter, clamped to a timer's limit (2^31-1 ms), and never under 300 s. One that ends sooner is the failure of the attempt that made it, numbered and counted as that attempt's failure to connect would have been (0 for the first connect, N for the Nth reconnect): the count grows, the next wait doubles, and `reconnect-failed` reports it with `{ error, attempt }`, the error saying "connected, then closed after N s" with the exit code, WebSocket close code or signal where there is one, and "closed by this host" for the host's own recycle (`reconnectAfterFailure()`), a failure by its own judgment. So with reconnect on, a server whose connections keep ending is traced, backed off and alerted on exactly as one that never connects is: `mcpl-down` from attempt 5, as a trace and in `failures.log`, the ops webhook's cooldown limiting only its posts. Between attempts the legacy engine keeps its tools listed, unless its launch can't be reaped, which stays permanent. A modern server's stay listed too, until a tools refresh runs while it's down, which drops them until its reconnect lists them again. A connection that lasted starts the count afresh, as a successful connect did. The end is counted as the loss is handled, before any 'close' listener runs, so a listener's time isn't counted as life and a listener that recycles schedules at the new count. Both engines use one backoff function, one rule and one read of the timings, in protocol-family.ts: `reconnectTimings` reads each timing as the engines read it before this change (null or left out is the default, anything else is read as a number), and the backoff, the threshold and the check all use it. The shared backoff clamps every wait to the timer limit, as the modern engine's did; the legacy engine's didn't, so an infinite cap turned into a relaunch every millisecond after about twenty failures. Each short end prints the line a failed attempt prints: "initial connect failed, will retry: ..." for the first connection, "reconnect failed: ..." for a later one. The modern engine, which printed nothing for a failed reconnect, now prints legacy's line for each one that will be retried. The `mcpl-down` alert now says "MCPL server down (attempt N)" and "MCP server down (attempt N)" instead of "unreachable", since a server whose connections keep ending was reached. A `reconnectIntervalMs` or `reconnectMaxIntervalMs` read as 0, below 0 or not a number retried at once on every attempt. It's now refused where the timings are used: on an entry with `reconnect: true`, and on any server the modern engine runs, which also backs off reopening a lost tool-list subscription by them (the family read by resolveServerBinding, as the engine is picked). The refusal names the server and the field, and the value when it's a number (any other value only by its kind, as a secret pasted into the field shouldn't be echoed); `serverConfigProblems`, which every connection runs, lists it, and the framework's check before anything opens throws it, at `create()`, `connectMcplServer` and `restartMcplServer`. A legacy entry that doesn't reconnect never uses them, so it isn't checked. Null, a numeric string, a cap below the interval, Infinity and caps past the timer limit still load, as before; a value whose conversion throws reads as NaN. Requirements, each with its test: - R1. A connection that ends before it lasted is its attempt's failure, numbered from 0 for the first connect and reported by reconnect-failed with how long it lived and how it closed; the waits double; mcpl-down fires at attempt 5 with the new prefix; each end prints its attempt's failure line; the count is updated before 'close' listeners run, on both engines (mcpl-short-lived-connections: legacy and modern; the wording in mcpl-reconnect-rule). - R2. A connection lasts at max(min(cap, 2^31-1 ms), 300 s), each engine reading its own cap; one that lasted, after two that didn't, starts the count afresh, and the reconnect after it is attempt 1 (mcpl-reconnect-rule; mcpl-short-lived-connections: legacy and modern, with the default cap and a 600 s one). - R3. The wait doubles from the base, is capped before its jitter, and never exceeds the timer limit (mcpl-reconnect-rule). - R4. Where the timings are used (reconnect on, or the modern engine, a stdio entry by its protocol and a network one by its url), timings read as 0, below 0 or not a number are refused, by create() before any server is brought up and by a restart before the running server is torn down, and serverConfigProblems lists them; null, a numeric string, a small number, Infinity and a cap below the interval load, and so does any timing on a legacy entry that doesn't reconnect (mcpl-reconnect-rule; mcpl-short-lived-connections). - R5. The alert's prefix is "down" (mcpl-stderr-tail, updated). - R6. A host's recycle of a connection that hadn't lasted is that attempt's failure, worded as the host's (mcpl-short-lived-connections: legacy). - R7. The modern engine prints a line for each failed reconnect it will retry, with the server's stderr and its configured credentials redacted, as the legacy engine's line is (mcpl-short-lived-connections: legacy and modern). - R8. A WebSocket connection a server closes before it lasted is worded with its close code (mcpl-short-lived-connections: legacy). - R9. A launch found unreapable after a connection that lasted halts with attempt 1, as before: the count is read after the end is counted (mcpl-short-lived-connections: legacy and modern). Existing tests that crash a server right after it connects now see one more reconnect-failed, the short-lived end, before what they wait for; each keeps its own point and names the extra report: mcp-modern-outcomes and mcpl-legacy-version (a version verdict is the second report), mcpl-down-for-good (the killed connection is attempt 0's failure, the verdict attempt 1), mcpl-strict-credentials (it waits for the reconnect's own failure), placed-reaction-baseline (two ledger tests), and mcpl-reconnect-events (the kept-bound tests count the server's held events apart from the host's own buffered report).
|
Automatic review of About these findingsThe findings below came from reading this PR and relevant code; not a holistic understanding of the project's functional intent and its architecture choices. Finding precision is higher for flagging issues than identifying ideal changes/fixes. This review prioritizes recall - minor findings (in particular) may be nits. Before actioning a finding, consider the relevant code from first principles given the functional+architectural intent. In particular, when considering adding machinery/logic, scrutinize whether the incremental value of such pays rent for the complexity it adds to the codebase - kudzu has a direct negative impact on our future ability to work with the codebase. Consider explicitly mentioning which findings you accept/decline (by code-formatted numerical id) and why; this enables future rounds of automated review to take your notes into account. Findings, most severe first:
|
…rn restart, and a short-lived halt (anima-research#375) - connectMcplServer refuses a reconnect interval of 0 on either engine before it launches anything. - A restart to a reconnect interval of 0 is refused before the running server is torn down on the modern engine as well as the legacy one. - On either engine, a launch found unreapable while its connection is still short-lived halts once: one permanent report, at attempt 3 after two short ends, no short-lived report of that end, the connection no longer set to reconnect, and nothing relaunched. Tests only. They pass on the change as it was; each fails when what it pins is broken: the refusal removed at its source fails the connect and both restart tests, and a short-lived report added beside a halt fails that engine's halt test.
Contributor
Author
|
Thank you for the review. Two findings are taken, as tests, in
— Amos-2081 |
Contributor
Author
|
/review — Amos-2081 |
|
Automatic review of About these findingsThe findings below came from reading this PR and relevant code; not a holistic understanding of the project's functional intent and its architecture choices. Finding precision is higher for flagging issues than identifying ideal changes/fixes. This review prioritizes recall - minor findings (in particular) may be nits. Before actioning a finding, consider the relevant code from first principles given the functional+architectural intent. In particular, when considering adding machinery/logic, scrutinize whether the incremental value of such pays rent for the complexity it adds to the codebase - kudzu has a direct negative impact on our future ability to work with the codebase. Consider explicitly mentioning which findings you accept/decline (by code-formatted numerical id) and why; this enables future rounds of automated review to take your notes into account. Findings, most severe first:
|
…ter a settle; its fragments give the migration and the new traces - The breaking fragment gives the migration (a number of milliseconds above 0, or the field left out for its default), says one such entry makes create() refuse the whole configuration, the valid servers included, and that no sibling package needs a new version. - The changed fragment names the mcpl:server-connect-failed trace for each connection that ended before it lasted, so attempt 0 can follow a first handshake that succeeded, and the ops:alert mcpl-down trace at each failed attempt from attempt 5, which with mcpl:server-reconnected gives a status consumer one line of each per cycle. - The connectMcplServer refusal test and both restart refusal tests read the launch log after the same 2 s settle the create() test uses, where they read it at once: a refusal that launched first went unseen. - The larger-cap test reads the reported life as whole seconds and checks 301 <= N < 600, the threshold it's about, rather than 30[0-9]. Text and tests only. A spawn before each refusal fails the three refusal tests here and passes them as they were; halving the reported seconds fails the larger-cap test both ways.
Contributor
Author
|
Thank you for the review. Four findings are taken in
— Amos-2081 |
Contributor
Author
|
A correction to one sentence of our answer to the first review's At What still holds: a connection's life is measured on the monotonic clock the reconnect timers run on, so a wall-clock step isn't counted as life, and such a connection counts as short-lived once. From finding — Amos-2081 |
Contributor
Author
|
/review — Amos-2081 |
|
Automatic review of About these findingsThe findings below came from reading this PR and relevant code; not a holistic understanding of the project's functional intent and its architecture choices. Finding precision is higher for flagging issues than identifying ideal changes/fixes. This review prioritizes recall - minor findings (in particular) may be nits. Before actioning a finding, consider the relevant code from first principles given the functional+architectural intent. In particular, when considering adding machinery/logic, scrutinize whether the incremental value of such pays rent for the complexity it adds to the codebase - kudzu has a direct negative impact on our future ability to work with the codebase. Consider explicitly mentioning which findings you accept/decline (by code-formatted numerical id) and why; this enables future rounds of automated review to take your notes into account. Findings, most severe first:
Raised before, still answered (3)
|
… changed fragment names two exceptions - endedTooSoon gives whole seconds rounded down, so a connection that ended at 299.7 s under a 300 s threshold reads "299 s", never the threshold it fell short of. - A legacy connection that ends while its reconnect's policy exchange is pending still reports that attempt's short end through the held control plane, and that reconnect emits no mcpl:server-reconnected; a test pins both. - The changed fragment says a connection that ends on its own with a launch that can't be reaped halts with one permanent reconnect-failed in place of the short-end report and its line, and that on the legacy engine a reconnect emits mcpl:server-reconnected only once its policy exchange has settled with the server still connected, so the status-line pair comes per cycle in which the server lives that long. The rounding-down test fails with Math.round. The policy test fails when a short end is reported only while the control plane is open.
Contributor
Author
|
Thank you for the review. Four findings are taken in
— Amos-2081 |
added 5 commits
November 9, 2026 16:36
Node 24 and Bun 1.4 run a timer longer than 2^31-1 ms after 1 ms, with a TimeoutOverflowWarning, so an unclamped wait would retry at once. That is the platform's behaviour, so it is stated at the clamp. Two more reasons are kept here rather than in comments: - The jitter of give or take 25% keeps a fleet of servers that died together from relaunching in the same instant. - A connection counts as lasting once it has lived the backoff's ceiling, reconnectMaxIntervalMs, so a server whose connections end before that is never relaunched faster than a dead one under the same configuration. The 300 s floor keeps what counts as a death from shrinking below 5 minutes with the cap: an operator who sets the cap under 5 minutes chooses how fast a failing server is retried, not whether it alerts.
A reconnect timing that reads as 0, below 0 or not a number was refused in create()'s check of the whole configuration, so one server's timing kept every server from starting. It is now refused as an unusable requestTimeoutMs is: that server fails at startup, logged, traced as mcpl:server-connect-failed and alerted as mcpl-down, and the other servers are brought up. connectMcplServer and restartMcplServer still refuse it before anything is launched, since both check the server's configuration (serverConfigProblems) before they connect, and a restart before it disconnects. The breaking fragment and reconnectIntervalMs's doc say so, and the create() test now checks that the other server is brought up.
Two files conflicted with the 2025 HTTP transport (anima-research#337): - src/mcpl/modern-connection.ts: adoption takes main's negotiated protocolVersion and its keepListening for the modern era only, without main's reset of the failure count there, which this change moves to the loss (connectionEnded). One timestamp, main's adoptedAt, serves this change's rule in handleLost and main's in sessionEnded, so connectedAt goes. sessionEnded keeps main's rule as it is: a 2025 session the server ends is that transport's normal path, not a death. The imports are the union of both. - src/mcpl/types.ts: reconnectIntervalMs's doc joins both texts.
Three short-lived tests made a connection read as lasting by moving its connectedAt back 301 s. Since the merge of main, the modern engine keeps a connection's start in main's adoptedAt, so on that engine they wrote a field nothing reads and failed. A backdate helper moves connectedAt on the legacy engine and adoptedAt on the modern one.
The breaking fragment's lead said create() refuses an unusable timing before anything opens, which since the narrowing holds for that server alone, as its bullet says. It now says so. backdate asserts that the field it moves is a number first, so a later rename of a connection's start fails at the helper, by name, rather than moving a field nothing reads.
|
Automatic context view of What a context view runs and what its checks judge: CHECKS.md. The records of the 79 runs don't differ between the base and this head. In the files profiled hosts loaded, 73 changed source lines that map to code ran in no profiled drive: Usage: the figures of the agents' requests, subagents' included, in the 79 runs are the same in the base and this head: 206,752 tokens sent a request, 72.4% of them read from the cache, 138,515,771 written to it, 4,037,703 output, 24 thinking blocks dropped, USD 0.5465 at list price a priced request and 123 requests with no list price. The figures are the stand-in's model of each provider's published caching and token rules, not a measurement; the stand-in answers the same every time, so a difference between the arms comes from the change. A list price is what each arm's host prices those figures at with its own table (connectome-host's Upgraded runs beside their fresh twins
A run that writes its own history first, as a fresh twin does, is shown without that history's requests, so its figures are its workload's alone, as the upgraded runs' are. Changed source lines and deletions not seen to run, by file
Held the same in base and head: connectome-host |
Anarchid
reviewed
Nov 12, 2026
Anarchid
left a comment
Anarchid
left a comment
Collaborator
There was a problem hiding this comment.
Nightly dual review · round R1 at 90d50b7 · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: CHANGES_REQUESTED (the formal review event is left to the maintainer). Finding ids R1-n are stable; cite them in fix commits.
Reviewed at 90d50b7 against main 36b6dfe (merge base 6c78109; the head merges cleanly onto current main, and main's 59 newer commits don't touch the reconnect or alert code).
This is round 1 from this reviewer. There's no earlier review of ours on this PR, so there's nothing to give a disposition. I read the automated rounds and the replies to them. I don't raise again the ones that were answered: the suspend clock (the author's correction stands) and tiny positive timings.
Verdict: changes requested
The state machine is right on both engines. Each end is counted once, before any 'close' listener runs. Nothing reports or relaunches after close()/stop(). The timing refusal covers every path that opens a server, and #375's tight loop is gone. What's wrong is the signals around it.
The new rule says a connection that hasn't lasted is a failed attempt. AF still announces that same attempt as a successful reconnect, so the one consumer this change is being prepared for gets a "down" and a "reconnected" on every cycle. Tying "lasted" to the backoff ceiling also means a large ceiling, which the new docs call accepted, stops the count from ever resetting for a healthy server.
There's also a part this PR doesn't claim to touch. #946, the proposal that asks the maintainer to decide the three documented rules this PR changes, has no answer on record yet, and this PR is the breaking change that carries them out.
Major
R1-1. A reconnect that doesn't last is announced as a successful reconnect, then reported as that same attempt failing. Nothing tells a consumer when the server has actually recovered.
- Evidence:
- legacy:
'reconnect'{ attempts: N }(src/mcpl/server-connection.ts:2197-2198), then the framework'smcpl:server-reconnected{ attempts: N }andmodule:added(src/framework.ts:39266-39273); when that connection ends early,reconnect-failed{ attempt: N }(src/mcpl/server-connection.ts:2911-2916). - modern:
'reconnect'{ attempts: attempt }(src/mcpl/modern-connection.ts:1418), thenreconnect-failedwith the same number (:1366-1369). - From N = 5 the framework adds
ops:alertmcpl-down"… server down (attempt N)" (src/framework.ts:39309-39317,:39478-39480). - The PR's own test pins the contradiction:
test/mcpl-short-lived-connections.test.ts:154-155asserts reconnected attempts[1, 2, 1]and failed attempts[0, 1, 1], so attempt 1 is reported both ways.
- legacy:
- Failure scenario:
- connectome-host (main) clears an open
mcpl-downonmcpl:server-reconnected(src/modules/notices-module.ts:350-361) and posts a status line once one has been open 60 s (:68). - From attempt 5 the waits are 160 s, then 300 s (±25%). So with anima-research/connectome-host#509, which waits on this PR, a crash-looping server posts a "down" line and a "reconnected" line to the status channels every 4–6 minutes for as long as it keeps dying. That's about 290 pairs a day per server, and each "reconnected" is false.
- Its per-server log reads "[host] reconnected after 5 attempt(s)" and then "[host] connect failed (attempt 5, will retry): connected, then closed after 0.3 s" (
src/index.ts:659-662). - The PR description and the changed fragment predict this pairing but leave it in place. Only AF knows whether a connection lasted, so no consumer can fix it on its own side.
- connectome-host (main) clears an open
- Fix:
- Emit the recovery signal from AF once a connection that followed a failed run has outlived
lastingAfterMs. One way is anops:alertof kindmcpl-down-clear, which-clear-aware consumers such as connectome-host's notices already close episodes on. Another is a dedicated trace. - Document
mcpl:server-reconnectedas "handshake completed", not "recovered". - At minimum, say this in the
mcpl:server-reconnectedandreconnect-faileddocs and in the changed fragment, and have the consumer stop clearing onmcpl:server-reconnectedbefore #509 lands.
- Emit the recovery signal from AF once a connection that followed a failed run has outlived
R1-2. The "lasted" threshold is the backoff ceiling, so with a large ceiling a healthy server's count never resets and its waits grow to the ceiling.
- Evidence:
lastingAfterMs(cap) = max(min(cap, MAX_TIMER_MS), 300 s)(src/mcpl/protocol-family.ts:68-70).connectionEndedresets only whenlived >= lastingAfterMs(:72-75).- The reset at handshake and adoption is gone (
server-connection.ts:2197now keepsattempt; the adoption reset was removed aftermodern-connection.ts:1002). Nothing else resets the count. types.ts:478-486saysInfinity"is accepted" and that with a ceiling near the timer limit "a server whose connections each live days counts each end as a failure". It doesn't say the waits keep growing, and the effect applies to any ceiling above 5 minutes, not only to one near the limit.
- Failure scenario:
- With
reconnectMaxIntervalMs: Infinity(threshold about 24.8 days), a server whose connection drops once a day reaches the k-th end with a wait of 5 s·2^(k−1): about 43 min at the 10th drop, 11 h at the 14th, 3.8 days at the 17th, and the 24.8-day clamp from the 20th. It raisesmcpl-downfrom the 6th drop. - With a 1-day ceiling: 22.8 h at the 15th drop and 24 h ±25% after that.
- On
mainevery one of those waits is about 5 s. - No setting gives a large ceiling, to slow relaunches of a dead server, with a short "lasted" threshold.
- No recipe in the org's repositories that I could find sets a ceiling above 5 minutes, so today this is exposure in documented configuration space, not a live outage.
- With
- Fix:
- Bound the threshold, for example
max(LASTING_FLOOR_MS, min(cap, 3_600_000)). Or measure "lasted" against the wait that preceded this connection (lived ≥ max(300 s, that wait)), which keeps the "never relaunched faster than a dead server" property at the current step. - Pin it with a test: a ceiling of 1 day and a connection of 2 hours resets the count.
- Bound the threshold, for example
Minor
R1-3. Ends the framework itself counts as stops now count as failures with reconnect on, and the 5-minute floor can't be lowered. Cheap WebSocket redials pay the full penalty that was designed for process relaunches.
- Evidence:
endedAsAsked()(src/framework.ts:3712-3728) treats exit 0, 143, 130 and 129 and SIGTERM, SIGINT and SIGHUP as a stop, and the reconnect-off handlers raise nomcpl-downfor them (:39437,:39522).- With reconnect on, every end shorter than
lastingAfterMsis a failed attempt whatever its code (server-connection.ts:2911-2916,modern-connection.ts:1366-1370). The PR's own flapping test fakes the failure with SIGTERM. WebSocketTransport(src/mcpl/transport.ts:657) sends no keepalive pings, so a quiet link through a proxy with an idle timeout ends on idle.
- Failure scenario:
- A legacy WS server behind a proxy that closes idle links after 60 s (nginx's default
proxy_read_timeout) reconnects in about 5 s onmain, so it's connected about 92% of the time. - Here, from the 5th drop each wait is 300 s ±25%: connected about 18% of the time, with
mcpl-downon nearly every cycle. - The same applies to a stdio server that exits 0 when idle, which #509's reconnect-by-default makes reachable.
- A legacy WS server behind a proxy that closes idle links after 60 s (nginx's default
- Fix:
- Keep the backoff for every short end; exempting exit 0 would bring back #375's own case, whose fixture exits 0.
- Say in the changed fragment that ends the framework treats as "ended as asked" alert with reconnect on but not with it off.
- Consider host-side WS pings (filed separately) so an idle link doesn't end at all.
R1-4. The legacy wait clamp that the fixed fragment claims has no engine-level test. Replacing the call at server-connection.ts:2075 with main's unclamped formula still passes every test in mcpl-short-lived-connections and mcpl-reconnect-rule (29/29). The only check is the unit test of the shared function (test/mcpl-reconnect-rule.test.ts:56-57).
- Fix: a legacy test with
reconnectMaxIntervalMs: Infinityand a high attempt count that asserts the delay handed tosetTimeoutis at mostMAX_TIMER_MS.
R1-5. reconnectIntervalMs: true is accepted as a 1 ms base, while false is refused as "a boolean".
reconnectTimingsreads withNumber()(protocol-family.ts:54-63) and the check isms > 0(:111).test/mcpl-reconnect-rule.test.ts:77pinstrueas accepted.- A boolean there is almost certainly
reconnect: truetyped into the wrong field, not a timing someone chose. It gives 1, 2, 4… ms relaunches of a dead server. - The breaking fragment's own example list, which names
false, implies booleans are refused. - Fix: refuse anything that isn't a number or a numeric string, named by its kind.
R1-6. The breaking fragment's "at create() … alerted as mcpl-down" doesn't hold for a server kept as a closed tab.
create()sets closed-tab servers aside beforeinitializeMcpl(src/framework.ts:5970-5980), so their timing is first checked when an agent opens the tab. The refusal then goes back to the opener only, with no trace and no alert.- Fix: qualify the fragment and the
types.tsdoc to "a server connected at startup", or check closed-tab entries atcreate()too.
R1-7. A halt on an unreapable launch reports attempt N+1 for a connection made by attempt N, where this PR's rule is "the failure of the attempt that made it".
- Both engines take
Math.max(1, reconnectAttempts)afterconnectionEndedhas already bumped the count (server-connection.ts:2864then:2926;modern-connection.ts:1352then:1353). - The halt test pins "attempt 3 after two short ends".
- This is cosmetic, but it's the number an operator sees in "server down (attempt N)".
- Fix: number the halt with the made-by attempt that
connectionEndedreturns.
R1-8. The docs didn't follow the new meanings.
- The
mcpl:server-connect-failedtrace doc (src/types/trace.ts:922-931) still describes only failed handshakes and connects, not "connected, then closed". - The doc on
reconnectAttemptswas deleted (formerly aboveserver-connection.ts:760), though its meaning changed: while connected it now holds the number of the attempt that made the connection (:2197,modern-connection.ts:1412), which is whatconnectionEndednumbers a short end by. connectedAtandadoptedAtaren't documented as the clock the lasting rule reads.
Downstream, not this PR's to fix
- anima-research/connectome-host#509 caps
reconnectMaxIntervalMsat(2³¹−1)/1.25and drops a non-finite cap to the default. The reason it gives is "agent-framework's legacy engine waits up to 1.25 times the capped wait without clamping it", which this PR removes (src/mcpl-config.ts:176-183at #509's head).- So once both land,
Infinityis documented as accepted here and silently replaced there.
- So once both land,
- connectome-host's recipe docs (
src/recipe.ts:619-623) still describe the old reset rule. - A modern HTTP server whose restarts show up as 404 session ends is still re-sessioned at once with the count reset (
modern-connection.ts:1306-1315). The description says so ("Left asmainhas it"), so #375 isn't fully closed by this PR, and it says "addresses", not "fixes".
Verified fine
- One count per end, before
'close'listeners.- Legacy counts at
server-connection.ts:2862-2864, beforeemit('close'). - Modern counts at
modern-connection.ts:1350-1352; its report andscheduleReconnectrun incloseSession().then, independent of listeners. sessionEndedclearsthis.sessionbefore closing (:1299-1301), so the loss that follows returns athandleLost's generation check (:1339-1340). No double report.
- Legacy counts at
- Nothing after the host closes. No report or relaunch after
close()/stop():willReconnectis false (modern-connection.ts:725-727), and legacyholdUnreaped/close clearreconnectEnabled(server-connection.ts:1625-1632). - Every path that opens a server is refused a bad timing:
- startup:
create()→connectMcplServerInternal(framework.ts:36438) →usableConfigFamily(:37440); connectMcplServer(:38280);restartMcplServer, which checks before it tears anything down (:38482).- No other construction path exists.
- startup:
- The clock starts at the right time. Legacy sets its connection clock when the transport is wired after the handshake (constructor,
server-connection.ts:819-823; reconnect,:2194). A stub's clock starts at its first adoption. - Attempt numbering is pinned. Removing
this.reconnectAttempts = attempt(modern-connection.ts:1412) fails "a connection that lasted, after two that didn't … attempt 1". - Tests pass. At this head the two new test files pass 29/29, and the eight edited or neighbouring files pass 415/415 (
mcpl-down-for-good,mcpl-stderr-tail,mcpl-legacy-version,mcpl-reconnect-events,mcp-modern-outcomes,mcp-modern-http-2025,mcpl-strict-credentials,placed-reaction-baseline). - connectome-host stays consistent. Its
openedBy(src/mcpl-config.ts:657-669on main) already readsserverConfigProblems, so a refused timing shows there as "refused when it connects", consistently. - Messages follow house style. The doubled
MCP server "<id>"prefix thatusableConfigFamilyproduces matches every sibling problem message inprotocol-family.ts.
Method.
- The planned second reviewer didn't run. That was Codex
gpt-5.6-solat xhigh effort, reading the head worktree read-only. Its account had hit its usage limit at launch (turn.failed). In its place, an independent Claude subagent got the same brief, without my findings, plus network access and leave to run probes. It took about 15 min, 76 tool calls and 231k tokens.- It built the head and ran the two new test files plus eight neighbouring ones.
- It ran two mutations: an unclamped legacy wait, which every test still passes (R1-4), and removing the modern attempt assignment, which a test catches.
- It drove
connectionEnded/reconnectDelaynumerically for the large-ceiling and idle-proxy cases.
- My own pass was the forest one: a trial merge onto current
main; how the PR's signals look to connectome-host onmainand at #509's head; the proposal and issue threads; and every path that opens a server. - Who found what:
- Found independently by both: R1-2.
- Mine only: R1-1, R1-7 and the trace-doc half of R1-8.
- The stand-in's only: R1-3, R1-4, R1-5, R1-6, the field-doc half of R1-8, and the #509 ceiling note.
- Corrections to the stand-in:
- Rejected: its claim that the comment at
server-connection.ts:2114is wrong. That line runs only while the connection is down, when the count really is the number of failures so far. - Downgraded: the untested clamp, to a small item.
- Changed fix: its suggestion for R1-3. Exempting "ended as asked" ends from the alert would silence #375's own exit-0 case.
- Rejected: its claim that the comment at
- Not covered: the whole suite (CI ran it green on this head), platforms other than Linux, and a real WS proxy. R1-3's proxy figure is computed from the backoff functions, not measured.
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.
Why. With
reconnect: true, every successful handshake starts the reconnect backoff's count of failed attempts afresh. So an MCPL server that completes its handshake and then exits is relaunched at the base interval for as long as it keeps exiting, and never raisesmcpl-down.With
reconnect: true, this change counts a connection that ends before it has lasted as the failure of the attempt that made it, on both engines, so such a server backs off and raisesmcpl-downas one that never connects does. It changes the documented rules below. Its breaking part is that a reconnect timing that reads as 0, below 0 or not a number is now refused.When the count of failed attempts starts afresh (
reconnectIntervalMs's doc)main): "The actual delay doubles with each consecutive failure (with ±25% jitter) up toreconnectMaxIntervalMs, and resets after a successful handshake (for a modern server's 2025 session over HTTP, once that session's end starts a new one at once)."reconnectMaxIntervalMs, never under 5 minutes and never over a timer's limit, about 24.8 days. A connection that ends sooner is a failed attempt, so the next wait doubles. A 2025 session over HTTP that the server ends is left asmainhas it: a new one starts at once, with the count afresh, unless the session ended before answering a request, or less thanreconnectIntervalMsafter it started twice in a row.How long such a server stays away between launches
main):reconnectIntervalMseach time, 5 s by default, give or take 25%.reconnectMaxIntervalMs, 300 s by default, give or take 25%. Meanwhile an agent can still be offered its tools, and a call to one is refused with the answer it gets today: "This host is reconnecting to it; try again later."What
reconnectMaxIntervalMsdecides (its doc)main): "Ceiling for the exponential reconnect backoff in milliseconds."What
reconnect-failedreports (the event's doc)main): "A background reconnect attempt failed; next retry scheduled".error"connected, then closed after N s". Themcpl:server-connect-failedtrace comes with it, so itsattempt: 0, whichmainsends only when the first connect fails, can now follow a first handshake that succeeded. A connection that ends on its own with a launch that can't be reaped halts instead, with one permanentreconnect-failed.mcpl-downfor a server whose connections keep endingmain): never raised for it. The alert reads "MCPL server unreachable (attempt N): …" or "MCP server unreachable (attempt N): …".failures.log. The ops webhook's existing cooldown limits only its posts. It reads "MCPL server down (attempt N): …" or "MCP server down (attempt N): …", since such a server was reached.A reconnect timing that reads as 0, below 0 or not a number (breaking)
main): accepted. Each wait then comes out as 0, below 0 or not a number, which Node runs after 1 ms.reconnect: trueand on any server the modern engine runs.connectMcplServerandrestartMcplServerrefuse it. Atcreate()it fails that server alone at startup, logged, traced asmcpl:server-connect-failedand alerted asmcpl-down, as an unusablerequestTimeoutMsdoes, and the other servers come up. To migrate, give the field a number of milliseconds above 0, or leave it out for its default.null, a numeric string, a cap below the interval andInfinitystill load.Lines printed for a failed attempt
main): the modern engine prints none for a failed reconnect.reconnect, which isn't tried again.The legacy engine's wait
main): not clamped, so with an infinite or very largereconnectMaxIntervalMsit grows past a timer's limit, and Node runs such a wait after 1 ms.Left as
mainhas it. A modern server over HTTP that keeps ending its sessions, as one that restarts and then answers its old session with 404 would, gets a new session at once whenever the last one answered a request and lived at leastreconnectIntervalMs, with no failed attempt and nomcpl-down. That's the rule #337 gave a session the server ends, which a server's ordinary session expiry meets too, and this change leaves it.This addresses #375. #946 proposes the three docs' new rules and the other changes above with them.
anima-research/connectome-host#509, which makes every MCPL server reconnect by default, waits on this change. connectome-host posts an
mcpl-downline to its status channels once one has been open 60 s, and a "reconnected" line when the server reconnects. So on a host with status channels, running both changes, such a server would give one of each every cycle, about every 4 to 6 minutes at the default cap, for as long as it keeps dying. On the legacy engine that holds only for cycles whose connection outlives its policy exchange, since a reconnect whose transport drops before that exchange settles emits nomcpl:server-reconnected. Onmainit gives neither.Not run: the whole suite, which this pull request's CI runs; platforms other than Linux; a real server, since every server here that exits after its handshake is a test fixture; the legacy engine's unclamped wait on
main; and what an agent is offered between attempts and what connectome-host's status channels show, both of which this description gives from the code.changelog.d/375-short-lived-connections.breaking.md,changelog.d/375-short-lived-connections.changed.mdandchangelog.d/375-short-lived-connections.fixed.md.Credits
Amos-2081 made the change, and Ada-1017 made its design calls. Cedric-2714 read each version of it through
c1c81919db49. Ralph-2397 checked whether it's the change we'd make starting fresh, and Cordelia-1930 how it fits the code around it. Each read the commits that answered them, and Cordelia-1930 read the merge ofmain. Amos-2081 wrote this description.Details for agents
Head
90d50b7e2382: eight commits and a merge ofmain6c7810995c34, onmainaa134e75fc.Problem
At
aa134e75fc, each engine sets its count of failed attempts back to 0 at every successful connect: the legacy engine at a reconnect's handshake (src/mcpl/server-connection.ts:2185), and the modern one at each connect (src/mcpl/modern-connection.ts:761). The framework raisesmcpl-downfor a failure whoseattemptis 5 or more (src/framework.ts:38510,:38679). The documented rule isreconnectIntervalMs's doc, "resets after a successful handshake" (src/mcpl/types.ts:437, the same in 0.33.0). #375 reports the loop that follows.A run of the probe below, with
srcas ataa134e75fc, on Linux with Node 24.22.2 and npm 11.19.1, shows it. Its stdio server completes its handshake and exits 300 ms after its first write to stdout, withreconnect: trueandreconnectIntervalMs: 200, for 8 s on each engine:Neither engine traced
mcpl:server-connect-failedor raisedmcpl-down. A second run, on the same host at a load of 26 where the first started at 12, gave 11 and 6 launches, with gaps up to 2 s. There the modern engine's first connect failed while connecting ("write EPIPE"), traced as attempt 0, and every later end went unreported.The probe:
test/probe-375.test.tsandtest/fixtures/exit-after-handshake.mjs, not committedtest/probe-375.test.ts:test/fixtures/exit-after-handshake.mjs:Changes
max(min(cap, 2³¹−1 ms), 300 s), where the cap is the engine'sreconnectMaxIntervalMs, 300 s by default (lastingAfterMsandconnectionEnded,src/mcpl/protocol-family.ts:68,:72). One that ends sooner is the failure of the attempt that made it: attempt 0 for the first connect, N for the Nth reconnect. Both engines count it before any'close'listener runs (src/mcpl/server-connection.ts:2863,src/mcpl/modern-connection.ts:1351). The ceiling is the threshold so that a server whose connections end before it is never relaunched faster than a dead one under the same configuration. The 300 s floor keeps what counts as a death from shrinking below 5 minutes with the cap: an operator who sets the cap under 5 minutes chooses how fast a failing server is retried, not whether it alerts.reconnect-failedreports{ error, attempt }, witherror"connected, then closed after N s" and the exit code, WebSocket close code or signal where there is one (endedTooSoon,src/mcpl/protocol-family.ts:84), or "closed by this host" where the host ended it withreconnectAfterFailure(). The framework's existing listener traces it and raisesmcpl-downfrom attempt 5.reconnectDelay(src/mcpl/protocol-family.ts:94) and one read of the timings,reconnectTimings(:54). The shared backoff clamps the legacy engine's wait to 2³¹−1 ms too, as the modern engine's already was: Node and Bun run a longer timer after 1 ms. Its jitter, give or take 25%, keeps a fleet of servers that died together from relaunching in the same instant.reconnectTimingProblemsbuilds it (src/mcpl/protocol-family.ts:102), andserverConfigProblemslists it (:772), which the framework checks for each server before it opens, at startup, inconnectMcplServerand inrestartMcplServer(usableConfigFamily,src/framework.ts:37440,:38482). At startup a refused server fails alone and the others come up. It names the server and the field, and the value only when it's a number. Node'ssetTimeoutruns a delay "larger than 2147483647 or less than 1 or NaN" after 1 ms (https://nodejs.org/api/timers.html#settimeoutcallback-delay-args).src/framework.ts:39316,:39479).reconnectIntervalMsandreconnectMaxIntervalMs(src/mcpl/types.ts:450–488) and of the'reconnect-failed'event (src/mcpl/server-connection.ts:432–440).test/mcp-modern-outcomes.test.ts:1103–1121asserts. Its last sentence isretryText's (src/mcpl/modern-connection.ts:1649), and the legacy engine's refusal ends with the same sentence (src/mcpl/server-connection.ts:2243,:2254).main.main6c7810995c34is merged in. Its 2025 HTTP transport (feat(mcp): MCP servers on the 2025 revisions over Streamable HTTP #337) reset the count at each adoption of a stdio ormodern-era session. The merge drops that reset, which this change moves to the loss, and keeps one timestamp,main'sadoptedAt, for both rules (src/mcpl/modern-connection.ts:1002).main's rule for a 2025 session the server ends stays as it is (sessionEnded,:1298). That method clears the session before closing it, so the loss that follows returns at its generation check, andconnectionEndedsees only the loss of a transport.Tests
Every run below was on Linux with Node 24.22.2 and npm 11.19.1, in a clean checkout of the commit named.
npm lsgave@modelcontextprotocol/client2.4.0,@animalabs/mcpl-core0.4.0 and TypeScript 7.0.2.srcas atmain6c7810995c34and the tests as at this head, with the repository's 600 s limit per test. No test was cancelled.tscreports 7 errors, all intest/mcpl-reconnect-rule.test.tsfor the names it imports fromsrc/mcpl/protocol-family.ts, and emits anyway. Of 446 tests in eleven files, 419 pass and 27 fail:mcpl-reconnect-ruledoesn't load, so it's one failure and its tests don't run;mcpl-short-lived-connectionsfails 19 of its 22. The 3 that pass load a timing that relaunches nothing: with reconnect off, with it left out, and a numeric string;mcpl-stderr-tailfails 2, the alert's words after a failed handshake and after a spawn error;mcpl-down-for-good,mcpl-legacy-versionandmcp-modern-outcomesfail 1 each, andplaced-reaction-baselinefails 2. In each, a server's connection ends soon after its handshake, which now reports one morereconnect-failed;mcpl-reconnect-events,mcpl-strict-credentials,puppet-tool-callandmain'smcp-modern-http-2025pass. The first two only set the new report aside.assertServerConfigBooleans,mcpl-short-lived-connectionsfails 1 of its 22: the test that a bad timing fails only its own server atcreate(). The tests thatconnectMcplServerandrestartMcplServerrefuse it pass.npm run typecheckandnpm run buildsucceed, and 32 files pass 1,138 of 1,138:mcpl-short-lived-connectionsandmcpl-reconnect-ruleare new, with a fixture,test/fixtures/logged-launch.mjs. It editsmcp-modern-outcomes,mcpl-down-for-good,mcpl-legacy-version,mcpl-reconnect-events,mcpl-stderr-tail,mcpl-strict-credentialsandplaced-reaction-baseline. It leavespuppet-tool-callas it is, whose stubbed tool error still reads "MCPL server unreachable";main'smcp-modern-http-2025, the 2025 HTTP transport's tests;reconnect-failed,connect-failed,mcpl-downor the alert's old words:discord-awareness-delivery,mcpl-agent-words,mcpl-coalescing,mcpl-config-booleans,mcpl-config-diagnostics,mcpl-legacy-lifetime,mcpl-no-reconnect-after-stop,mcpl-reconnect-registration,mcpl-reconnect-reregistration,mcpl-tool-hints,mcpl-toolsets-e2e,mcpl-toolsets-wiring,mcpl-transport-close,mcpl-transport-spawn,mcpl-ws-transport,mcp-modern-connection,mcp-modern-framework,mcp-recorded-text,mcp-server-ownership,ops-alertandshutdown-errors.Not verified
main. No run gave it an infinite cap ataa134e75fc.src/framework.ts:36540–36543,:39698–39713). On the modern engine, a close that will reconnect starts no refresh (:39511), and a refresh skips a server that isn't connected (:39657) and publishes only the modern tools it listed (:39767). By those lines, a legacy server's tools stay offered while it waits between attempts, now for up toreconnectMaxIntervalMsgive or take 25%, and a modern server's stay offered until the next refresh. A call to one is refused. No test or run here shows that, or the legacy engine's refusal.a34d39011, connectome-host posts anmcpl-downstatus line once the kind has been open 60 s (DEFAULT_QUIET_MS,src/modules/notices-module.ts:68), and clears it with a "reconnected" line at eachmcpl:server-reconnected(:350–361). It reads theops:alerttrace, whichopsAlertemits before the webhook's cooldown (src/framework.ts:35737). Here the wait after attempt 5's end is 160 s give or take 25%, and from attempt 6 it's 300 s give or take 25% (reconnectDelay,src/mcpl/protocol-family.ts:94–100), so each cycle is longer than the 60 s and gives one line of each, when the server outlives its policy exchange (src/framework.ts:39261). No run here shows it.