test(gcp): keep canceled HTTPS responses incomplete - #2484
Conversation
|
🦞👀 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. |
Bound Cloudflare Access signing-key fetch and body consumption to 15 seconds, abort stalled transport, and publish only the winning cache result. Preserve optional shared/admin bearer identity fallback, key-rotation refresh allowance, failure caching, and concurrent-load coalescing. Maintain documentation and changelog. Maintainer disposition: complete introduced-scope Codex review through P2 is clean. Native loopback HTTP with a real clock recorded the partial response closing after 15 seconds before cleanup while bearer authentication retained its fallback identity. All 3,410 Worker tests, both typechecks/builds, lint and formatting passed. Exact-head CI including Go core and Release Check succeeded; no unresolved review threads. No live Cloudflare tenant claim. An earlier main CI run hit the separate GCP cancellation-fixture race tracked in #2484; this candidate's complete CI passed without retry. #2483
clawsweeper
Bot
added
P3
|
Codex review: needs maintainer review before merge. Reviewed September 22, 2026, 8:02 AM ET / 12:02 UTC. ClawSweeper reviewWhat this changesThe PR keeps two GCP cancellation-test HTTPS handlers blocked until cleanup, preventing them from completing empty responses during cancellation assertions. Merge readiness✅ Ready for maintainer review The fixture correction remains useful and is absent from fetched main. No actionable defect was found; cleanup safely releases both handlers, and the supplied real-HTTPS evidence supports the change. Priority: P3 Review scores
Verification
How this fits togetherCrabbox’s GCP readiness checks use the Google SDK to observe a provisioned machine before SSH setup. These fixtures exercise cancellation through real HTTPS transport and verify the resulting errors and exit classifications. flowchart LR
A[Readiness request] --> B[Google SDK client]
B --> C[Local HTTPS fixture]
D[Caller cancellation or timeout] --> B
C --> E[Observe request cancellation]
E --> F[Wait for cleanup release]
B --> G[Assert cause and exit classification]
G --> F
Before mergeNone. Agent review detailsSecurityNone. Review metricsNone. Technical reviewBest possible solution: Keep canceled fixture responses incomplete until teardown while preserving production cancellation semantics and completed-response precedence. Do we have a high-confidence way to reproduce the issue? No deterministic reproduction of the original CI race is established: the author reports 100 unchanged-main repetitions passed. Source supports the unwanted handler-completion possibility, and supplied after-fix output verifies cancellation behavior. Is this the best way to solve the issue? Yes. Waiting on the existing cleanup channel narrowly prevents premature response completion without changing error classification, timing budgets, or assertions. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against be7cf9d50a61. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
Landed as 823588b through the normal protected squash path. The resulting tree matches the expected integration with current main. Both canceled HTTPS handlers now wait for their existing cleanup release instead of completing an implicit empty response; production code and assertions are unchanged. Both cancellation tests passed ten further race-detector repetitions on the actual merged checkout. Main CI, including the real readiness-budget test in Go core, succeeded at https://github.com/openclaw/crabbox/actions/runs/35726820631. The earlier CI error remains a source-supported fixture-race diagnosis, not a claim that its exact transport ordering was reproduced locally. |
1 participant
Summary
Keep the two GCP cancellation-test handlers blocked after observing request cancellation, until their existing cleanup release closes. Returning from a handler without writing a response can produce an implicit empty HTTP 200 while the client is still handling cancellation. The Google REST client may then attempt to decode that empty success response.
Main CI failed the real HTTPS readiness cancellation case with a protobuf syntax error instead of the expected cancellation cause and exit code: https://github.com/openclaw/crabbox/actions/runs/35721672744 . That failure is consistent with this fixture race; its exact transport ordering was not captured, and 100 repetitions of unchanged main passed locally. This is a source-supported fixture correction, not a claimed deterministic local reproduction of the CI failure.
No production code, timeout budgets, error classification, or assertions change. Existing teardown releases the handlers before closing the test servers. The adjacent direct SDK cancellation fixture uses the same pattern and gets the same correction.
Verification