fix(gcp): preserve readiness cancellation causes and deadlines - #2467
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. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 21, 2026, 7:08 PM ET / 23:08 UTC (Revision 2). ClawSweeper reviewWhat this changesThe PR preserves cancellation causes and deadline classification during GCP public-IP discovery, with regression coverage and documentation. Merge readiness✅ Ready for maintainer review The fix remains necessary on main. The new production-client HTTPS evidence resolves the previous proof blocker, and no actionable correctness or security findings remain. Priority: P2 Review scores
Verification
How this fits togetherCrabbox’s GCP adapter polls a newly created virtual machine for its public IP before starting SSH readiness checks. Polling failures feed acquisition cleanup and the CLI’s error classification. flowchart TD
A[New GCP virtual machine] --> B[Public IP discovery]
C[Caller cancellation or time budget] --> B
B --> D[Google Compute REST client]
D --> E{Observation result}
E -->|Public IP available| F[SSH readiness]
E -->|Error or interrupted request| G[Preserved error and run classification]
G --> H[Acquisition cleanup]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Use the shared termination wrapper while keeping GCP’s existing readiness, response-precedence, and diagnostic policies in its adapter. Do we have a high-confidence way to reproduce the issue? Yes, source establishes the failure path: main’s PollReady loses deadline identity on owned timeout and does not normalize interrupted-read cancellation. The contributor supplies failing-baseline and passing-after-fix evidence; this review did not execute tests. Is this the best way to solve the issue? Yes. Reusing the existing shared readiness policy avoids a second cancellation implementation while preserving GCP-specific behavior and public diagnostics. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 574d3d2808a2. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
|
@clawsweeper re-review Current head 23414b9 adds the requested real HTTPS/SDK proof through the production GCP client and changed readiness function. Both interrupted-read cancellation and the actual two-minute owned timeout pass, including cause identity, exact diagnostics, exit codes, classification, and server-observed cancellation. The same tests fail against the original backend. Full current GCP/shared race suites, vet, and independent review pass. Captured output and source attribution are in the updated PR body; native lifecycle proof remains attributed to its actual source. |
|
🦞👀 |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
1 participant
Summary
Move GCP's ordinary public-IP wait onto
shared.PollReadinessso interrupted reads retain the original caller cause and canonical context identity, and owned budget expiry retains deadline classification. The old helper dropped the custom cause when the SDK returnedctx.Err()and returned a plain timeout error without deadline identity.Preserve the two-minute IP-observation budget, five-second interval, nonempty-IP-only readiness predicate, immediate API-error handling, exact timeout message, and normal CLI exit code 1. Do not add trimming, running-state requirements, retries, or a provider-independent exit code 5. Completed ready responses and typed API errors retain precedence at coincident cancellation. Caller diagnostics retain the existing pre-cancel/wait cause text and typed exit code; interrupted reads now behave consistently with those paths.
The response predicate recognizes
*googleapi.Errorthrougherrors.As: the real REST Compute client returns completed HTTP errors through GAX wrappers that preserve that cause. Transport failures and a later SDK retry-context stop are not classified as completed responses merely because an earlier attempt received a response.No creation, rollback, claim, SSH, or heartbeat behavior is changed. Provider docs and the maintainer-added Unreleased entry explain the error/classification correction. Three other adapters still use the old helper; this PR does not redirect them wholesale.
Verification
Fresh native lifecycle proof
A newly built, credential-free binary at source
bfdf326e5c847a880f4a76e26b90a303da5b2e8ccompleted a real one-instance GCP lifecycle in the previously approved test project. It used ane2-micro, 30 GB boot disk, 30-minute TTL, a unique task tag, restricted SSH ingress, isolated user/SDK state, and no guest service account. Provider API calls and SSH were real.Acquire, readiness, remote command execution, explicit and omitted heartbeat policy, native TTL capping, and read-only status checks passed. Warmup took 189.394 seconds including creation and SSH/bootstrap; the two-minute IP-discovery budget is not a limit for that whole operation. Normal CLI stop succeeded. The task's managed firewall was then removed, and independent checks found zero instances, disks, firewalls, local claims, or private keys. Temporary credentials, Cloud SDK caches, runtime directories, and the owned credential window were removed.
Field-selected captured evidence, omitting project/account/resource identities and addresses:
{ "nativeSource": "bfdf326e5c847a880f4a76e26b90a303da5b2e8c", "binarySHA256": "23ff9bc8117f689817d2b735614b5053c47c0e28ced8fd27aeb08eb3028a7831", "machineType": "e2-micro", "rootGB": 30, "ttl": "30m", "acquireExit": 0, "readyExit": 0, "remoteCommandMarkerVerified": true, "ready": true, "heartbeatExplicitIdle": "1h30m0s", "heartbeatOmittedIdle": "1h30m0s", "nativeTTLClampVerified": true, "statusDidNotMutateNativeLabels": true, "normalStopSucceeded": true, "cleanup": { "instances": 0, "firewalls": 0, "disks": 0, "claims": 0, "privateKeys": 0 } }This healthy native run verifies integration; deterministic tests establish cancellation/deadline behavior that a successful lifecycle cannot prove.
Source attribution after integration
Current head
23414b9b850f4319d83db5d1c929df517d9c83b2integrates main574d3d2808a29f3d1281202283a8ca150cb0cf80. Relative to the native-tested source, the only changed files are the unrelated Scaleway local SDK-error implementation, its tests/docs, the changelog, and the additional GCP test file. GCP production code, core, shared polling, command/build inputs, manifests, and initialization code are unchanged. The complete current introduced diff is still only the four GCP code/test/docs/changelog files; integration tests and review pass.The rebuilt integrated binary may differ. Native execution remains attributed to the actual source above, not relabeled as a new-head run. No new account permissions, release actions, production deployment, or unrelated qualification-hold changes are included.
The latest commit adds only the real-HTTPS tests below. No production executable input changed after the integrated source review or the source-attributed native run.
Real HTTPS proof of the changed failure paths
The added
TestWaitForServerIPRealHTTPScalls the productioncore.NewGCPClient, its actual Google Compute REST SDK,GetServer, and the changedwaitForServerIPtogether. It uses synthetic ADC and a local TLS-verified HTTPS fixture; a concrete transport permits only the two expected Google destinations and dials only the fixture. The test process has no live credentials and runs under a loopback-only network sandbox. This is real HTTP/SDK proof, not a native Google outage or a new cloud lifecycle claim.Caller cancellation occurs only after the HTTPS server observes the request. The owned-budget case waits the real two minutes with an uncanceled three-minute parent. Both verify the server observes request cancellation, the diagnostic, exit code, cause identity, and final run classification. The same test against the original backend fails both cases. Full current GCP/shared race suites and vet pass; complete independent Codex review through P2 is clean.
Captured after-fix output, with only fixed synthetic names:
The baseline cancellation retained a URL-wrapped custom error but lost the canonical canceled identity and consistent public diagnostic. The baseline owned timeout preserved text/code but lost deadline identity. Neither baseline result is counted as passing proof. The actual two-minute case runs normally and skips only under Go's explicit short-test mode.