Implement containers snapshot API by gpanders · Pull Request #6376 · cloudflare/workerd

gpanders · 2026-03-20T17:34:29Z

Expose a new experimental snapshotDirectory API for containers and include an implementation using Docker volumes (used by Wrangler for local dev).

This is not yet implemented in production for Cloudflare Workers, but lays the groundwork for that feature.

This PR is (as of now) identical in content to #6337 to make reviewing easier for those who reviewed the last PR. I'll address any review feedback in separate commits to keep things separated.

Implementation notes

The local dev implementation of snapshots uses a combination of Docker volumes and the Docker archive API. We use Docker volumes to manage and store the snapshots on disk (this is mostly a UX convenience for the user, so that workerd created snapshots are visible in docker volume ls and can be easily removed with docker volume prune), but we do not bind-mount the volumes when restoring snapshots, since Docker volumes do not have the same properties as snapshots (namely, snapshots should be immutable).

Instead when creating or restoring a snapshot we create a temporary container (using the same image as the "main" container) with the volume attached and then copy between the containers using the archive API.

NOTE: This is known to perform pretty badly on macOS since each of the archive API calls sends the entire snapshot tar contents over the Docker UNIX socket, which traverses the VM<->host boundary on macOS, so has a lot of overhead. There may be some ways to improve this but for now the plan is to defer those performance improvements to a follow up PR so that we can at least land the functional implementation sooner.

ask-bonk

I'm Bonk, and I've done a quick review of your PR.

Adds an experimental snapshotDirectory API for containers backed by Docker volumes, including snapshot create, restore (with custom mount points), and stale-volume cleanup.

Findings (highest severity first):

[MEDIUM] Dangling kj::StringPtr in deferred deleteDockerVolume cleanup — container-client.c++:1421
[MEDIUM] latest TypeScript types expose experimental-only snapshot fields and types — types/generated-snapshot/latest/index.d.ts:3785
[LOW] Global mutable bool staleSnapshotVolumeCheckScheduled without thread-safety annotation — container-client.c++:48

These comments were generated by an AI code review assistant and may contain inaccuracies.

src/workerd/server/container-client.c++

types/generated-snapshot/latest/index.d.ts

src/workerd/server/container-client.c++

ask-bonk · 2026-03-20T17:40:25Z

Review posted successfully on PR #6376. Here's a summary of the findings:

3 issues found, ranked by severity:

[MEDIUM] Dangling kj::StringPtr in deferred deleteDockerVolume cleanup (container-client.c++:1420-1422): The KJ_DEFER in snapshotDirectory passes volumeName as a kj::StringPtr to deleteDockerVolume. When the deferred cleanup fires on scope exit, the deleteDockerVolume coroutine suspends, then snapshotDirectory's frame is destroyed — leaving the StringPtr dangling when the inner coroutine resumes. Suggested fix: pass an owned copy via kj::str(volumeName).
[MEDIUM] latest TypeScript types expose experimental-only fields (latest/index.d.ts:3785): Despite the JSG_STRUCT_TS_OVERRIDE_DYNAMIC correctly excluding snapshots in the non-experimental branch, the generated latest types include snapshots, hardTimeout, and all the new snapshot-related types (ContainerDirectorySnapshot, ContainerSnapshotRestoreParams, etc.). Users on latest compat date would see these in autocomplete but get runtime errors.
[LOW] Global mutable bool without synchronization (container-client.c++:48): staleSnapshotVolumeCheckScheduled is a non-atomic global bool. Safe in practice (single event loop), but fragile. Suggested using std::atomic<bool>.

github run

codecov-commenter · 2026-03-20T17:40:29Z

Codecov Report

❌ Patch coverage is 0% with 318 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.64%. Comparing base (d62d62a) to head (530ff41).

Files with missing lines	Patch %	Lines
src/workerd/server/container-client.c++	0.00%	265 Missing ⚠️
src/workerd/api/container.c++	0.00%	53 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6376      +/-   ##
==========================================
- Coverage   70.81%   70.64%   -0.18%     
==========================================
  Files         425      425              
  Lines      116296   116586     +290     
  Branches    18773    18829      +56     
==========================================
+ Hits        82357    82364       +7     
- Misses      22707    22997     +290     
+ Partials    11232    11225       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codspeed-hq · 2026-03-20T17:48:53Z

Merging this PR will not alter performance

✅ 70 untouched benchmarks
⏩ 129 skipped benchmarks¹

_{Comparing ganders/snapshot-2 (530ff41) with main (d62d62a)}

129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

danlapid · 2026-03-20T18:42:37Z

CC @anonrig for review

gpanders · 2026-03-20T18:42:52Z

/bonk review

ask-bonk

3 findings ranked by severity:

[MEDIUM] normalizePath performs O(n) allocations for n trailing slashes — container-client.c++:51-56
[LOW] Snapshot ID validation allows all-hyphens, which produces a Docker volume named workerd-snap---- — container-client.c++:1258-1263
[LOW] normalizePath ordering with validateAbsolutePath in snapshotDirectory — container-client.c++:1388-1389

Previous review findings from PR #6337 and the prior review on this PR have been addressed:

The dangling kj::StringPtr in KJ_DEFER (finding #1 from ask-bonk) is already fixed — line 1421 passes kj::str(volumeName) (owned copy).
The staleSnapshotVolumeCheckScheduled global is already std::atomic_bool (line 48).
The latest types issue is a pre-existing pipeline problem (author confirmed), not a regression.

Overall: the design is sound. The capnp schema additions, compat-flag gating, path validation, snapshot ID validation during restore, and test coverage (round-trip, named, multi-directory, custom mount point, root restore, cross-DO, non-existent directory) are thorough. The KJ_DEFER + volumeCommitted cleanup pattern correctly handles partial failures.

These comments were generated by an AI code review assistant and may contain inaccuracies.

src/workerd/server/container-client.c++

ask-bonk · 2026-03-20T18:46:37Z

Review posted on PR #6376. Here's a summary:

Review body (top-level comment, review ID 3983426108):

3 ranked findings with confirmation that prior review findings have been addressed

3 inline comments:

[MEDIUM] container-client.c++:53 — normalizePath allocates a new kj::String per trailing slash in its while loop. Suggested a single-pass approach that does at most one allocation.
[LOW] container-client.c++:1258-1263 — Snapshot ID validation allows degenerate IDs consisting entirely of hyphens. Suggested requiring at least one hex digit for defense-in-depth.
[LOW] container-client.c++:1388-1389 — Noted the validate-after-normalize ordering is correct and consistent between both the snapshot creation and restore paths. Flagged as informational for future maintenance.

Key verification results:

The dangling kj::StringPtr finding from the prior review is already fixed (line 1421 uses kj::str(volumeName))
The staleSnapshotVolumeCheckScheduled global is already std::atomic_bool
The latest types issue is a pre-existing pipeline concern, confirmed by the author
The capnp schema, compat-flag gating, path validation, and test coverage are all solid
No coroutine lifetime issues found — no jsg::Lock or V8 handles held across co_await

github run

github-actions · 2026-03-20T18:51:46Z

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

Expose a new experimental `snapshotDirectory` API for containers and include an implementation using Docker volumes (used by Wrangler for local dev). This is not yet implemented in production for Cloudflare Workers, but lays the groundwork for that feature.

Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>

gpanders · 2026-03-23T13:35:54Z

Rebased on main and resolved conflicts.

* Use .exchange() to update atomic boolean * Use consistent ordering for normalize -> validate code paths * Add tests for snapshotting a stopped container and restoring a container with a non-existent snapshot ID

src/workerd/api/container.c++

anonrig · 2026-03-23T14:36:22Z

/bigbonk roast this pr

Co-authored-by: anonrig <anonrig@users.noreply.github.com>

ask-bonk · 2026-03-23T14:38:42Z

Here's a summary of what was done:

Addressed anonrig's review feedback on src/workerd/api/container.c++:139:

Extracted the magic number (1ull << 53) - 1 into a named constexpr uint64_t MAX_SAFE_INTEGER at file scope (line 17)
Added a comment explaining the reasoning: snapshot sizes are uint64 in capnp but exposed as double in JS, so we cap at Number.MAX_SAFE_INTEGER to avoid precision loss
Updated both usage sites (line 95 in start() and line 143 in snapshotDirectory()) to reference the named constant
This follows existing precedent in the codebase (identity-transform-stream.c++:358 uses the same pattern)
Replied to the review comment on the PR

github run

When a volume is bind mounted Docker will create it if it doesn't exist. However we don't want that behavior for snapshots: a snapshot ID for a snapshot that doesn't exist should result in an error, not silently create a new volume. This bug also exposed a second bug which is that if the start() code path fails for any reason, the container remains in a zombie state at which point monitor() hangs forever. If any of the dependencies on the start() path fail then we destroy the container, and we also require the container to be in a started state when monitor() is called.

gpanders requested review from a team as code owners March 20, 2026 17:34

petebacondarwin approved these changes Mar 20, 2026

View reviewed changes

ask-bonk bot reviewed Mar 20, 2026

View reviewed changes

src/workerd/server/container-client.c++ Show resolved Hide resolved

types/generated-snapshot/latest/index.d.ts Show resolved Hide resolved

src/workerd/server/container-client.c++ Outdated Show resolved Hide resolved

danlapid requested a review from anonrig March 20, 2026 18:42

ask-bonk bot reviewed Mar 20, 2026

View reviewed changes

src/workerd/server/container-client.c++ Outdated Show resolved Hide resolved

ask-bonk bot reviewed Mar 20, 2026

View reviewed changes

src/workerd/server/container-client.c++ Show resolved Hide resolved

ask-bonk bot reviewed Mar 20, 2026

View reviewed changes

src/workerd/server/container-client.c++ Outdated Show resolved Hide resolved

gpanders force-pushed the ganders/snapshot-2 branch from 971c76e to 7c84356 Compare March 20, 2026 18:59

gpanders and others added 4 commits March 23, 2026 08:34

Use std::atomic_bool for shared variable

fdb340b

Fix dangling StringPtr in deleteDockerVolume

bd51fa1

Reduce allocations in normalizePath

6fad9a1

Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>

gpanders force-pushed the ganders/snapshot-2 branch from 7c84356 to 6fad9a1 Compare March 23, 2026 13:35

Address AI review feedback

4517d20

* Use .exchange() to update atomic boolean * Use consistent ordering for normalize -> validate code paths * Add tests for snapshotting a stopped container and restoring a container with a non-existent snapshot ID

anonrig approved these changes Mar 23, 2026

View reviewed changes

src/workerd/api/container.c++ Outdated Show resolved Hide resolved

Extract MAX_SAFE_INTEGER constant

e703cdc

Co-authored-by: anonrig <anonrig@users.noreply.github.com>

Move MAX_SAFE_INTEGER constant into jsg namespace

e91f795

gpanders force-pushed the ganders/snapshot-2 branch from 7ab62c9 to 4c2ce51 Compare March 23, 2026 17:23

Merge branch 'main' into ganders/snapshot-2

530ff41

gpanders merged commit 8948afd into main Mar 23, 2026
22 checks passed

gpanders deleted the ganders/snapshot-2 branch March 23, 2026 18:57

This was referenced Mar 24, 2026

Improve snapshot restore performance and semantics #6406

Merged

Add API for "full" container snapshots #6430

Merged

Conversation

gpanders commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ask-bonk bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ask-bonk bot commented Mar 20, 2026

Uh oh!

codecov-commenter commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codspeed-hq bot commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

danlapid commented Mar 20, 2026

Uh oh!

gpanders commented Mar 20, 2026

Uh oh!

ask-bonk bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ask-bonk bot commented Mar 20, 2026

Uh oh!

github-actions bot commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gpanders commented Mar 23, 2026

Uh oh!

Uh oh!

anonrig commented Mar 23, 2026

Uh oh!

ask-bonk bot commented Mar 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gpanders commented Mar 20, 2026 •

edited

Loading

codecov-commenter commented Mar 20, 2026 •

edited

Loading

codspeed-hq bot commented Mar 20, 2026 •

edited

Loading

github-actions bot commented Mar 20, 2026 •

edited

Loading