Repository navigation
bug(compute): periodic reconciliation can persist Error during an explicit start #4932
Description
User Story
I use OpenShell-backed agent sandboxes and need reliable stop/start with the same sandbox identity and retained workspace. My Linux validation encountered a successful driver start followed by a persistent gateway Error while the replacement container was running.
Problem Statement
On OpenShell 0.0.116, periodic reconciliation can read an old exited container after start_sandbox has committed Starting but before its driver start completes. It writes Error into that new transition. The successful start returns the poisoned row, and later ready snapshots do not recover it because Error is terminal.
The resource-version check does not prevent this: the sweep can read the already-committed Starting version while the driver operation is still in flight. Snapshot reconciliation and missing-resource pruning do not take the lifecycle gate held by start.
This mechanism is deterministically reproduced against tag v0.0.116, commit d1155aa70042d3e2ee49dbfa15346b108b7c1d92. Follow-up also reproduced the mechanism on current main d0a163a. See the separately scoped candidate, test results and unresolved qualification boundaries at #4932 (comment).
Impact / Why This Matters
A supported stop/start can leave a healthy replacement unusable through the gateway. Waiting longer does not repair a terminal persisted Error. Ignoring Error, rewriting the database, or deleting and recreating the sandbox is not an acceptable data-preserving solution.
This differs from #4828's external Docker restart and #4129's late watch update: the observed writer here is the periodic reconcile.sandboxes sweep during an explicit start. #4115 is a broader lifecycle-coordination proposal.
Acceptance Criteria
- Periodic reconciliation cannot apply a pre-start exited or temporarily absent backend observation during an owned start operation.
- After a successful start and valid replacement readiness, the same sandbox reaches Ready.
- A genuine replacement failure still reaches Error.
- Unrelated sandbox events and cleanup remain responsive; existing cancellation, stop/start, and cleanup tests pass.
- Qualify the correction with the affected Docker stop/start E2E path, without suppressing errors or weakening readiness checks.
Reproduction Steps
The deterministic reproduction uses only OpenShell's existing unit-test runtime and controlled driver, with an in-memory SQLite store. It does not need a third-party workload or credentials.
- Check out OpenShell
v0.0.116at the commit above. - Add the regression tests from the
compute/mod.rstest hunk in the candidate patch below, leaving the production hunks unapplied. - Run
cargo test --locked -p openshell-server --lib --features bundled-z3 waits_for_start_before_reading_backend -- --nocapture. - All three tests fail: the successful start returns Error (3), not Starting (8).
- Apply the production hunks, then run
cargo test --locked -p openshell-server --lib --features bundled-z3 compute::tests::.
The tests pause the real start path after its durable transition, force a concurrent sweep to observe an old exit or a missing backend, replace that observation with the ready replacement, then release start. They require Ready afterward and still require Error for a subsequent genuine replacement exit.
Environment
- Observed failure: OpenShell CLI/gateway/sandbox 0.0.116; Ubuntu 24.04.5 amd64; Docker compute driver, single gateway.
- Deterministic tests: exact 0.0.116 source, Linux aarch64, Rust 1.95.0, SQLite in-memory store, existing ControlledDriver, isolated network-disabled test environment.
- Original source:
crates/openshell-server/src/compute/mod.rs, blobb85641c986aa3a960936d832fac198d41ab0a294.
Minimal Timing Evidence
Gateway and Docker timestamps, UTC on 2026-10-26:
18:23:54.945490 StopSandbox succeeds
18:23:56.192808 replacement container StartedAt
18:23:56.234658 reconcile.sandboxes changes Starting -> Error
18:23:56.234679 ContainerExited / Container exited
18:23:56.324333 StartSandbox completes successfully
The replacement supervisor subsequently launched; Docker inspection reported running=true, OOMKilled=false, restarting=false. This does not alone establish gateway usability.
Candidate Repair and Verification
The local 0.0.116 patch holds the existing per-sandbox lifecycle gate across the fresh backend lookup and conditional write in both sweep paths. It preserves lifecycle-before-global lock ordering and releases the global lock during driver I/O. This is process-local protection, not distributed fencing; a sweep may wait for the active lifecycle operation.
- Original production source plus new tests: 0 passed, 3 failed.
- Patched compute suite: 116 passed, 0 failed, 2 pre-existing ignored tracing tests, 0.98 seconds.
- The two pre-existing ignored tests pass separately with
--ignored --test-threads=1, 0.14 seconds; no ignore annotations were changed. - Rustfmt and
git diff --checkpass. - Full workspace CI, patched Docker E2E, current-main qualification, and release artifacts remain outstanding.
Please assess this issue for acceptance and confirm the supported integration/backport target. No PR or branch has been pushed, no release has been made, and no dependency consumer has adopted this patch.
Reproducible candidate patch against v0.0.116
diff --git a/architecture/gateway.md b/architecture/gateway.md
index dcb1dd5..ae30e12 100644
--- a/architecture/gateway.md
+++ b/architecture/gateway.md
@@ -377,6 +377,13 @@ This matters for HA deployments where multiple gateway replicas share the same
Postgres database, and for single-node deployments where concurrent gRPC
handlers or the reconciler mutate the same sandbox.
+Within one gateway process, reconciliation also holds the sandbox's lifecycle
+gate across its fresh backend lookup and conditional write, including pruning
+checks for missing containers. CAS alone cannot protect a `Starting` row while
+its driver operation is still in flight. The lifecycle gate is acquired before
+the global lock; the global lock is released during backend I/O so unrelated
+sandbox events can proceed. The gate is process-local, not a cross-replica fence.
+
**Compile-time enforcement.** The unconditional write methods `put` and
`put_message` are gated behind `#[cfg(test)]`. Production code must use
`put_if` with an explicit `WriteCondition` or `update_message_cas`. The
diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs
index b85641c..8b186eb 100644
--- a/crates/openshell-server/src/compute/mod.rs
+++ b/crates/openshell-server/src/compute/mod.rs
@@ -3179,8 +3179,11 @@ impl ComputeRuntime {
snapshot: DriverSandbox,
sweep_started_at_ms: i64,
) -> Result<(), String> {
+ // A committed Starting row can still describe an in-flight driver start.
+ // Hold the lifecycle gate across the fresh backend read and its write.
+ let lifecycle_guard = self.lifecycle_gates.lock_for(&snapshot.id).await;
let expected_resource_version = {
- let _guard = self.sync_lock.lock().await;
+ let _guard = self.lock_global_for_lifecycle(&lifecycle_guard).await;
let Some(existing) = self
.store
.get(Sandbox::object_type(), &snapshot.id)
@@ -3203,7 +3206,7 @@ impl ComputeRuntime {
return Ok(());
};
- let _guard = self.sync_lock.lock().await;
+ let _guard = self.lock_global_for_lifecycle(&lifecycle_guard).await;
let Some(existing) = self
.store
.get(Sandbox::object_type(), &snapshot.id)
@@ -3228,8 +3231,9 @@ impl ComputeRuntime {
sweep_started_at_ms: i64,
grace_ms: i64,
) -> Result<(), String> {
+ let lifecycle_guard = self.lifecycle_gates.lock_for(&record.id).await;
let (sandbox_id, sandbox_name, expected_resource_version, age_ms) = {
- let _guard = self.sync_lock.lock().await;
+ let _guard = self.lock_global_for_lifecycle(&lifecycle_guard).await;
let Some(current_record) = self
.store
.get(Sandbox::object_type(), &record.id)
@@ -3260,7 +3264,7 @@ impl ComputeRuntime {
let current = self.get_driver_sandbox(&sandbox_id, &sandbox_name).await?;
- let _guard = self.sync_lock.lock().await;
+ let _guard = self.lock_global_for_lifecycle(&lifecycle_guard).await;
let Some(current_record) = self
.store
.get(Sandbox::object_type(), &sandbox_id)
@@ -7748,6 +7752,137 @@ mod tests {
);
}
+ #[derive(Clone, Copy)]
+ enum RestartSweepCase {
+ ListedExited,
+ UnlistedExited,
+ Missing,
+ }
+
+ async fn assert_sweep_waits_for_start(case: RestartSweepCase) {
+ let driver = ControlledDriver::new();
+ driver.block_start();
+ let runtime = test_runtime(driver.clone()).await;
+ let sandbox = sandbox_record("sb-restart", "sandbox-restart", SandboxPhase::Stopped);
+ runtime.store.put_message(&sandbox).await.unwrap();
+ let mut exited = ready_driver_sandbox(sandbox.object_id(), sandbox.object_name());
+ exited.status = Some(make_driver_status(make_driver_condition(
+ "ContainerExited",
+ "previous container exited",
+ )));
+ driver.set_get_outcome(match case {
+ RestartSweepCase::Missing => ControlledGetOutcome::Missing,
+ _ => ControlledGetOutcome::Sandbox(Box::new(exited.clone())),
+ });
+
+ let start_runtime = runtime.clone();
+ let start = tokio::spawn(async move {
+ start_runtime
+ .start_sandbox("default", "sandbox-restart")
+ .await
+ });
+ tokio::time::timeout(Duration::from_secs(1), driver.start_started.notified())
+ .await
+ .expect("start did not reach the paused driver");
+
+ let reconcile_runtime = runtime.clone();
+ let mut reconcile = tokio::spawn(async move {
+ match case {
+ RestartSweepCase::ListedExited => {
+ reconcile_runtime
+ .reconcile_snapshot_sandbox(exited, i64::MAX)
+ .await
+ }
+ _ => {
+ let record = reconcile_runtime
+ .store
+ .get(Sandbox::object_type(), &exited.id)
+ .await
+ .unwrap()
+ .unwrap();
+ reconcile_runtime
+ .prune_missing_sandbox(record, i64::MAX, 0)
+ .await
+ }
+ }
+ });
+ let early = tokio::time::timeout(Duration::from_millis(100), &mut reconcile).await;
+
+ // Replace the old exit observation before allowing the owned start to finish.
+ driver.set_get_outcome(ControlledGetOutcome::Sandbox(Box::new(
+ ready_driver_sandbox(sandbox.object_id(), sandbox.object_name()),
+ )));
+ register_test_supervisor_session(&runtime, sandbox.object_id());
+ driver.release_start();
+ let started = tokio::time::timeout(Duration::from_secs(1), start)
+ .await
+ .unwrap()
+ .unwrap()
+ .unwrap();
+ let reconciled_early = early.is_ok();
+ match early {
+ Ok(result) => result.unwrap().unwrap(),
+ Err(_) => tokio::time::timeout(Duration::from_secs(1), reconcile)
+ .await
+ .unwrap()
+ .unwrap()
+ .unwrap(),
+ }
+ let stored = runtime
+ .store
+ .get_message::<Sandbox>(sandbox.object_id())
+ .await
+ .unwrap()
+ .unwrap();
+ assert_eq!(
+ started.phase(),
+ SandboxPhase::Starting as i32,
+ "a stopped-container snapshot must not poison an active start"
+ );
+ assert!(
+ !reconciled_early,
+ "reconciliation must wait for the lifecycle owner"
+ );
+ assert_eq!(stored.phase(), SandboxPhase::Ready as i32);
+
+ let mut failed = ready_driver_sandbox(sandbox.object_id(), sandbox.object_name());
+ failed.status = Some(make_driver_status(make_driver_condition(
+ "ContainerExited",
+ "replacement container exited",
+ )));
+ driver.set_get_outcome(ControlledGetOutcome::Sandbox(Box::new(failed.clone())));
+ runtime
+ .reconcile_snapshot_sandbox(failed, i64::MAX)
+ .await
+ .unwrap();
+ let stored = runtime
+ .store
+ .get_message::<Sandbox>(sandbox.object_id())
+ .await
+ .unwrap()
+ .unwrap();
+ assert_eq!(
+ stored.phase(),
+ SandboxPhase::Error as i32,
+ "a genuine exit after start must still fail"
+ );
+ }
+
+ #[tokio::test]
+ async fn reconciliation_waits_for_start_before_reading_backend() {
+ assert_sweep_waits_for_start(RestartSweepCase::ListedExited).await;
+ }
+
+ #[tokio::test]
+ async fn pruning_exited_waits_for_start_before_reading_backend() {
+ assert_sweep_waits_for_start(RestartSweepCase::UnlistedExited).await;
+ }
+
+ #[tokio::test]
+ async fn pruning_missing_waits_for_start_before_reading_backend() {
+ assert_sweep_waits_for_start(RestartSweepCase::Missing).await;
+ }
+
#[tokio::test]
async fn blocked_reconciliation_lookup_does_not_delay_watch_events() {
let driver = ControlledDriver::new();
Activity
prekshivyas commentedon Oct 26, 2026
Current-source reproduction and bounded candidate
The same persisted-error mechanism also reproduces on current main,
d0a163a4284bdcd6ab656aa008e62192b7c3ed34, not just 0.0.116. No production source was changed for the baseline.Two tests fail on that base with Error (3) instead of Starting (8):
reconciliation_does_not_poison_inflight_start: pause the real start after driver dispatch, run the periodic snapshot reconciliation against the old exit, then complete start.reconciliation_does_not_poison_pending_driver_operation: retain the durable pending-operation record without a local lifecycle owner, then reconcile the old exit.Current main has durable provisioning-operation tracking that 0.0.116 lacks. A four-line candidate mirrors the existing pruning check in
reconcile_snapshot_sandbox: decode the current record and defer whiledriver_operation_pendingis true. The existing resource-version checks remain unchanged. Tests verify that configuration admission still blocks Ready, the admitted replacement becomes Ready, and genuine failures are still reported after settlement.Verification of this scoped candidate: 346 compute tests passed, 0 failed, 3 existing PostgreSQL-only tests ignored, in 2.66 seconds. Command:
cargo test --locked --offline -p openshell-server --lib --features vendored-z3 compute::. Rust 1.95.0, Linux aarch64, isolated network-disabled test environment. Rustfmt and diff whitespace checks passed. Clippy, PostgreSQL and Docker E2E have not run.Limits requiring owner review
This is not an integration-ready, comprehensive lifecycle repair. The pending check protects the demonstrated post-claim overlap; source review identifies additional boundaries to qualify:
claim_provisioning_operationsets the pending marker. The pre-claim window needs separate coverage/fencing.I tried a direct lifecycle-gate port, but rejected it for current main. It initially broke two pruning-cleanup tests. Passing the gate into background cleanup repaired those tests, but further review found that deletion-watch cleanup also assumes a busy gate holder owns cleanup. Extending gate ownership to reconciliation would require broader cleanup coordination. That candidate is not proposed for integration.
The 0.0.116 patch in the original report and this scoped current-main candidate are different, separately tested artifacts. Neither has been pushed, shipped, or adopted by a dependency consumer. Please assess acceptance and the intended target/backport path before a repair PR; additional source repair and E2E qualification remain necessary.
Current scoped patch SHA-256:
72f98cd4e2bc9db4895ede795b52a3ae329b32a9967037b67055f99c78e4a50a.Scoped current-main candidate and regression tests, not an admitted repair
Continuation: atomic explicit-start claim and legacy sweep protection (2026-10-26)
The two gaps identified above now have a broader local source candidate on the same main snapshot,
d0a163a4284bdcd6ab656aa008e62192b7c3ed34. This candidate supersedes the four-line candidate for further review; neither candidate has been published as a branch, shipped or adopted by a consumer.Starting, launch identity and the durable driver claim in one CAS. A retry claims without rotating identity, after revalidating the phase, generation and settled operation.Startingrows without a provisioning record, they also probe local lifecycle ownership under the sandbox mutation lock before backend lookup and before applying observations. Probes are dropped immediately. They are never held across backend I/O or deletion cleanup. The retry CAS also advances the legacy row version without adding a synthetic deadline.The rejected approach described above remains rejected: this candidate does not hold a sweep-owned lifecycle gate during backend I/O or pass one into asynchronous cleanup.
New regression evidence against the inherited four-line candidate, before applying this repair:
start_commits_driver_claim_with_starting_transition: fails because the first durableStartingpayload has no pending claim. A SQLite trigger records actual persisted writes, rather than only checking the eventual row.reconciliation_does_not_poison_legacy_start: fails with Error (3), not Starting (8), after a successful start.Final local verification: 350 compute tests passed, 0 failed, 3 unchanged PostgreSQL-only tests ignored, 2.68 seconds. The added tests cover the atomic claim, legacy exited/missing observations and genuine later exit, known pre-dispatch preparation failure without dispatch, and deletion-watch driver cleanup for the same sandbox while a sweep lookup is blocked. Existing admission, readiness, cancellation/recovery and cleanup tests also pass. The older resource-version test now requires the combined transition/claim write plus settlement; identity and authentication assertions remain intact.
Command:
cargo test --locked --offline -p openshell-server --lib --features vendored-z3 compute:: -- --nocapture. Rust 1.95.0, isolated Linux ARM64, network disabled. Rustfmt andgit diff --checkpass. Patch SHA-256:65489cfe02314e021cc7762ddeaf463ac0b0f20e77f84875a42def01ca9fa38b. Test-log SHA-256:580ccc0aca37f314205ae2787dd6e9e8ab31e115900cd89083555cdbea2c0804.Limits: legacy protection is process-local; no distributed-fencing claim is made for records without durable provisioning state. PostgreSQL integration, Clippy, full workspace CI, Docker E2E and a reviewed replacement runtime remain outstanding. Issue acceptance and the integration/backport target are still needed before a non-write contributor pushes a branch or opens a PR under the repository policy. No label, release, consumer pin, installed Windows application or credential was changed.
Local atomic-start candidate against d0a163a, for source review