engine: fix provider-attach race dropping a provider's capabilities at node up by willwashburn · Pull Request #251 · AgentWorkforce/relaycast

willwashburn · 2026-07-10T13:52:49Z

The bug (P0)

Two providers attaching to one node concurrently — the broker plus a fleet serveNode at node up — non-deterministically drop one provider's entire capability set from the node roster (~50% of boots, flips which). This silently breaks placement/dispatch for the lost capabilities on every multi-provider node up.

Root cause

Registration (duplicate-provider check → node_providers upsert → registry bind → node-aggregate recompute) and connection teardown both read and write shared node state, but node-control operations aren't serialized per node — they interleave at await points. Three concrete failure modes, all reproduced:

Same-name overwrite. Two registrations for the same provider name both pass providerAttachConflict (which reads the in-memory registry) before either calls attachProvider, then the second upsertProvider — keyed on (workspace, node, name) — overwrites the first's row. One capability set vanishes; the two providers collapse to one row. (This is why a per-node mutex around only the DB functions didn't help: serializing the calls doesn't stop the overwrite, and the conflict check is in the wrong layer.)
Aggregate clobber. A teardown's recomputeNodeAggregate reads the provider set before a concurrent register commits and writes it after — clobbering the newly registered provider out of nodes.capabilities. The provider row survives; the roster goes stale. This is the production (async D1) manifestation with distinct provider names.
Self-host deadlock. On better-sqlite3, the teardown's write on the main handle blocks a register's isolated-transaction write lock for the full busy_timeout (5s), then swallows SQLITE_BUSY.

Fix

All control operations for a node run one-at-a-time through a per-node queue keyed by workspace:node (serializeNodeOp). handleNodeControlMessage, the socket-close path (handleProviderDisconnect), the offline sweep, and the provider DELETE route serialize through it. Consequences:

Distinct providers coexist (the intended multi-provider behavior).
Same-name duplicates resolve deterministically — the second is rejected loudly with provider_instance_conflict instead of silently overwriting the first.
No cross-operation aggregate clobber and no isolated-transaction deadlock.
No-op for callers that already serialize per node (a per-node Durable Object).

Tests

packages/engine/src/__tests__/conformance/providerAttachRace.test.ts — a file-backed suite (isolated transactions, like production) that reproduces all three modes and asserts a stable aggregate. Verified it fails without the fix (the register-vs-disconnect case deadlocks/times out; the same-name case asserts the silent overwrite) and passes with it. Full engine suite (423) and types suite (161) green; lint clean.

Coordination note

For the broker and a fleet serveNode to coexist on one node they must register under distinct provider names — the engine now rejects a same-name duplicate deterministically rather than losing a cap set. If node up currently points both at the same provider name (e.g. default), that needs a matching CLI change in the relay repo so serveNode uses a distinct name. Flagging for relay-impl.

Rides the v6.0.1 patch.

🤖 Generated with Claude Code

…race Two providers attaching to one node concurrently (e.g. the broker and a fleet serveNode at `node up`) could non-deterministically drop one provider's entire capability set from the node roster, silently breaking placement/dispatch for the lost capabilities. Root cause: registration (duplicate-provider check + node_providers upsert + registry bind + node-aggregate recompute) and connection teardown both read and write shared node state but weren't serialized per node, and they interleave at await points: - Two same-name registrations both pass `providerAttachConflict` (which reads the in-memory registry) before either calls `attachProvider`, then the second `upsertProvider` overwrites the first's row — one cap set vanishes. - A teardown's `recomputeNodeAggregate` reads the provider set before a concurrent register commits and writes it after, clobbering the new provider out of the aggregate (the row survives; the roster goes stale). - On better-sqlite3, the teardown's write on the main handle deadlocks a register's isolated transaction (5s busy_timeout, then a swallowed error). Fix: run all control operations for a node one-at-a-time through a per-node queue keyed by workspace:node. `handleNodeControlMessage`, the socket-close path (`handleProviderDisconnect`), the offline sweep, and the provider DELETE route all serialize through it. Same-name duplicates now resolve deterministically (the second is rejected loudly with `provider_instance_conflict`, no silent overwrite); distinct providers coexist; and there is no cross-operation clobber or deadlock. No-op for callers that already serialize (a per-node DO). A file-backed conformance suite reproduces all three failure modes and asserts a stable aggregate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

coderabbitai · 2026-07-10T13:53:14Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9d4abf31-d6a2-4428-9269-219b7a894603

📥 Commits

Reviewing files that changed from the base of the PR and between 271be84 and e526eff.

📒 Files selected for processing (3)

packages/engine/src/engine/node.ts
packages/engine/src/engine/nodeLock.ts
packages/sdk-rust/src/credentials.rs

✅ Files skipped from review due to trivial changes (1)

packages/sdk-rust/src/credentials.rs

🚧 Files skipped from review as they are similar to previous changes (2)

packages/engine/src/engine/nodeLock.ts
packages/engine/src/engine/node.ts

📝 Walkthrough

Walkthrough

The changes serialize node-scoped control and provider lifecycle operations, add race-focused provider attachment conformance tests, and simplify two Rust UUID formatting call sites.

Changes

Provider attach race handling

Layer / File(s)	Summary
Per-node operation serialization `packages/engine/src/engine/nodeLock.ts`, `packages/engine/src/engine/node.ts`	Adds a workspace/node promise queue and serializes node control message dispatch through it.
Provider lifecycle serialization `packages/engine/src/engine/node.ts`, `packages/engine/src/routes/node.ts`	Serializes provider disconnects, offline sweeps, and HTTP provider deregistration with node state updates and aggregate recomputation.
Provider attachment race tests `packages/engine/src/__tests__/conformance/providerAttachRace.test.ts`	Adds isolated integration tests for concurrent attachments, disconnect races, and deterministic provider conflicts.

Rust fallback name formatting

Layer / File(s)	Summary
Direct UUID formatting `packages/sdk-rust/src/credentials.rs`	Passes generated short UUIDs directly to two fallback name formatters.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ProviderSocket
  participant NodeEngine
  participant serializeNodeOp
  participant NodeDatabase
  ProviderSocket->>NodeEngine: send provider registration or disconnect
  NodeEngine->>serializeNodeOp: enqueue workspaceId/nodeId operation
  serializeNodeOp->>NodeEngine: execute lifecycle update
  NodeEngine->>NodeDatabase: update provider state and node aggregate

Possibly related issues

AgentWorkforce/relaycast issue 250 — Covers the concurrent provider-attachment capability-loss behavior addressed by per-node serialization.

Possibly related PRs

AgentWorkforce/relaycast#243 — Modifies the same node control message handler with an additional node.spawn case.
AgentWorkforce/relaycast#244 — Overlaps the provider disconnect and node offline handling changed here.

Poem

A rabbit queued each node with care,
So racing providers share the air.
Attach and disconnect, side by side,
Keep every capability safe inside.
One name may win, conflicts grow tame—
The node remembers the rightful name.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: a provider-attach race causing capability loss at node up.
Description check	✅ Passed	The description is detailed and directly matches the changeset, describing the race, root cause, fix, and tests.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch provider-attach-race

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

gemini-code-assist · 2026-07-10T13:53:28Z

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 271be843cf

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…ueue Resolving `boundProviderName` inside the serialized callback let a queued heartbeat/deregister/agent.register fall back to `default` when the socket closed first (onNodeConnectionClose clears the connection's provider binding synchronously), acting on the wrong provider. Capture the binding at frame arrival so it reflects the provider that sent the frame; the SDK gates heartbeats on registration completion so the binding is present at arrival. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

cubic-dev-ai

All reported issues were addressed

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

… lock key - sweepOfflineNodes re-reads the node inside the serialized callback and skips it if a queued heartbeat/register refreshed it, so a now-live node isn't taken offline on a stale snapshot. - serializeNodeOp keys on an encoded (workspaceId, nodeId) tuple so ids containing the separator can't cross-serialize unrelated nodes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/engine/src/engine/node.ts`:
- Around line 512-531: The direct node-offline branch in onNodeConnectionClose
is not serialized and can race with node.register. Wrap the else-if
(!hasRemaining) markNodeOffline operation in serializeNodeOp using the same
workspaceId and nodeId keys, while preserving the existing provider-disconnect
path and early-return behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fc21b1da-a628-42f5-96cf-2df2793b77e6

📥 Commits

Reviewing files that changed from the base of the PR and between e905f3a and 271be84.

📒 Files selected for processing (4)

packages/engine/src/__tests__/conformance/providerAttachRace.test.ts
packages/engine/src/engine/node.ts
packages/engine/src/engine/nodeLock.ts
packages/engine/src/routes/node.ts

…on main) Unblocks the Rust SDK CI job, red repo-wide since the CI clippy toolchain moved to 1.97. Behavior-neutral; not part of the provider-attach fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

onNodeConnectionClose discarded any error from handleProviderDisconnect / markNodeOffline with `.catch(() => {})`. A failure there (e.g. SQLITE_BUSY) would silently drop the provider-offline / aggregate recompute, leaving the node roster with stale capabilities and no signal — a lock timeout turning into invisible data loss. Log the failure with context; a close still must not throw. Hardens the register-vs-teardown path fixed in #251. The conformance test's register-vs-disconnect loop now asserts zero `[node.teardown]` errors are logged, so a regression that reintroduced the swallow is caught. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* engine: log node teardown failures instead of swallowing them onNodeConnectionClose discarded any error from handleProviderDisconnect / markNodeOffline with `.catch(() => {})`. A failure there (e.g. SQLITE_BUSY) would silently drop the provider-offline / aggregate recompute, leaving the node roster with stale capabilities and no signal — a lock timeout turning into invisible data loss. Log the failure with context; a close still must not throw. Hardens the register-vs-teardown path fixed in #251. The conformance test's register-vs-disconnect loop now asserts zero `[node.teardown]` errors are logged, so a regression that reintroduced the swallow is caught. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address review: preserve stack in teardown logs; don't swallow unrelated errors - Pass the error object to console.error at both teardown sites so the runtime logs its stack trace, not just the message. - The register-vs-disconnect test spy forwards non-teardown console.error calls to the original, so unrelated errors during the run aren't hidden. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

chatgpt-codex-connector Bot reviewed Jul 10, 2026

View reviewed changes

Comment thread packages/engine/src/engine/node.ts Outdated

cubic-dev-ai Bot reviewed Jul 10, 2026

View reviewed changes

Comment thread packages/engine/src/engine/node.ts Outdated

Comment thread packages/engine/src/engine/nodeLock.ts Outdated

coderabbitai Bot reviewed Jul 10, 2026

View reviewed changes

Comment thread packages/engine/src/engine/node.ts

willwashburn mentioned this pull request Jul 10, 2026

Fix sdk-rust clippy 1.97 lint failing CI repo-wide #255

Closed

Drop redundant references in format! args (clippy 1.97, pre-existing …

e526eff

…on main) Unblocks the Rust SDK CI job, red repo-wide since the CI clippy toolchain moved to 1.97. Behavior-neutral; not part of the provider-attach fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

willwashburn merged commit d234fee into main Jul 10, 2026
4 checks passed

willwashburn deleted the provider-attach-race branch July 10, 2026 16:18

willwashburn mentioned this pull request Jul 10, 2026

Provider capability aggregation drops a provider non-deterministically when two providers attach concurrently on one node #250

Closed

This was referenced Jul 10, 2026

engine: log node teardown failures instead of swallowing them #256

Merged

engine: gate self-host SQLite writes so a raw write can't deadlock a transaction #257

Merged

coderabbitai Bot mentioned this pull request Jul 11, 2026

engine: log rejected node-control messages server-side #259

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

engine: fix provider-attach race dropping a provider's capabilities at node up#251

engine: fix provider-attach race dropping a provider's capabilities at node up#251
willwashburn merged 4 commits into
mainfrom
provider-attach-race

willwashburn commented Jul 10, 2026 •

edited by cubic-dev-ai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

gemini-code-assist Bot commented Jul 10, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

willwashburn commented Jul 10, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The bug (P0)

Root cause

Fix

Tests

Coordination note

Uh oh!

coderabbitai Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

gemini-code-assist Bot commented Jul 10, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

willwashburn commented Jul 10, 2026 •

edited by cubic-dev-ai Bot

Loading

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading