Skip to content

fix: fixed leases fail to replay after heartbeat - #1699

Open
steipete wants to merge 1 commit into
mainfrom
codex/heartbeat-metadata
Open

fix: fixed leases fail to replay after heartbeat#1699
steipete wants to merge 1 commit into
mainfrom
codex/heartbeat-metadata

Conversation

@steipete

@steipete steipete commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

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 onto ad2e50382c17dd178fcc8a1ac599a4c891cf04df. 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.

@clawsweeper

clawsweeper Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@steipete
steipete marked this pull request as ready for review August 31, 2026 08:38
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 31, 2026
@clawsweeper

clawsweeper Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 6:01 AM ET / 10:01 UTC.

ClawSweeper review

What this changes

The PR makes direct-provider heartbeats update lifecycle fields without rewriting provider metadata required to replay fixed leases.

Regression provenance

Possible 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
Reviewed head: ed170851c4a313464dfa9d932c5cd2b2ef609ead

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The implementation and focused regression coverage are solid, but the external-PR real-behavior proof gate is not yet met.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The changed production owner is the shared lease-label updater used by provider heartbeat paths. The supplied evidence describes focused fake-client race tests, including AWS acquire → heartbeat → replay, but provides no redacted after-fix trace from a real provider or transport on this head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The changed production owner is the shared lease-label updater used by provider heartbeat paths. The supplied evidence describes focused fake-client race tests, including AWS acquire → heartbeat → replay, but provides no redacted after-fix trace from a real provider or transport on this head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 5 items Current-main defect: The current main/base heartbeat helper sanitizes the entire copied label map; its sanitizer converts empty values to "unknown" and limits values to 63 characters, which changes admitted ownership metadata.
Replay requires exact fingerprint: AWS fixed-lease validation rejects a server when fixed_intent_sha256 differs from the durable fingerprint, making truncation on heartbeat a concrete replay failure.
Introduced repair and regression: The PR stops re-sanitizing existing labels, still sanitizes the newly written state, and extends the AWS acquire-to-heartbeat-to-fresh-backend replay test.
Findings None None.
Security None None.

How this fits together

Crabbox 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]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The changed production owner is the shared lease-label updater used by provider heartbeat paths. The supplied evidence describes focused fake-client race tests, including AWS acquire → heartbeat → replay, but provides no redacted after-fix trace from a real provider or transport on this head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - Without exact-head real-provider proof, the replay repair remains unverified against provider persistence and tag-update behavior outside the fake clients.
  • Complete next step (P2) - This active PR has no mechanical repair candidate; it needs contributor-supplied real behavior evidence before normal maintainer merge review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production −53 net, tests +52 net, docs +3 The patch removes provider-specific restoration paths while adding focused coverage for the shared behavior.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    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.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best 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.

Labels

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed production owner is the shared lease-label updater used by provider heartbeat paths. The supplied evidence describes focused fake-client race tests, including AWS acquire → heartbeat → replay, but provides no redacted after-fix trace from a real provider or transport on this head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove proof: sufficient: Current real behavior proof status is mock_only, not sufficient.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: 📣 needs proof.

Label justifications:

  • P2: The PR repairs fixed-lease replayability after a heartbeat, a bounded provider reliability defect.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed production owner is the shared lease-label updater used by provider heartbeat paths. The supplied evidence describes focused fake-client race tests, including AWS acquire → heartbeat → replay, but provides no redacted after-fix trace from a real provider or transport on this head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Current-main defect: The current main/base heartbeat helper sanitizes the entire copied label map; its sanitizer converts empty values to "unknown" and limits values to 63 characters, which changes admitted ownership metadata. (internal/cli/provider_labels.go:109, 69ea6f017017)
  • Replay requires exact fingerprint: AWS fixed-lease validation rejects a server when fixed_intent_sha256 differs from the durable fingerprint, making truncation on heartbeat a concrete replay failure. (internal/providers/aws/backend.go:369, 69ea6f017017)
  • Introduced repair and regression: The PR stops re-sanitizing existing labels, still sanitizes the newly written state, and extends the AWS acquire-to-heartbeat-to-fresh-backend replay test. (internal/providers/aws/backend_test.go:231, ed170851c4a3)
  • Provider-specific label boundary: The generic CLI touch path writes through provider clients, while GCP independently converts labels to its allowed representation before its API request; the shared helper can retain exact local metadata without bypassing that conversion. (internal/cli/gcp.go:449, ed170851c4a3)
  • Proof gap: The fully captured PR body reports focused race tests and explicitly does not present historical provider observations as current-head live proof; no redacted real-provider acquire→heartbeat→replay trace is supplied.

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Post redacted exact-head evidence from a real fixed lease showing acquisition, heartbeat, a fresh process, and replay of the same resource; redact endpoints, account IDs, and tokens.
  • After updating the PR body with that evidence, trigger a fresh review automatically or ask a maintainer to comment @clawsweeper re-review.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-08-31T08:42:59.844Z sha 5db5365 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-31T08:55:30.584Z sha 5db5365 :: needs maintainer review before merge. :: none
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Aug 31, 2026
@steipete
steipete force-pushed the codex/heartbeat-metadata branch from 5db5365 to ed17085 Compare September 1, 2026 09:55
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

1 participant