Skip to content

Commit bb0b90d

Browse files
committed
fix(compute): fence stale restart lifecycle events
Signed-off-by: Seth Jennings <[email protected]>
1 parent e76b509 commit bb0b90d

4 files changed

Lines changed: 437 additions & 40 deletions

File tree

‎crates/openshell-driver-docker/src/lib.rs‎

Lines changed: 215 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,10 @@
88
use bollard::Docker;
99
use bollard::errors::Error as BollardError;
1010
use bollard::models::{
11-
ContainerCreateBody, ContainerSummary, ContainerSummaryStateEnum, CreateImageInfo,
12-
DeviceRequest, EndpointSettings, HostConfig, Mount, MountTmpfsOptions, MountTypeEnum,
13-
MountVolumeOptions, NetworkCreateRequest, NetworkingConfig, ProgressDetail, RestartPolicy,
14-
RestartPolicyNameEnum, SystemInfo,
11+
ContainerCreateBody, ContainerState, ContainerStateStatusEnum, ContainerSummary,
12+
ContainerSummaryStateEnum, CreateImageInfo, DeviceRequest, EndpointSettings, HostConfig, Mount,
13+
MountTmpfsOptions, MountTypeEnum, MountVolumeOptions, NetworkCreateRequest, NetworkingConfig,
14+
ProgressDetail, RestartPolicy, RestartPolicyNameEnum, SystemInfo,
1515
};
1616
use bollard::query_parameters::{
1717
CreateContainerOptionsBuilder, CreateImageOptions, DownloadFromContainerOptionsBuilder,
@@ -64,7 +64,7 @@ use tokio::sync::{Mutex, broadcast, mpsc};
6464
use tokio::task::JoinHandle;
6565
use tokio_stream::wrappers::ReceiverStream;
6666
use tonic::{Request, Response, Status};
67-
use tracing::{info, warn};
67+
use tracing::{debug, info, warn};
6868
use url::Url;
6969

7070
const WATCH_BUFFER: usize = 128;
@@ -205,6 +205,85 @@ pub struct DockerComputeDriver {
205205
events: broadcast::Sender<WatchSandboxesEvent>,
206206
pending: Arc<Mutex<HashMap<String, PendingSandboxRecord>>>,
207207
gpu_selector: Arc<CdiGpuDefaultSelector>,
208+
lifecycle_event_fences: DockerLifecycleEventFences,
209+
}
210+
211+
/// Per-sandbox container exit timestamps that fence snapshots from an earlier run.
212+
///
213+
/// Docker's polling loop can observe the stopped container before a restart and
214+
/// publish that snapshot after the gateway has moved the sandbox to `Starting`.
215+
/// Comparing the container's transition timestamp prevents that old observation
216+
/// from regressing the new lifecycle operation to `Error`.
217+
#[derive(Clone, Debug, Default)]
218+
struct DockerLifecycleEventFences {
219+
state: Arc<std::sync::Mutex<DockerLifecycleFenceState>>,
220+
}
221+
222+
#[derive(Debug, Default)]
223+
struct DockerLifecycleFenceState {
224+
previous_finished_at: HashMap<String, String>,
225+
starts_in_progress: HashSet<String>,
226+
}
227+
228+
impl DockerLifecycleEventFences {
229+
fn begin_start(&self, sandbox_id: &str) {
230+
self.state
231+
.lock()
232+
.unwrap_or_else(std::sync::PoisonError::into_inner)
233+
.starts_in_progress
234+
.insert(sandbox_id.to_string());
235+
}
236+
237+
fn finish_start(&self, sandbox_id: &str) {
238+
self.state
239+
.lock()
240+
.unwrap_or_else(std::sync::PoisonError::into_inner)
241+
.starts_in_progress
242+
.remove(sandbox_id);
243+
}
244+
245+
fn start_in_progress(&self, sandbox_id: &str) -> bool {
246+
self.state
247+
.lock()
248+
.unwrap_or_else(std::sync::PoisonError::into_inner)
249+
.starts_in_progress
250+
.contains(sandbox_id)
251+
}
252+
253+
fn record_previous_exit(&self, sandbox_id: &str, finished_at: Option<&str>) {
254+
let mut state = self
255+
.state
256+
.lock()
257+
.unwrap_or_else(std::sync::PoisonError::into_inner);
258+
match finished_at.filter(|finished_at| !finished_at.is_empty()) {
259+
Some(finished_at) => {
260+
state
261+
.previous_finished_at
262+
.insert(sandbox_id.to_string(), finished_at.to_string());
263+
}
264+
None => {
265+
state.previous_finished_at.remove(sandbox_id);
266+
}
267+
}
268+
}
269+
270+
fn previous_exit(&self, sandbox_id: &str) -> Option<String> {
271+
self.state
272+
.lock()
273+
.unwrap_or_else(std::sync::PoisonError::into_inner)
274+
.previous_finished_at
275+
.get(sandbox_id)
276+
.cloned()
277+
}
278+
279+
fn remove(&self, sandbox_id: &str) {
280+
let mut state = self
281+
.state
282+
.lock()
283+
.unwrap_or_else(std::sync::PoisonError::into_inner);
284+
state.previous_finished_at.remove(sandbox_id);
285+
state.starts_in_progress.remove(sandbox_id);
286+
}
208287
}
209288

210289
struct PendingSandboxRecord {
@@ -394,6 +473,7 @@ impl DockerComputeDriver {
394473
cdi_gpu_inventory,
395474
allow_all_default_gpu,
396475
)),
476+
lifecycle_event_fences: DockerLifecycleEventFences::default(),
397477
};
398478

399479
let poll_driver = driver.clone();
@@ -912,6 +992,19 @@ impl DockerComputeDriver {
912992
&self,
913993
sandbox_id: &str,
914994
sandbox_name: &str,
995+
) -> Result<bool, Status> {
996+
self.lifecycle_event_fences.begin_start(sandbox_id);
997+
let result = self
998+
.start_sandbox_with_lifecycle_fence(sandbox_id, sandbox_name)
999+
.await;
1000+
self.lifecycle_event_fences.finish_start(sandbox_id);
1001+
result
1002+
}
1003+
1004+
async fn start_sandbox_with_lifecycle_fence(
1005+
&self,
1006+
sandbox_id: &str,
1007+
sandbox_name: &str,
9151008
) -> Result<bool, Status> {
9161009
let Some(container) = self
9171010
.find_managed_container_summary(sandbox_id, sandbox_name)
@@ -927,6 +1020,26 @@ impl DockerComputeDriver {
9271020
return Ok(true);
9281021
}
9291022

1023+
// Fence a poll that observed this stopped run but has not published it
1024+
// yet. Use Docker's transition timestamp so a later, genuine exit from
1025+
// the restarted container remains observable.
1026+
let previous_finished_at = if state == ContainerSummaryStateEnum::EXITED {
1027+
let inspected = self
1028+
.docker
1029+
.inspect_container(&target, None)
1030+
.await
1031+
.map_err(|err| internal_status("inspect docker sandbox before start", err))?;
1032+
inspected
1033+
.state
1034+
.as_ref()
1035+
.filter(|state| state.status == Some(ContainerStateStatusEnum::EXITED))
1036+
.and_then(|state| state.finished_at.clone())
1037+
} else {
1038+
None
1039+
};
1040+
self.lifecycle_event_fences
1041+
.record_previous_exit(sandbox_id, previous_finished_at.as_deref());
1042+
9301043
match self.docker.start_container(&target, None).await {
9311044
Ok(()) => Ok(true),
9321045
// Already running — race with another start path or the
@@ -1177,7 +1290,7 @@ impl DockerComputeDriver {
11771290
tokio::time::sleep(backoff).await;
11781291
match self.current_snapshot_map().await {
11791292
Ok(current) => {
1180-
emit_snapshot_diff(&self.events, &previous, &current);
1293+
self.publish_snapshot_diff(&previous, &current).await;
11811294
previous = current;
11821295
backoff = WATCH_POLL_INTERVAL;
11831296
}
@@ -1202,6 +1315,78 @@ impl DockerComputeDriver {
12021315
})
12031316
}
12041317

1318+
async fn publish_snapshot_diff(
1319+
&self,
1320+
previous: &HashMap<String, DriverSandbox>,
1321+
current: &HashMap<String, DriverSandbox>,
1322+
) {
1323+
for (sandbox_id, sandbox) in current {
1324+
if previous.get(sandbox_id) == Some(sandbox) {
1325+
continue;
1326+
}
1327+
if self.stale_polled_exit(sandbox).await {
1328+
continue;
1329+
}
1330+
self.publish_sandbox_snapshot(sandbox.clone());
1331+
}
1332+
1333+
for sandbox_id in previous.keys() {
1334+
if current.contains_key(sandbox_id) {
1335+
continue;
1336+
}
1337+
self.publish_deleted(sandbox_id.clone());
1338+
}
1339+
}
1340+
1341+
async fn stale_polled_exit(&self, sandbox: &DriverSandbox) -> bool {
1342+
if !driver_sandbox_reports_container_exit(sandbox) {
1343+
return false;
1344+
}
1345+
if self.lifecycle_event_fences.start_in_progress(&sandbox.id) {
1346+
debug!(
1347+
sandbox_id = %sandbox.id,
1348+
"Ignoring Docker container exit snapshot while sandbox start is in progress"
1349+
);
1350+
return true;
1351+
}
1352+
let Some(previous_finished_at) = self.lifecycle_event_fences.previous_exit(&sandbox.id)
1353+
else {
1354+
return false;
1355+
};
1356+
let Some(container_id) = sandbox
1357+
.status
1358+
.as_ref()
1359+
.map(|status| status.instance_id.as_str())
1360+
.filter(|container_id| !container_id.is_empty())
1361+
else {
1362+
return false;
1363+
};
1364+
1365+
let inspected = match self.docker.inspect_container(container_id, None).await {
1366+
Ok(inspected) => inspected,
1367+
Err(err) => {
1368+
debug!(
1369+
sandbox_id = %sandbox.id,
1370+
container_id,
1371+
error = %err,
1372+
"Could not verify whether polled Docker exit predates sandbox start"
1373+
);
1374+
return false;
1375+
}
1376+
};
1377+
if !docker_polled_exit_is_stale(&previous_finished_at, inspected.state.as_ref()) {
1378+
return false;
1379+
}
1380+
1381+
debug!(
1382+
sandbox_id = %sandbox.id,
1383+
container_id,
1384+
previous_finished_at,
1385+
"Ignoring Docker container exit snapshot from before the latest sandbox start"
1386+
);
1387+
true
1388+
}
1389+
12051390
async fn list_managed_container_summaries(&self) -> Result<Vec<ContainerSummary>, Status> {
12061391
let filters = managed_container_label_filters(&self.config.sandbox_namespace, []);
12071392
self.docker
@@ -1510,6 +1695,7 @@ impl ComputeDriver for DockerComputeDriver {
15101695
let deleted = self
15111696
.delete_sandbox_inner(&request.sandbox_id, &request.sandbox_name)
15121697
.await?;
1698+
self.lifecycle_event_fences.remove(&event_sandbox_id);
15131699
if deleted && !event_sandbox_id.is_empty() {
15141700
let _ = self.events.send(WatchSandboxesEvent {
15151701
payload: Some(watch_sandboxes_event::Payload::Deleted(
@@ -2994,36 +3180,31 @@ fn docker_stop_timeout_secs(timeout_secs: u32) -> i32 {
29943180
i32::try_from(timeout_secs).unwrap_or(i32::MAX)
29953181
}
29963182

2997-
fn emit_snapshot_diff(
2998-
events: &broadcast::Sender<WatchSandboxesEvent>,
2999-
previous: &HashMap<String, DriverSandbox>,
3000-
current: &HashMap<String, DriverSandbox>,
3001-
) {
3002-
for (sandbox_id, sandbox) in current {
3003-
if previous.get(sandbox_id) == Some(sandbox) {
3004-
continue;
3005-
}
3006-
let _ = events.send(WatchSandboxesEvent {
3007-
payload: Some(watch_sandboxes_event::Payload::Sandbox(
3008-
WatchSandboxesSandboxEvent {
3009-
sandbox: Some(sandbox.clone()),
3010-
},
3011-
)),
3012-
});
3013-
}
3183+
fn driver_sandbox_reports_container_exit(sandbox: &DriverSandbox) -> bool {
3184+
sandbox.status.as_ref().is_some_and(|status| {
3185+
status.conditions.iter().any(|condition| {
3186+
condition.r#type == "Ready"
3187+
&& condition.status.eq_ignore_ascii_case("false")
3188+
&& condition.reason == "ContainerExited"
3189+
})
3190+
})
3191+
}
30143192

3015-
for sandbox_id in previous.keys() {
3016-
if current.contains_key(sandbox_id) {
3017-
continue;
3018-
}
3019-
let _ = events.send(WatchSandboxesEvent {
3020-
payload: Some(watch_sandboxes_event::Payload::Deleted(
3021-
WatchSandboxesDeletedEvent {
3022-
sandbox_id: sandbox_id.clone(),
3023-
},
3024-
)),
3025-
});
3193+
fn docker_polled_exit_is_stale(
3194+
previous_finished_at: &str,
3195+
current_state: Option<&ContainerState>,
3196+
) -> bool {
3197+
let Some(current_state) = current_state else {
3198+
return false;
3199+
};
3200+
3201+
if current_state.status != Some(ContainerStateStatusEnum::EXITED) {
3202+
// The list response said Exited, but inspect has already observed a
3203+
// newer state. Publishing the older list result would regress it.
3204+
return true;
30263205
}
3206+
3207+
current_state.finished_at.as_deref() == Some(previous_finished_at)
30273208
}
30283209

30293210
fn label_filters(values: impl IntoIterator<Item = String>) -> HashMap<String, Vec<String>> {

‎crates/openshell-driver-docker/src/tests.rs‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ fn test_driver_with_config(config: DockerDriverRuntimeConfig) -> DockerComputeDr
156156
CdiGpuInventory::default(),
157157
allow_all_default_gpu,
158158
)),
159+
lifecycle_event_fences: DockerLifecycleEventFences::default(),
159160
}
160161
}
161162

@@ -2494,3 +2495,49 @@ fn container_state_needs_start_matches_startable_states() {
24942495
);
24952496
}
24962497
}
2498+
2499+
#[test]
2500+
fn lifecycle_fence_rejects_polled_exit_from_before_restart() {
2501+
let fences = DockerLifecycleEventFences::default();
2502+
fences.begin_start("sandbox-1");
2503+
assert!(fences.start_in_progress("sandbox-1"));
2504+
fences.finish_start("sandbox-1");
2505+
assert!(!fences.start_in_progress("sandbox-1"));
2506+
2507+
fences.record_previous_exit("sandbox-1", Some("2026-08-12T16:39:13Z"));
2508+
assert_eq!(
2509+
fences.previous_exit("sandbox-1").as_deref(),
2510+
Some("2026-08-12T16:39:13Z")
2511+
);
2512+
2513+
let previous_exit = ContainerState {
2514+
status: Some(ContainerStateStatusEnum::EXITED),
2515+
finished_at: Some("2026-08-12T16:39:13Z".to_string()),
2516+
..Default::default()
2517+
};
2518+
assert!(docker_polled_exit_is_stale(
2519+
"2026-08-12T16:39:13Z",
2520+
Some(&previous_exit),
2521+
));
2522+
2523+
let running = ContainerState {
2524+
status: Some(ContainerStateStatusEnum::RUNNING),
2525+
..previous_exit.clone()
2526+
};
2527+
assert!(docker_polled_exit_is_stale(
2528+
"2026-08-12T16:39:13Z",
2529+
Some(&running),
2530+
));
2531+
2532+
let new_exit = ContainerState {
2533+
finished_at: Some("2026-08-12T16:40:00Z".to_string()),
2534+
..previous_exit
2535+
};
2536+
assert!(!docker_polled_exit_is_stale(
2537+
"2026-08-12T16:39:13Z",
2538+
Some(&new_exit),
2539+
));
2540+
2541+
fences.remove("sandbox-1");
2542+
assert!(fences.previous_exit("sandbox-1").is_none());
2543+
}

0 commit comments

Comments
 (0)