[PR #403] refactor(orchestrator): typed lifecycle claims #406

New issue

Open

opened 2026-10-29 14:23:03 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:23:03 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/403
Author: @yingdi-shan
Created: 10/20/2026
Status: 🔄 Open

Base: main ← Head: refactor/orchestrator-lifecycle-claims

📝 Commits (3)

2ecb7fc refactor(orchestrator): hold transitional lifecycle states through typed claims
1d92ff2 fix(orchestrator): keep pause artifacts when persisting the paused record fails
00f22c7 refactor(orchestrator): split lifecycle operations out of service.rs

📊 Changes

17 files changed (+2911 additions, -1982 deletions)

View changed files

📝 CLAUDE.md (+6 -3)
➕ src/orchestrator/claim.rs (+327 -0)
📝 src/orchestrator/launch_plan.rs (+37 -21)
📝 src/orchestrator/mod.rs (+1 -0)
📝 src/orchestrator/persistence/file_backed.rs (+10 -15)
📝 src/orchestrator/persistence/mod.rs (+5 -0)
📝 src/orchestrator/service.rs (+97 -1841)
➕ src/orchestrator/service/create.rs (+257 -0)
➕ src/orchestrator/service/delete.rs (+305 -0)
➕ src/orchestrator/service/evict.rs (+174 -0)
➕ src/orchestrator/service/fork.rs (+238 -0)
➕ src/orchestrator/service/launch.rs (+272 -0)
➕ src/orchestrator/service/pause.rs (+274 -0)
➕ src/orchestrator/service/resume.rs (+185 -0)
➕ src/orchestrator/service/snapshot.rs (+158 -0)
📝 src/orchestrator/tests.rs (+407 -98)
📝 tests/integration/orchestrator.rs (+158 -4)

📄 Description

What

Makes orchestrator lifecycle state handling type-checked, fixes two lifecycle bugs, and splits service.rs per lifecycle. Three commits, meant to be reviewed one at a time:

refactor(orchestrator): hold transitional lifecycle states through typed claims (fixes #372)
- Adds Claim<K> in src/orchestrator/claim.rs. Acquiring one is the only way into Creating, Resuming, Pausing, Forking or Snapshotting.
- A claim is consumed exactly once, by commit, rollback or teardown. #[must_use] warns when one is ignored. A claim dropped unresolved logs a warning and rolls back in the background, so an early return or ? can no longer strand a sandbox.
- Resume is typed in two steps. Claim<PendingResume>::mark_resuming() returns Claim<Resume>, and a resume LaunchPlan can only be built from a Claim<Resume>. The persisted Resuming mark is therefore undone exactly when it was made.
- launch_sandbox now starts the runtime, waits until it is ready, then commits once. This replaces FailedLaunchStage and the rollback helpers that were written separately for each failure stage.
- Killing stays forward-only through DeleteProgress.
fix(orchestrator): keep pause artifacts when persisting the paused record fails (fixes #391)
- persist_paused no longer removes the artifact root when encoding or writing the record fails. That root can hold the sealed live rootfs layer that the running VM still reads.
- Terminal teardown now deletes the persisted record and artifacts after the VM is stopped, as delete does.
refactor(orchestrator): split lifecycle operations out of service.rs
- Moves each lifecycle into its own file under src/orchestrator/service/: create, launch, resume, pause, fork, snapshot, delete and evict.
- This is a pure move. The only changes besides the moved code are imports, pub(super) on the methods called across files, and rustfmt re-wrapping those signatures. git diff --color-moved shows it.

Why

Every lifecycle operation moved a sandbox into a transitional state and relied on each error branch to write its own way back. #372 is what happens when a branch forgets: the launch shutdown gate and the missing-paused-state check skipped the rollback and left sandboxes in Resuming, and their paused artifacts were then destroyed on the next startup. With claims, leaving a transitional state is enforced by types and checked by tests, instead of depending on review discipline.

In #391, a failed pause persist deleted a layer the live VM still reads. The VM was then resumed on a missing lower layer, so its next pause or resume failed.

At about 3,000 lines, service.rs was hard to navigate, which is why commit 3 splits it.

Closes #372
Closes #391

Scope and non-goals

In scope: orchestrator lifecycle state handling, pause artifact retention on failure, and the file split.
Out of scope: delete (Killing) keeps its existing forward-only DeleteProgress and does not use claims. No changes to the public API, configuration or persistence format.

Design and behavior changes

Claim resolution: each kind declares TRANSITIONAL, COMMITTED and ROLLBACK; stable-state kinds also declare FROM. The rollback is one of:
- Restore(state)
- RestorePausedRecord: Paused, plus undoing the persisted resuming mark
- Remove: for create
Failure paths:
- Recoverable errors roll back to the source state.
- Terminal errors call teardown, which detaches and stops the runtime, finalizes volumes, deletes persisted state and removes the sandbox.
Launch:
- The proxy target is now resolved before committing to Running.
- Abort removes the runtime handle only if it is still the one this launch registered.
Resume: a missing paused state is now rejected before the sandbox is claimed, so the sandbox stays Paused.
Pause persistence failure: the artifact root is kept while the VM resumes. If the VM cannot be resumed, the sandbox is torn down and its persisted state deleted.
Other terminal teardowns (fork, pause, snapshot) now also delete any persisted record and artifacts for the sandbox. Before, these were left for the startup orphan sweep.

Compatibility and operations

Public API or generated protocol: N/A, no changes.
Configuration or defaults: N/A, no changes.
Snapshot manifest, artifact layout, or storage format: no format change. Pause artifact roots are now kept after a failed persist instead of being removed.
Upgrade and rollback: N/A, no persisted-state migration.
Host requirements, permissions, ports, or dependencies: N/A.

Validation

make fmt
make clippy
make test-unit
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                                   # ok
make clippy                                # ok
cargo test -p agentenv --lib               # 1021 passed; 0 failed; 4 ignored
orchestrator real-VM integration tests     # 3 passed (KVM + ublk, isolated AENV_TEST_STATE_DIR)
  orchestrator::orchestrator_lifecycle
  orchestrator::orchestrator_capture_snapshot_can_be_published_and_relaunched
  orchestrator::failed_pause_persistence_keeps_live_rootfs_layer

New tests:

#372 regressions: resume_racing_shutdown_rolls_back_to_paused and resume_without_paused_state_leaves_sandbox_paused.
Claim-level tests for create and resume rollback, the background rollback of a dropped claim, and the replacement-handle guard in launch abort.
lifecycle_failures_never_leave_transitional_states: injects faults into create, resume, pause, fork and snapshot. It asserts that no transitional state remains and that no claim was dropped without being resolved.
#391: pause_persistence_and_resume_failure_removes_sandbox_and_artifacts (unit) and failed_pause_persistence_keeps_live_rootfs_layer (real VM).
I confirmed both bugs reproduce without the fixes: the #372 tests leave the sandbox Resuming, and the #391 test fails with "failed to open lower layer".

Skipped checks and reasons:

make test-unit as an aggregate target: I ran the agentenv lib suite and the targeted real-VM orchestrator suite instead.
make -C services test: services/ is unchanged.

Risks and reviewer notes

Review commit by commit. Commit 3 is a mechanical move, so git show --color-moved=dimmed-zebra <sha> is the easiest way to read it.
Most important files:
- src/orchestrator/claim.rs
- launch_sandbox and its helpers (service/launch.rs after commit 3)
- pause_claimed and recover_failed_pause (service/pause.rs)
- the ClaimHost impl in service.rs
Behavior change to check: terminal teardown now deletes persisted pause artifacts. That is safe because the VM is already stopped and the sandbox is removed, but it is a broader change than #391 alone.
The background rollback on drop is a safety net. Tests record every claim dropped without being resolved and fail on it, so production paths must resolve their claims explicitly.

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.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/kvcache-ai/AgentENV/pull/403 **Author:** [@yingdi-shan](https://github.com/yingdi-shan) **Created:** 10/20/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `refactor/orchestrator-lifecycle-claims` --- ### 📝 Commits (3) - [`2ecb7fc`](https://github.com/kvcache-ai/AgentENV/commit/2ecb7fc6d6a82999cb3a36c1909880fa221f0db2) refactor(orchestrator): hold transitional lifecycle states through typed claims - [`1d92ff2`](https://github.com/kvcache-ai/AgentENV/commit/1d92ff2981550933149988c785cb2820bab1e904) fix(orchestrator): keep pause artifacts when persisting the paused record fails - [`00f22c7`](https://github.com/kvcache-ai/AgentENV/commit/00f22c7c2fa5291833584349d6d95ac54f391d7e) refactor(orchestrator): split lifecycle operations out of service.rs ### 📊 Changes **17 files changed** (+2911 additions, -1982 deletions) <details> <summary>View changed files</summary> 📝 `CLAUDE.md` (+6 -3) ➕ `src/orchestrator/claim.rs` (+327 -0) 📝 `src/orchestrator/launch_plan.rs` (+37 -21) 📝 `src/orchestrator/mod.rs` (+1 -0) 📝 `src/orchestrator/persistence/file_backed.rs` (+10 -15) 📝 `src/orchestrator/persistence/mod.rs` (+5 -0) 📝 `src/orchestrator/service.rs` (+97 -1841) ➕ `src/orchestrator/service/create.rs` (+257 -0) ➕ `src/orchestrator/service/delete.rs` (+305 -0) ➕ `src/orchestrator/service/evict.rs` (+174 -0) ➕ `src/orchestrator/service/fork.rs` (+238 -0) ➕ `src/orchestrator/service/launch.rs` (+272 -0) ➕ `src/orchestrator/service/pause.rs` (+274 -0) ➕ `src/orchestrator/service/resume.rs` (+185 -0) ➕ `src/orchestrator/service/snapshot.rs` (+158 -0) 📝 `src/orchestrator/tests.rs` (+407 -98) 📝 `tests/integration/orchestrator.rs` (+158 -4) </details> ### 📄 Description ## What Makes orchestrator lifecycle state handling type-checked, fixes two lifecycle bugs, and splits `service.rs` per lifecycle. Three commits, meant to be reviewed one at a time: 1. **`refactor(orchestrator): hold transitional lifecycle states through typed claims`** (fixes #372) - Adds `Claim<K>` in `src/orchestrator/claim.rs`. Acquiring one is the only way into `Creating`, `Resuming`, `Pausing`, `Forking` or `Snapshotting`. - A claim is consumed exactly once, by `commit`, `rollback` or `teardown`. `#[must_use]` warns when one is ignored. A claim dropped unresolved logs a warning and rolls back in the background, so an early return or `?` can no longer strand a sandbox. - Resume is typed in two steps. `Claim<PendingResume>::mark_resuming()` returns `Claim<Resume>`, and a resume `LaunchPlan` can only be built from a `Claim<Resume>`. The persisted `Resuming` mark is therefore undone exactly when it was made. - `launch_sandbox` now starts the runtime, waits until it is ready, then commits once. This replaces `FailedLaunchStage` and the rollback helpers that were written separately for each failure stage. - `Killing` stays forward-only through `DeleteProgress`. 2. **`fix(orchestrator): keep pause artifacts when persisting the paused record fails`** (fixes #391) - `persist_paused` no longer removes the artifact root when encoding or writing the record fails. That root can hold the sealed live rootfs layer that the running VM still reads. - Terminal teardown now deletes the persisted record and artifacts after the VM is stopped, as delete does. 3. **`refactor(orchestrator): split lifecycle operations out of service.rs`** - Moves each lifecycle into its own file under `src/orchestrator/service/`: `create`, `launch`, `resume`, `pause`, `fork`, `snapshot`, `delete` and `evict`. - This is a pure move. The only changes besides the moved code are imports, `pub(super)` on the methods called across files, and rustfmt re-wrapping those signatures. `git diff --color-moved` shows it. ## Why Every lifecycle operation moved a sandbox into a transitional state and relied on each error branch to write its own way back. #372 is what happens when a branch forgets: the launch shutdown gate and the missing-paused-state check skipped the rollback and left sandboxes in `Resuming`, and their paused artifacts were then destroyed on the next startup. With claims, leaving a transitional state is enforced by types and checked by tests, instead of depending on review discipline. In #391, a failed pause persist deleted a layer the live VM still reads. The VM was then resumed on a missing lower layer, so its next pause or resume failed. At about 3,000 lines, `service.rs` was hard to navigate, which is why commit 3 splits it. ## Related issue Closes #372 Closes #391 ## Scope and non-goals - **In scope:** orchestrator lifecycle state handling, pause artifact retention on failure, and the file split. - **Out of scope:** delete (`Killing`) keeps its existing forward-only `DeleteProgress` and does not use claims. No changes to the public API, configuration or persistence format. ## Design and behavior changes - **Claim resolution:** each kind declares `TRANSITIONAL`, `COMMITTED` and `ROLLBACK`; stable-state kinds also declare `FROM`. The rollback is one of: - `Restore(state)` - `RestorePausedRecord`: `Paused`, plus undoing the persisted resuming mark - `Remove`: for create - **Failure paths:** - Recoverable errors roll back to the source state. - Terminal errors call `teardown`, which detaches and stops the runtime, finalizes volumes, deletes persisted state and removes the sandbox. - **Launch:** - The proxy target is now resolved before committing to `Running`. - Abort removes the runtime handle only if it is still the one this launch registered. - **Resume:** a missing paused state is now rejected before the sandbox is claimed, so the sandbox stays `Paused`. - **Pause persistence failure:** the artifact root is kept while the VM resumes. If the VM cannot be resumed, the sandbox is torn down and its persisted state deleted. - **Other terminal teardowns** (fork, pause, snapshot) now also delete any persisted record and artifacts for the sandbox. Before, these were left for the startup orphan sweep. ## Compatibility and operations - Public API or generated protocol: N/A, no changes. - Configuration or defaults: N/A, no changes. - Snapshot manifest, artifact layout, or storage format: no format change. Pause artifact roots are now kept after a failed persist instead of being removed. - Upgrade and rollback: N/A, no persisted-state migration. - Host requirements, permissions, ports, or dependencies: N/A. ## Validation - [x] `make fmt` - [x] `make clippy` - [ ] `make test-unit` - [x] Relevant Rust integration tests - [ ] `make -C services test` (required when `services/` changes) - [ ] Generated clients/server regenerated with the documented `make` target - [x] Documentation updated - [ ] Benchmarks or performance comparison completed Commands and results: ```text make fmt # ok make clippy # ok cargo test -p agentenv --lib # 1021 passed; 0 failed; 4 ignored orchestrator real-VM integration tests # 3 passed (KVM + ublk, isolated AENV_TEST_STATE_DIR) orchestrator::orchestrator_lifecycle orchestrator::orchestrator_capture_snapshot_can_be_published_and_relaunched orchestrator::failed_pause_persistence_keeps_live_rootfs_layer ``` New tests: - #372 regressions: `resume_racing_shutdown_rolls_back_to_paused` and `resume_without_paused_state_leaves_sandbox_paused`. - Claim-level tests for create and resume rollback, the background rollback of a dropped claim, and the replacement-handle guard in launch abort. - `lifecycle_failures_never_leave_transitional_states`: injects faults into create, resume, pause, fork and snapshot. It asserts that no transitional state remains and that no claim was dropped without being resolved. - #391: `pause_persistence_and_resume_failure_removes_sandbox_and_artifacts` (unit) and `failed_pause_persistence_keeps_live_rootfs_layer` (real VM). - I confirmed both bugs reproduce without the fixes: the #372 tests leave the sandbox `Resuming`, and the #391 test fails with "failed to open lower layer". Skipped checks and reasons: - `make test-unit` as an aggregate target: I ran the `agentenv` lib suite and the targeted real-VM orchestrator suite instead. - `make -C services test`: `services/` is unchanged. ## Risks and reviewer notes - Review commit by commit. Commit 3 is a mechanical move, so `git show --color-moved=dimmed-zebra <sha>` is the easiest way to read it. - Most important files: - `src/orchestrator/claim.rs` - `launch_sandbox` and its helpers (`service/launch.rs` after commit 3) - `pause_claimed` and `recover_failed_pause` (`service/pause.rs`) - the `ClaimHost` impl in `service.rs` - **Behavior change to check:** terminal teardown now deletes persisted pause artifacts. That is safe because the VM is already stopped and the sandbox is removed, but it is a broader change than #391 alone. - The background rollback on drop is a safety net. Tests record every claim dropped without being resolved and fail on it, so production paths must resolve their claims explicitly. ## Checklist - [x] The PR contains one coherent change and no unrelated formatting or refactoring. - [x] New behavior is covered by tests, or I explained why testing is impractical. - [x] Logs and examples contain no credentials, tokens, or private registry information. - [x] I did not manually edit generated code without updating its source and regenerating it. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-29 14:23:03 +00:00

bsakaguchi referenced this issue

2026-10-29 14:23:05 +00:00

[PR #406] [MERGED] chore(deps): bump github.com/klauspost/compress from 1.17.9 to 1.18.7 in /services #410