[4/6] feat(api): E2B build-context upload endpoints and [template_build] config - #72
[4/6] feat(api): E2B build-context upload endpoints and [template_build] config#72JoyboyBrian wants to merge 4 commits into
Conversation
|
🔍 OpenCodeReview found 17 issue(s) in this PR.
[bug · high] 📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if self | ||
| .client | ||
| .exists(&key) | ||
| .await | ||
| .map_err(|error| RepositoryError::backend("check build archive", error))? | ||
| { | ||
| return Ok(()); | ||
| } | ||
| self.client | ||
| .put_file(&key, staged) | ||
| .await | ||
| .map_err(|error| RepositoryError::backend("upload build archive", error)) |
There was a problem hiding this comment.
[bug · high]
This check-then-write does not provide the TemplateBuildFileStore::import first-write-wins guarantee. Two imports can both observe a missing key and then issue unconditional put_file operations, so the later completion replaces the first archive; a build can consequently materialize different bytes for the same hash. This is also reachable through concurrent replay because OSS grant claiming is non-atomic. Publish through a backend primitive that rejects overwrite, coordinate creation through an atomic shared lock/metadata store, or verify the content key so all racing writers are guaranteed to upload identical bytes.
| self.client | ||
| .put_bytes(&key, grant) | ||
| .await | ||
| .map_err(|error| RepositoryError::backend("write upload grant", error))?; | ||
| Ok(token) |
There was a problem hiding this comment.
[performance · medium]
Expired or abandoned grants are never removed by this implementation; only successfully claimed grants are deleted. Unless every deployment independently installs the referenced bucket lifecycle rule, repeated upload-link requests leave durable JSON objects indefinitely. Add backend cleanup/lifecycle provisioning (or make the required policy an explicitly validated deployment prerequisite) so expiry bounds retained grants as well as authorization.
| let Some(grant) = self.read_grant(&key).await? else { | ||
| return Ok(false); | ||
| }; | ||
| if !grant.authorizes(template_id, hash, expires_unix, now_unix) { | ||
| return Ok(false); | ||
| } | ||
| // Consume the grant so the upload URL cannot be replayed. S3-compatible | ||
| // stores offer no conditional delete, so simultaneous replays of one | ||
| // token can both observe the grant; archives are immutable, which is | ||
| // what keeps that from mattering. | ||
| self.client | ||
| .delete(&key) | ||
| .await | ||
| .map_err(|error| RepositoryError::backend("consume upload grant", error))?; | ||
| Ok(true) |
There was a problem hiding this comment.
[security · high]
The read-authorize-delete sequence does not actually consume the bearer grant once: concurrent requests can both read it, and object-store DELETE is normally idempotent, so both deletes can succeed and both calls return true. The stated safety argument does not hold because import above is also an exists-then-unconditional-write race, allowing replayed uploads to replace one another. Use an atomic claim primitive in a shared coordination store (or otherwise ensure only one request can transition the grant to claimed) before reporting success.
| if existing != record.id && self.snapshot_exists(&existing).await? { | ||
| return Err(RepositoryError::AliasConflict { | ||
| alias: alias.to_string(), | ||
| existing, | ||
| new_id: record.id.clone(), | ||
| }); | ||
| bind_on_create = false; | ||
| } |
There was a problem hiding this comment.
[bug · medium]
When this branch is taken, the new record is still written with the same alias even though the binding remains on existing. If the rebuild fails, mark_build_error preserves that alias, and list() exposes records directly, so the old template and failed rebuild can advertise the same name indefinitely. Store an unbound/pending alias separately, or make listing derive the visible name from the alias catalog and expose the requested rebuild name through distinct build metadata.
| let record = self | ||
| .write_committed_record( | ||
| metadata.id.clone(), | ||
| metadata.alias.clone(), | ||
| metadata.resources, | ||
| committed, | ||
| metadata.source.clone(), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
[bug · medium]
This durable write occurs before the alias move and has no cancellation/crash recovery. Cancellation, process termination, or an unrecoverable failure before bind_alias permanently leaves a committed record claiming an alias that still resolves to the previous snapshot. list() filters raw records, so clients can observe duplicate/misleading template names. Add reconciliation that derives alias ownership from alias bindings, or use a staged record state that listings do not expose until the alias move completes.
| if current.as_ref() != Some(id) { | ||
| return; | ||
| } | ||
|
|
||
| let key = match validated_alias_key(alias) { |
There was a problem hiding this comment.
[bug · high]
This check and the subsequent unconditional restore are a lost-update race. After this read observes id, another publisher can bind the alias successfully, and this rollback then overwrites or deletes that newer binding. Because OSS offers no CAS here, rollback must use the same external per-alias serialization as binding; otherwise it is safer not to mutate the alias than to clobber a successful concurrent publish.
| if let Some(mut previous) = self.read_record(id).await? { | ||
| let claims_moved_alias = previous | ||
| .alias | ||
| .as_ref() | ||
| .is_some_and(|alias| alias.as_ref() == moved_alias); |
There was a problem hiding this comment.
[bug · medium]
This is an unguarded read-modify-write of the entire record. A concurrent build-state update or deletion between read_record and write_record can be overwritten (and a deletion can even be resurrected). The cleanup also fails best-effort, leaving duplicate alias claims in listings after publish returns success. Serialize updates per snapshot/alias or introduce versioned conditional record updates; if consistent listing metadata is part of successful publication, propagate cleanup failure instead of warning only.
| if scanned >= MAX_PRUNE_SCAN { | ||
| break; | ||
| } | ||
| scanned += 1; |
There was a problem hiding this comment.
[performance · medium]
This always scans only the first 256 matching entries returned by read_dir. Directory iteration is commonly stable, so a prefix of fresh entries can permanently hide expired entries later in the directory; successive calls do not necessarily drain the backlog and disk usage can grow without bound. Persist/rotate a scan cursor, shard the directory and rotate shards, or otherwise ensure every entry is eventually visited.
| if final_path.exists() { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
[security · high]
The caller-supplied hash is trusted without validating the staged bytes, and this global first-write-wins cache is shared across templates. An authenticated caller can request a known hash for its own template, upload arbitrary content first, and make other builds consume that content because subsequent imports return here. Verify the archive against the expected content identifier before publication, or namespace cache entries by the authorization/tenant boundary if this SDK hash is not a digest of the uploaded archive.
| // Copy into the store filesystem first (the staged file usually | ||
| // lives on node-local tmp), then link it into place within the | ||
| // store directory so readers only ever observe complete archives. | ||
| let store_staged = root.join(format!(".import-{}.tmp", uuid::Uuid::new_v4())); |
There was a problem hiding this comment.
[bug · medium]
Interrupted imports can leave full-size .import-*.tmp files permanently. Normal error paths remove them, but process termination after copy/sync bypasses cleanup, and prune_expired only scans .tar files. Include stale import temporaries in opportunistic/startup pruning (using a sufficiently conservative age) so repeated crashes cannot exhaust repository storage.
| Ok(_) => { | ||
| Self::touch(&path); | ||
| Ok(Some(path)) | ||
| } |
There was a problem hiding this comment.
[bug · medium]
Returning a pathname after metadata/touch does not protect the archive from concurrent pruning. A pruner can observe the old mtime and unlink it after this check (or unlink between the check and touch), leaving the caller with a path that no longer exists. Materialize a hard link/copy into scratch_dir before returning, or introduce a lease/reference mechanism that pruning honors.
| // committed record whose `alias` field names an alias that still | ||
| // resolves to the previous snapshot, so readers of `record.alias` | ||
| // (listings) may observe the stale claim until the next rebind. | ||
| store.write_json(&alias_path, &snapshot_id)?; |
There was a problem hiding this comment.
[other · high]
This unconditional rebind allows multiple rebuilds for the same alias to race. create leaves the alias on the current snapshot and does not reserve it for a particular pending build, so every pending build can reach this point; whichever build commits last wins, even if it was an older/stale rebuild that started before a newer successful one. Add an alias generation/reservation token (or otherwise serialize builds per alias) and verify it under this lock before replacing the binding.
| Some(existing) | ||
| if existing != record.id | ||
| && store.load_record_by_id_unlocked(&existing)?.is_some() => | ||
| { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
[bug · medium]
Leaving the binding untouched is correct for lookup availability, but the newly written record still contains the same alias. list() filters and returns record.alias directly without validating it against the authoritative alias file, so during every rebuild both the live snapshot and the pending build are exposed as owners of the alias. The documented commit crash window and best-effort cleanup can make that duplication permanent. Reconcile aliases while listing (clear/suppress an alias unless its binding points to that record), or persist pending/rebinding intent separately from the visible alias field.
| now_unix <= expires_unix | ||
| && self.expires_unix == expires_unix |
There was a problem hiding this comment.
[security · low]
This accepts the credential at the exact expiration timestamp. Expiry instants are normally exclusive (now < expires), and the POSIX pruning code similarly retains a grant while expires_unix == now_unix. This leaves a one-second boundary window and can disagree with clients or gateways using standard expiration semantics. Use < consistently and add a boundary test for now_unix == expires_unix.
Suggestion:
| now_unix <= expires_unix | |
| && self.expires_unix == expires_unix | |
| now_unix < expires_unix | |
| && self.expires_unix == expires_unix |
| /// authenticity: importing a hash that is already stored keeps the stored | ||
| /// archive, so an in-flight build can never observe its build context | ||
| /// change underneath it. | ||
| async fn import(&self, hash: &str, staged: &Path) -> RepositoryResult<()>; |
There was a problem hiding this comment.
[security · high]
The shared cache key is caller-controlled, but this contract explicitly does not verify that the uploaded archive corresponds to it. The upload-link handler accepts any syntactically valid hash, and both backends store archives globally as {hash}.tar; therefore, a caller can pre-populate another build's hash with arbitrary bytes. First-write-wins then makes the poisoned archive persistent and prevents the legitimate context from replacing it. Verify the archive using the SDK's hash algorithm before publishing, or scope keys/grants by the authenticated tenant/template so an untrusted key cannot affect other builds.
| /// Verification never removes the grant, so a request that fails before | ||
| /// the archive is stored can be retried with the same upload URL. Callers | ||
| /// must `claim_upload_grant` after publishing the archive, so a failed | ||
| /// publication leaves the URL retryable. |
There was a problem hiding this comment.
[security · high]
Requiring claim only after publication creates a check-then-act authorization race. The upload handler verifies, imports, and then claims; concurrent requests with the same token can both verify and race to first-write-wins import, so the request that loses the claim may still supply the permanently stored bytes. This violates the single-use guarantee even on POSIX where claiming itself is atomic. Claim/reserve the token after staging but before publishing (requiring a fresh link if publication fails), or introduce an atomic reservation state that can be released on failure and is required to commit the archive.
1 participant
What
Expose the E2B build-context upload contract:
GET /templates/{templateID}/files/{hash}returns201 {present, url}, and the returned bearer-token URL accepts a streamingPUT. Add bounded upload settings under[template_build]and regenerate the HTTP server from the OpenAPI source.e2b-build/01-error-envelopee2b-build/02-alias-rebuilde2b-build/03-build-file-storee2b-build/04-upload-apie2b-build/05-copy-plane2b-build/06-copy-execWhy
The maintainer asked that the full change in #28 be split into smaller reviewable PRs. This is layer 4 of that six-PR stack and implements the E2B SDK's upload handshake on top of the layer-3 repository store.
Related issue
Related: #28
#28 remains the full reference PR containing the design discussion, prior review history, and two-node E2E evidence.
Scope and non-goals
This layer includes the OpenAPI operation/model, regenerated server, upload router, API implementation, configuration/defaults, and tests. It depends on layer 3. COPY/ADD build-spec steps remain explicitly rejected until layer 6 wires archive consumption and execution.
Until the preceding PRs merge, the Files view also shows their commits (this branch stacks on them); this layer itself = the last commit, or JoyboyBrian/AgentENV@e2b-build/03-build-file-store...e2b-build/04-upload-api.
Design and behavior changes
The GET endpoint validates the template/hash, returns
present: truefor an existing archive, or issues a single-use grant and upload URL. The streaming PUT verifies the grant before staging, imports the archive by hash, claims the grant, enforces configured size/time bounds, and maps upload timeout to HTTP 408.public_base_urlsupports TLS-terminated or gateway-fronted deployments where the request Host fallback is insufficient.Compatibility and operations
TemplateBuildFileUploadandGET /templates/{templateID}/files/{hash}, plus the returned upload-only PUT route.[template_build]settings for upload/context/build budgets, URL TTL, request timeout, and optional public base URL; documented defaults preserve zero-config direct-node behavior.public_base_urlbefore use.Validation
make fmtmake clippymake test-unitcargo clippy --workspace --all-targets --all-features -- -D warningscargo test -p agentenv --libmake -C services test(required whenservices/changes)maketargetCommands and results:
The format, clippy, and library-test commands pass in the Linux verification environment. The tmpfs
TMPDIRavoids a pre-existingETXTBSYfrom devmachine's shared/tmpin an unrelated credential-helper test.make agentenv-serverregenerated the committed API output fromsrc/api/openapi.yml; a repository-formatter pass restores one pre-existing generatedNodeline that the generator's nested formatter unwraps on the verification toolchain. The resulting tree is byte-identical to the reviewed source. Unit tests cover URL construction, grant behavior, validation, limits, timeout/error mapping, and configuration validation. #28 retains the two-node upload E2E evidence.Skipped checks and reasons: service tests, integration tests requiring later COPY execution, docs checks, and benchmarks are outside this layer's scope.
Risks and reviewer notes
The upload URL contains a short-lived bearer token in its query string, so gateway/TLS deployments must configure the externally reachable HTTPS base URL and avoid logging query strings. Review this layer through the last commit or compare link.
Checklist