Repository navigation
fix(test): avoid false native polling timeouts - #140119
Merged
Merged
Conversation
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: needs maintainer review before merge. Reviewed September 6, 2026, 5:27 PM ET / 21:27 UTC. ClawSweeper reviewWhat this changesThe PR makes two native test waiters recheck completion before throwing a timeout and adds regression coverage for delayed observations, timeout labels, and cancellation. Merge readiness✅ Ready for maintainer review The repair remains useful: current main still has both false-timeout paths. No blocking defect was found in the introduced changes, and the maintainer-authored PR is protected from automatic closure. Priority: P2 Review scores
Verification
How this fits togetherOpenClaw’s native test waiters poll asynchronous observations while testing shared Swift components and the macOS node coordinator. They return when the observed work completes or report a labeled timeout. flowchart TD
A[Native test] --> B[Async condition]
B --> C[Polling waiter]
C --> D{Completion observed?}
D -->|Yes| E[Test continues]
D -->|No and time remains| B
D -->|Deadline passed| F[Final condition read]
F -->|Complete| E
F -->|Incomplete| G[Labeled timeout]
Before mergeNone. Agent review detailsSecurityNone. PR surfaceOther +76. Total +76 across 3 files. View PR surface stats
Review metricsNone. Technical reviewBest possible solution: Keep the final observation at each waiter’s timeout boundary while preserving existing deadlines, cancellation propagation, and substantive native-test assertions. Do we have a high-confidence way to reproduce the issue? Yes, source establishes the failure path: a false asynchronous snapshot can return after completion and the deadline, causing the old waiter to throw without rereading. The PR reports before/after reproduction; this review did not execute it. Is this the best way to solve the issue? Yes. One final observation repairs the waiter-owned decision without increasing timeout budgets or weakening the assertions in its callers. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 270762e51d65. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles) |
steipete
marked this pull request as ready for review
September 6, 2026 13:08
Contributor
Author
|
Merged via squash.
|
vincentkoc
added a commit
that referenced
this pull request
Sep 6, 2026
* commit '294df333c58f2da104eccffc623ccb3c4d43d4d8': (1802 commits) fix: clear stale diagnostics after Gateway changes (#140426) fix: keep plugin catalogs intact after discovery timeouts (#140441) perf(tui): prepare session picker rows once per overlay (#140462) refactor(config): reuse environment substitution for write-back (#140460) refactor(discord): remove retired auto-presence templates (#140463) fix(ui): finish roster refreshes during session activity (#140459) fix(setup): match prepared credentials to the model provider (#140461) fix: keep MiniMax discovery on the selected credential (#140438) refactor(tests): remove unused parent context cache reset (#140456) fix: avoid repeated xAI OAuth refresh during discovery (#140434) perf(routing): reuse prepared event session targets (#140454) fix(media): keep deferred audio and extraction providers discoverable (#139231) refactor(codex): remove redundant app-server forwarding (#140451) fix(test): avoid false native polling timeouts (#140119) test(gateway): retain saved source in reload fixtures (#140453) fix(gateway): reduce repeated model work when listing sessions (#140414) fix(feishu): fall back to post mode over table limit (#136382) fix: use the bundle-compatible Proxyline runtime under Bun (#140411) refactor(trace): reuse the canonical usage counter type (#140444) refactor(node-host): share execution policy result fields (#140446) ...
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Sep 7, 2026
* test(native): recheck completion before polling timeout * test(macos): recheck completed coordinator waits at timeout
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
Resolves false native test timeouts when an asynchronous observation and the final polling sleep cross the deadline after the observed work has completed. The shared test helper and the macOS coordinator's test waiter previously threw without reading the condition again.
Why This Change Was Made
Both waiters check the condition once more before their existing timeout error. This follows their existing semantics: a true asynchronous result can already finish after the polling deadline. The shared helper retains its 15-second default and wall clock; the coordinator retains its two-second continuous-clock deadline. Both retain 10ms polling, typed errors and cancellation propagation from their throwing sleep. The coordinator's unused timeout parameter is removed.
User Impact
Test reliability only; production code is unchanged. Shared tests cover completed asynchronous state, the unchanged incomplete-condition timeout label, and cancellation of an owned child task. The coordinator regression reuses its existing completion probe. The total change is 76 net test and test-support lines across three files.
Evidence
Isolated Foundation/Testing targets reproduced the false timeout with each original waiter and passed with each repair. The shared helper's exact three permanent cases failed only the stale-observation regression before the fix and passed all three afterward. The coordinator's exact new regression likewise failed before and passed afterward. These runs imported no app modules and touched no operator state; they do not establish a historical CI scheduling interval.
Formatting and typechecking passed. All 13 canonical changed checks passed, including 1,125 Node/tooling executions across 21 file runs. One independent P2 review of the complete three-file change is clean. The committed files match that reviewed candidate exactly.
The combined head's CI run is green. All four regressions passed in their actual native targets. The shared package reports 1,673 tests / 127 suites, and the macOS default profile reports 2,075 / 226. Its three normal skips remain recorded; separate isolation (2 / 1) and health-render (1 / 1) invocations passed. The response-loss and embedded-handshake probes remain unexecuted. The tested merge contains the exact three reviewed files, and iOS/Android tests, builds and screenshot jobs passed.
The earlier shared-only CI run passed its native waiter proof but failed an Android composer assertion and the aggregate gate. That historical failure is retained without attributing it to this repair; the fresh combined-head Android Play job passed.