Skip to content

fix(docker): reclaim sandbox token files on out-of-band removal #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
letv1nnn wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

fix(docker): reclaim sandbox token files on out-of-band removal #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6c8573
fix(driver-docker): scope pending sandbox matching by id and workspac…
letv1nnn Sep 10, 2026
097e5ba
fix(docker): reclaim sandbox token files on out-of-band removal
letv1nnn Sep 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/openshell-driver-docker/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ opentelemetry = { workspace = true }
opentelemetry_sdk = { workspace = true, features = ["testing"] }
prost-types = { workspace = true }
tar = "0.4"
temp-env = "0.3"
temp-env = { version = "0.3", features = ["async_closure"] }
tempfile = "3"
tracing-subscriber = { workspace = true }

Expand Down
138 changes: 103 additions & 35 deletions crates/openshell-driver-docker/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,7 +795,7 @@ impl DockerComputeDriver {
return Ok(Some(sandbox));
}

Ok(self.pending_snapshot(sandbox_id, sandbox_name).await)
self.pending_snapshot(sandbox_id, sandbox_name).await
}

async fn current_snapshots(&self) -> Result<Vec<DriverSandbox>, Status> {
Expand Down Expand Up @@ -1068,7 +1068,9 @@ impl DockerComputeDriver {
sandbox_id: &str,
sandbox_name: &str,
) -> Result<bool, Status> {
let pending = self.remove_pending_sandbox(sandbox_id, sandbox_name).await;
let pending = self
.remove_pending_sandbox(sandbox_id, sandbox_name)
.await?;
if let Some(record) = pending.as_ref()
&& let Some(task) = record.task.as_ref()
{
Expand Down Expand Up @@ -1102,6 +1104,11 @@ impl DockerComputeDriver {
}
}
}
// Container gone and no in-memory record survived (gateway
// restarted after an out-of-band `docker rm`). DeleteSandbox is
// the only thing that ever reclaims the token file, so reclaim it
// here too.
cleanup_sandbox_token_file_for_delete(sandbox_id, None, &self.config);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return Ok(false);
};
let Some(target) = summary_container_target(&container) else {
Expand Down Expand Up @@ -1133,7 +1140,10 @@ impl DockerComputeDriver {
.find_managed_container_summary(sandbox_id, sandbox_name)
.await?
else {
if let Some(record) = self.remove_pending_sandbox(sandbox_id, sandbox_name).await {
if let Some(record) = self
.remove_pending_sandbox(sandbox_id, sandbox_name)
.await?
{
if let Some(task) = record.task {
task.abort();
}
Expand Down Expand Up @@ -1250,10 +1260,11 @@ impl DockerComputeDriver {

async fn reserve_pending_sandbox(&self, sandbox: &DriverSandbox) -> Result<(), Status> {
let mut pending = self.pending.lock().await;
if pending
.values()
.any(|record| record.sandbox.id == sandbox.id || record.sandbox.name == sandbox.name)
{
if pending.values().any(|record| {
record.sandbox.id == sandbox.id
|| (record.sandbox.name == sandbox.name
&& record.sandbox.workspace == sandbox.workspace)
}) {
return Err(Status::already_exists("sandbox already exists"));
}

Expand All @@ -1276,12 +1287,12 @@ impl DockerComputeDriver {
&self,
sandbox_id: &str,
sandbox_name: &str,
) -> Option<DriverSandbox> {
) -> Result<Option<DriverSandbox>, Status> {
let pending = self.pending.lock().await;
pending
.values()
.find(|record| pending_sandbox_matches(&record.sandbox, sandbox_id, sandbox_name))
.map(|record| record.sandbox.clone())
let Some(id) = resolve_pending_id(&pending, sandbox_id, sandbox_name)? else {
return Ok(None);
};
Ok(pending.get(&id).map(|record| record.sandbox.clone()))
}

async fn pending_snapshot_map(&self) -> HashMap<String, DriverSandbox> {
Expand All @@ -1301,12 +1312,12 @@ impl DockerComputeDriver {
&self,
sandbox_id: &str,
sandbox_name: &str,
) -> Option<PendingSandboxRecord> {
) -> Result<Option<PendingSandboxRecord>, Status> {
let mut pending = self.pending.lock().await;
let id = pending.iter().find_map(|(id, record)| {
pending_sandbox_matches(&record.sandbox, sandbox_id, sandbox_name).then(|| id.clone())
})?;
pending.remove(&id)
let Some(id) = resolve_pending_id(&pending, sandbox_id, sandbox_name)? else {
return Ok(None);
};
Ok(pending.remove(&id))
}

async fn fail_pending_sandbox(
Expand Down Expand Up @@ -1561,21 +1572,14 @@ impl DockerComputeDriver {
.map_err(|err| internal_status("find Docker sandbox container", err))?;

Ok(containers.into_iter().find(|summary| {
let Some(labels) = summary.labels.as_ref() else {
return false;
};
let namespace_matches = labels
.get(LABEL_SANDBOX_NAMESPACE)
.is_some_and(|value| value == &self.config.sandbox_namespace);
let id_matches = sandbox_id.is_empty()
|| labels
.get(LABEL_SANDBOX_ID)
.is_some_and(|value| value == sandbox_id);
let name_matches = sandbox_name.is_empty()
|| labels
.get(LABEL_SANDBOX_NAME)
.is_some_and(|value| value == sandbox_name);
namespace_matches && id_matches && name_matches
summary.labels.as_ref().is_some_and(|labels| {
managed_container_identity_matches(
labels,
&self.config.sandbox_namespace,
sandbox_id,
sandbox_name,
)
})
}))
}

Expand Down Expand Up @@ -2163,9 +2167,73 @@ fn pending_sandbox_snapshot(
}
}

fn pending_sandbox_matches(sandbox: &DriverSandbox, sandbox_id: &str, sandbox_name: &str) -> bool {
(!sandbox_id.is_empty() && sandbox.id == sandbox_id)
|| (!sandbox_name.is_empty() && sandbox.name == sandbox_name)
/// Decides whether a managed container satisfies a lifecycle request.
///
/// `sandbox_id` is authoritative, matching [`resolve_pending_id`]. Requiring
/// the name to agree as well would discard a correct id match whenever the
/// caller pairs it with a stale name, leaving the container and its token file
/// behind while the driver reports the sandbox as absent.
///
/// A request with no identifier matches nothing. `require_sandbox_identifier`
/// rejects that upstream, but the label filters degenerate to "every managed
/// container in the namespace", so this does not rely on the caller to guard it.
fn managed_container_identity_matches(
labels: &HashMap<String, String>,
namespace: &str,
sandbox_id: &str,
sandbox_name: &str,
) -> bool {
if labels
.get(LABEL_SANDBOX_NAMESPACE)
.is_none_or(|value| value != namespace)
{
return false;
}
if !sandbox_id.is_empty() {
return labels
.get(LABEL_SANDBOX_ID)
.is_some_and(|value| value == sandbox_id);
}
!sandbox_name.is_empty()
&& labels
.get(LABEL_SANDBOX_NAME)
.is_some_and(|value| value == sandbox_name)
}

/// Resolves a lifecycle request to at most one pending sandbox id.
///
/// `sandbox_id` is authoritative: when the caller supplies one, the name is
/// never consulted as an alternative. The name fallback rejects ambiguity
/// instead of letting `HashMap` iteration order pick a match, because sandbox
/// names are unique per workspace and the driver request carries no workspace.
fn resolve_pending_id(
pending: &HashMap<String, PendingSandboxRecord>,
sandbox_id: &str,
sandbox_name: &str,
) -> Result<Option<String>, Status> {
if !sandbox_id.is_empty() {
return Ok(pending
.contains_key(sandbox_id)
.then(|| sandbox_id.to_string()));
}
if sandbox_name.is_empty() {
return Ok(None);
}

let mut matches = pending
.iter()
.filter(|(_, record)| record.sandbox.name == sandbox_name)
.map(|(id, _)| id.clone());

let Some(id) = matches.next() else {
return Ok(None);
};
if matches.next().is_some() {
return Err(Status::failed_precondition(
"sandbox_name matches multiple pending sandboxes; specify sandbox_id",
));
}
Ok(Some(id))
}

fn provisioning_condition() -> DriverCondition {
Expand Down
Loading
Loading