Repository navigation
fix(engine): recover strict spawn claims after crashes - #646
khaliqgant wants to merge 2 commits into
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
 |
|
Blocking independent exact-head finding at
The current SQLite/fake-D1 race tests serialize this interleaving and do not prove PostgreSQL safety. Please reverse the ownership order inside the atomic unit: first CAS the invocation marker to a unique reservation token, then increment the exact node guarded by that exact token. A capacity refusal must roll both writes back. Add a real PostgreSQL concurrency regression (where available) plus adapter-agnostic coverage proving two concurrent recovery calls produce one marker, one slot, and one provider frame. Independent reproduction is in progress. The PR must remain draft/held; no merge or deployment is approved. |
|
Additional confirmed PostgreSQL race in the same exact head: |
|
Secondary robustness finding at the same exact head: |
|
Independent exact-head review verdict for The existing focused/full tests pass, but a real PostgreSQL 17.11 reproduction confirms both concurrency defects: Thus reservation leaks a slot and cleanup can steal an unrelated slot. The repair must use a unique durable ownership token: win the invocation token CAS first, then mutate exact-node capacity guarded by that exact token in the same atomic unit. A third finding remains: strict-row parsing occurs outside per-row sweep isolation, so a malformed persisted strict row can abort the global sweep and block unrelated expiry work. Verification at the reviewed head: These passing tests do not cover the reproduced PostgreSQL interleavings. No reviewer source changes, merges, deployments, publications, or gate changes were made. |
|
Exact-head repair evidence for The PostgreSQL READ COMMITTED races are repaired with one cross-adapter ownership protocol:
Real PostgreSQL 17.11 mutation, old reservation order restored: Real PostgreSQL 17.11 mutation, old cleanup order restored: Malformed-row mutation, parser moved back outside the per-row guard: After byte-for-byte restoration, the exact three PostgreSQL cases pass: Final exact-head engine suite: Migration/compile/static checks: I did not claim the whole |
|
Independent exact-head review of Reviewed PostgreSQL READ COMMITTED ownership ordering, D1 ordered required-atomic batches, concurrent cleanup preserving an unrelated reservation, dispatch-vs-cleanup CAS behavior, nullable-token compatibility, all token clear/restore paths, SQLite/Actor/PostgreSQL migrations, malformed-row isolation, and the literal mutation evidence. The evidence accurately reproduces the old reserve leak ( Independent commands: Boundary: GitHub's exact-head real-image check was still running at review completion, so this is source/test signoff, not a CI or merge-readiness claim. |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| 409, | ||
| ); | ||
| } | ||
| const input = recordInput(args.invocation.input); |
There was a problem hiding this comment.
🔴 Recovered spawns use stale working directories
After a pre-dispatch crash, a strict invoke with a new worker_cwd dispatches the original directory. resumeStrictSpawnInvocation uses persisted input, ignoring the retry's permitted rematerialization. The worker starts in the wrong directory or fails to start.
Learn more
A strict spawn stores the first materialized input when createInvocation inserts the idempotency claim. If the server stops before dispatch, a retry can supply a different worker_cwd for the same logical request. The new recovery path reads the stored input rather than that retry's materialization and sends the stale directory to the provider. The replay endpoint has no materialized input, so it can continue using stored input.
Example: A first strict invoke stores worker_cwd: '/old/repo' and crashes before dispatch. The client retries the same logical request with worker_cwd: '/new/repo'; the provider receives /old/repo instead of /new/repo.
Recommended fix: Pass the retry's prepared input into resumeStrictSpawnInvocation from dispatchSpawn. When a strict invoke finds a pre-dispatch claim, atomically update only worker_cwd before claiming dispatch, guarded by the invocation's pending, zero-attempt state; use the resulting input for the frame. Keep /spawn/replay on persisted input.
Was this helpful? React with 👍 or 👎 to provide feedback.
| return resumeStrictSpawnInvocation({ | ||
| db, | ||
| registry: options.nodeConnections, | ||
| invocation, | ||
| replayed: true, | ||
| }); |
There was a problem hiding this comment.
🟡 Spawn replay documentation promises read-only behavior
For a pending strict claim, replaySpawnInvocation now dispatches a spawn. The API description and README guidance still promise read-only replay, misleading callers about its side effects.
Learn more
The replay route previously only recovered a durable claim. It now calls resumeStrictSpawnInvocation, which can reserve a node slot and send a provider frame when the claim is still pre-dispatch. The API description says it never selects a node or dispatches a frame, and the README guidance makes the same promise. Repository instructions require updating both documents when API behavior changes.
Example: A client calls /v1/actions/spawn/replay expecting only to inspect an accepted claim after a crash. The server sends the pending spawn to the provider, contrary to both documented contracts.
Recommended fix: Update both the replay route description and response text in openapi.yaml and the corresponding README paragraph to describe recovery dispatch and its failure outcomes.
Was this helpful? React with 👍 or 👎 to provide feedback.
![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: cb8814be97
ℹ️ 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".
| if (!dispatched.accepted) { | ||
| const [winner] = await args.db |
There was a problem hiding this comment.
If sendToProvider throws after the provider may have received the frame, dispatchNodeInvocation restores the invocation to pending with zero attempts and returns sendOutcomeUnknown: true. This new recovery path ignores that distinction, so the next strict invoke or /spawn/replay treats the restored row as a recoverable predispatch claim and sends the same spawn again. Previously, keyed replay only observed that row rather than sending again. Preserve a durable non-replayable dispatch state for unknown outcomes so a transport/RPC failure cannot cause duplicate execution.
Useful? React with 👍 / 👎.
| preserveSpawnReservation: !!current.spawnReservedAt, | ||
| observedSpawnReservedAt: current.spawnReservedAt, |
There was a problem hiding this comment.
With verify_ready: false, concurrent recoveries can observe different node readiness around a reconnect. Both can read an unreserved claim, then the live contender reserves capacity while the queued contender skips this reservation branch. The queued contender still dispatches using its stale spawnReservedAt: null; dispatchNodeAttempt adds a reservation predicate only for non-null holds, so it can win the CAS and clear the other contender's durable reservation marker without decrementing reservedAgents. That slot then cannot be returned by completion or sweeping. Make the dispatch CAS match the observed null reservation state as well, and reload/retry if another recovery has acquired a hold.
Useful? React with 👍 / 👎.
 [Medium impact] The changes reach strict spawn recovery and pending-invocation expiry cleanup. Fix the reconnect race that leaks reserved capacity before merging.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pending: queued, | ||
| reservationHeld: !!current.spawnReservedAt, | ||
| claimStatus: 'pending', | ||
| observedRetryAfterAt: current.retryAfterAt, | ||
| observedDispatchAttempts: 0, | ||
| observedDispatchedNodeId: null, | ||
| preserveSpawnReservation: !!current.spawnReservedAt, | ||
| observedSpawnReservedAt: current.spawnReservedAt, |
There was a problem hiding this comment.
With verify_ready: false, request A can see the node offline and skip reservation. A concurrent request B then sees it online and reserves a slot. A can still win dispatch using its old snapshot: dispatchNodeAttempt checks spawnReservedAt only when it is non-null, then clears both reservation fields because A passed reservationHeld: false.
B loses dispatch, but its counted slot remains. Completion cannot return it because the marker is gone, so later spawns can be rejected as full despite available room. Make dispatch match the observed reservation state even when it is null, and retry from the current row when that check loses.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/engine/src/engine/action.ts
Line: 2325-2332
Comment:
**Reconnect retries leak capacity**
With `verify_ready: false`, request A can see the node offline and skip reservation. A concurrent request B then sees it online and reserves a slot. A can still win dispatch using its old snapshot: `dispatchNodeAttempt` checks `spawnReservedAt` only when it is non-null, then clears both reservation fields because A passed `reservationHeld: false`.
B loses dispatch, but its counted slot remains. Completion cannot return it because the marker is gone, so later spawns can be rejected as full despite available room. Make dispatch match the observed reservation state even when it is null, and retry from the current row when that check loses.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
Readiness and prior |
Outcome
Successor to #644 for the two strict-spawn pre-dispatch crash windows identified in the exact-head review.
spawnReservedAtatomically viarunAtomicWrites(..., { requireAtomic: true })and the existing node-capacity sentinel (state B);/spawn/replay, without creating claims from replay;Exact scope
c74c34065c3fa7bcc95d98a3ea4038e44dee9340(unchanged feat(engine): bind strict spawn replay to exact nodes #644 head)afba5d631672a2dd80910f90d380137e9e84db50Verification
Literal output is included in the commands below.
The focused crash/restart matrix covers state A, state B, concurrent invoke/replay, D1 first- and second-write failure, interactive-transaction rollback, missing atomic capability, exact-once sweep cleanup, dispatch-vs-sweep, recovered proof, and no alternate-node fallback:
Mutation evidence
Specific mutation: replace the atomic reservation+marker batch with two sequential awaits. The D1 marker-failure test then leaves one reserved slot:
After byte-for-byte restoring
runAtomicWrites(..., { requireAtomic: true }):Environment note
A fresh
npm ciin this/tmpclone could not complete because the host exhausted/tmpinodes. Verification used the existingnode_modulesfrom the exact same #644 head worktree; CI remains the independent clean-install check.No merge, deploy, publish, or gate edit is included.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.