fix: preserve Cloudflare workspaces with shared archive sync - #1814
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs changes before merge. Reviewed September 4, 2026, 9:07 AM ET / 13:07 UTC. ClawSweeper reviewWhat this changesThe branch moves Cloudflare workspace synchronization onto the shared transactional archive flow, preserving existing workspaces when upload or extraction fails and enforcing full-snapshot limits before fresh allocation. Merge readiness⛔ Needs changes before merge - 2 items remain Keep this PR open: the transactional archive approach and real-container proof support the Cloudflare workspace fix, but the contributor-authored changelog entry conflicts with repository policy. Priority: P2 Review scores
Verification
How this fits togetherCloudflare is Crabbox’s remote-container provider: the CLI packages a local checkout, sends it to a Durable Object-backed container, then runs the requested command there. The changed sync path determines whether an existing remote workspace is preserved, merged, or atomically replaced before command execution. flowchart LR
A[Local checkout] --> B[Archive preparation]
B --> C[Cloudflare provider]
C --> D[Remote disk admission]
D --> E[Upload archive]
E --> F[Staged extraction]
F --> G[Workspace replacement or merge]
G --> H[Remote command execution]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Land the transactional Cloudflare sync flow after removing the contributor-authored Unreleased bullet; retain the shared archive lifecycle and the provider-specific disk and upload hooks. Do we have a high-confidence way to reproduce the issue? Yes, source reproducible: the parent implementation removed the live workspace before upload, and the added fixtures exercise rejected uploads and corrupt archives against that failure boundary. Is this the best way to solve the issue? Yes, apart from the changelog metadata: using the established shared archive owner removes the destructive ordering while leaving Cloudflare-specific transport and disk checks in the provider adapter. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 9f1b19a89ffe. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What 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
|
|
Maintainer disposition: the changelog entry is intentional. This is a maintainer/agent-authored fix, not a request for a contributor to edit release notes. This repository requires maintainer/agent Unreleased entries for user-visible fixes as they land, with full PR links. The entry records the preserved-workspace behavior and matches that policy. There are no accepted technical findings in the review; no source change or rerun is needed for this author-role classification. |
|
Merged as 5f8f118. The landed tree exactly matches the verified merge preview. Cloudflare now uses the shared prepared-archive and replacement owner while retaining its own HTTP upload and disk-admission behavior. A failed upload or extraction no longer deletes the previous checkout; fresh allocation is also preceded by sizing the complete snapshot rather than a potentially misleading dirty delta. Proof includes regression failures on the unchanged baseline; Cloudflare/shared race tests, core archive tests, vet/build/docs and Windows cross-compilation; and a 16-step local Wrangler/Worker/Durable Object plus Linux Go-runner test covering partial upload, corrupt archive, replacement, merge mode, reuse and exit 42. Both native containers and claims were confirmed absent afterward. This was local native-runtime proof with synthetic inputs, not a production Cloudflare deployment. All 24 non-skipped exact-head checks passed without reruns, and Cloudflare/shared races passed again on main. The maintainer changelog entry was retained after resolving the bot's author-role misclassification. The separate run-finalization and captured-claim lifecycle follow-up remains in progress; this archive change does not claim to fix those boundaries. |
Summary
The legacy Cloudflare Containers adapter deleted the live checkout before preparing and uploading its replacement. An upload refusal or invalid archive therefore erased an otherwise reusable workspace. It also checked dirty-delta limits before uploading a full checkout, and could leave partially uploaded archives behind.
Use the existing shared archive owner for preparation, upload, staged extraction, replacement, and exact temporary-path cleanup. Fresh sync prepares and checks its full snapshot before allocating a container. Cloudflare still owns disk admission, HTTP upload with exact Content-Length, and runner execution; no new shared abstraction or upload protocol is introduced. Deletion-disabled sync retains its merge behavior.
This is the
cloudflare/cfprovider, not Cloudflare Sandbox or Dynamic Workers. Full Run finalization, keep-on-failure during preparation, and claim-revision fencing are separate follow-up work; this change does not claim to fix them.Verification
Four unchanged-baseline regressions fail for the expected reasons: fresh/reused full-archive limits permit remote work, and rejected/corrupt uploads destroy existing files. Candidate tests execute real Bash/tar/filesystem operations through the transport fixture and verify preservation, replacement, merge semantics, Content-Length, and exact archive/staging cleanup.
Cloudflare/shared Go race tests, focused core archive races, vet, CLI build, docs build, and dead-code scan passed. The POSIX filesystem fixture now gates Windows/Bash availability after independent review caught that portability gap; Windows test compilation also passed. Managed Codex review is clean at the configured P0 threshold, and independent review found no production issue.
Live local Worker/container proof
The built CLI ran through repository-pinned Wrangler 4.127.0, the actual Worker/Durable Object code, and the checked-in Go runner in a real local Linux/amd64 Docker container (on an arm64 Mac). A loopback fault proxy truncated actual uploads to the runner; one returned a synthetic 500 after the partial write, another allowed extraction to discover the corrupt archive. This was real local container execution with synthetic fault injection, not a production Cloudflare deployment.
All 16 CLI steps completed with expected results. Both exact sandbox IDs reported
stopped, their local claims were absent, and no container remained for the unique proof image. Wrangler and the fault proxy stopped, all three loopback listeners were absent, and task-only runtime/config state was removed. The image cache was retained.CLI SHA256:
ac06ed6b9344403686613f810c84d0a11aff47a37b74b311109dd56f39fdf9f9.Runner image:
sha256:458f6dc81d22ee6c350dcb30fd305cde0e499b61bda3a30a07fbdaca89746001.Initial harness setup corrections concerned CLI config routing, proxy response encoding, required runner bindings, and discovering the installed Docker Buildx plugin inside an isolated configuration. None required a production code workaround, real credential, provider deployment, or changing the user's Docker configuration. Remote temporary cleanup remains best-effort and preserves the primary failure; this is not a durable cleanup journal.