Repository navigation
fix: establish direct AWS checkpoint captures before non-waiting success - #2834
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbdeb881a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err == nil && req.Capture == nil && !result.CaptureEstablished && !nativeImageAvailable(result.Image.State) { | ||
| err = Exit(5, "capture of image %s is uncertain in state %s; inspect the retained checkpoint before continuing source writes", result.Image.ID, blank(result.Image.State, "unknown")) |
There was a problem hiding this comment.
active snapshots
For every ordinary direct Daytona checkpoint, the provider waits for completion but returns the successful snapshot with State: "active" and leaves CaptureEstablished false (internal/providers/daytona/checkpoint.go:180-192). Because nativeImageAvailable does not recognize active, this new gate converts the successful operation into exit 5 after the source has already been snapshotted and restarted, so users never receive a successful checkpoint record. Have the Daytona adapter set CaptureEstablished after its provider-specific completion check rather than rejecting its success in core.
AGENTS.md reference: AGENTS.md:L24-L28
Useful? React with 👍 / 👎.
steipete
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b77c143265
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| return err | ||
| } | ||
| record.CaptureEstablished = image.captureEstablished |
There was a problem hiding this comment.
For coordinator-managed checkpoints, this assignment persists the attestation only in the initial local record. A subsequent checkpoint inspect or checkpoint list reconstructs the record through checkpointRecordFromCoordinator, which cannot recreate CaptureEstablished, and readCheckpointRecord preserves only Capture; the refresh therefore omits the previously returned true value and overwrites the cache with false. Preserve an existing attestation when the coordinator identity matches, or include it in the coordinator checkpoint contract.
Useful? React with 👍 / 👎.
The first cloud session for a repository waited in `saving` until the provider finished uploading the warm image: about 200 s or more on AWS, measured live. EBS captures are point-in-time when they start, and enrollment only needs the capture point fixed before node identity is written, not the image upload to finish. With Crabbox 0.78.0 or newer, a first AWS capture (no existing image or previous generation, not a reserve or teardown) now runs `checkpoint create --wait=false` and accepts the result only when Crabbox reports `captureEstablished: true`. It publishes the exact checkpoint as pending, ends `saving`, skips the post-capture SSH refresh because the source never stopped, and enrolls immediately. Later sessions fork the image once allocation-time verification sees it available and start cold until then; refresh captures, reserves, teardown captures and other backends keep the blocking path. A missing or false `captureEstablished` is treated as an uncertain capture, never as success. The capture parser now maps real native states instead of always reporting `available`. Crabbox version policy now has one owner: `isCrabboxVersionAtLeast` in `crabbox-managed-binary.ts`, used by the managed-binary probe and the capture gate (`CRABBOX_NONBLOCKING_CAPTURE_VERSION = "0.78.0"`). Behavior is unchanged until a Crabbox release containing openclaw/crabbox#2834 (`captureEstablished`) is installed. CI note: the only reds were typecheck and SDK-boundary jobs failing on `src/config/sessions/transcript-payload.ts` and `src/gateway/operator-approval-store.kernel.worker.ts` from main's #171521, fixed on main by 920ab44; this PR does not touch those files. Refs: #171729, openclaw/crabbox#2834
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52900ae4fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result.CaptureEstablished = true | ||
| if err := req.Persist(result); err != nil { |
There was a problem hiding this comment.
When the process is interrupted after this req.Persist(result) returns but before CreateNativeCheckpoint returns—for example, during temporary snapshot cleanup—the durable Incus record loses the established-capture evidence: checkpointStore.WriteNativeProgress copies only the image and metadata and never copies result.CaptureEstablished. Since direct records are not reconstructed from a coordinator, subsequent inspection continues to omit the attestation even though the provider had already established and persisted an available image; preserve a true attestation monotonically in progress writes.
AGENTS.md reference: AGENTS.md:L24-L28
Useful? React with 👍 / 👎.
…law#171729) The first cloud session for a repository waited in `saving` until the provider finished uploading the warm image: about 200 s or more on AWS, measured live. EBS captures are point-in-time when they start, and enrollment only needs the capture point fixed before node identity is written, not the image upload to finish. With Crabbox 0.78.0 or newer, a first AWS capture (no existing image or previous generation, not a reserve or teardown) now runs `checkpoint create --wait=false` and accepts the result only when Crabbox reports `captureEstablished: true`. It publishes the exact checkpoint as pending, ends `saving`, skips the post-capture SSH refresh because the source never stopped, and enrolls immediately. Later sessions fork the image once allocation-time verification sees it available and start cold until then; refresh captures, reserves, teardown captures and other backends keep the blocking path. A missing or false `captureEstablished` is treated as an uncertain capture, never as success. The capture parser now maps real native states instead of always reporting `available`. Crabbox version policy now has one owner: `isCrabboxVersionAtLeast` in `crabbox-managed-binary.ts`, used by the managed-binary probe and the capture gate (`CRABBOX_NONBLOCKING_CAPTURE_VERSION = "0.78.0"`). Behavior is unchanged until a Crabbox release containing openclaw/crabbox#2834 (`captureEstablished`) is installed. CI note: the only reds were typecheck and SDK-boundary jobs failing on `src/config/sessions/transcript-payload.ts` and `src/gateway/operator-approval-store.kernel.worker.ts` from main's openclaw#171521, fixed on main by e2e8fda; this PR does not touch those files. Refs: openclaw#171729, openclaw/crabbox#2834
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e22b2a8320
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // AWS create responses have already passed the coordinator's exact backing | ||
| // snapshot ownership observation. Upload may still be pending. | ||
| return image.Provider == "aws" && image.State == "pending" && (len(image.SnapshotIDs) > 0 || len(image.Snapshots) > 0) |
There was a problem hiding this comment.
When a coordinator-managed AWS source has multiple EBS volumes and the first post-CreateImage read exposes only part of the block-device mapping, this condition emits captureEstablished: true as soon as any snapshot ID is present. The coordinator currently checks only that the list is nonempty (worker/src/fleet.ts:32078-32094), whereas the new direct barrier explicitly recognizes that DescribeImages can return partial mappings (internal/cli/aws_checkpoint_capture.go:126-129). Orchestration can therefore resume writes before the remaining volume snapshots are established. Have the AWS provider/coordinator compare every source device and return an explicit attestation instead of inferring provider-specific completeness in core.
AGENTS.md reference: AGENTS.md:L24-L28
Useful? React with 👍 / 👎.
#172846) #171729 made the first AWS warm-image capture non-blocking once the Crabbox CLI reported at least 0.78.0, and required `captureEstablished: true` in the `--wait=false` result. Crabbox 0.78.0 then shipped without that field (it lands with openclaw/crabbox#2834, merged after the release), so on main every first AWS capture with Crabbox 0.78.0 rejected a successful pending checkpoint as invalid. The capture became uncertain and blocked enrollment until manual recovery. No OpenClaw release contains #171729. The fast path now requires Crabbox 0.79.0, the first release that will carry #2834. A non-waiting result without the attestation is no longer rejected: OpenClaw polls the exact checkpoint's availability about every 5 s within the existing native wait budget, honoring cancellation and live-authority checks, then publishes it available and enrolls exactly as the blocking path does. So a Crabbox build that cannot attest the capture point degrades to the old wait instead of stranding the session. Docs now say Crabbox 0.79.0. CI note: the only red was check-docs formatting in ui/src/components/wizard-step-controls.ts and ui/src/pages/chat/components/chat-picker-overlay.test.ts, inherited from main; this change does not touch them. Refs: #172846, #171729, openclaw/crabbox#2834
openclaw#172846) openclaw#171729 made the first AWS warm-image capture non-blocking once the Crabbox CLI reported at least 0.78.0, and required `captureEstablished: true` in the `--wait=false` result. Crabbox 0.78.0 then shipped without that field (it lands with openclaw/crabbox#2834, merged after the release), so on main every first AWS capture with Crabbox 0.78.0 rejected a successful pending checkpoint as invalid. The capture became uncertain and blocked enrollment until manual recovery. No OpenClaw release contains openclaw#171729. The fast path now requires Crabbox 0.79.0, the first release that will carry openclaw#2834. A non-waiting result without the attestation is no longer rejected: OpenClaw polls the exact checkpoint's availability about every 5 s within the existing native wait budget, honoring cancellation and live-authority checks, then publishes it available and enrolls exactly as the blocking path does. So a Crabbox build that cannot attest the capture point degrades to the old wait instead of stranding the session. Docs now say Crabbox 0.79.0. CI note: the only red was check-docs formatting in ui/src/components/wizard-step-controls.ts and ui/src/pages/chat/components/chat-picker-overlay.test.ts, inherited from main; this change does not touch them. Refs: openclaw#172846, openclaw#171729, openclaw/crabbox#2834
1 participant
What Problem This Solves
Fixes: direct AWS
checkpoint create --wait=falsecould let orchestration resume source writes before the image's backing snapshots had started.User Impact
Direct AWS non-waiting creation now establishes the capture before returning, while allowing the source to resume writing during the snapshot upload. The optional top-level JSON field
"captureEstablished": trueattests that every included volume's capture is fixed; it does not imply fork availability, source health, or application consistency.Other providers retain their previous create success and wait semantics. Their adapters attest capture at their existing completion boundary, or omit the field when their existing contract has not established it. Recipes and failures omit the assertion.
capture.phaseremains the authority for source retirement.Native availability polling drops from 15 seconds to 3 seconds. Direct AWS public-IP observation starts at 250 ms and doubles to a 5-second cap. Pending coordinator capture recovery remains at 15 seconds.
Why This Change Was Made
CreateImageacceptance alone did not establish the backing-snapshot capture point. Waiting for full image availability avoided uncertainty but held the source through a multi-minute upload. The direct path persists the accepted AMI before observing it, then polls its exact backing snapshots about once per second. Every source EBS device must appear in the AMI mapping, and each snapshot must match the source volume and account and bependingorcompleted. Eventual visibility and incomplete mappings are retried within--wait-timeout; errors, cancellation, and timeout report capture as uncertain and retain the exact accepted identity. The barrier never resubmitsCreateImage.Core forwards the provider's capture attestation without rejecting normal provider states or adding another wait. Daytona attests after its existing active-snapshot barrier; Machine0 attests its completed snapshot even while the image is a draft. Hetzner pending success remains successful and omits the assertion. Completed Docker, Incus, Cloudflare, Azure, and Parallels captures use their existing completion boundaries. Established brokered AWS captures retain their existing success path.
No configuration or credential changes. Direct AWS observation uses existing EC2 describe permissions; denied reads fail as uncertain. Source boot work and recursive chown are unchanged. Command and provider-authoring docs describe the contract, and one
CHANGELOG.mdline is added underUnreleased.Evidence
All Go proof uses Go 1.26.5 and CI's
GOFLAGS='-mod=readonly -trimpath'. Only failing packages and the earlier focused regression groups were rerun locally.go test -race -count=1 -timeout=10m ./internal/providers/daytonago test -count=1 -timeout=20m -covermode=atomic -coverprofile=<local artifact> ./internal/providers/daytonago test -count=1 ./internal/providers/daytonago test -timeout=20m ./internal/providers/daytona-count=1 -timeout=10m -parallel=2)go test -race -timeout=10m ./internal/cli -run '^TestCheckpointCaptureBuiltBinaryContract$' -parallel=2 -count=1)-count=1 -timeout=10m)go vet ./...gofmt -lon changed Go files /git diff --checkThe new regression cases verify that adapter-defined success states remain successful, explicit attestations reach persisted JSON, unattested success omits the field, and non-waiting coordinator pending success introduces no observation request. Earlier fake-EC2 and CLI fixtures continue to check exact snapshot ownership and volume identity, partial mappings, terminal snapshot errors, cancellation, deadlines, accepted-image retention, and direct/brokered JSON output. Virtual-time tests verify the cadence changes. No live cloud latency improvement is claimed.
CI on the initial head
dbdeb881a015aa870d07eab46ae3584877bffc15exposed one PR-caused failure:TestDaytonaCheckpointCLILeavesStoppedSourceStoppedrejected Daytona's normalactivestate through a new shared state gate. It appeared in Go core, Go modules, Go non-CLI coverage, Native Windows contracts, and Connector Lifecycles. Go test, Go coverage, Windows Pond Mesh Cancellation, and Go propagated those producer failures. Each failed job log was fetched once. The same test passed on freshorigin/main(ca9f95ca883fd4278ec2560fbcac7d75b7a59fca, 65.67 s); no inherited failure was found. The final candidate removes the shared gate and implicit coordinator wait, and carries provider-owned attestations instead. Native Windows execution is left to the new head's CI; the portable failing package was verified locally with its CI flags.