fix(plugin): fence lifecycle compatibility by daemon - #60
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (status === null) { | ||
| return { | ||
| ...compatibility, | ||
| readiness: { | ||
| transport: { state: "ready", reason: "healthy" }, | ||
| storage: { state: "unavailable", reason: "storage_unavailable" }, | ||
| synapse: { state: "degraded", reason: "synapse_degraded" }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
When the compatibility probe short-circuits before host.status (daemon or module mismatch, status === null), this fabricates storage: "unavailable" / synapse: "degraded" readiness records that were never actually observed.
In McHostLifecyclePolicy.observationalCommand (policy.ts), addCheck runs unconditionally over observed.readiness.*, so these fabricated values get published as real readiness.storage/readiness.synapse "fail" checks (with specific reasons like storage_unavailable) in the status/doctor output — even though storage/synapse were never probed. The top-level ok/reason correctly prefer the compatibility verdict, but the checks array itself misrepresents unknown state as a specific, actionable failure, which will mislead anyone reading mc-host doctor output to troubleshoot storage/synapse when the real (and only known) problem is a daemon/module version mismatch.
Consider marking these as "unsupported"/not-yet-known rather than synthesizing "unavailable"/"degraded", or omitting the storage/synapse checks entirely when status === null.
There was a problem hiding this comment.
Fixed in d4cc027. probeManagedReadiness now returns only what the handshake proved when status === null:
if (status === null) {
return { ...compatibility, readiness: { transport: { state: "ready", reason: "healthy" } } };
}Storage and Synapse are absent rather than synthesized, so observationalCommand emits no readiness.storage/readiness.synapse check for a component the probe never observed and remediation points at the version mismatch. Covered by a policy test asserting those ids are missing from checks.
| const expectedDaemonKey = | ||
| options.expectedDaemonId === undefined | ||
| ? "" | ||
| : Array.from(options.expectedDaemonId, (byte) => | ||
| byte.toString(16).padStart(2, "0"), | ||
| ).join(""); | ||
| const key = `${routeCacheKey(target, identity, consumerIdentity)}\0${expectedDaemonKey}`; |
There was a problem hiding this comment.
Folding expectedDaemonId into the managed-route cache key means every daemon restart mints a brand-new this.routes entry (a fresh key per daemon incarnation) instead of reusing/replacing the old one for the same (target, identity, consumerIdentity). Entries are only ever removed from this.routes via closeRoute's explicit this.routes.delete(key) (client.ts:739) — there's no bulk eviction on connection invalidation/generation change and no cleanup keyed on "stale expectedDaemonId no longer matches the current daemon". Over a long-lived process that lives through many mc-host restarts (e.g. upgrades), this.routes will accumulate one dead CachedManagedRoute per prior daemon generation indefinitely.
The per-entry cost is small, but this is an unbounded-growth pattern tied directly to a mechanism (daemon rotation) this PR now actively causes to happen more visibly. Worth considering pruning entries whose expectedDaemonKey no longer matches this.compatibleDaemonId-equivalent state when a new connection is established, or keying by target/identity only and revalidating identity per-use (as assertExpectedDaemon already does) rather than fragmenting the cache.
There was a problem hiding this comment.
Fixed in d4cc027. The managed route-cache key is daemon-independent again — routeCacheKey(target, identity, consumerIdentity), no expectedDaemonId — so one logical binding owns one slot and a rotation strands nothing. Correctness is kept where you suggested: isPrimaryLiveHandle refuses a handle from a retired generation and assertExpectedDaemon fences publication per use.
Review summaryThis PR extends lifecycle compatibility checking to fence live traffic to a specific authenticated daemon generation ( Two lower-severity issues found, posted inline:
Also worth noting (not flagged inline, lower confidence): No security concerns beyond the two items above — the fencing logic is a genuine hardening (defense against cross-daemon-generation request delivery), not a regression. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
9d2c58b to
16b1876
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d2c58b231
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ...(this.compatibleDaemonId === null | ||
| ? {} | ||
| : { expectedDaemonId: this.compatibleDaemonId }), | ||
| }, |
There was a problem hiding this comment.
When concurrent embedding operations overlap a module_restarted failure, logCallFailure clears compatibleDaemonId while a sibling operation may still be entering callWithRetry; this conditional then omits expectedDaemonId and allows that sibling to publish an embedding request to the newly connected daemon before it has passed lifecycle compatibility. The same gap exists when a second initialize() clears the ID before joining the existing initialization. For managed-default, a missing fence must reject or wait for revalidation rather than silently sending an unfenced request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d4cc027, fail-closed as you asked. callWithRetry now refuses before client.call rather than omitting the expectation:
if (this.connectionOrigin === "managed-default" && this.compatibleDaemonId === null)
throw new SynapseEmbeddingError("module_restarted", "managed Synapse lane has no certified daemon identity");The initialize() half is closed too: the identity is written only on the demand success path, so a concurrent failing initialize cannot erase one another caller certified. module_restarted routes to the caller's single authorized resubmission, which re-certifies first, so a real rotation still converges.
| remainingMs, | ||
| ); | ||
| return { result, storage }; | ||
| return { result: compatibleResult, storage, authenticatedDaemonId }; |
There was a problem hiding this comment.
For a managed Magic Context demand, the production compatibility probe and storage probe each create an independent McHostClient, but this return combines the first probe's daemon ID with the second probe's storage state without checking their identities. If the daemon restarts between those probes, the outcome can claim daemon A passed compatibility while reporting readiness from daemon B; downstream publication fenced to A then fails pre-send, or the caller receives a misleading storage state. Read storage from the authenticated compatibility connection or return and compare the storage probe's daemon ID before constructing this outcome.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 39a87d8 — the two probes really were combined without comparing identities.
The storage probe cannot share the compatibility connection, because it polls across starting while the compatibility probe is one-shot. So it now receives the certified identity and re-checks its own authenticated peer after the handshake and after every host.status response, rejecting when it cannot observe that incarnation. demandStart maps that rejection to native_probe_unavailable/run_daemon_restart and returns storage: null, so remediation names the rotation instead of blaming storage for it. Caller detachment still propagates unchanged.
New test storage readiness read on another incarnation is refused, not reported fails without the fix (the probe never sees an expectation and the rejection escapes as a raw error).
| readiness: { | ||
| transport: { state: "ready", reason: "healthy" }, | ||
| storage: { state: "unavailable", reason: "storage_unavailable" }, | ||
| synapse: { state: "degraded", reason: "synapse_degraded" }, |
There was a problem hiding this comment.
When readCompatibilityProbe stops at an incompatible daemon or module, it returns status: null without issuing host.status; this branch nevertheless reports storage as unavailable and Synapse as degraded. observationalCommand converts those fabricated values into failing readiness checks, so status and doctor diagnose storage and Synapse failures that were never observed alongside the actual compatibility error. Leave the unprobed readiness components absent instead of synthesizing failure states.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d4cc027. The short-circuit branch returns readiness: { transport: ... } only, so storage and Synapse stay absent and observationalCommand fabricates no failing check for a component host.status never reported. The compatibility error is the sole diagnosis.
| status: null, | ||
| }; | ||
| } | ||
| const catalog = await client.catalogList(); |
There was a problem hiding this comment.
On a slow authenticated connection, McHostClient.connect may consume much of the supplied compatibility budget, but catalogList() starts a fresh request timeout equal to the original full budget rather than the remaining time until deadline. The deadline is checked only after this await, so status and doctor—which do not race the readiness probe against their outer deadline—can exceed the documented 60-second hard aggregate by nearly another full probe budget before failing. Pass the remaining deadline into catalog collection or race this request against it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 39a87d8. catalogList/catalogSnapshot now take a per-request timeoutMs, and the probe spends only what is left:
const catalogMs = deadline - Date.now();
if (catalogMs <= 0) throw new Error("compatibility probe deadline expired");
const catalog = await client.catalogList({ timeoutMs: catalogMs });That also closes the second half you noted — the deadline is now checked before the request, not only after it. Two tests cover it: one asserts the request budget is bounded by the remaining deadline, one asserts an already-expired deadline sends no catalog.list at all. Both fail without the fix.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if (this.permanentFailure) return false; | ||
| if (this.connectionOrigin === "managed-default") { | ||
| try { | ||
| this.compatibleDaemonId = null; |
There was a problem hiding this comment.
CRITICAL: Concurrent initialize() resets compatibleDaemonId and bypasses initialization coalescing
The managed lifecycle demand block runs before checking if (this.initializing) return this.initializing; (line 703). When a caller invokes initialize() while an existing initialization flight is already in progress, the second caller executes this.compatibleDaemonId = null; at line 662.
This causes a race condition: if the first in-flight initialization reaches callWithRetry("models.list", ...) while the second caller is running demand, models.list is dispatched with expectedDaemonId: undefined because this.compatibleDaemonId was wiped back to null. In addition, demand start calls are duplicated instead of being coalesced under this.initializing.
Initialization coalescing (if (this.initializing) return this.initializing;) should guard the entire initialization workflow, including the managed lifecycle demand and compatibleDaemonId assignment.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in d4cc027, and both halves are closed.
The identity is no longer cleared in initialize() — it is written only on the demand success path, so a second caller cannot wipe an identity the first already certified. The demand itself is coalesced through this.managedDemand, so a concurrent caller joins the in-flight demand instead of starting a second one. Even with the ordering you describe, callWithRetry now refuses a managed call with a null identity rather than dispatching with expectedDaemonId: undefined.
| epochs: observedEpochsFromMagicContextMetrics(magicContextMetrics), | ||
| evaluatedThrough: "epochs" as const, | ||
| }; | ||
| evaluateCompatibility({ |
There was a problem hiding this comment.
WARNING: Return value of pure function evaluateCompatibility is discarded
evaluateCompatibility is a pure function that returns a CompatibilityVerdict ({ ok: true } | { ok: false, reason: ... }); it does not throw an exception on compatibility mismatches. Calling evaluateCompatibility here without checking or asserting its return value has no effect.
Compatibility evaluation is already performed on the returned snapshot by McHostLifecyclePolicy.applyCompatibility. If early validation in readCompatibilityProbe is not required, this call can be removed; otherwise, the returned verdict should be checked.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in d4cc027 — the discarded call is gone. readCompatibilityProbe now checks the per-stage verdicts it actually uses (evaluateDaemonCompatibility, evaluateModuleCompatibility) to decide where to stop, and applyCompatibility owns the composite verdict on the returned snapshot. evaluateCompatibility no longer appears in managed-policy.ts.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (19 files)
Previous Review Summaries (4 snapshots, latest commit 39a87d8)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 39a87d8)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (19 files)
Fix these issues in Kilo Cloud Previous review (commit d4cc027)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (19 files)
Fix these issues in Kilo Cloud Previous review (commit 2cd1837)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (16 files)
Fix these issues in Kilo Cloud Previous review (commit 16b1876)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (16 files)
Reviewed by gemini-3.7-flash · Input: 104.6K · Output: 14.6K · Cached: 481.3K |
16b1876 to
2cd1837
Compare
Review of the fencing work found paths that relaxed the fence to "no expectation" at exactly the moment a rotation was detected, so the traffic the fence exists to stop was the traffic that escaped. Publication: - Refuse a managed Synapse call whose certified identity is absent instead of publishing unfenced; re-certify on the authorized restart resubmission so a rotation still converges. - Write the Synapse identity only on the demand success path, so a concurrent failing initialize cannot erase an identity already certified. - Carry the certified identity on the ensured route rather than re-reading a mutable field across awaits, so a concurrent invalidation cannot drop the fence between route open and body send. Load and classification: - Treat an unset peer identity as facade recovery, not rotation, and gate the lifecycle demand behind the dial backoff it now arms on failure. - Coalesce the compatibility probe per data root; the snapshot describes the daemon incarnation, not the requesting capability. - Map a failed probe to native_probe_unavailable instead of an unclassified rejection callers cannot act on. Reporting and hygiene: - Report no readiness check for a component the probe never observed. - Bound a stream's retained item count; the pending byte budget counts wire bytes only. - Keep the managed route-cache key daemon-independent so a rotation strands no entry and no second concurrent route opens for one binding. - Own daemon-id equality in one helper, let applyCompatibility own the verdict, drop a discarded evaluateCompatibility call, and build the epoch fixture from wire names so the compatible probe path is proven.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4cc027664
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const existing = this.inflightCompatibility.get(root); | ||
| if (existing) return existing; | ||
| const shared = probe(budgetMs); |
There was a problem hiding this comment.
When concurrent managed demands have different remaining deadlines, the first caller creates shared using only its own budgetMs, and every later caller joins that same probe. If a nearly expired waiter arrives first, its short handshake/request timeout can make a concurrent long-lived waiter receive native_probe_unavailable even though the latter still has ample time to complete a fresh probe. Treat owner-budget exhaustion as replaceable for surviving waiters, or run the shared probe under a caller-independent budget while continuing to detach each waiter with raceDetached.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 5e8ca28. The creating waiter's remaining deadline no longer sizes the shared probe:
snapshot = await this.raceDetached(
this.sharedCompatibility(root, this.outerAggregateMs),
request.signal,
remainingMs,
);This is your second option — a caller-independent budget with per-waiter detachment still handled by raceDetached, which keeps the "one detaching caller cannot cancel the probe another awaits" property the method already documents. Since every creator now uses the same budget, the asymmetry disappears rather than being repaired after the fact.
New test the shared compatibility probe budget does not come from the creating waiter fails without the fix (the recorded budget follows the caller's deadlineMs).
| if (entry.streamItems.length >= entry.maxStreamItems) { | ||
| releaseQuietly(lease); | ||
| this.settleCallerReject( | ||
| entry, | ||
| new McHostCallError( | ||
| "terminal", | ||
| `stream exceeded ${entry.maxStreamItems} retained items`, | ||
| "stream_item_limit", | ||
| ), | ||
| ); | ||
| this.finishEntry(entry); |
There was a problem hiding this comment.
When a peer sends the first StreamData beyond the retained-item ceiling, this rejects the caller and finishEntry removes the pending request and clears its deadline, but no correlation-scoped Cancel is sent and the generation remains live. A host producing a very large or unending stream therefore keeps doing work and sending frames that the client merely drops, consuming socket and frame-processing capacity after the caller has settled and potentially delaying unrelated requests on the connection. Cancel the stream and retain bounded cleanup semantics, or retire the generation if cancellation cannot be completed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 5e8ca28. The ceiling now sends a correlation-scoped Cancel:
this.finishEntry(entry);
if (header.channel !== 0) {
this.enqueueCancel(header.channel, header.epoch, header.corr);
}Settling first is deliberate so the caller still reads stream_item_limit rather than a retirement. Your fallback is already covered by the primitive: enqueueControlHeader no-ops if the generation is retired, and a refused control frame fails the channel and retires the generation synchronously — so a Cancel that cannot be completed retires instead of leaving the peer producing.
The existing ceiling test now also asserts the Cancel's channel, epoch, and correlation; it fails without the fix.
| private async recertifyForRestart(signal?: AbortSignal): Promise<boolean> { | ||
| if (this.connectionOrigin !== "managed-default") return true; | ||
| this.initialized = false; | ||
| this.compatibleDaemonId = null; |
There was a problem hiding this comment.
WARNING: recertifyForRestart prematurely resets compatibleDaemonId before recertification completes
Setting this.compatibleDaemonId = null; before await this.initialize(signal) clears the certified daemon identity while initialize() is in flight. If concurrent operations or sibling requests have already completed recertification and are dispatching their resubmitted requests, this premature wipe causes their callWithRetry() calls to hit line 1550 (this.connectionOrigin === "managed-default" && this.compatibleDaemonId === null) and immediately fail with module_restarted. For operations that have already spent their single restart budget (restarted = true), this causes an avoidable permanent failure.
As noted in the comment on lines 694–697, compatibleDaemonId should only be written on the success path inside initialize() once the new daemon identity is proven by demandStart.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Confirmed and fixed in 5e8ca28. You are right that this contradicted the invariant the comment above initialize() declares, and the consequence is exactly as described: a sibling that had already re-certified and spent its one restart budget failed permanently on a fence it had just proved.
recertifyForRestart now clears only initialized, which is enough to force a fresh demand, and initialize() is the identity's only writer. An identity that genuinely rotated away is refused by the client fence before any byte is written, and that refusal is the sibling's own module_restarted to spend.
New test re-certification leaves an identity a sibling already certified in place gates the demand and asserts the identity survives the in-flight window; it fails without the fix.
…ts deadline Two probe paths reported an observation that no single incarnation or budget actually supported. Storage readiness came from its own connection, and the outcome combined that reading with the daemon id the compatibility probe certified without comparing the two. A rotation between the probes produced an outcome claiming daemon A passed compatibility while describing daemon B's storage, so a caller either acted on readiness it would never reach or read a storage failure for an incarnation it had already left. The storage probe now receives the certified identity, re-checks its own authenticated peer after the handshake and after every response, and rejects when it cannot observe that incarnation. The policy turns that rejection into native_probe_unavailable, which sends remediation at the rotation rather than blaming storage for it. Catalog collection started a fresh full-length request budget instead of spending the time left until the probe deadline, and the deadline was checked only after the request returned. A slow authenticated handshake could therefore consume most of the budget and still leave catalog.list free to consume another, so status and doctor could overrun the aggregate they promise. catalogList and catalogSnapshot now accept a per-request timeout, and the probe passes the remaining deadline and refuses an already-expired one before sending. Each test fails without its fix: the storage-rotation case rejects rather than returning a typed result, and the catalog cases show an unbounded request and a request sent past the deadline.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39a87d8be8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this.initialized = false; | ||
| this.compatibleDaemonId = null; | ||
| return await this.initialize(signal); |
There was a problem hiding this comment.
When the lifecycle probe certifies daemon B while the shared McHostClient still has a live generation for daemon A (for example, before A's FIN/Goodbye is dispatched), this only replaces the expected ID and initialize() reuses A through getSharedClient. The client's daemon_generation_changed check rejects without retiring A, so the single restart resubmission fails against the same stale generation and never reaches B. Retire or replace the shared client when recertifying after an identity change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified the mechanism, but declining the fix here — it needs a change larger than this PR, and I want to be precise about what does and does not hold.
The premise is right. ensureConnection rejects rather than replaces:
const active = this.active;
if (active && !active.generation.isRetired()) {
this.assertExpectedDaemon(active, expectedDaemonId); // throws not_sent
return active;
}So while a live generation for A exists, a caller fenced to B cannot obtain a connection to B, and initialize() reusing the shared client cannot reach it either.
Two reasons this is not a drive-by fix:
- Retiring A to unblock B is not safe.
retire()settles every in-flight request on that generation, and during a drain the operations fenced to A are the valid ones. "One B-fenced caller kills every A-fenced operation" is the same cross-operation failure the other findings in this round were about, just pointed the other way. - The real constraint is that
McHostClientholds one live generation (this.active,this.connecting). That predates this PR; the fence made it observable rather than introducing it. Serving A and B concurrently, or refusing B as an explicitly retryable wait rather than a spent-budget failure, is a design change with its own test matrix.
What holds today: the rejection is not_sent, so nothing is published to the wrong incarnation — the fence's purpose is intact, and the gap is liveness, not cross-daemon delivery. In the ordinary restart case A's socket closes, the generation retires, and the loop reconnects to B. The exposure is the window where A's socket outlives the connection file.
Tracking it as follow-up work rather than patching it under review.
| const slot = cached; | ||
| flight = makeSetupFlight( | ||
| (f) => this.openCachedRoute(slot, stage, f), | ||
| (f) => this.openCachedRoute(slot, stage, f, options.expectedDaemonId), |
There was a problem hiding this comment.
When calls certified immediately before and after a daemon rotation overlap, the old-ID caller can create this route-opening flight and the new-ID caller joins it because the cache slot does not distinguish their expectations. If daemon B is active, the flight fenced to A rejects with daemon_generation_changed, and the caller correctly fenced to B inherits that rejection even though it could open the route; moreover, call() awaits the handle outside its retry block, so this valid call is not retried. Coalesce openings only when their expected daemon IDs match, or detach and retry a waiter whose expectation differs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified both halves; declining the fix here for scope reasons, with the reasoning recorded.
The flight really is created with the owner's options.expectedDaemonId and joiners inherit it, and managedRouteHandle is awaited on line 668 outside the try, so a not_sent from route-open never reaches the not_sent && mayReplay replay at 691.
Why I am not changing it in this pass:
- Coalescing by expectation, or detaching a mismatched waiter, edits the route-open state machine that carries the documented KTD2/KTD3/KTD4 rules plus
flight.replaceable. The failure mode of getting it wrong is an unbounded replacement loop or a double route-open for one binding — worse than the finding. - Moving
managedRouteHandleinside thetrylooks like a one-liner but changes which route-open rejections replay.route_closedafter an explicitcloseRoutewould become retryable, which reopens a route the caller closed (the KTD9 owner-close race), and route-stage exhaustion would silently get a second budget. - As with the sibling finding on
ensureConnection, the underlying constraint is one live generation per client. Distinguishing expectations at the flight level is worth doing on top of that, not instead of it.
What holds today: the inherited rejection is not_sent, so no body reaches the wrong incarnation, and the caller's own rotation handling (module_restarted → re-certify → single authorized resubmission) converges. The cost is a spuriously spent restart budget in the overlap window, not a correctness break.
Also noting the tension for the record: the same slot was flagged in this PR for the opposite reason — folding expectedDaemonId into the cache key stranded one entry per rotation. The key stays daemon-independent; the flight-level distinction is the follow-up.
| } catch (error) { | ||
| // A failed demand (probe failure, incompatibility, storage not ready) | ||
| // must not be re-issued at request rate: arm the same dial backoff a | ||
| // failed connect arms, so the next caller is gated above. | ||
| this.nextProbeMs = Date.now() + this.backoffMs; | ||
| this.backoffMs = Math.min(this.backoffMs * 2, CONNECT_BACKOFF_MAX_MS); |
There was a problem hiding this comment.
When a request is aborted or its deadline expires while demandManagedReadiness is pending, the lifecycle policy rejects with the caller-scoped detachment error, but this catch treats it like a failed daemon probe and arms the transport-wide connection backoff. A single canceled request therefore makes unrelated sessions fail with MC_HOST_CONNECTION_BACKOFF for at least one second, and repeated cancellations can extend that to 30 seconds even while the daemon is healthy. Propagate caller abort/deadline errors without updating the shared backoff.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 55c5bb1. The handler armed the shared gate for every error the demand could raise, including the policy's detachment:
if (!(error instanceof WaiterDetachedError)) {
this.nextProbeMs = Date.now() + this.backoffMs;
this.backoffMs = Math.min(this.backoffMs * 2, CONNECT_BACKOFF_MAX_MS);
}
throw error;A probe failure, incompatibility, or storage-not-ready verdict still arms it, since those are the claims that must not be re-issued at request rate. Aborts arrive as WaiterDetachedError too — raceDetached rejects with it for both the signal and the deadline — so both cases are covered.
New test drives two sessions through a detaching demand and asserts the second still reaches the demand instead of failing on MC_HOST_CONNECTION_BACKOFF; it fails without the fix.
Three paths let a recovery step take something a concurrent operation was still relying on. `recertifyForRestart` cleared the certified daemon identity before its demand resolved. A sibling that had already re-certified and was dispatching against that fence found it null and failed with module_restarted, and a sibling whose one restart budget was already spent failed permanently on a fence it had just proved. `initialize` is now the identity's only writer, as its own comment already declared: it writes on the success path, and an identity that really did rotate away is refused by the client fence before any byte is written, which is the sibling's own rotation to spend. The shared compatibility probe took its budget from whichever waiter created it. A nearly expired caller minted a probe too short for the long-lived waiters that joined it, and they read that truncated failure as an unproven compatibility claim while still holding ample time. The probe now runs on the policy's aggregate budget, and each waiter still detaches at its own deadline through raceDetached. A stream that breached its retained-item ceiling settled the caller and dropped the entry without telling the host to stop, so the peer kept producing frames this connection could only discard, spending socket and frame-processing capacity unrelated requests need. The ceiling now sends a correlation-scoped Cancel after settling, so the caller still reads the limit it hit, and a refused Cancel retires the generation as the bounded fallback. Each test fails without its fix: the identity is null mid-flight, the probe budget follows the caller's deadline, and no Cancel frame reaches the peer.
The managed demand's failure handler armed the transport-wide dial backoff for every error the demand could raise, including the detachment the policy throws when a caller's own signal aborts or its own deadline expires. That error is evidence about one caller, but the gate it armed is shared: one cancelled request made every other session fail with MC_HOST_CONNECTION_BACKOFF for at least a second, and a burst of cancellations walked the gate toward its thirty-second cap while the daemon was healthy and reachable. Detachment now propagates without touching the backoff. A probe failure, incompatibility, or storage-not-ready verdict still arms it, because those are the claims that must not be re-issued at request rate.
1 participant
Summary
Managed callers now evaluate daemon, module, epoch, and readiness data from one authenticated daemon identity. Route and application publication fail before use when that identity rotates, while unsupported-platform and typed compatibility outcomes remain intact.
Verification
bun run typecheckbun run lintStack