Skip to content

[2/6] feat(snapshot): E2B alias rebuild semantics on template publish - #70

Open
JoyboyBrian wants to merge 2 commits into
kvcache-ai:mainfrom
JoyboyBrian:e2b-build/02-alias-rebuild
Open

[2/6] feat(snapshot): E2B alias rebuild semantics on template publish#70
JoyboyBrian wants to merge 2 commits into
kvcache-ai:mainfrom
JoyboyBrian:e2b-build/02-alias-rebuild

Conversation

@JoyboyBrian

@JoyboyBrian JoyboyBrian commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What

Implement E2B rebuild semantics for snapshot aliases: an existing alias remains live while a replacement builds, moves only after the new snapshot commits, is restored on publish failure, and leaves the previous snapshot addressable by ID.

Layer PR Branch Scope
1/6 #69 e2b-build/01-error-envelope JSON 404 envelope for unmatched routes
2/6 #70 e2b-build/02-alias-rebuild Alias rebuild semantics
3/6 #71 e2b-build/03-build-file-store Build-context archive store
4/6 #72 e2b-build/04-upload-api Upload API and configuration
5/6 #73 e2b-build/05-copy-plan Host-side COPY planning
6/6 #74 e2b-build/06-copy-exec COPY/ADD execution and documentation

Why

The maintainer asked that the full change in #28 be split into smaller reviewable PRs. This is layer 2 of that six-PR stack and prevents rebuilds from taking a working template alias offline before the replacement is committed.

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 repository-only layer changes POSIX and OSS alias binding, publish ordering, rollback, previous-record cleanup, and focused tests. Build-context storage is added in layer 3; the HTTP upload surface is layer 4; COPY planning and execution are layers 5–6.

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/01-error-envelope...e2b-build/02-alias-rebuild.

Design and behavior changes

Create leaves an alias pointing to its committed snapshot while a replacement record waits. Publish commits the new record before rebinding the alias, clears the moved alias from the prior record, and restores prior record/alias state on failure. The POSIX catalog uses atomic filesystem operations; the OSS backend retains its documented best-effort compare/readback behavior where conditional writes are unavailable.

Compatibility and operations

  • Public API or generated protocol: N/A; repository behavior only.
  • Configuration or defaults: N/A; no configuration changes.
  • Snapshot manifest, artifact layout, or storage format: Existing records and alias keys remain compatible; only update ordering and alias metadata cleanup change.
  • Upgrade and rollback: Existing aliases remain readable. Rolling back restores the earlier conflict-on-rebuild behavior.
  • Host requirements, permissions, ports, or dependencies: N/A; no new requirements.

Validation

  • make fmt
  • make clippy
  • make test-unit
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo test -p agentenv --lib
  • Relevant Rust integration tests
  • make -C services test (required when services/ changes)
  • Generated clients/server regenerated with the documented make target
  • Documentation updated
  • Benchmarks or performance comparison completed

Commands and results:

make fmt
cargo clippy --workspace --all-targets --all-features -- -D warnings
TMPDIR=/dev/shm/agentenv-e2b-tests cargo test -p agentenv --lib

All listed commands pass in the Linux verification environment. The tmpfs TMPDIR avoids a pre-existing ETXTBSY from devmachine's shared /tmp in an unrelated credential-helper test. Added unit tests cover successful rebinding, failed-publish rollback, pending rebuild visibility, previous-record cleanup, and OSS rollback paths. #28 retains the end-to-end rebuild evidence.

Skipped checks and reasons: integration tests, service tests, code generation, docs checks, and benchmarks are outside this repository-only layer.

Risks and reviewer notes

Alias movement is deliberately ordered after the new record is readable. OSS cannot provide a true cross-object transaction, so its existing rare concurrent-publisher limitation is documented in code. Review this layer through the last commit or the compare link above.

Checklist

  • The PR contains one coherent change and no unrelated formatting or refactoring.
  • New behavior is covered by tests, or I explained why testing is impractical.
  • Logs and examples contain no credentials, tokens, or private registry information.
  • I did not manually edit generated code without updating its source and regenerating it.

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 4 issue(s) in this PR.

  • ✅ Successfully posted inline: 4 comment(s)

⚠️ 1 warning(s) occurred during review.


⚠️ Warnings:

  • src/snapshot/repository/backends/oss/repository.rs (subtask_error): context deadline exceeded

Comment on lines +536 to +538
// `import_built_artifacts` copies `vm_state.bin` first, so removing it
// fails the publish before any catalog state is committed.
fs::remove_file(&broken_artifacts.vm_state.path).expect("remove seeded vm state");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[test · medium]
This failure occurs before commit_publish, so the test does not exercise the newly added rollback ordering that must preserve the prior alias after the commit marker/new record has been written but alias rebinding fails. A regression in rollback_failed_publish (for example, leaving the committed marker/record or damaging the previous binding) would still pass this test. Add fault injection around the catalog alias write, or otherwise force a late commit failure, and assert the old binding/record are restored and the new directory is removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point that this test does not inject a failure at the final alias write. Its purpose is to verify the repository-level contract that a failed rebuild leaves the previous alias usable and cleans up the new snapshot directory, rather than to exercise every internal rollback branch.

A deterministic failure between the committed-record write and the atomic alias rename would require a catalog fault-injection hook solely for this test. I’d prefer not to add that machinery in this focused change. The ordering is explicit: the atomic alias rename is the final fallible catalog operation, so the previous binding remains untouched if it fails.

Comment thread src/snapshot/repository/backends/posixfs/catalog.rs
Comment thread src/snapshot/repository/backends/posixfs/catalog.rs
Comment thread src/snapshot/repository/backends/posixfs/catalog.rs
@JoyboyBrian

Copy link
Copy Markdown
Contributor Author

@yingdi-shan I reviewed the four automated comments against the supported snapshot publication lifecycle and replied to each thread individually. Three threads are resolved: they assume unsupported concurrent/same-ID publication or treat record.alias as the authoritative alias binding. I left the late alias-write fault-injection thread open because it identifies a real test-coverage boundary, although adding a catalog fault-injection hook would be broader than this focused change. Would appreciate a human review of the reasoning when you have a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant