REMOTE-2111 (1/3): Add checkpoint upload/commit pipeline mechanics - #14588
REMOTE-2111 (1/3): Add checkpoint upload/commit pipeline mechanics#14588joeywangzr wants to merge 4 commits into
Conversation
Adds the storage mechanics for one periodic-handoff checkpoint attempt (REMOTE-2111), as a self-contained module the (forthcoming, stacked) periodic checkpoint coordinator will drive: - harness_support.rs: SnapshotUploadMode (legacy|checkpoint) and generation on SnapshotUploadRequest; CheckpointGeneration (a validated, client-minted generation identifier); CommitSnapshotRequest/ Response; HarnessSupportClient::commit_snapshot. - snapshot.rs: CheckpointResult (Committed/Skipped/Failed outcome of one attempt); PipelineMode selects legacy vs. checkpoint-mode uploads; mint_generation (per-attempt, collision-free generation IDs); storage_name (generation-prefixed object naming); and the run_checkpoint_from_declarations_file / run_checkpoint_pipeline entry points: gather, upload every blob plus the manifest in checkpoint mode, then commit the exact object set that landed. Commit is withheld entirely (never partial) if the manifest or any required blob fails to upload, or if upload-target allocation fails. The whole pipeline has no production caller yet -- the coordinator that drives it periodically lands in a follow-up, stacked PR -- so the new items are marked #[allow(dead_code)] with a comment explaining why; each annotation is removable once that PR merges on top of this one. Co-Authored-By: Oz <oz-agent@warp.dev>
This stack of pull requests is managed by Graphite. Learn more about stacking. |
joeywangzr
changed the title
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds checkpoint-mode snapshot upload mechanics, generation-prefixed object naming, exact-set checkpoint commit plumbing, and tests around success and failure cases.
Concerns
- Checkpoint commits are withheld for hard upload failures, but not when a required blob is skipped because the server omitted an upload target; that still commits a partial checkpoint.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| if outcome | ||
| .entries | ||
| .iter() | ||
| .any(|e| e.status == EntryStatus::Failed) |
There was a problem hiding this comment.
Failed entries, so a required blob with no returned upload target is marked Skipped by upload_entry and the checkpoint still commits without it; distinguish cap-skipped entries from target-missing skips and withhold commit for the latter.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
Review follow-ups on the checkpoint upload/commit pipeline.
1. Withhold the commit when the server omits a blob's upload target.
`upload_gathered_snapshot` already anticipates a short `upload-snapshot`
response and only warns; the target-less blob then reached `upload_entry`
and was recorded as `EntryStatus::Skipped`. The checkpoint gate rejected
only `Failed`, so the attempt committed a silently smaller object set and
made it the selected checkpoint — discarding a previously complete one.
`Skipped` conflated two very different causes. Split out
`EntryStatus::NoTarget` for "server returned no presigned target" and
treat it as fatal for a checkpoint attempt, leaving `Skipped` to mean only
the deliberate `MAX_SNAPSHOT_FILES_PER_RUN` cap. Both still surface in the
manifest as `skipped` so rehydration consumers keep a stable status
vocabulary; the distinguishing detail stays in `error`. The legacy
end-of-run path is unchanged in behavior.
2. Sanitize agent-controlled filenames before they reach storage names.
`gather_repo` sanitized its filename component but `gather_file` used the
raw basename, which flows into `checkpoint_<generation>__<logical_name>`.
`__` is documented as the reserved separator and the charset as
`[A-Za-z0-9._-]`, but nothing enforced that for the filename half — and
basenames come from agent-created files. `a__b.txt`, or
`checkpoint_1700000000000-0__evil.txt`, produced an ambiguous storage
name that either fails the server's existence check at commit time
(losing the whole checkpoint) or lands under a different generation.
`sanitize_name_component` now collapses to the server charset and squashes
`_` runs so `__` can never appear; `gather_file` and
`sanitize_filename_component` both route through it, and `storage_name`
debug-asserts the invariant.
3. Smaller fixes.
- `CheckpointGeneration::from_validated` now actually validates (debug
assertion) instead of only claiming to in its name and docs.
- Move the "pending commit" log after the outcome check so it no longer
fires for attempts that failed to allocate targets.
- Correct the `CheckpointResult::Failed { generation: None }` doc: an
external timeout also reports `None` after a generation was minted, so
`None` must not be read as "nothing landed in storage".
Tests: commit is withheld when the server omits a blob target (chunked so
the truncation hits a blob rather than the always-last manifest);
sanitization never yields `__` or out-of-charset bytes; and an end-to-end
guard that a hostile basename cannot produce an ambiguous storage name.
Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
Overview
This PR adds the client-side mechanics for checkpoint-mode snapshot uploads, including generation IDs, checkpoint upload-target requests, exact-set commit requests, and focused tests for upload/commit success and failure cases.
Concerns
- The checkpoint commit guard can still commit a generation when the server returns too few upload targets and a required blob is never uploaded, because that path is represented as
EntryStatus::Skippedrather thanEntryStatus::Failed.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| if outcome | ||
| .entries | ||
| .iter() | ||
| .any(|e| e.status == EntryStatus::Failed) |
There was a problem hiding this comment.
EntryStatus::Failed, but upload_entry marks blobs with no returned upload target as EntryStatus::Skipped. A short get_snapshot_upload_targets response can therefore commit a checkpoint after a required blob was never uploaded; distinguish cap skips from missing-target skips and withhold the commit for the latter.
|
Pushed a follow-up commit to this branch from a review pass (a682589). Summary of what changed and why, so you can push back on any of it: Blocking — a short Split out Blocking — agent-controlled filenames flowed unsanitized into the generation-prefixed storage names.
Smaller: Three regression tests added. Verified locally: One thing I did not change, but think is worth a decision: the client reconstructs commit object names rather than using anything the server echoed ( |
Review follow-ups on the checkpoint upload/commit pipeline, plus a pass to cut the added comments down to what is not already obvious from the code. 1. Mint only filenames the server will accept. `sanitize_name_component` guaranteed the charset and the absence of the reserved `__` separator, but not the rest of the server's logical-name contract: at most 255 bytes, no leading `-`, not `.`/`..`, and -- on the legacy path -- nothing in the reserved `checkpoint_` namespace or the commit marker's own name. Those names come from agent-created workspace files, and the server rejects the *entire* upload-targets request when any one of them fails validation. So a single file called `-v.txt`, or one with a 300-character basename, permanently failed every subsequent checkpoint attempt (with a Sentry report each cycle), and a file called `checkpoint_notes.txt` took the end-of-run snapshot down with it. Sanitization now covers the whole contract, escaping reserved names rather than dropping the file. 2. Stop `unique_filename` from undoing that sanitization. Two files named `a_.txt` produced stem `a_` and therefore `a__2.txt`, reintroducing the separator and tripping `storage_name`'s debug assertion -- a panic in debug builds, from a function documented never to panic. The stem's trailing `_` is now trimmed, which makes the invariant hold at the single place logical names are minted; the assertion at the consumer is redundant and goes away. 3. Retry the commit call. Every blob and the manifest upload through `with_bounded_retry`, but the commit -- the one call where all the work is already done and paid for -- had none, so a single transient failure discarded a complete checkpoint and left its objects as debris. Committing the same generation twice is idempotent server-side, so the same bounded retry applies. Tests: the sanitizer and `unique_filename` are checked against a mirror of the server's validation, an end-to-end attempt with hostile basenames asserts every name we send and commit passes it, the commit-failure test now pins the retry count, and new cases pin the JSON for legacy uploads (unchanged wire format), checkpoint uploads, and commits, none of which the in-process test client exercises. Co-Authored-By: Oz <oz-agent@warp.dev>
Follow-up comment pass. `harness_support.rs` explained the same `allow(dead_code)` reason three times; that is now one note covering the file, matching what `snapshot.rs` already does. Dropped the doc comments that only restated `FALLBACK_SNAPSHOT_FILENAME` and `RESERVED_NAME_ESCAPE`, and shortened the rest where the code already carried the meaning. Kept the ones that encode contracts the code cannot show on its own: the server's filename rules, why the length cap is 240 rather than 255, why `String::truncate` cannot split a char, why `unique_filename` trims a trailing underscore, and why re-committing a generation is safe. Co-Authored-By: Oz <oz-agent@warp.dev>
1 participant
Part 1 of 3 in a stack splitting the periodic workspace-handoff checkpoint client work into reviewable chunks.
Stack
master)What this does
Adds the storage mechanics for one periodic-handoff checkpoint attempt, as a self-contained module the coordinator (#14573) will drive:
harness_support.rs:SnapshotUploadMode(legacy|checkpoint) andgenerationonSnapshotUploadRequest;CheckpointGeneration(a validated, client-minted generation identifier);CommitSnapshotRequest/Response;HarnessSupportClient::commit_snapshot.snapshot.rs:CheckpointResult(Committed/Skipped/Failed outcome of one attempt);PipelineModeselects legacy vs. checkpoint-mode uploads;mint_generation(per-attempt, collision-free generation IDs);storage_name(generation-prefixed object naming); and therun_checkpoint_from_declarations_file/run_checkpoint_pipelineentry points: gather, upload every blob plus the manifest in checkpoint mode, then commit the exact object set that landed. Commit is withheld entirely (never partial) if the manifest or any required blob fails to upload, or if upload-target allocation fails.Note on
#[allow(dead_code)]: the whole pipeline has no production caller yet — the coordinator that drives it periodically lands in #14573 — so the new items are marked with an explanatory comment. These become genuinely reachable once the stack is applied in full; removable but harmless to leave.Validation
cargo clippy --all-targets --tests -- -D warnings,cargo fmt --check, and the fulldriver::snapshottest suite (54 tests) pass in isolation on this branch. Checkpoint coverage includes manifest-upload failure, upload-target allocation failure, a server-omitted blob target, commit retry exhaustion, filename sanitization against a mirror of the server's validation, and the JSON wire shape of legacy uploads, checkpoint uploads, and commits.Co-Authored-By: Oz oz-agent@warp.dev