feat(template): E2B build-context uploads, COPY/ADD steps, and alias rebuild (full PR — see stacked PRs) - #28
Conversation
Unmatched control-plane routes fell through to the proxy fallback and returned 404 with an empty body, so JSON clients (including the E2B SDK) surfaced a bare JSONDecodeError instead of "not found". Return the spec's error envelope from the fallback so unimplemented routes self-diagnose.
…rebuild
Close the two gaps that kept the stock E2B SDK template builder from
working end to end: Dockerfiles with COPY had no build-context upload
endpoint (the SDK died on a bare 404), and rebuilding an existing alias
was rejected with 400.
- Implement GET /templates/{templateID}/files/{hash} (201 + {present,
url}, matching the E2B spec) plus a streaming PUT endpoint the
returned URL points at. Archives are stored in the snapshot repository
(posix_fs and oss), so any node can issue the link, accept the upload,
and read it back at build time; multi-node deployments need no routing
changes. The SDK PUTs with no auth headers, so the URL carries a
short-lived bearer grant persisted in the shared repository, and spent
grants plus expired archives are pruned opportunistically.
- Execute COPY/ADD steps by rewriting the uploaded archive host-side to
final absolute guest paths per Docker COPY semantics (single file,
directory contents, globs, WORKDIR-relative dest, root:root
ownership, --chown/--chmod), then streaming it into the build sandbox
via envd and extracting with a single tar -xpf. The rewrite is a pure
function with unit tests; path escapes are rejected.
- Allow alias rebuilds: the alias keeps resolving to the previous
template while the new build runs and moves atomically when the build
commits (E2B semantics). Failed builds leave the old binding
untouched, and the previous template stays addressable by id. The
alias bind is now the final fallible operation of publish in both
backends.
- Tolerate envd's text/plain response to the files upload endpoint; the
generated client expects JSON and the upload has already completed
when the decode error surfaces.
Match the long-line style used everywhere else in the docs tree.
Review findings on the build-context work, all in the COPY path: - `--chown` was applied as `chown -R` over the destination after extraction, so it also rewrote every pre-existing file underneath it (`COPY --chown=u app/ /usr/local/` re-owned all of /usr/local, and a destination of / re-owned the rootfs). Ownership is now resolved to numeric ids inside the build sandbox — the way Docker resolves names, against the image's own /etc/passwd and /etc/group — and written into the rewritten tar headers, so a copy can only touch what it creates. An unknown user now fails the build instead of silently landing as root, and the uploader's account names are cleared from the headers so they cannot bind to an unrelated guest account at extraction time. - The archive rewrite read every entry fully into memory and kept them all live, so a compressed upload could expand without bound on the node running the build. The rewrite now runs in two passes — index paths, then stream each entry's bytes — and rejects archives past a decompressed budget (`template_build.files_max_context_mib`, 4 GiB by default) or an unreasonable entry count. - A relative `WORKDIR` was stored verbatim, which left it undefined for RUN steps and made any later relative COPY destination fail. It now resolves against the current working directory like Docker. - A GNU sparse entry is read back expanded, so the rewritten entry is now marked as a plain regular file rather than keeping a sparse type it no longer matches.
An upload grant was validated but never consumed, so an upload URL kept working for its whole TTL and could re-upload under the same hash; `import` overwrote unconditionally, which could also swap an archive out from under a build already reading it. Claiming a grant now consumes it — on POSIX the `remove_file` that consumes it is the atomic claim, so concurrent replays of one token cannot both win — and stored archives are immutable. Since a hash addresses its content, keeping the first archive costs nothing and means no build can observe its context change while it runs. Verifying that an upload matches its `filesHash` was considered instead and rejected: the SDK derives that hash from the COPY arguments plus per-file metadata and content in its own traversal order, not from the archive bytes, so recomputing it server-side would pin us to SDK internals.
|
Thanks for approving the earlier run! I gave the PR another read-through afterwards and pushed two follow-up commits — sorry, that means it needs approving once more on the new head.
|
|
🔍 OpenCodeReview found 33 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
WORKDIR now creates its directory in the build sandbox (mkdir -p, matching Docker), so a template built from a Dockerfile whose WORKDIR names a path absent from the base image no longer records a default cwd that does not exist in the guest. Review fixes, validated comment by comment against the code: - copy_plan: polynomial, segment-aware glob matching (was exponential and matched across '/'); the source archive is opened once and both passes read a capped reader, so GNU long-name/PAX records cannot allocate unboundedly; per-entry tar framing counts against the context budget; non-UTF-8 entry names are rejected. - COPY: extraction uses --no-overwrite-dir and no header is emitted for the destination root, so pre-existing directories keep their metadata while a missing destination is created with the requested --chown/--chmod; chown specs reject option-like parts. - Upload API: grants are verified up front but consumed only after the body is staged, so recoverable failures no longer burn the single-use URL; staging moves off the async worker; uploads time out via template_build.files_upload_timeout_secs; expiry math saturates. - Build-file stores: posixfs publishes via no-clobber hard_link, prunes grants by their own expiry, maps I/O errors instead of swallowing them, and refreshes archive mtimes on use; OSS materialize treats NotFound as a miss without a pre-check race. - Template API: filesHash on non-COPY/ADD steps, modes above 0o7777, and over-long COPY sources are rejected with 400s. - Catalog: alias rebinds take the previous owner's record lock (alias before record, documented); clearing a moved alias is guarded by the alias value on both backends; added a failure-path publish test. - Builder: a spec references at most 32 distinct archives, bounded in total by template_build.files_max_build_context_mib. - Config: public_base_url is validated at startup and documented as required behind TLS termination (upload URLs carry a bearer token); empty env values mean unset; new knobs are bounded at load time. Deferred (tracked in review replies): per-template archive namespacing and template-scoped authorization (blocked on the tenant model), conditional-write single-use grants on OSS, and alias reconciliation for duplicate listings.
|
All 48 review comments have been triaged — each one validated against the actual code — and every thread is now resolved. The fixes landed in b632852, together with one separately-found bug: Fixed (37 comments). Highlights by area:
Validation: Pushed back (8 comments, detailed replies in the threads). The common reasons: the described attack path is not reachable at HEAD (publish paths either mint fresh UUIDs or serialize through Deferred (3 threads, each with a note). Per-template archive namespacing and template-scoped authorization are blocked on the tenant model (credential validation is an acknowledged TODO in |
Upload lifecycle: the handler now runs verify -> stage -> import -> claim, so a failed store leaves the single-use URL retryable; the grant is consumed only after the archive is durably published (safe: the token binds one template_id/hash and import is first-write-wins). posixfs fsyncs the staged archive before the hard-link publish, caps opportunistic pruning at 256 entries per call, and catalog create is all-or-nothing under the alias lock. copy_plan: the capped-reader slack scales with the configured budget instead of a fixed ~195 MiB; a glob resolving to exactly one regular file gets single-file destination semantics with the base name taken from the resolved entry; an explicit "dest/" directory destination is prepared for single-file copies without inheriting the file's --chmod mode. Validation and errors: filesHash is shape-checked at the API edge with the store's own validator; files_max_upload_mib and files_max_context_mib reject zero at startup; envd upload errors keep their source chain; a failed COPY ownership lookup no longer asserts the user does not exist.
|
Round 2 (35 comments) triaged the same way as round 1: every comment validated against the code, using the diff against Fixed (13).
fmt / clippy ( Pushed back (10, replies in threads). Mostly round-1 re-raises without new arguments (Host-header URL, numeric Deferred (12, each answered in its thread). Three known boundaries, acknowledged in code/trait docs: content verification and tenant scoping of the build-file cache are blocked on the auth/tenant TODO (the hash is a caller-supplied cache key the server cannot recompute); strict single-use on OSS needs a conditional-write primitive |
|
Round 3 (33 comments) was triaged with the same per-comment validation as rounds 1–2, this time against a deliberately strict bar: fix only concretely reachable correctness defects introduced by this PR. Outcome: zero code changes — all 33 threads are answered and resolved individually. The round breaks down as: repeats of threads already answered twice (the Host-header URL, unverified-hash cache keys, OSS conditional writes, catalog rollback premise, 201-on-GET); nitpicks of the round-2 fixes whose failing scenarios are unreachable under shipped defaults (e.g. the 300s upload timeout sits well inside the 3600s grant TTL) or whose worst case is an already-documented, self-recovering condition; and one that asks to move the grant claim back before publication — the opposite of what round 2 requested and 6f4b6a5 implemented (r1: 3663195264 → r2: 3668146444 → this round: 3668561262). Since no commit results from this round, the review loop terminates here. The full triage record is in the threads; happy to address anything a human reviewer considers substantive. |
|
Thanks so much for the contribution! This PR is pretty large, so it'll take a while to review carefully, and we'll likely run into merge conflicts along the way. Would you mind starting with a smaller PR instead? |
JoyboyBrian
changed the title
|
Hi @yingdi-shan ! Agreed, I split this into a six-PR chain so each layer can be reviewed and merged independently while #28 stays open as the full reference PR:
The branches are chained in that order. Until preceding PRs merge, PR k's commits/files include layers 1..k; the review scope for each PR after the first is its last commit (and each body includes a direct fork compare link for that layer). Please merge strictly 1 → 6. After each merge I will rebase the remaining chain onto #28 will remain open as the full reference for the design discussion, completed review rounds, and two-node E2E evidence until the stack lands. |
2 participants
Summary
The stock E2B SDK's template builder could not build Dockerfiles containing
COPY, and could not rebuild an existing alias. This PR closes both gaps soAsyncTemplate.buildworks end to end against AgentENV:COPYthroughGET /templates/{templateID}/files/{hash}; AgentENV had no such route, so the SDK died on a 404 with an empty body. This PR implements the endpoint (201+{present, url}, matching the E2B spec) plus a streamingPUTendpoint the returned URL points at. Archives are stored in the snapshot repository (posix_fsandossbackends), so in multi-node deployments any node can issue the link, accept the upload, and read it back at build time — no gateway or routing changes needed. The SDKPUTs with a bare HTTP client (no auth headers), so the URL carries a bearer grant persisted in the shared repository — the same shape as an object-store presigned URL. Grants are single-use, and stored archives are immutable: a hash addresses its content, so keeping the first archive costs nothing and means no build can observe its context change while it runs. Spent grants and expired archives are pruned opportunistically. A new optional[template_build]config section (all defaults provided) bounds the upload size, the decompressed archive size, and the URL TTL, and can override the public base URL.COPYsemantics — single file, directory contents-into-dest, globs,WORKDIR-relative destinations (including a relativeWORKDIR, which now resolves against the current one), root:root ownership,--chown/--chmod— then streamed into the build sandbox via envd and extracted with a singletar -xpf.--chownis resolved to numeric ids inside the build sandbox, the way Docker resolves names against the image's own/etc/passwd, and written into the rewritten tar headers, so a copy can only ever change the files it creates. The rewrite runs in two passes (index paths, then stream each entry's bytes) so an archive is never held in memory, and rejects archives past the configured decompressed budget. Its mapping logic is a pure function with unit tests;../absolute entry escapes are rejected.400 … cannot rebind. The alias keeps resolving to the previous template while the new build runs, and moves atomically when the new build commits (E2B rebuild semantics). A failed rebuild leaves the old binding untouched; the previous template stays addressable by id. Implemented in both backends, with the alias bind reordered to be the final fallible operation of publish.{"code":404,"message":"…"}instead of an empty body, so future gaps surface as a readable 404 rather than a client-side JSON parse error (separate first commit).Behavior notes
COPY app.py /optwhere/optis an existing directory fails the build with a clear error; write directory destinations with a trailing slash (/opt/), the style E2B's own docs use. Documented indocs/src/integration/e2b.md.COPY --chownapplies only to the files the copy creates, and an unknown user fails the build rather than silently landing as root.COPYrequirestarin the base image (RUNalready requires/bin/bash); documented.filesHashwas considered and rejected: the SDK derives that hash from the COPY arguments plus per-file metadata and content in its own traversal order, not from the archive bytes, so recomputing it server-side would pin the server to SDK internals. Single-use grants and immutable archives close the same window without that coupling.text/plainbody while the generated client expects JSON; the upload path tolerates the decode error after a completed upload (commented inline — the root fix belongs in the envd http-client spec).Testing
make fmt,cargo clippy --workspace --all-targets --all-features -- -D warnings, and theagentenv/envd/linux-capunit suites. New unit tests cover the Docker COPY plan semantics (gzip, globs, ownership, path escapes, the decompressed budget), the build-file stores, single-use upload grants and their pruning, archive immutability, and alias rebind behavior in create/publish.from_image,from_dockerfilewithout COPY,from_dockerfilewith COPY, alias rebuild, cold sandboxes, and the full exec/files runtime suite all pass. Separately verified in a booted sandbox thatCOPY --chownleaves pre-existing files under the destination untouched, that a relativeWORKDIRresolves against the previous one, that an upload URL is rejected on replay, and that a stored archive keeps its original bytes across a repeat upload on both nodes.make agentenv-server.