fix: distinguish terminal fixed lease replays - #1925
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 12, 2026, 5:43 AM ET / 09:43 UTC (Revision 2). ClawSweeper reviewWhat this changesThe coordinator distinguishes matching requests for ended leases from conflicting create requests, with regression coverage and updated documentation. Merge readiness✅ Ready for maintainer review The correction remains necessary: current main and v0.57.0 still misclassify matching terminal replays. The new production-path transcript satisfies the previous proof request, and no blocking patch defects were found. Priority: P2 Review scores
Verification
How this fits togetherCrabbox’s coordinator receives fixed-ID lease requests from the CLI and compares them with stored lease identities before provisioning. Its replay response tells the CLI whether to reuse a live lease or stop with a definitive error. flowchart TD
A[CLI fixed-ID request] --> B[Coordinator replay check]
C[Stored lease and attempt] --> B
B --> D{Owner and intent match?}
D -->|No| E[Conflict response]
D -->|Yes| F{Lease live?}
F -->|Yes| G[Return existing lease]
F -->|No| H[Terminal response without provisioning]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep terminal replay diagnostics distinct while preserving HTTP 409, existing identity checks, and the prohibition on repeated provisioning. Do we have a high-confidence way to reproduce the issue? Yes, source inspection establishes that an identical request against a terminal stored lease reaches main’s misleading conflict response; this review did not execute a failing main run. Is this the best way to solve the issue? Yes, splitting the terminal response after existing identity checks is a narrow correction, and the unchanged CLI already handles the definitive HTTP status. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 751f6a420e98. 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)
|
Integrate current main while retaining the original terminal-replay diagnostic split, regression coverage, and documentation. Add the maintainer changelog entry with contributor credit. Verified the real CLI and Node coordinator over HTTP with PostgreSQL and the actual provider adapter against an owned local fixture: identical terminal replays remain distinct from intent drift and do not repeat provider creation. This is local integration proof, not hosted Hetzner proof. Co-authored-by: Andrew Melbourne <melbourneandrew@gmail.com>
clawsweeper
Bot
added
proof: sufficient
|
Merged as 6ab890a, preserving Andrew Melbourne's contribution and adding the maintainer changelog entry. The old replay check treated an already-ended lease as a different create intent. The fix separates those cases after the existing identity checks: matching terminal requests return Before merge, the repeated focused race tests, 3,148 passing Worker tests (15 existing skips), static/build/docs gates, managed P0-scoped Codex review, and all 12 CI jobs passed. The five Connector checks, Docs UI, and Release Check also passed. Fork workflow approval was handled by the maintainer; no test rerun was needed. CI: https://github.com/openclaw/crabbox/actions/runs/34686358617. After merge, a fresh clean checkout, offline dependency install, CLI build, and Node build passed. The actual-merge CLI then exercised the shipped coordinator, PostgreSQL storage, and real provider client over owned local HTTP sockets. The trace again showed initial 500, identical recovery 409 The initial database-startup timeout and incomplete diagnostic remain preserved. Post-merge CI passed all 12 jobs at https://github.com/openclaw/crabbox/actions/runs/34687818856; all five Connector and four CodeQL analysis jobs also passed. Pages and the automatic coordinator deployment workflow succeeded. Those are workflow observations, not a serving-version or hosted-cloud proof claim. |
2 participants
Summary
Distinguish a matching fixed-ID replay whose stored lease has already ended from an actual change to its create intent. Keep HTTP 409, the existing owner/organization/intent/attempt checks, and the rule that a terminal replay never re-enters provisioning. The CLI already propagates the definitive response; no CLI production compatibility path is added.
Integrated current main, retained Andrew Melbourne's original correction and regression coverage, clarified the command/coordinator/identifier documentation, and added the maintainer changelog entry thanking @Melbourneandrew.
Verification
go test -race ./internal/cli -run 'TestCoordinatorFixedCreateAmbiguousError(RepeatsPutAndDoesNotAdoptConflictingGet|ReportsSameIntentTerminalResult)$' -count=20: 40 focused test invocations passed.Actual coordinator / PostgreSQL / HTTP proof
The actual built CLI contacted the shipped Node coordinator through a socket recorder. The coordinator used its normal shared-token authentication, PostgreSQL runtime/storage, and real Hetzner provider/client. A task-process-only outbound routing shim sent the adapter's requests through native HTTP fetch to an owned loopback fixture. The fixture returned an ordinary provider HTTP 412; it did not replace the coordinator/provider classes, seed terminal state, or script coordinator responses.
Observed trace:
The first three PUT request bodies had identical hashes. Actual PostgreSQL readback showed the same failed lease with no resource ID and no provisioning uncertainty/retryability. The provider fixture's SSH key was removed. All owned processes and HTTP handlers finished; the task PostgreSQL container/volume and isolated HOME/keys/repository were removed, with zero cleanup errors. Source, index, runtime, and artifact bindings remained unchanged.
This is local integration proof over real sockets and PostgreSQL, not hosted Hetzner/cloud proof. Only synthetic fixture credentials were used. An initial database-readiness timeout and an incomplete startup diagnostic are retained as failed/incomplete evidence. The successful attempt explicitly waited for PostgreSQL's final TCP listener before the unchanged host SQL and replay checks; production behavior and replay assertions were not weakened.
Configuration / rollout
No secrets or configuration changes are required. Callers still receive HTTP 409 for a terminal replay, but its diagnostic code is now
fixed_lease_terminalinstead of the misleadinglease_id_conflict. Existing request-drift checks and provider-side-effect boundaries remain unchanged.Pre-merge CI passed all 12 jobs, plus the five Connector checks, Docs UI, and Release Check: https://github.com/openclaw/crabbox/actions/runs/34686358617. Merged as 6ab890a. Fresh actual-merge builds and the complete local PostgreSQL/HTTP proof passed; post-merge CI passed all 12 jobs at https://github.com/openclaw/crabbox/actions/runs/34687818856, with the five Connector and four CodeQL analysis jobs also passing.