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

This PR adds a containers snapshot API: snapshotDirectory() to create snapshots and snapshots option in start() to restore them, backed by Docker volumes and the Docker archive API for the local dev implementation.

I posted 3 inline review comments on PR #6337, ranked by severity:

1. [MEDIUM] Snapshot restore path nesting (container-client.c++:1054) — The archive read from /mnt in the temp container includes a mnt/ prefix in the tar, which when extracted to the parent directory could introduce an extra directory level. Asked for confirmation that the round-trip works correctly.

2. [LOW] Unbounded readAllBytes() (container-client.c++:577) — The binary response is fully buffered in memory before the 4 GiB size check runs.

3. [LOW] Unvalidated snapshot id during restore (container-client.c++:1039) — Deserialized snapshot IDs are used in Docker volume names without format validation.

The overall design is sound — the serialization, compat flag gating, capnp schema additions, and test coverage all look good. The DirectorySnapshot JSG binding pattern with JSG_SERIALIZABLE, visitForMemoryInfo, and prototype properties follows existing conventions correctly.

github run

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

Codecov Report

❌ Patch coverage is 0% with 310 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.55%. Comparing base (580f4b0) to head (705de4d).
⚠️ Report is 155 commits behind head on main.

@@            Coverage Diff             @@
##             main    #6337      +/-   ##
==========================================
- Coverage   70.71%   70.55%   -0.16%     
==========================================
  Files         420      420              
  Lines      112752   113229     +477     
  Branches    18483    18570      +87     
==========================================
+ Hits        79729    79894     +165     
- Misses      22021    22297     +276     
- Partials    11002    11038      +36     

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

/bonk review

Review posted on PR #6337. Here's a summary of what was posted:

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

• 4 ranked findings with overall assessment that the design is sound

3 inline comments:

1. [MEDIUM] container-client.c++:265 — dockerApiBinaryRequest has maxResponseSize = kj::maxValue default, making it easy to read unbounded Docker responses. Suggested changing the default to MAX_JSON_RESPONSE_SIZE (16 MiB) since callers needing the 1 GiB tar limit already pass it explicitly.

2. [MEDIUM] container-client.c++:277 — cleanupSnapshotVolumes constructs the JSON filter by string interpolation without escaping. Safe in practice due to URI-encoding of containerName, but noted as a defensive coding concern.

3. [LOW] container.c++:44 — Deserialized size not validated against the max limit or double precision boundary. Minor since legitimate values are bounded.

1 reply to kentonv's comment:

• Confirmed gpanders's reply is correct: JSG_SERIALIZABLE requires JSG_RESOURCE_TYPE (uses jsgSuper typedef), cannot be used with JSG_STRUCT. Cited the JSG README, ser.h, and the 17 existing examples in the codebase.

github run

/bonk Review this PR again

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

Review body (top-level comment):

• 2 ranked findings with confirmation that the snapshot create/restore round-trip logic is correct and previous findings have been addressed

1 inline comment:

1. [MEDIUM] types/generated-snapshot/latest/index.d.ts:3780 — The latest/ generated TypeScript types expose snapshots?: DirectorySnapshot[] on ContainerStartupOptions, but the non-experimental TS override explicitly excludes it. Since latest uses compat date 2999-12-31 (not experimental), the non-experimental branch should apply. hardTimeout has the same pre-existing issue. Users with latest types will see snapshots in autocomplete but get a runtime error if they use it without the experimental flag.

Key verification results:

• The snapshot create/restore round-trip is correct for paths like /app/data, /data, and /a/b/c/d
• The KJ_DEFER cleanup pattern with volumeCommitted correctly handles partial failures
• No use-after-move or lifetime issues in the coroutine-based Docker API helpers
• Previous review findings (default maxResponseSize, JSON string interpolation) have been addressed
• Snapshot ID validation and validateSnapshotDir path traversal rejection are solid

github run

[LOW] The experimental guard for snapshots in start() (lines 61-63 of container.c++) was flagged as redundant by gabivlj — gpanders acknowledged but hasn't removed it yet. However, the guard provides defense-in-depth since users can manually construct DirectorySnapshot objects (it's a JSG_STRUCT) without calling the experimental-gated snapshotDirectory().

Incorrect, @gabivlj's comment was about the snapshotDirectory implementation, not start(). It is not redundant in start() which is why I left it. I removed it from snapshotDirectory() as Gabi recommended.

1. [MEDIUM] types/generated-snapshot/latest/index.d.ts:3780 — The latest/ generated TypeScript types expose snapshots?: DirectorySnapshot[] on ContainerStartupOptions, but the non-experimental TS override explicitly excludes it. Since latest uses compat date 2999-12-31 (not experimental), the non-experimental branch should apply. hardTimeout has the same pre-existing issue. Users with latest types will see snapshots in autocomplete but get a runtime error if they use it without the experimental flag.

This is not correct, the type generator is launched with the experimental flag:

This is a pre-existing issue with the type generation pipeline, not a regression in this PR. hardTimeout has the same behavior for the same reason. If the type generation should distinguish latest (all compat flags, no experimental) from experimental (all compat flags + experimental), that would be a change to the build-types.ts pipeline, not to the container API code, which should happen in a separate PR.

After talking with containers team internally (@IRCody, @spahl) we also need the ability to restore a directory snapshot to an arbitrary mount point (the implementation as-is always mount a snapshot to the same directory that was snapshotted).

So the StartParams will actually change to accepting an array of {mountPoint?: string; snapshot: ContainerDirectorySnapshot} objects, rather than an array of just ContainerDirectorySnapshots.

@gpanders while confusing based on approvals, this PR wasn't properly reviewed.
As per comments:
Kenton only approved the API
Pete only approved the types.
Nobody actually reviewed the implementation.
Let's please revert and make sure the implementation itself gets approvals as well.