fix: fixed leases fail to replay after heartbeat - #1699
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 6:01 AM ET / 10:01 UTC. ClawSweeper reviewWhat this changesThe PR makes direct-provider heartbeats update lifecycle fields without rewriting provider metadata required to replay fixed leases. Regression provenancePossible regression — probable (reviewed change; failure trace). No predecessor PR is attributed. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 3 items remain Keep open. The base implementation truncates exact ownership metadata during heartbeat, while the introduced change preserves it and adds the relevant replay regression coverage; no introduced correctness defect was found. The supplied exact-head evidence is test-only, so real provider behavior proof is still required before merge. Priority: P2 Review scores
Verification
How this fits togetherCrabbox heartbeats refresh a remote lease and persist its metadata for later acquisition or cleanup. Provider adapters pass lease labels through the shared lifecycle updater, then write them to provider APIs or local lease claims. flowchart LR
A[Fixed lease] --> B[Heartbeat command]
B --> C[Shared lifecycle label update]
C --> D[Provider adapter]
D --> E[Provider or local lease claim]
E --> F[Later lease replay]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the shared exact-metadata design and merge only after a redacted real fixed-lease acquire → heartbeat → fresh-process replay proves the same resource is adopted successfully. Do we have a high-confidence way to reproduce the issue? Yes—source establishes a precise path: a 64-character fixed fingerprint is truncated by the base updater, then exact AWS replay validation rejects it. The added test encodes acquire → heartbeat → fresh-backend replay, though it was not executed during this read-only review. Is this the best way to solve the issue? Yes—the shared updater is the narrow ownership boundary, and preserving admitted metadata there removes duplicated per-provider restoration logic while lifecycle values remain updated. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 69ea6f017017. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
5db5365 to
ed17085
Compare
What Problem This Solves
A fixed lease could stop replaying after a successful heartbeat. The shared lease updater sanitized an already-admitted metadata map, truncating ownership fingerprints, replacing meaningful empty attestation values, and rewriting exact provider facts.
Why This Change Was Made
The updater now changes lifecycle fields while preserving admitted provider metadata. Redundant provider restoration loops are removed; local-container retains its existing creation-time and TTL representation. No stored format, fingerprint encoding, configuration or command contract changes.
Already-corrupted metadata still fails ownership validation. This change does not reconstruct or guess its original identity. #1700 uses this shared repair for Daytona prepared-worker replay.
Evidence
Current candidate:
ed170851c4a313464dfa9d932c5cd2b2ef609ead, rebased ontoad2e50382c17dd178fcc8a1ac599a4c891cf04df. Production and test patches are unchanged by the rebase; the only conflict was changelog context.The fixed-acquisition → heartbeat → replay regressions reproduced the failure before the repair. Focused race tests passed across ten CLI/provider packages, including AWS and Daytona ownership preservation and sibling lease implementations. The AWS fixture applies the dependency's tag-merge semantics so metadata corruption remains observable. Independent isolated P0 review of the resolved lower candidate found no findings; the upper stack also passed its separate review through P2.
Net change: production −53 lines, tests +52 lines. New exact-head CI is running. Historical provider observations are not relabeled as current-head live proof. GitHub's independent approval requirement remains unsatisfied; this PR has not merged.