[1/4] feat(snapshot): template build-context archive store (posixfs + oss) - #71
[1/4] feat(snapshot): template build-context archive store (posixfs + oss)#71JoyboyBrian wants to merge 5 commits into
Conversation
|
🔍 OpenCodeReview found 3 issue(s) in this PR.
|
b33dc13 to
81a8edb
Compare
JoyboyBrian
changed the title
|
@yingdi-shan Following up on your suggestion in #70: I parked #70 as a draft and removed it from the dependency chain. #71–#74 are now rebuilt as a four-PR COPY/ADD stack directly on current In brief:
This PR is now the first layer of the stack. When convenient, could you review starting from #71? Thanks! |
![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.
[other · high]
This does not provide the trait's required first-write-wins immutability. OssClient::put_file performs an unconditional upload, so two concurrent imports can both observe the object as absent and the later completed PUT overwrites the first archive. That can change the bytes beneath an in-flight build, and it also makes concurrent replay of an OSS upload grant materially unsafe. Use an atomic create-only OSS operation; if multipart OSS uploads cannot support that, publish each upload to a unique temporary key and use a backend primitive that conditionally establishes the canonical key, or redesign the key/content verification so concurrent writers are guaranteed to upload identical bytes.
There was a problem hiding this comment.
Alibaba OSS does not support create-only semantics for multipart uploads. This protocol treats uploads sharing the SDK hash as equivalent input, so the contract now guarantees atomic complete-object publication rather than first-write-wins.
| let mut scanned: usize = 0; | ||
| for entry in entries.flatten() { | ||
| let path = entry.path(); | ||
| if path.extension().is_none_or(|ext| ext != extension) { | ||
| continue; | ||
| } | ||
| if scanned >= MAX_PRUNE_SCAN { | ||
| break; | ||
| } | ||
| scanned += 1; |
There was a problem hiding this comment.
[performance · medium]
The bounded scan does not actually drain a backlog over successive calls. read_dir commonly returns the same directory order each time, so once the first 256 matching entries are all fresh, expired entries later in that order are never inspected and the directory can grow without bound. Persist/rotate a cursor, randomize the starting point, shard records into time buckets, or use a full/background scan so every entry is eventually visited.
There was a problem hiding this comment.
Cleanup is intentionally opportunistic rather than a strict retention mechanism. The fairness claim has been removed; persistent cursors or background scanning would add disproportionate complexity for a re-uploadable cache.
| match fs::metadata(&path) { | ||
| Ok(_) => { | ||
| Self::touch(&path); | ||
| Ok(Some(path)) | ||
| } |
There was a problem hiding this comment.
[bug · medium]
This does not keep an archive safe from the concurrent pruner. The pruner can read a stale mtime before touch, then unlink the file after this method returns but before the build opens the returned path; it can also unlink between this metadata call and touch. The caller then receives Some(path) that no longer exists. Materialize a hard link in scratch_dir (and return that path), or coordinate pruning/materialization with an atomic lease/lock so an active archive cannot be removed.
There was a problem hiding this comment.
This narrow race is accepted. Hard links are unreliable across filesystems, while copying every archive would remove the shared-filesystem fast path. The documentation now explicitly states that the mtime refresh is not a lease.
| self.client | ||
| .put_bytes(&key, grant) | ||
| .await | ||
| .map_err(|error| RepositoryError::backend("write upload grant", error))?; |
There was a problem hiding this comment.
[performance · medium]
Expired or abandoned grants are never removed by this implementation. The repository contains no lifecycle policy/provisioning for this prefix, so installations without an externally configured bucket rule will accumulate one durable JSON object per issued upload URL indefinitely. Please provide an in-code cleanup mechanism or make a lifecycle rule for template-build-files/upload-grants/ an explicitly provisioned/validated requirement (and document its retention period).
There was a problem hiding this comment.
Cleanup remains an operator-managed bucket lifecycle policy because AgentENV does not provision the external OSS bucket. The backend documentation, sample configuration, and configuration reference now explicitly require a seven-day expiration rule for <prefix>/template-build-files/, covering cached archives and abandoned grants.
| if scanned >= MAX_PRUNE_SCAN { | ||
| break; | ||
| } | ||
| scanned += 1; |
There was a problem hiding this comment.
[performance · medium]
Every invocation restarts read_dir and stops after the same first 256 matching entries. Directory iteration is commonly stable, so if that prefix contains unexpired archives/grants, expired entries later in the directory are never inspected and the repository can grow without bound. Keep a persistent cursor, randomize/rotate the starting point, or move cleanup to a periodic full/incremental scan that guarantees eventual coverage.
There was a problem hiding this comment.
This is the same bounded-scan tradeoff as the earlier thread. Cleanup is intentionally opportunistic and request cost remains capped; strict eventual coverage would require persistent or background state for a re-uploadable cache.
| file.write_all(&bytes) | ||
| .and_then(|()| file.sync_all()) | ||
| .map_err(|error| { | ||
| let _ = fs::remove_file(&path); | ||
| RepositoryError::backend("write upload grant", error) | ||
| })?; | ||
| return Ok(token); |
There was a problem hiding this comment.
[bug · medium]
sync_all makes the grant contents durable but does not make the newly created directory entry durable. A crash after this function returns can lose the grant and invalidate an already-issued upload URL, contrary to the durable-grant contract. After syncing the file, sync grants_dir before returning the token (with an explicit best-effort policy only if callers are designed to regenerate lost grants).
There was a problem hiding this comment.
After syncing the grant file, the implementation now best-effort fsyncs grants_dir before returning the token. Directory fsync failures remain non-fatal for shared filesystems that reject it; a lost grant only requires requesting a fresh upload URL.
| match fs::metadata(&path) { | ||
| Ok(_) => { | ||
| Self::touch(&path); | ||
| Ok(Some(path)) | ||
| } |
There was a problem hiding this comment.
[bug · medium]
This check/touch/return sequence races with prune_expired: pruning can read the old mtime, then remove the archive before or after touch, leaving materialize returning Some(path) for a file that no longer exists. A build opening that path then fails instead of treating the cache entry as absent. Materialize into scratch_dir while holding an open source file handle (and handle/retry NotFound while opening), so pruning cannot invalidate the returned file.
There was a problem hiding this comment.
Materializing a private copy for every read would remove the shared-filesystem fast path and duplicate potentially large archives. This remains an accepted narrow race; the mtime refresh is explicitly documented as best-effort rather than a lease.
| /// 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]
This required verify → publish → claim ordering does not enforce single-use publication. Two requests can both verify the same token and race import; in the POSIX first-writer-wins implementation, the request that later loses the claim may already have permanently published its bytes. Because hash is explicitly not verified against the archive, a replay racing the legitimate request can poison the shared cache even though only one claim succeeds. Make authorization reservation/claim atomic before publication (with a recoverable in-progress state for retries), combine claim and import into one store operation, or independently verify the archive content against the hash before publication.
There was a problem hiding this comment.
The claim is intentionally post-publication so failed staging or storage remains retryable. Possession of the bearer token already authorizes publication for its bound (template_id, hash); the claim ensures only one request reports success, not content authenticity for simultaneous use of a leaked credential. The SDK hash is not the tar digest and cannot be reconstructed from the upload body alone. The trait contract now states this guarantee explicitly.
| match fs::metadata(&path) { | ||
| Ok(_) => Ok(true), |
There was a problem hiding this comment.
[bug · medium]
This reports success for any filesystem object at {hash}.tar, including directories and symlinks. import then keeps that object because its final_path.exists() fast path also accepts it, and materialize can return an unusable or redirected path. Use symlink_metadata and require file_type().is_file() (rejecting symlinks if the repository is an integrity boundary), returning a backend error for an unexpected object type.
There was a problem hiding this comment.
POSIX archive lookups now use symlink_metadata and accept only regular files. exists, import, and materialize return a backend error for directories, symlinks, or other unexpected object types. Tests cover directory and symlink entries.
| match fs::metadata(&path) { | ||
| Ok(_) => { | ||
| Self::touch(&path); | ||
| Ok(Some(path)) | ||
| } |
There was a problem hiding this comment.
[bug · medium]
materialize can return a path that has already been removed by a concurrent import's prune_expired. The metadata check, mtime refresh, and later caller open are not atomic; pruning can unlink an old archive between any of them (or immediately after this method returns), causing intermittent build failures. Materialize a node-local copy in scratch_dir (opening the source before copying so unlink cannot invalidate the read), or otherwise return a pinned/open handle instead of the pruneable shared pathname.
There was a problem hiding this comment.
This is the same accepted narrow race as the earlier materialize thread. Copying every cache hit into scratch space would duplicate potentially large archives and remove the shared-filesystem fast path, while returning an open handle would broaden the store and caller contract. The mtime refresh remains explicitly best-effort rather than a lease.
| let token = generate_upload_token(); | ||
| let key = Self::grant_key(&token).expect("generated token is valid"); |
There was a problem hiding this comment.
[other · low]
This expect is on a production request path even though the method returns RepositoryResult. It is currently backed by the implementation invariant that generate_upload_token() always produces a token accepted by is_valid_upload_token, but a future change to either helper would turn a recoverable internal inconsistency into a process panic. Propagate the invariant failure as a contextual repository error instead.
Suggestion:
| let token = generate_upload_token(); | |
| let key = Self::grant_key(&token).expect("generated token is valid"); | |
| let token = generate_upload_token(); | |
| let key = Self::grant_key(&token).ok_or_else(|| RepositoryError::backend( | |
| "validate generated upload token", | |
| std::io::Error::new(std::io::ErrorKind::InvalidData, "invalid generated token"), | |
| ))?; |
There was a problem hiding this comment.
The token is generated internally by generate_upload_token, and generation and validation use the same URL-safe encoding and UPLOAD_TOKEN_LEN; a unit test covers that invariant. This is not request-derived data, so the expect remains an internal invariant rather than a recoverable backend condition.
| 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 normally be replayed. |
There was a problem hiding this comment.
[security · high]
The grant is authorized from a read and then consumed with a separate unconditional delete. Two concurrent requests can both read the object before either delete completes, both return true, and both proceed with an upload using the same bearer token. This violates the trait's single-use claim contract for the OSS backend and allows replay within the grant TTL; use a storage primitive that provides conditional delete/compare-and-delete, or change the API/authorization flow so OSS does not advertise a successful single-use claim.
There was a problem hiding this comment.
The trait explicitly documents that S3-compatible backends lack conditional delete and therefore provide best-effort single-use within the grant TTL. Simultaneous use of the same bearer token is an accepted backend limitation; changing it requires a reservation protocol beyond this store contract.
| let Ok(entries) = fs::read_dir(dir) else { | ||
| return; | ||
| }; | ||
| let mut scanned: usize = 0; | ||
| for entry in entries.flatten() { |
There was a problem hiding this comment.
[performance · medium]
This bounded scan has no progress cursor and always starts from the directory's beginning. If the first 256 matching entries are fresh (or repeatedly fail deletion), expired entries later in a large directory are never examined on subsequent calls, so the stated opportunistic cleanup can permanently miss old archives/grants and allow storage to grow without bound. Persist traversal progress or schedule an occasional full sweep.
There was a problem hiding this comment.
This is the same bounded-scan tradeoff as the earlier threads. Cleanup is intentionally opportunistic and request cost remains capped; strict eventual coverage would require persistent or background state for a re-uploadable cache.
| let store_staged = root.join(format!(".import-{}.tmp", uuid::Uuid::new_v4())); | ||
| fs::copy(&staged, &store_staged).map_err(|error| { | ||
| let _ = fs::remove_file(&store_staged); | ||
| RepositoryError::backend("copy build archive into store", error) | ||
| })?; |
There was a problem hiding this comment.
[performance · medium]
The temporary store file is removed only on the normal return paths. A process crash or forced termination after fs::copy leaves a potentially large .import-*.tmp file, and prune_expired scans only tar entries, so these abandoned imports are never reclaimed by this backend. Include temporary files in recovery/retention cleanup, or use a startup sweep for stale .tmp entries.
There was a problem hiding this comment.
Import cleanup now also scans .import-*.tmp entries and removes those older than the seven-day retention window. A regression test plants a stale staging file and verifies that the next import removes it.
| fs::copy(&staged, &store_staged).map_err(|error| { | ||
| let _ = fs::remove_file(&store_staged); | ||
| RepositoryError::backend("copy build archive into store", error) | ||
| })?; |
There was a problem hiding this comment.
[security · high]
import publishes the staged bytes under the caller-provided hash without computing and comparing the archive's digest. The shared trait explicitly treats hash as an unverified cache key, so a corrupted or malicious upload can win the first-writer race and permanently associate incorrect content with that hash; subsequent exists calls skip re-upload and builds consume the poisoned archive. Validate the staged file against the expected content hash before copying/publishing (or enforce that invariant at the upload boundary).
There was a problem hiding this comment.
hash is the SDK cache key, not a digest of the tar bytes, so the archive cannot be validated against it at this boundary. The trait explicitly assumes equivalent uploads for one key and does not provide content authenticity; the bearer grant is the authorization boundary.
| if !Self::archive_exists(&path)? { | ||
| return Ok(None); | ||
| } | ||
| Self::touch(&path); | ||
| Ok(Some(path)) |
There was a problem hiding this comment.
[other · medium]
materialize returns the canonical path after an existence check and an mtime touch, but an overlapping import can call prune_expired and unlink that same archive before the caller opens it. The caller then receives Some(path) that no longer exists. Return a scratch-owned copy/open handle, coordinate pruning with materialization, or make the consumer retry a missing path.
There was a problem hiding this comment.
This is the same accepted narrow race as the earlier materialize threads. Copying every cache hit into scratch space would duplicate potentially large archives and remove the shared-filesystem fast path, while returning an open handle would broaden the store and caller contract. The mtime refresh remains explicitly best-effort rather than a lease.
| Self::touch(&path); | ||
| Ok(Some(path)) |
There was a problem hiding this comment.
[maintainability · medium]
Although scratch_dir is supplied specifically for materialization, this implementation returns the shared canonical archive path. The trait contract says callers must treat the result as read-only, but that is only a convention: any downstream extraction or cleanup that mutates/removes the returned file can corrupt the shared cache for all builds and nodes. Copy the archive into scratch_dir (or return an ownership-safe abstraction) before handing it to consumers.
There was a problem hiding this comment.
The trait explicitly allows shared-filesystem implementations to return the canonical path and requires callers to treat it as read-only. Copying every cache hit would duplicate potentially large archives and eliminate the shared-filesystem fast path, so the ownership convention remains part of the caller contract.
| // Consume the grant. `remove_file` succeeds for exactly one | ||
| // caller, so it is the claim: concurrent replays of the same | ||
| // token lose the race and are rejected. | ||
| match fs::remove_file(&path) { |
There was a problem hiding this comment.
[security · high]
The grant is only consumed after the archive has been imported, while verify_upload_grant is a separate non-consuming read. Two concurrent requests with the same valid token can both pass verification and import their bodies before either remove_file wins; because the archive is first-writer-wins, this permits replayed uploads and unbounded duplicate work (and becomes cache poisoning if the hash is not independently validated). The upload flow needs an atomic reservation/claim before accepting the body, or an atomic post-upload commit that binds the claim to the validated content.
There was a problem hiding this comment.
Post-publication claim is intentional so failed staging or storage remains retryable. POSIX unlink ensures only one claim reports success, while simultaneous holders may publish before that claim; possession of the bearer token already authorizes publication for its bound (template_id, hash). A pre-publication reservation would change the retry semantics and is outside this store contract.
| /// SDK-supplied cache key rather than a digest the store verifies, so the | ||
| /// protocol assumes repeated uploads for one hash describe equivalent | ||
| /// build input; the store does not provide content authenticity. | ||
| async fn import(&self, hash: &str, staged: &Path) -> RepositoryResult<()>; |
There was a problem hiding this comment.
[security · high]
hash is used as the sole global archive key by the repository backends, while this contract explicitly permits importing bytes without verifying that they correspond to the hash. A client holding a valid grant for one template can therefore publish arbitrary bytes under a hash that another template/build later requests; with the OSS backend's overwrite/race behavior this can also replace the cached content. Bind archive storage to template_id (and pass it through import/materialize) or verify the staged archive's canonical digest before making it globally visible.
There was a problem hiding this comment.
Global keying by the SDK hash is the protocol-defined cache behavior and enables sharing equivalent build input across templates. Template-scoped keys would change that behavior and the store API, while the SDK hash is not the tar digest and cannot be verified from the staged body. Content authenticity is explicitly outside this contract.
| let mut scanned: usize = 0; | ||
| for entry in entries.flatten() { | ||
| let path = entry.path(); | ||
| if path.extension().is_none_or(|ext| ext != extension) { | ||
| continue; | ||
| } | ||
| if scanned >= MAX_PRUNE_SCAN { | ||
| break; | ||
| } |
There was a problem hiding this comment.
[performance · medium]
This always examines the first 256 matching entries in the filesystem's read_dir order. If that order is stable and those entries are fresh, expired entries after them are never reached, even across repeated imports/grant creations; the cache can therefore grow without bound and exhaust the repository filesystem. Keep a persistent/rotating cursor, randomize the scan start, or periodically run a complete background sweep so later entries cannot be permanently starved.
There was a problem hiding this comment.
This is the same bounded-scan tradeoff as the earlier threads. Cleanup is intentionally opportunistic and request cost remains capped; strict eventual coverage would require persistent or background state for a re-uploadable cache.
| if !Self::archive_exists(&path)? { | ||
| return Ok(None); | ||
| } | ||
| Self::touch(&path); | ||
| Ok(Some(path)) |
There was a problem hiding this comment.
[other · medium]
The existence check and mtime update do not pin the archive. A concurrent prune_expired can unlink it after this check (or after touch) but before the caller opens the returned path, causing an intermittent missing-file build failure; touch failures on a read-only node make this more likely. Materialize into the caller's scratch directory (for example by copying from an already-open source handle) before returning, or introduce a lease/pinning mechanism that pruning honors.
There was a problem hiding this comment.
This is the same accepted narrow race as the earlier materialize threads. Copying every cache hit into scratch space would duplicate potentially large archives and remove the shared-filesystem fast path, while a lease or open-handle contract would broaden this store substantially. The mtime refresh remains explicitly best-effort rather than a lease.
| /// Claiming is deliberately not a pre-publication reservation: callers | ||
| /// verify first so failed staging or publication remains retryable, then | ||
| /// claim after publication. Simultaneous holders of the same bearer token | ||
| /// can therefore race publication before one claim succeeds. Possession of | ||
| /// the token already authorizes publication for its bound | ||
| /// (template_id, hash); this protocol does not add content authenticity, | ||
| /// and repeated uploads for one hash must describe equivalent build input. | ||
| /// | ||
| /// S3-compatible backends have no conditional delete and therefore | ||
| /// degrade the claim itself to best-effort single-use within the grant TTL. |
There was a problem hiding this comment.
[security · high]
Publication before consumption creates a TOCTOU authorization flaw. Two requests can both pass verify_upload_grant and publish, while only one later claims the token; in the OSS implementation, put_file is last-writer-wins, so the request whose claim is rejected can still overwrite the durable archive. This means a failed/replayed request can change build input. Reserve/consume the grant before publication with a retryable in-progress state/lease, or make OSS publication an atomic create-if-absent operation so a losing request cannot mutate an existing archive.
There was a problem hiding this comment.
Post-publication claim and OSS last-writer-wins publication are deliberate documented semantics. Possession of the bearer token authorizes publication for its bound (template_id, hash), and concurrent uploads for one SDK cache key are treated as equivalent input; the SDK hash is not a tar digest. Multipart OSS publication has no create-if-absent primitive here, while a retryable reservation state machine is outside this store contract.
|
One concern I have is that, now that we already support uploading and downloading files and folders through envd, there may be a simpler design: upload the build context directly into the build sandbox. For deduplication, we could consider snapshoting and resuming after each The remaining questions are how to preserve compatibility with the E2B upload workflow and how to identify and reuse cached OverlayBD layers correctly. One possible approach is to generate a tree of cache keys for the build steps and use prefix matching to find the longest reusable sequence of layers. Before proceeding with the implementation, could we open an RFC issue to discuss these designs? |
|
@yingdi-shan Thanks for the proposal — I've opened the RFC: #141 TL;DR: it recommends adopting your per-step layer caching model as the core design (each mutating step sealed into an OverlayBD layer via the existing Impact on the stack: #73/#74 are unaffected; #71/#72 would be simplified per the RFC's Phase 0. I'll hold the stack until the RFC discussion settles. |
|
Closing as superseded by the direction agreed in #141: build context streams directly into the build VM via envd in the |
2 participants
What
Add repository-backed template build-context archive storage for POSIX and OSS deployments, including immutable content-addressed archives, single-use upload grants, bounded pruning, and a
SnapshotManageraccessor.This is the first layer of the active COPY/ADD stack, replayed directly onto current main at
00ba6cb. The alias-rebuild work in #70 is intentionally not a dependency and is parked pending the planned alias-system refactor.e2b-build/03-build-file-storee2b-build/04-upload-apie2b-build/05-copy-plane2b-build/06-copy-execWhy
The E2B SDK uploads build-context archives separately from the template build request. This layer provides the shared repository primitive needed to accept those uploads on one node and consume them during a later build, including clustered deployments backed by the OSS repository.
Related: #28. The original PR remains the full concept/reference discussion.
Scope and non-goals
This layer contains only the storage interfaces, POSIX/OSS implementations, repository wiring, accessors, and focused storage tests. It does not expose an HTTP upload surface or execute COPY/ADD steps. Existing-alias rebuild remains unsupported.
Exact layer diff: 00ba6cb...e2b-build/03-build-file-store
Design
TemplateBuildFileStoreabstracts archive lookup, import, materialization, upload-grant issue/verify/claim, and pruning.Compatibility and operations
Validation
The rewritten stack was validated on Linux against the #74 tip:
This layer includes focused tests for immutable import, first-write-wins publication, grants/claims, pruning, mtime refresh, and failure cleanup. GitHub CI validates this cumulative branch independently.
Reviewer notes
The main review surfaces are single-use grant behavior, no-clobber publication, and bounded cleanup. #70 is not required to merge this stack.