Repository navigation
fix(test): keep provider teardown deadline tests from wedging their shard - #139090
Merged
Merged
Conversation
…hard The five-minute checkpointing tests held the provider mock on an unresolved promise and awaited mock readiness without advancing fake timers. A watchdog timeout then left the provider held, and afterEach joined it via service.stop() while fake timers were still installed, turning one slow test into a test-timeout plus hook-timeout pair (seen failing repeatedly on main CI). Await mock readiness through the existing timer-aware waitForFast, race held mocks against the test cancellation signal, and restore real timers before draining operations and before service.stop() in the shared fixture. Assertions and production code are unchanged.
Contributor
|
🦞👀 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. |
Contributor
|
Codex review: blocked before merge. Reviewed September 5, 2026, 9:02 AM ET / 13:02 UTC. ClawSweeper reviewWhat this changesMakes worker teardown tests release held provider mocks when cancelled and restore real timers before fixture shutdown. Merge readiness⛔ Blocked before merge - 3 items remain Keep open: the test-lifecycle hardening remains absent from main and preserves meaningful deadline coverage. No concrete introduced defect was found; direct verification of the pinned Vitest contract remains incomplete. Priority: P2 Review scores
Verification
How this fits togetherGateway worker tests exercise cloud-worker creation and teardown through fake providers and a real service instance. Their shared fixture must drain pending operations before closing its database and allowing subsequent tests to run. flowchart TD
A[Deadline test cases] --> B[Worker environment service]
B --> C[Held provider mock]
D[Test cancellation or explicit release] --> C
C --> E[Provider operation settles]
E --> F[Fixture shutdown and database cleanup]
G[Restore real timers] --> F
Before merge
Agent review detailsSecurityNone. PR surfaceTests +7. Total +7 across 2 files. View PR surface stats
Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep provider-drain guarantees intact while making held test fixtures cancellable and their shutdown independent of fake timers. Do we have a high-confidence way to reproduce the issue? Unclear for the initiating CI stall. Source establishes the held-provider/shutdown wait cycle, and the PR reports synthetic reproduction; no reproduction was executed during this read-only review. Is this the best way to solve the issue? Acceptable mitigation: the patch repairs test-owned lifetimes without weakening production shutdown or deadline assertions. Final best-fix verification still needs direct confirmation of the pinned Vitest contract. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 4cb260cd132f. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Contributor
Author
|
Maintainer disposition for the three open items (head bdfedd8): Vitest contract verification (the two dependency-inspection items): verified directly against the installed vitest 4.1.11.
The initiating stall remains unexplained: accepted and stated as such in the PR body. The patch converts the two mechanisms that turn a slow turn into a shard-wedging test+hook timeout pair; it does not claim to eliminate the original stall. If main CI stalls again, the recorded follow-up is phase-boundary instrumentation in the non-isolated worker. Containment with intact assertions is the right landing unit here — the alternative is leaving a known shard-wedger live on main while chasing an unreproduced trigger. CI is green at bdfedd8; the full gateway-core shard file set also passed on a Linux Testbox replay (3,397 tests). |
Contributor
Author
|
Landed as 89adb23. How this was tested:
Known limit, stated on the PR: the initiating stall was not naturally reproduced — this patch hardens the two mechanisms that produced the observed test-timeout + hook-timeout signature. If main stalls again, the follow-up is phase-boundary instrumentation in the non-isolated worker. |
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Sep 6, 2026
…hard (openclaw#139090) The five-minute checkpointing tests held the provider mock on an unresolved promise and awaited mock readiness without advancing fake timers. A watchdog timeout then left the provider held, and afterEach joined it via service.stop() while fake timers were still installed, turning one slow test into a test-timeout plus hook-timeout pair (seen failing repeatedly on main CI). Await mock readiness through the existing timer-aware waitForFast, race held mocks against the test cancellation signal, and restore real timers before draining operations and before service.stop() in the shared fixture. Assertions and production code are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
src/gateway/worker-environments/provider-destroy-timeout.test.ts("allows provider-owned checkpointing beyond five minutes during destroy/bootstrap-failure") failed four times on main CI today (runs 33959274889, 33960371295, 33961192294, 33962562919), each time as a 120s test timeout followed by a 180s hook timeout that wedged the whole shard.Why This Change Was Made
Two lifetime defects amplified any slow turn into a shard wedge:
finish.promisewhose only release was the test body'sfinally. Vitest's watchdog aborts the test context but does not unwind a suspended async body, so a timed-out test kept the provider held — andafterEach'sservice.stop()correctly joined it while fake timers were still installed, hanging the hook too.await started.promisewaited for mock readiness without advancing fake timers, so any chain progress that requires a timer turn deadlocks permanently.The fix awaits mock readiness through the existing timer-aware
waitForFast, races held mocks against the test cancellation signal (racePromiseWithAbortSignal), and restores real timers before draining operations and beforeservice.stop()in the shared fixture. All assertions and the five-minute checkpointing semantics are unchanged; production code is untouched.User Impact
No user-facing change — test robustness only. Main CI stops losing the
checks-node-compact-large-1shard to this wedge.Evidence
waitForFast; a suspended test cancellation wedges the original hooks and unwinds cleanly with the patch.tbx_01m1rr5m48gzwx01nhhk44es9w): the exact gateway-core-3 file set, 182 files / 3,397 tests passed.Checklist