engine: reschedule node-scoped actions to the owning provider, not the broker#262
Conversation
…e broker rescheduleNodeInvocation's generic-placement dispatch (action.ts) called dispatchNodeInvocation with no providerName, so on a multi-provider fallback node it fell back to the default provider (the broker) via sendToNode -> defaultProviderName. The broker rejects a node-scoped action with handler_unavailable, and the invocation loops between nodes without completing. Resolve the chosen node's action row (fetchNodeAction) and dispatch to its handlerProvider, mirroring the initial-invoke path. When there is no node action row (spawn/capacity, legacy single-socket nodes) the provider falls back to the node default as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughNode-scoped action rescheduling now resolves the action-owning provider on fallback nodes, applies provider liveness and queue policies, retries alternate placements when providers are unavailable, and records action ownership during dispatch. Conformance tests cover routing, queueing, failures, and capacity release. ChangesProvider-aware rescheduling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Rescheduler
participant FallbackNode
participant FleetProvider
participant Database
Rescheduler->>FallbackNode: Resolve the action-owning provider
FallbackNode-->>Rescheduler: Return handlerProvider
Rescheduler->>FleetProvider: Dispatch action.invoke
FleetProvider->>Database: Persist invocation status
Database-->>Rescheduler: Return dispatch state
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where rescheduling a node-scoped action onto a fallback node incorrectly targeted the node's default (broker) provider instead of the provider that owns the action. This is resolved by fetching the action-owning provider on the chosen node and passing it to dispatchNodeInvocation. A conformance test was also added to verify this behavior. There are no review comments, so no further feedback is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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: 3a96be6935
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/engine/src/engine/action.ts (1)
921-939: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear
actionIdon fallback dispatches.dispatchNodeAttemptpreserves the previousactionIdwhenopts.actionIdis omitted, andrescheduleNodeInvocationcan hit that path when a retry lands on a node that has the capability but no node-scoped action row. That leavesactionInvocations.actionIdpointing at the old node, sodrainNodeInvocationsjoins the wrong handler and can skip the invocation on the node it was actually dispatched to.Proposed fix
.set({ ...stateFields, dispatchedNodeId: nodeId, spawnReservedAt: opts.reservationHeld ? new Date() : null, - ...(opts.actionId ? { actionId: opts.actionId } : {}), + actionId: opts.actionId ?? null, ...attemptFields, })🤖 Prompt for 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. In `@packages/engine/src/engine/action.ts` around lines 921 - 939, Update dispatchNodeAttempt so fallback dispatches clear actionInvocations.actionId when opts.actionId is omitted, rather than preserving the previous node’s action ID; retain the provided action ID when opts.actionId exists. Ensure rescheduleNodeInvocation paths without a node-scoped action row leave the invocation associated with the newly dispatched node.
🧹 Nitpick comments (1)
packages/engine/src/engine/action.ts (1)
1151-1157: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoidable per-row provider lookup in the drain loop.
getProvideris now queried once per pending invocation row inside thefor (const row of rows)loop. Multiple queued invocations commonly share the same(nodeId, providerName), so this is an N+1 query pattern on the reconnect-drain hot path. Consider caching the provider row perproviderNamefor the duration of the function call.♻️ Proposed refactor
+ const providerCache = new Map<string, ProviderRow | null>(); for (const row of rows) { ... if (!registry.isProviderConnected(workspaceId, nodeId, providerName)) continue; - const provider = await getProvider(db, workspaceId, nodeId, providerName); + let provider = providerCache.get(providerName); + if (provider === undefined) { + provider = await getProvider(db, workspaceId, nodeId, providerName); + providerCache.set(providerName, provider); + } const explicitlyAttached = registry.isProviderAttached?.(workspaceId, nodeId, providerName) ?? true; if (provider && explicitlyAttached && !isProviderLive(provider)) continue;🤖 Prompt for 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. In `@packages/engine/src/engine/action.ts` around lines 1151 - 1157, Cache provider lookups within the reconnect-drain function so repeated rows sharing a providerName reuse the same getProvider result instead of issuing one query per row. Update the loop around registry.isProviderConnected and isProviderLive to read from this per-call providerName cache while preserving the existing connection, attachment, and liveness checks.
🤖 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/action.ts`:
- Around line 1320-1328: Update the queued placement branch in the dispatch flow
to capture the result returned by dispatchNodeAttempt and return its accepted
status instead of unconditionally returning true. Preserve the existing dispatch
arguments and ensure race-lost guarded updates report false to callers such as
rescheduleInvocationsForLostNode.
---
Outside diff comments:
In `@packages/engine/src/engine/action.ts`:
- Around line 921-939: Update dispatchNodeAttempt so fallback dispatches clear
actionInvocations.actionId when opts.actionId is omitted, rather than preserving
the previous node’s action ID; retain the provided action ID when opts.actionId
exists. Ensure rescheduleNodeInvocation paths without a node-scoped action row
leave the invocation associated with the newly dispatched node.
---
Nitpick comments:
In `@packages/engine/src/engine/action.ts`:
- Around line 1151-1157: Cache provider lookups within the reconnect-drain
function so repeated rows sharing a providerName reuse the same getProvider
result instead of issuing one query per row. Update the loop around
registry.isProviderConnected and isProviderLive to read from this per-call
providerName cache while preserving the existing connection, attachment, and
liveness checks.
🪄 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: 68d25611-54d1-4a16-81ad-311547708719
📒 Files selected for processing (5)
packages/engine/CHANGELOG.mdpackages/engine/src/__tests__/conformance/rescheduleNodeProvider.test.tspackages/engine/src/adapters/node/realtime.tspackages/engine/src/engine/action.tspackages/engine/src/ports/realtime.ts
✅ Files skipped from review due to trivial changes (1)
- packages/engine/CHANGELOG.md
| if (placement.queued) { | ||
| await dispatchNodeAttempt(db, invocation.workspaceId, invocation.id, placement.node.id, { | ||
| pending: true, | ||
| retryAfterAt: opts.retryAfterAt ?? null, | ||
| reservationHeld: false, | ||
| actionId: target?.id, | ||
| }); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Discarded dispatch result: queued fallback always reports success.
dispatchNodeAttempt's return value (whether the guarded DB update actually applied) is discarded here, and the branch unconditionally return trues — unlike the sibling branches at Lines 1315 and 1340, which correctly propagate dispatched.accepted. If the guarded update loses the race (invocation left OPEN_INVOCATION_STATUSES concurrently, e.g. completed/cancelled), this branch still reports the reschedule as successful to callers like rescheduleInvocationsForLostNode, which increments its rescheduled counter on a no-op.
🛠️ Proposed fix
if (placement.queued) {
- await dispatchNodeAttempt(db, invocation.workspaceId, invocation.id, placement.node.id, {
+ const accepted = await dispatchNodeAttempt(db, invocation.workspaceId, invocation.id, placement.node.id, {
pending: true,
retryAfterAt: opts.retryAfterAt ?? null,
reservationHeld: false,
actionId: target?.id,
});
- return true;
+ return accepted;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (placement.queued) { | |
| await dispatchNodeAttempt(db, invocation.workspaceId, invocation.id, placement.node.id, { | |
| pending: true, | |
| retryAfterAt: opts.retryAfterAt ?? null, | |
| reservationHeld: false, | |
| actionId: target?.id, | |
| }); | |
| return true; | |
| } | |
| if (placement.queued) { | |
| const accepted = await dispatchNodeAttempt(db, invocation.workspaceId, invocation.id, placement.node.id, { | |
| pending: true, | |
| retryAfterAt: opts.retryAfterAt ?? null, | |
| reservationHeld: false, | |
| actionId: target?.id, | |
| }); | |
| return accepted; | |
| } |
🤖 Prompt for 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.
In `@packages/engine/src/engine/action.ts` around lines 1320 - 1328, Update the
queued placement branch in the dispatch flow to capture the result returned by
dispatchNodeAttempt and return its accepted status instead of unconditionally
returning true. Preserve the existing dispatch arguments and ensure race-lost
guarded updates report false to callers such as
rescheduleInvocationsForLostNode.
1 participant
What this is
When a node hosting an invoked node-scoped action dies, the crash-recovery reschedule dispatches the action to the provider that owns it on the chosen fallback node, instead of the node's default (broker) provider.
Why
rescheduleNodeInvocation's generic-placement branch calleddispatchNodeInvocationwith noproviderName. On a multi-provider node that routes viasendToNode→defaultProviderName, which returns thedefault(broker) provider. The broker rejects a node-scoped action withhandler_unavailable, so the invocation loops between nodes and never completes.Concretely: a node-scoped
workaction invoked on node-a; node-a is lost; the engine reschedulesworkto node-b, which hosts both a broker and a fleet provider — the reschedule went to node-b's broker and was rejected.The initial-invoke path (
invokeNodeAction/invokeAction) and the agent-target reschedule branch already resolve their provider; only the generic-placement branch omitted it.Change
Resolve the chosen node's action row with
fetchNodeActionand pass itshandlerProvidertodispatchNodeInvocation. When there is no node action row (spawn/capacity or a legacy single-socket node), the provider falls back toundefined→ the node default, preserving current behavior.Test
rescheduleNodeProvider.test.ts:workowned by a fleet provider on node-a; node-b is multi-provider (broker + fleet owningwork); invoke on node-a, lose node-a, reschedule — asserts the dispatch reaches node-b's fleet provider and not its broker. Verified to fail without the fix.Refs #250.
🤖 Generated with Claude Code