Repository navigation
test(postgres): stop the provisioning worker in the pass that ends the first attempt - #2667
Conversation
…e first attempt The "losing native Sandbox containment" case waited for its work's first worker.completed event, then stopped the worker. A retry's backoff is full-jitter (up to 1 s on attempt 1) and the worker claims back to back, so a legal second attempt could land in between and the case read attempt_count 2 (finding 1610; 1 in 104 natural runs at 2 CPUs, 9 in 30 with a 300 ms window between the event and the stop). Add a fixture option, stopWorkerAfter(event), that stops the worker inside the pass that emitted the event, as stopAfterProvisioning already does, and key it to the work item. Apply it to the same shape in "an unusable Secret Driver leaves the provisioning work retryable", which now also pins that only one attempt ran. Also pin the replay of the accepted request ID after containment loss to the stored work's retryable 503, not the new-request 409 (finding 1614).
Internal review found the same race shape in "guided source provisioning authorizes its source and rechecks ownership before worker effects": it waited for the first worker.completed, then stopped the worker, so a legal retry could run in between. Stop in the pass instead and pin its single retryable attempt. Label the fixture comment per option and say why the Secret Driver case guards an unset workId.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Failure
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: needs maintainer review before merge. What this changesThis PR stops three provisioning tests inside their first completed worker pass and checks replay behavior after Sandbox containment is lost. Example: An accepted provisioning request loses process containment before its first worker attempt.
Review scores
ProductKind: Test only · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open: it repairs a credible integration-test race and adds useful replay coverage without changing production behavior; no actionable patch defects were found. Priority: P3 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe PostgreSQL integration fixture sends authenticated requests through the controller app and runs the real durable worker against PostgreSQL, using fixture Drivers to inspect provisioning outcomes. flowchart LR
A[Integration scenario] --> B[Controller API]
B --> C[PostgreSQL work queue]
C --> D[Controller worker]
D --> E[Completion hook]
E --> F[Stop before next claim]
Technical reviewBest possible solution: Use completion-hook shutdown to make first-attempt assertions deterministic while preserving production retry behavior. Do we have a high-confidence way to reproduce the issue? Source establishes the race: jitter permits an immediate retry, the worker claims back to back, and the original tests stop only after polling completion; the PR also reports observed attempt_count 2 failures. Is this the best way to solve the issue? The fixture uses the existing worker shutdown contract at the completion boundary, removes the scheduling window, and retains independent queue-state and API assertions. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6fd73ecd514a. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 16, 2026, 9:35 PM ET / October 17, 2026, 01:35 UTC. |
freeqaz-openai
left a comment
freeqaz-openai
left a comment
There was a problem hiding this comment.
I verified the race on 51a9995a. With a 1.4-second delay before the test calls stopWorker(), the published code still recorded one attempt. With the same delay and only the new in-pass stop disabled, the assertion failed on attempts 1, 2, and 3. That is the distinction we need: production may retry promptly; this test deliberately observes the end of the first attempt.
All three changed cases passed against a private PostgreSQL 18 database using the repository migrations, real worker, and fixture drivers. The containment case also exercised replay of the original accepted request (503, not fresh-admission 409), then recovered the same work ID when containment returned. In the worker, stop() sets the stopping flag before its first await; the fixture saves that promise and awaits it outside the event callback. No production timing change or exactly-once claim.
Problem
The PostgreSQL test "losing native Sandbox containment before the worker runs leaves provisioning retryable without effects" passed 19 of 20 runs at 2 CPUs (finding 1610). In the failing run,
occ.controller_workshowedattempt_count: 2.This is a race in the test, not a product bug:
worker.completedevent, then calledstopWorker().1000 ms * 2^(n-1) * random), so the first retry can be due almost at once.maxAttemptsis 3) could run between the test seeing the event (it polls every 20 ms) and the stop.Two other cases in the same file have the same shape: "an unusable Secret Driver leaves the provisioning work retryable" and "guided source provisioning authorizes its source and rechecks ownership before worker effects". Their assertions happened to tolerate a second attempt.
Separately, no test pinned what a replay of an already-accepted request ID returns after containment is lost (finding 1614). It should be the stored work's retryable 503, not the 409 a new request gets. Changing the replay branch to full admission broke no test.
Change
Test-only, in
tests/integration/postgres-agent-provisioning.test.mjs:stopWorkerAfter(event). It stops the worker inside the pass that emitted the matching event, asstopAfterProvisioningalready does, so no further claim can start. The emit hook now holds its own worker reference and does not replace a stop that is already in flight (??=).workId. Each now asserts its full list of passes: exactly oneretry/PROVISIONING_DEPENDENCY_UNAVAILABLEattempt. The Secret Driver case also pinsqueuedwithattempt_count: 1.Every other
stopWorker()in the file follows a terminal state, a permanent outcome or a deferred settle-window row, so none of them can race this way.Verification
All runs at 2 CPUs (
taskset -c 0,1) against a preparedpostgres-applicationdatabase:attempt_count: 2). That is about 1 in 104.attempt_count: 2.Mutants (all three cases in one prepared run; queued leftovers parked between runs):
processPROVISIONING_DEPLOYMENT_PENDING)permanent/PROVISIONING_REJECTED)stopWorkerAfter, 1.5 s windowstopWorkerAfter, 1.5 s windowauthorizeProvisioningRecord(..., { admission: true })Other checks:
prettier --checkandeslint --max-warnings 0pass on the touched file.git merge-treeagainst this head shows the same conflicted files and hunks as againstorigin/main. No new conflicts.Risks
Low. The change is test-only. The new fixture option is opt-in, and
stopAfterProvisioningbehaves as before, except that a second matching event no longer replaces the stop that is already in flight.