fix(synapse): rebind retries to the replacement daemon - #65
Conversation
|
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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const classified = classifyError(error); | ||
| if (classified.code !== "module_restarted" || restarted) throw classified; | ||
| restarted = true; | ||
| await this.rebindAfterModuleRestart(deadlineAt, signal); |
There was a problem hiding this comment.
Correctness: if rebindAfterModuleRestart throws here (either "timeout" because the page deadline already elapsed, or "transport" because the re-demand call failed), the exception propagates to the outer catch in embedItems (lines 939-950). Neither "timeout" nor "transport" is a permanent code (isPermanentSynapseCode, lines 188-198), so this.permanentFailure is never set and the break at line 948 never runs — the for loop simply continues to the next page.
But rebindAfterModuleRestart already reset this.compatibleDaemonId = null and this.initialized = false unconditionally as its first lines (799-801) before throwing. Nothing re-checks this.initialized or re-calls this.initialize() per page in embedItems's loop (only signal?.aborted || this.permanentFailure is checked, line 872) or in embedItemsDetailed's group/page loops (line 989) — runDetailedPage itself never calls initialize() either.
Failure scenario: a 3-page embedItems/embedItemsDetailed call hits module_restarted on page 1; the rebind's re-demand fails or the deadline has just elapsed. Pages 2..3 are then dispatched via callWithRetry (line 1592-1594) with expectedDaemonId omitted entirely, since this.compatibleDaemonId is stuck at null — silently bypassing the daemon-compatibility check this PR exists to enforce, for the rest of that top-level call. The only place state gets re-initialized is the single this.initialize() call at the very top of the next external invocation.
There was a problem hiding this comment.
Verified, and the version this describes is gone. The rebase moved this branch onto the reviewed base, whose rebindAfterModuleRestart deliberately does not clear compatibleDaemonId. initialize is its only writer and writes it on the success path, so a failed rebind cannot leave the identity null for later pages.
The specific bypass is independently closed as well: callWithRetry refuses to publish when the lane is managed-default and the identity is absent, throwing module_restarted rather than dispatching with expectedDaemonId omitted. Later pages therefore cannot silently skip the fence; they fail closed and route back into rebind. embedItemsDetailed also re-runs initialize per page while !this.initialized.
| } | ||
| } | ||
|
|
||
| private async rebindAfterModuleRestart( |
There was a problem hiding this comment.
Altitude: this bolts explicit "rebind after module_restarted" calls onto three separate catch sites (lines 906, 1168, 1267) rather than making compatibility self-healing inside initialize()/callWithRetry. module-transport.ts's ensureConnected (lines 1210-1290) already solves this exact problem — "the daemon identity may have changed under us" — lazily, by checking compatibleDaemonId at the top of every call and reconnecting/re-demanding as needed, so no call site needs to know about the failure mode.
Cost: recovery here is scattered and order-dependent. A future call path that can also surface module_restarted won't rebind unless someone remembers to bolt the same three lines on again — which is also the direct cause of the bug flagged on line 906 (a failed rebind leaves state stale for call sites that don't re-check it). The self-healing version one file away would have avoided both the duplication and that bug.
There was a problem hiding this comment.
Agreed on the direction, and the structure you are describing is no longer this branch's.
The three catch sites and the rebindAfterModuleRestart / recertifyForRestart split are the base branch's committed design, arrived at through its own review. This branch carried a stale duplicate of that work, and the rebase dropped it rather than replaying it over base's reviewed version.
callWithRetry did gain the self-healing half you are pointing at: it fails closed centrally when the managed lane has no certified identity, so a call path that forgets to rebind refuses to publish instead of publishing unfenced.
What this branch still adds is one more trigger, at submission time, and that is exactly where a residual gap showed up. See the pre-publication fence finding on this file, fixed in 17984bb. Consolidating the remaining recovery into a single self-healing path is a fair follow-up, but it would rewrite base's design and belongs in its own change.
| signal?: AbortSignal, | ||
| ): Promise<void> { | ||
| this.initialized = false; | ||
| this.compatibleDaemonId = null; |
There was a problem hiding this comment.
Simplification: this reset is dead in every path. For connectionOrigin === "managed-default", initialize()'s managed-default branch (line 662) unconditionally sets this.compatibleDaemonId = null again before assigning any new value (line 698) — so this line's write is always overwritten before it matters. For any non-managed origin, compatibleDaemonId is never set anywhere else, so it's already null. Low-cost, but it misleads a reader into thinking this line carries meaning independent of what initialize() already guarantees.
There was a problem hiding this comment.
Correct, and the line is gone. This branch's diff now contains no compatibleDaemonId writes at all.
Base's version of the function does not clear it, and documents why: initialize is its sole writer and writes only on the success path, so one operation entering recertification cannot erase an incarnation a sibling already certified and is dispatching against.
| this.initialized = false; | ||
| this.compatibleDaemonId = null; | ||
| this.managedDemand = null; | ||
| const remainingMs = deadlineAt - Date.now(); |
There was a problem hiding this comment.
Reuse: this hand-rolls deadlineAt - Date.now() / "throw timeout if ≤0" arithmetic (repeated again at lines 879-888 and elsewhere in this file) instead of reusing the Deadline abstraction (remainingMs(), stageBudgetMs(...)) that module-transport.ts already factors this exact pattern through (lines ~1155-1183). This diff threads a caller deadline through three new call sites — a natural opportunity to converge on the shared abstraction instead of adding more copies of the same signed-remaining-time edge case that each need to get ≤ 0 right independently.
There was a problem hiding this comment.
Fair point. Those copies of deadlineAt - Date.now() with the <= 0 guard now live in base's code rather than this branch's, since the rebase dropped this branch's duplicate of the work that introduced them. The helper added here threads the page's existing absolute deadline through rather than adding another copy of the arithmetic.
Converging this file on Deadline's remainingMs() is worth doing, but it is a cleanup across base's call sites, not something this PR can do locally.
a537527 to
658f13d
Compare
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
Previous Review Summaries (3 snapshots, latest commit e17ebf9)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit e17ebf9)Status: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous review (commit 31c75b1)Status: No Issues Found | Recommendation: Merge Files Reviewed (11 files)
Previous review (commit 658f13d)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Reviewed by gemini-3.7-flash · Input: 145.8K · Output: 13.9K · Cached: 893.9K |
![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: 658f13d5f0
ℹ️ 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".
| if (!(await this.initialize(signal, remainingMs))) { | ||
| throw new SynapseEmbeddingError( | ||
| "transport", | ||
| "Synapse daemon compatibility rebind failed", |
There was a problem hiding this comment.
When a managed rebind returns false, this helper has already cleared initialized and compatibleDaemonId; both embedItems and embedItemsDetailed catch the resulting page error and continue to subsequent pages without calling initialize again. Those pages therefore reach callWithRetry without an expectedDaemonId, bypassing the lifecycle-compatible daemon fence precisely while replacement discovery is failing. Either stop the remaining page loop or require initialization again before submitting another page.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified, and closed on the current head, though by a different mechanism than the one proposed.
Rather than stopping the page loop, the managed lane fails closed at the publication point: callWithRetry refuses when the origin is managed-default and compatibleDaemonId is null, throwing module_restarted instead of dispatching with expectedDaemonId omitted. A page that runs while replacement discovery is failing therefore cannot bypass the fence; it refuses and routes back into rebind.
The premise also no longer holds. Base's rebindAfterModuleRestart does not clear compatibleDaemonId, and embedItemsDetailed re-runs initialize per page while !this.initialized.
| terminal.ledgerRowId = row.rowId; | ||
| throw terminal; | ||
| } | ||
| await this.rebindAfterModuleRestart(readyDeadlineAt, signal); |
There was a problem hiding this comment.
When a retained ready job reports module_restarted, this rebind consumes time from readyDeadlineAt, but the following freshPage() assigns Date.now() + timeoutMs to the replacement row. A restart near the original deadline can consequently receive an entire additional page timeout for resubmission and polling, contrary to the absolute-deadline behavior added here; the replacement row needs to retain readyDeadlineAt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and still live, but on the base branch rather than in this diff.
freshPage() assigns deadlineAt: Date.now() + timeoutMs, and it is called immediately after the ready-path rebind, so a restart observed near the original deadline does receive a full additional page budget. Both the rebind call and freshPage() are base's code; this branch's duplicate of that work was dropped in the rebase, and its diff no longer touches either.
This deserves its own change against base: carry readyDeadlineAt into the replacement row instead of restarting the budget. Leaving it out here keeps this PR to the rebind path it actually owns.
| deadlineMs: | ||
| demandDeadlineMs ?? | ||
| this.options.queryTimeoutMs ?? | ||
| SYNAPSE_DEFAULT_QUERY_TIMEOUT_MS, |
There was a problem hiding this comment.
When a page has little time remaining, this creates the cached managedDemand promise with that shortened deadline. Any unrelated caller entering initialize() while the rebind is in flight reuses that same promise and therefore inherits the first page's near-expired deadline instead of its configured query deadline; it can report the lane unavailable even though compatibility succeeds shortly afterward. Cache only the shared underlying start, or race each initialization waiter against its own deadline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a real sharing hazard, and it is base's initialize, which this diff does not modify.
managedDemand is cached with whatever demandDeadlineMs the first caller supplied, so a rebind entering with a nearly exhausted page budget does publish that shortened deadline to any unrelated caller that joins the in-flight demand.
This branch reaches that path through the same rebindAfterModuleRestart base already calls from three sites, so it neither introduces nor widens the sharing. The fix you describe, caching only the shared start and racing each waiter against its own deadline, is the right shape and belongs in a change that owns initialize.
| this.initialized = false; | ||
| this.compatibleDaemonId = null; | ||
| this.managedDemand = null; |
There was a problem hiding this comment.
If two requests observe module_restarted concurrently, each invocation clears managedDemand, so the second discards the first in-flight rebind and starts another compatibility demand. Their outcomes then write the shared compatibleDaemonId without a generation check; if the daemon changes between probes or the results settle out of order, the later completion can overwrite the newer identity and both pages resume against that global value, potentially spending their only restart immediately. Coalesce the whole replacement rebind or ignore results from superseded generations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Half of this is already addressed on the current head; the other half is real.
Addressed: the outcomes no longer race to clobber a certified identity. Base's version writes compatibleDaemonId only on initialize's success path and never clears it during recertification, precisely so a concurrent rebind cannot erase an incarnation a sibling already certified.
Still open: two demands that both succeed can settle out of order, and the later completion writes the older identity, because there is no generation check on that write. That is base's initialize, untouched by this diff. A monotonic guard on the write, or coalescing the whole rebind, is the fix, and it needs a change that owns that function.
| expectedStateVersion: row.stateVersion, | ||
| jobId, | ||
| }); | ||
| await this.rebindAfterModuleRestart(deadlineAt, signal); |
There was a problem hiding this comment.
A successful detailed-page rebind calls initialize() again, which invokes onLaneReady even though the pinned metadata is unchanged. For primary registrations, commitPrimarySynapseLane unconditionally increments registration.generation; the DetailedLane captured before embedItemsDetailed then fails its stillCurrent() check in embedAndApplyDetailed, so the successfully recovered vectors are not written to their destinations and the pass returns no applied items. Rebinding should not republish an unchanged lane or advance its registration generation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified end to end and fixed in 17984bb.
The mechanism is exactly as described. commitPrimarySynapseLane ran registration.generation = ++globalRegistrationGeneration unconditionally, stillCurrent() compares that same counter against the value captured before the pass, and embedAndApplyDetailed does if (!lane.stillCurrent()) return new Map(). A rebind that re-derived identical pinned metadata therefore discarded the vectors it had just recovered.
The fix holds the generation steady when the provider identity, chunk model, and runtime fingerprint are all unchanged, while still refreshing the resolved config. That matches the re-registration path, which already keeps the generation steady for an unchanged config.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31c75b16d1
ℹ️ 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 PAYLOAD_INDEX_PATH = "release/mc-host-payload-index.json"; | ||
| const STOP_PROVENANCE_PATH = "release/mc-host-n-minus-one-stop.json"; | ||
| const SHA256_RE = /^[0-9a-f]{64}$/; | ||
| const QUALIFICATION_WORKFLOW_PATH = ".github/workflows/mc-host-release-qualification.yml"; |
There was a problem hiding this comment.
The qualified-evidence verifier now requires every proof to name this exact workflow and passes it to gh attestation verify --signer-workflow, but a repo-wide tree search shows that .github/workflows/mc-host-release-qualification.yml does not exist in this commit (the workflow directory contains only ci.yml, claude-code-review.yml, retrieval-benchmark.yml, and shm-hardening-optin.yml). Since gh attestation verify --help defines this flag as enforcing the Actions workflow that signed the attestation, no run at this release commit can produce the required signer identity, making release:evidence:check impossible to satisfy for qualified evidence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed still true on the base branch: verify-mc-host-release-evidence.ts declares QUALIFICATION_WORKFLOW_PATH = ".github/workflows/mc-host-release-qualification.yml" and passes it to --signer-workflow, while .github/workflows/ contains only ci.yml, claude-code-review.yml, retrieval-benchmark.yml, and shm-hardening-optin.yml.
It is out of this PR's diff now. This branch carried a stale duplicate of the release-evidence work that base had already landed in reviewed form, and the rebase dropped it, so scripts/verify-mc-host-release-evidence.ts is no longer modified here.
The finding stands against base and needs its own change: either add the workflow, or stop requiring a signer identity that no run at the release commit can produce.
| status: null, | ||
| }; | ||
| } | ||
| const catalog = await client.catalogList(); |
There was a problem hiding this comment.
When connection and authentication consume part of budgetMs, this call starts catalogList() with the client's fresh full requestTimeoutMs instead of the remaining time to deadline; for example, a 50-second handshake followed by a stalled catalog can keep this supposedly 60-second probe alive for roughly 110 seconds. Because demandStart() caches that promise in inflightProbes, later callers continue joining the over-deadline probe and may detach rather than starting a compatibility check against a recovered daemon; pass the remaining budget into the catalog request or otherwise race it against the absolute deadline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed on base, so there is nothing to change here.
The probe now derives the remaining budget before the catalog call:
const catalogMs = deadline - Date.now();
if (catalogMs <= 0) throw new Error("compatibility probe deadline expired");
const catalog = await client.catalogList({ timeoutMs: catalogMs });so the handshake's elapsed time is charged against the probe deadline instead of the catalog request starting a fresh requestTimeoutMs.
This diff no longer touches managed-policy.ts either: the rebase dropped this branch's duplicate of that work in favour of base's reviewed version.
…probes Address review findings on the retry-rebind path: - Refuse to publish managed Synapse calls while the compatibility-validated daemon binding is unknown instead of silently omitting expectedDaemonId; a missing binding now surfaces as module_restarted and routes into rebind. - Re-derive managed compatibility at the top of the embedItems and embedItemsDetailed page loops after logCallFailure discards the binding, so later pages never run against an unvalidated replacement daemon. - Absorb a submission-time module_restarted in the durable page path with one rebind-and-resubmit of the same request key, matching the ephemeral path's handling of restarts observed while polling. - Enforce the expectedDaemonId gate once at the awaitRequest publication choke point instead of per entry point; share one sameDaemonId predicate and one DAEMON_GENERATION_CHANGED_CODE constant across the facade, transport, and lifecycle layers. - Keep a live transport client whose facade is mid-re-dial (authenticated transiently null) instead of tearing it down into a full demand-start. - Coalesce the demand-start compatibility probe with the shared start so concurrent callers pay one connect + handshake + two RPCs. - Derive the compatibility verdict and per-stage checks from one authoritative stage list; drop the no-op evaluateCompatibility call in the probe. - Restore macOS coverage for the shipped ck-mc-host lifecycle binary in CI.
31c75b1 to
e17ebf9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e17ebf9443
ℹ️ 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".
| return await this.submitBatchPage(page, requestKey, deadlineAt, signal); | ||
| } catch (error) { | ||
| const classified = classifyError(error); | ||
| if (classified.code !== "module_restarted") throw classified; |
There was a problem hiding this comment.
On a managed lane, if the client reconnects to a replacement daemon before embed.batch is published, McHostClient raises daemon_generation_changed; callWithRetry clears the certified identity, but classifyError deliberately maps that code to transport. This condition therefore skips the new rebind branch, so the current detailed page is recorded as failed and the legacy path omits it rather than immediately certifying the replacement and resubmitting the unchanged request key. Handle this pre-publication fence here as another rebind trigger without charging the durable module_restarted budget.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and fixed in 17984bb. I verified the whole chain first: the fence throws not_sent with daemon_generation_changed, classifyError maps it to transport deliberately so it cannot spend the page's durable restart budget, and the submit helper recognised only module_restarted. The page was therefore reported as failed instead of re-certifying and resubmitting its unchanged request key.
One detail worth recording, because it makes the obvious fix silently ineffective: by the time the error reaches the submit helper, callWithRetry has already replaced it with the classified SynapseEmbeddingError, whose code is transport. Testing readErrorCode(error) there never matches. The provenance has to travel on the classified error, so the fence now sets a prePublicationFence flag, following the existing ledgerRowId pattern, and the helper treats that as a rotation trigger.
Budget accounting is unchanged: the ledger spends the restart, never this helper, so a fence-driven resubmission leaves restart_count at 0. The regression test asserts that, and it fails without the trigger change, with the page coming back as a retryable failure that is never resubmitted.
…ng an unchanged lane Two review findings on the rebind path, both verified against the code before changing it. The client's daemon fence refuses a request before any byte is enqueued, so the outcome is `not_sent` and the page provably never reached the daemon. Classification maps that code to `transport` on purpose, so it cannot masquerade as `module_restarted` evidence and spend a page's single durable restart budget. Submission-time rebind only recognized `module_restarted`, so a page refused by the fence was reported as a failure instead of being re-certified and resubmitted under its unchanged request key. Carry the fence provenance on the classified error and treat it as a rotation trigger, which keeps the budget accounting intact because the ledger, not the submit helper, spends the restart. Committing the primary lane advanced the registration generation on every report, including a rebind that re-derives the same pinned metadata. `stillCurrent()` compares exactly that counter, so an in-flight lane captured before the rebind failed its check and discarded the vectors the rebind had just recovered. Hold the generation steady when the provider identity, chunk model, and runtime fingerprint are all unchanged, matching the re-registration path, which already keeps it steady for an unchanged config. The fence test fails without the trigger change: the page is reported as a retryable failure and the request is never resubmitted.
1 participant
Summary
Synapse retry paths now discard a stale daemon identity after
module_restarted, rerun managed compatibility, and resubmit the same request key against the replacement daemon. Ephemeral and durable paths retain their original absolute page deadline through rebind, resubmission, and polling.Verification
bun test packages/plugin/src/features/magic-context/memory/embedding-synapse.test.ts: 47 passedbun run typecheckbun run lintStack
Layer 7 of 7 above #46. Parent: #64.