Skip to content

Commit befb0b6

Browse files
committed
fix(openshell): preserve tracing and accept ready create responses
Carry the request span into the detached provisioning worker so compute driver calls remain attached to their parent trace after task handoff. Update the compensation regression to require no backend DELETE when the durable cleanup claim fails, matching the operation ownership requirement. Accept the gateway's current Ready snapshot when a sandbox becomes ready before CREATE returns. Do not require the client to observe an earlier provisioning phase. Cover the Ready-only watch and command attachment. Signed-off-by: Shiju <shiju@nvidia.com>
1 parent 3542c62 commit befb0b6

4 files changed

Lines changed: 94 additions & 26 deletions

File tree

‎crates/openshell-cli/src/run.rs‎

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -801,11 +801,8 @@ pub async fn sandbox_create(
801801
// Non-interactive mode: track start time for timestamps.
802802
let provision_start = Instant::now();
803803

804-
// Don't use stop_on_terminal on the server — the Kubernetes CRD may
805-
// briefly report a stale Ready status before the controller reconciles
806-
// a newly created sandbox. Instead we handle termination client-side:
807-
// we wait until we have observed at least one non-Ready phase followed
808-
// by Ready (a genuine Provisioning → Ready transition).
804+
// Handle terminal states here so a provisional container exit can wait
805+
// for the supervisor's canonical-process result before cleanup.
809806
let sandbox_name = sandbox.object_name().to_string();
810807
let sandbox_workspace = sandbox.object_workspace().to_string();
811808
let mut stream = client
@@ -833,8 +830,6 @@ pub async fn sandbox_create(
833830
let mut last_sandbox = sandbox.clone();
834831
let mut last_error_reason = String::new();
835832
let mut last_condition_message = ready_false_condition_message(sandbox.status.as_ref());
836-
// Track whether we have seen a non-Ready phase during the watch.
837-
let mut saw_non_ready = SandboxPhase::try_from(sandbox.phase()) != Ok(SandboxPhase::Ready);
838833
let provision_timeout = Duration::from_secs(
839834
std::env::var("OPENSHELL_PROVISION_TIMEOUT")
840835
.ok()
@@ -912,10 +907,6 @@ pub async fn sandbox_create(
912907
last_condition_message = Some(message);
913908
}
914909

915-
if phase != SandboxPhase::Ready {
916-
saw_non_ready = true;
917-
}
918-
919910
let main_process_result = has_main_process_result(&s);
920911
if matches!(
921912
phase,
@@ -950,9 +941,10 @@ pub async fn sandbox_create(
950941
break;
951942
}
952943

953-
// Only accept Ready as terminal after we've observed a
954-
// non-Ready phase, proving the controller has reconciled.
955-
if saw_non_ready && phase == SandboxPhase::Ready {
944+
// The gateway owns readiness. Its initial watch snapshot may
945+
// already be Ready if provisioning finished before CREATE
946+
// returned; requiring an earlier phase would miss that state.
947+
if phase == SandboxPhase::Ready {
956948
if let Some(d) = display.as_interactive_mut() {
957949
d.clear();
958950
}

‎crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs‎

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ struct SandboxState {
7070
vm_error_with_observed_exit: Arc<AtomicBool>,
7171
vm_slow_progress_before_ready: Arc<AtomicBool>,
7272
vm_log_churn_before_ready: Arc<AtomicBool>,
73+
ready_before_create_returns: Arc<AtomicBool>,
7374
terminal_before_relay: Arc<AtomicBool>,
7475
terminal_after_provisional_container_exit: Arc<AtomicBool>,
7576
provisional_container_exit_without_result: Arc<AtomicBool>,
@@ -194,7 +195,17 @@ impl OpenShell for TestOpenShell {
194195
}),
195196
..Sandbox::default()
196197
};
197-
sandbox.set_phase(SandboxPhase::Provisioning as i32);
198+
sandbox.set_phase(
199+
if self
200+
.state
201+
.ready_before_create_returns
202+
.load(Ordering::SeqCst)
203+
{
204+
SandboxPhase::Ready as i32
205+
} else {
206+
SandboxPhase::Provisioning as i32
207+
},
208+
);
198209
Ok(Response::new(SandboxResponse {
199210
sandbox: Some(sandbox),
200211
service_urls,
@@ -677,6 +688,10 @@ impl OpenShell for TestOpenShell {
677688
.vm_slow_progress_before_ready
678689
.load(Ordering::SeqCst);
679690
let vm_log_churn_before_ready = self.state.vm_log_churn_before_ready.load(Ordering::SeqCst);
691+
let ready_before_create_returns = self
692+
.state
693+
.ready_before_create_returns
694+
.load(Ordering::SeqCst);
680695
let terminal_before_relay = self.state.terminal_before_relay.load(Ordering::SeqCst);
681696
let terminal_after_provisional_container_exit = self
682697
.state
@@ -723,6 +738,18 @@ impl OpenShell for TestOpenShell {
723738
}
724739
let mut ready = provisioning.clone();
725740
ready.set_phase(SandboxPhase::Ready as i32);
741+
if ready_before_create_returns {
742+
// A watch starts with the current snapshot. Keep it open so
743+
// stream closure cannot hide a client that ignores Ready.
744+
let _ = tx
745+
.send(Ok(SandboxStreamEvent {
746+
payload: Some(sandbox_stream_event::Payload::Sandbox(ready)),
747+
cursor: String::new(),
748+
}))
749+
.await;
750+
tx.closed().await;
751+
return;
752+
}
726753
let mut completed = provisioning.clone();
727754
completed.status = Some(SandboxStatus {
728755
exit_code: Some(0),
@@ -2372,6 +2399,55 @@ async fn sandbox_create_preserves_vm_error_when_exit_code_is_observed() {
23722399
assert!(rendered.contains("ProcessExited: VM process exited with status 0"));
23732400
}
23742401

2402+
#[tokio::test]
2403+
async fn sandbox_create_accepts_ready_before_create_returns() {
2404+
let server = run_server().await;
2405+
server
2406+
.openshell
2407+
.state
2408+
.ready_before_create_returns
2409+
.store(true, Ordering::SeqCst);
2410+
let fake_ssh_dir = tempfile::tempdir().unwrap();
2411+
let xdg_dir = tempfile::tempdir().unwrap();
2412+
let _env = test_env_with(
2413+
&fake_ssh_dir,
2414+
&xdg_dir,
2415+
&[("OPENSHELL_PROVISION_TIMEOUT", "1".to_string())],
2416+
);
2417+
let tls = test_tls(&server);
2418+
install_fake_ssh(&fake_ssh_dir);
2419+
2420+
let exit_code = tokio::time::timeout(
2421+
Duration::from_secs(10),
2422+
run::sandbox_create(
2423+
&server.endpoint,
2424+
"openshell",
2425+
run::SandboxCreateConfig {
2426+
name: Some("already-ready"),
2427+
command: &["echo".into(), "OK".into()],
2428+
..test_config()
2429+
},
2430+
"default",
2431+
&tls,
2432+
),
2433+
)
2434+
.await
2435+
.expect("creation must finish while the watch remains open")
2436+
.expect("an already-Ready sandbox must not wait for a new provisioning transition");
2437+
2438+
assert_eq!(exit_code, 0);
2439+
assert_eq!(create_requests(&server).await.len(), 1);
2440+
assert_eq!(
2441+
server
2442+
.openshell
2443+
.state
2444+
.ssh_session_requests
2445+
.load(Ordering::SeqCst),
2446+
1,
2447+
"the initial Ready snapshot must allow the command to attach"
2448+
);
2449+
}
2450+
23752451
#[tokio::test]
23762452
async fn sandbox_create_keeps_waiting_while_vm_progress_arrives() {
23772453
let server = run_server().await;

‎crates/openshell-server/src/compute/mod.rs‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8419,7 +8419,7 @@ mod tests {
84198419
}
84208420

84218421
#[tokio::test]
8422-
async fn create_compensation_deletes_backend_when_delete_transition_cannot_be_stored() {
8422+
async fn create_compensation_retains_backend_when_delete_transition_cannot_be_stored() {
84238423
let directory = tempfile::tempdir().expect("temporary database directory");
84248424
let database_url = format!("sqlite://{}", directory.path().join("gateway.db").display());
84258425
let store = Arc::new(Store::connect(&database_url).await.expect("connect store"));
@@ -8462,14 +8462,10 @@ mod tests {
84628462
.message()
84638463
.contains("could not claim the sandbox record")
84648464
);
8465-
assert_eq!(driver.delete_calls(), 1);
8466-
assert_eq!(
8467-
driver.delete_requests(),
8468-
vec![(
8469-
sandbox.object_id().to_string(),
8470-
sandbox.object_name().to_string()
8471-
)]
8472-
);
8465+
// A rejected durable claim leaves cleanup ownership unknown. An
8466+
// unclaimed DELETE could destroy newer compute with the same ID.
8467+
assert_eq!(driver.delete_calls(), 0);
8468+
assert!(driver.delete_requests().is_empty());
84738469
let retained = runtime
84748470
.store
84758471
.get_message::<Sandbox>(sandbox.object_id())

‎crates/openshell-server/src/compute/provisioning_operation.rs‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ use std::time::Duration;
1212

1313
use openshell_core::{ObjectId, proto::Sandbox};
1414
use tonic::Status;
15+
use tracing::Instrument as _;
1516

1617
use super::{
1718
ComputeRuntime, provisioning_deadline, sandbox_provisioning_attempt_id,
@@ -127,7 +128,7 @@ impl ComputeRuntime {
127128
let owned_attempt = starting.clone();
128129
// Dropping a JoinHandle detaches its task. Do not abort it on a monitor
129130
// error or deadline: the remote side may still commit the request.
130-
let mut worker = tokio::spawn(async move {
131+
let worker = async move {
131132
let result = operation.await;
132133
let settled = if tracked {
133134
let settled = runtime
@@ -155,7 +156,10 @@ impl ComputeRuntime {
155156
settled: Box::new(settled),
156157
}),
157158
}
158-
});
159+
};
160+
// Retain the request span so detaching ownership preserves the driver
161+
// call's parent trace, including when the caller stops waiting.
162+
let mut worker = tokio::spawn(worker.in_current_span());
159163
loop {
160164
tokio::select! {
161165
result = &mut worker => return result.map_err(|error| {

0 commit comments

Comments
 (0)