Skip to content

dev_container: Prefer canonical compose project on multi-match #7

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

Merged
antont merged 3 commits into from
Apr 19, 2026
Merged

dev_container: Prefer canonical compose project on multi-match #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
38620a3
dev_container: Add canonical-compose-project tiebreak test (RED)
antont Apr 18, 2026
7935dc1
dev_container: Prefer canonical compose project on multi-match (GREEN)
antont Apr 18, 2026
303ebca
dev_container: Add serde test for compose_project label rename
antont Apr 18, 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
197 changes: 187 additions & 10 deletions crates/dev_container/src/devcontainer_manifest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2015,14 +2015,60 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${PATH:-\3}/g' /etc/profile || true
}

async fn check_for_existing_container(&self) -> Result<Option<DockerPs>, DevContainerError> {
self.docker_client
.find_process_by_filters(
self.identifying_labels()
.iter()
.map(|(k, v)| format!("label={k}={v}"))
.collect(),
)
.await
let filters = self
.identifying_labels()
.iter()
.map(|(k, v)| format!("label={k}={v}"))
.collect();
match self.docker_client.find_process_by_filters(filters).await {
Ok(v) => Ok(v),
Err(DevContainerError::MultipleMatchingContainers(ids)) => {
self.pick_canonical_container(ids).await
}
Err(other) => Err(other),
}
}

/// Resolves a multi-match against the identifying labels by preferring
/// the container whose `com.docker.compose.project` label equals
/// `self.project_name()`. Falls through to `MultipleMatchingContainers`
/// when zero or ≥2 candidates claim the canonical project — users then
/// have to clean up the duplicate state themselves, per #54068.
///
/// Handles the common upgrade path where a Zed install from before the
/// compose-project-name fix (PR #6) left behind a container under the
/// legacy `safe_id_lower(devcontainer.name)` project, now colliding at
/// the label layer with a new container under the canonical project.
async fn pick_canonical_container(
&self,
ids: Vec<String>,
) -> Result<Option<DockerPs>, DevContainerError> {
let canonical_project = self.project_name();
let mut canonical: Option<String> = None;
let mut others: Vec<String> = Vec::new();
for id in &ids {
let inspect = self.docker_client.inspect(id).await?;
if inspect.config.labels.compose_project.as_deref() == Some(canonical_project.as_str())
{
if canonical.is_some() {
return Err(DevContainerError::MultipleMatchingContainers(ids));
}
canonical = Some(id.clone());
} else {
others.push(id.clone());
}
}
match canonical {
Some(id) => {
log::warn!(
"Multiple containers match dev container labels; reusing `{id}` under \
compose project `{canonical_project}`, ignoring legacy duplicate(s): {}",
others.join(", ")
);
Ok(Some(DockerPs { id }))
}
None => Err(DevContainerError::MultipleMatchingContainers(ids)),
}
}

fn project_name(&self) -> String {
Expand Down Expand Up @@ -2672,6 +2718,7 @@ mod test {
config: DockerInspectConfig {
labels: DockerConfigLabels {
metadata: Some(vec![metadata]),
compose_project: None,
},
image_user: None,
env: Vec::new(),
Expand Down Expand Up @@ -2700,6 +2747,7 @@ mod test {
config: DockerInspectConfig {
labels: DockerConfigLabels {
metadata: Some(vec![metadata]),
compose_project: None,
},
image_user: None,
env: Vec::new(),
Expand Down Expand Up @@ -2753,7 +2801,10 @@ mod test {
image: DockerInspect {
id: "mcr.microsoft.com/devcontainers/base:ubuntu".to_string(),
config: DockerInspectConfig {
labels: DockerConfigLabels { metadata: None },
labels: DockerConfigLabels {
metadata: None,
compose_project: None,
},
image_user: None,
env: Vec::new(),
},
Expand Down Expand Up @@ -4975,15 +5026,114 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de
test_dependencies
.docker
.set_duplicate_container_ids(vec!["abc123".to_string(), "def456".to_string()]);
// Both candidates claim non-canonical compose projects, so the
// tiebreak introduced above falls through to the safety-net error.
test_dependencies.docker.add_inspect_override(
"abc123".to_string(),
docker_inspect_with_compose_project("abc123", "non_canonical_a"),
);
test_dependencies.docker.add_inspect_override(
"def456".to_string(),
docker_inspect_with_compose_project("def456", "non_canonical_b"),
);

let result = devcontainer_manifest.check_for_existing_devcontainer().await;
let result = devcontainer_manifest
.check_for_existing_devcontainer()
.await;

let Err(DevContainerError::MultipleMatchingContainers(ids)) = result else {
panic!("expected MultipleMatchingContainers, got {result:?}");
};
assert_eq!(ids, vec!["abc123".to_string(), "def456".to_string()]);
}

#[cfg(not(target_os = "windows"))]
fn docker_inspect_with_compose_project(id: &str, compose_project: &str) -> DockerInspect {
DockerInspect {
id: id.to_string(),
config: DockerInspectConfig {
labels: DockerConfigLabels {
metadata: None,
compose_project: Some(compose_project.to_string()),
},
image_user: None,
env: Vec::new(),
},
mounts: None,
state: None,
}
}

#[cfg(not(target_os = "windows"))]
#[gpui::test]
async fn check_for_existing_container_prefers_canonical_compose_project(
cx: &mut TestAppContext,
) {
// Upgrade scenario: a pre-existing Zed container under the legacy
// `safe_id_lower(name)` compose project collides at the label layer
// with a container under the new CLI-matching project. Labels +
// project-name derivation (PR #6) mean one of the candidates lives
// under the canonical `project_name()`; prefer that one and treat the
// other as an orphan to ignore.
cx.executor().allow_parking();
let (test_dependencies, devcontainer_manifest) =
init_default_devcontainer_manifest(cx, r#"{"name": "Rust and PostgreSQL"}"#)
.await
.unwrap();
test_dependencies
.docker
.set_duplicate_container_ids(vec!["canonical_id".to_string(), "legacy_id".to_string()]);
// `project_name()` for TEST_PROJECT_PATH (`/path/to/local/project`)
// resolves to `project_devcontainer` under the PR #6 derivation.
test_dependencies.docker.add_inspect_override(
"canonical_id".to_string(),
docker_inspect_with_compose_project("canonical_id", "project_devcontainer"),
);
test_dependencies.docker.add_inspect_override(
"legacy_id".to_string(),
docker_inspect_with_compose_project("legacy_id", "rust_and_postgresql"),
);

let result = devcontainer_manifest.check_for_existing_container().await;

let Ok(Some(docker_ps)) = result else {
panic!("expected Ok(Some(canonical)), got {result:?}");
};
assert_eq!(docker_ps.id, "canonical_id".to_string());
}

#[cfg(not(target_os = "windows"))]
#[gpui::test]
async fn check_for_existing_container_errors_when_none_canonical(cx: &mut TestAppContext) {
// When none of the multi-match candidates claim the canonical compose
// project, fall through to the safety net from #54068 so the user
// must resolve the ambiguity manually — the tiebreak must not pick
// an arbitrary container.
cx.executor().allow_parking();
let (test_dependencies, devcontainer_manifest) =
init_default_devcontainer_manifest(cx, r#"{"image": "image"}"#)
.await
.unwrap();
test_dependencies
.docker
.set_duplicate_container_ids(vec!["foo_id".to_string(), "bar_id".to_string()]);
test_dependencies.docker.add_inspect_override(
"foo_id".to_string(),
docker_inspect_with_compose_project("foo_id", "foo_project"),
);
test_dependencies.docker.add_inspect_override(
"bar_id".to_string(),
docker_inspect_with_compose_project("bar_id", "bar_project"),
);

let result = devcontainer_manifest.check_for_existing_container().await;

let Err(DevContainerError::MultipleMatchingContainers(ids)) = result else {
panic!("expected MultipleMatchingContainers, got {result:?}");
};
assert_eq!(ids, vec!["foo_id".to_string(), "bar_id".to_string()]);
}

#[test]
fn test_aliases_dockerfile_with_pre_existing_aliases_for_build() {}

Expand All @@ -5009,6 +5159,10 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de
/// `MultipleMatchingContainers` with these IDs. Used to exercise the
/// duplicate-container error path.
duplicate_container_ids: Mutex<Option<Vec<String>>>,
/// Per-ID inspect responses consulted before the hardcoded pattern
/// matches in `inspect`. Lets tests that exercise multi-match recovery
/// control the `com.docker.compose.project` label of each candidate.
inspect_overrides: Mutex<HashMap<String, DockerInspect>>,
}

impl FakeDocker {
Expand All @@ -5018,6 +5172,7 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de
has_buildx: true,
exec_commands_recorded: Mutex::new(Vec::new()),
duplicate_container_ids: Mutex::new(None),
inspect_overrides: Mutex::new(HashMap::new()),
}
}
#[cfg(not(target_os = "windows"))]
Expand All @@ -5031,11 +5186,27 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de
.lock()
.expect("should be available") = Some(ids);
}
#[cfg(not(target_os = "windows"))]
fn add_inspect_override(&self, id: String, inspect: DockerInspect) {
self.inspect_overrides
.lock()
.expect("should be available")
.insert(id, inspect);
}
}

#[async_trait]
impl DockerClient for FakeDocker {
async fn inspect(&self, id: &String) -> Result<DockerInspect, DevContainerError> {
if let Some(inspect) = self
.inspect_overrides
.lock()
.expect("should be available")
.get(id)
.cloned()
{
return Ok(inspect);
}
if id == "mcr.microsoft.com/devcontainers/typescript-node:1-18-bookworm" {
return Ok(DockerInspect {
id: "sha256:610e6cfca95280188b021774f8cf69dd6f49bdb6eebc34c5ee2010f4d51cc104"
Expand All @@ -5046,6 +5217,7 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de
"remoteUser".to_string(),
Value::String("node".to_string()),
)])]),
compose_project: None,
},
env: Vec::new(),
image_user: Some("root".to_string()),
Expand All @@ -5064,6 +5236,7 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de
"remoteUser".to_string(),
Value::String("vscode".to_string()),
)])]),
compose_project: None,
},
image_user: Some("root".to_string()),
env: Vec::new(),
Expand All @@ -5082,6 +5255,7 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de
"remoteUser".to_string(),
Value::String("node".to_string()),
)])]),
compose_project: None,
},
image_user: Some("root".to_string()),
env: vec!["PATH=/initial/path".to_string()],
Expand All @@ -5100,6 +5274,7 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de
"remoteUser".to_string(),
Value::String("node".to_string()),
)])]),
compose_project: None,
},
image_user: Some("root".to_string()),
env: vec!["PATH=/initial/path".to_string()],
Expand All @@ -5121,6 +5296,7 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de
"remoteUser".to_string(),
Value::String("vscode".to_string()),
)])]),
compose_project: None,
},
image_user: Some("root".to_string()),
env: Vec::new(),
Expand All @@ -5139,6 +5315,7 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de
"remoteUser".to_string(),
Value::String("node".to_string()),
)])]),
compose_project: None,
},
env: Vec::new(),
image_user: Some("root".to_string()),
Expand Down
48 changes: 44 additions & 4 deletions crates/dev_container/src/docker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ pub(crate) struct DockerConfigLabels {
deserialize_with = "deserialize_metadata"
)]
pub(crate) metadata: Option<Vec<HashMap<String, serde_json_lenient::Value>>>,
#[serde(default, rename = "com.docker.compose.project")]
pub(crate) compose_project: Option<String>,
}

#[derive(Debug, Clone, Deserialize, Serialize, Eq, PartialEq)]
Expand Down Expand Up @@ -590,7 +592,10 @@ mod test {
#[test]
fn should_parse_simple_env_var() {
let config = super::DockerInspectConfig {
labels: super::DockerConfigLabels { metadata: None },
labels: super::DockerConfigLabels {
metadata: None,
compose_project: None,
},
image_user: None,
env: vec!["KEY=value".to_string()],
};
Expand All @@ -602,7 +607,10 @@ mod test {
#[test]
fn should_parse_env_var_with_equals_in_value() {
let config = super::DockerInspectConfig {
labels: super::DockerConfigLabels { metadata: None },
labels: super::DockerConfigLabels {
metadata: None,
compose_project: None,
},
image_user: None,
env: vec!["COMPLEX=key=val other>=1.0".to_string()],
};
Expand All @@ -614,7 +622,10 @@ mod test {
#[test]
fn should_parse_database_url_with_equals_in_query_string() {
let config = super::DockerInspectConfig {
labels: super::DockerConfigLabels { metadata: None },
labels: super::DockerConfigLabels {
metadata: None,
compose_project: None,
},
image_user: None,
env: vec![
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin".to_string(),
Expand All @@ -633,7 +644,10 @@ mod test {
#[test]
fn should_skip_env_var_without_equals() {
let config = super::DockerInspectConfig {
labels: super::DockerConfigLabels { metadata: None },
labels: super::DockerConfigLabels {
metadata: None,
compose_project: None,
},
image_user: None,
env: vec![
"VALID_KEY=valid_value".to_string(),
Expand Down Expand Up @@ -1144,4 +1158,30 @@ mod test {
let inspect: DockerInspect = serde_json_lenient::from_str(given_config).unwrap();
assert!(inspect.config.labels.metadata.is_none());
}

#[test]
fn should_deserialize_inspect_with_compose_project_label() {
// Guards the `com.docker.compose.project` serde rename on
// `DockerConfigLabels`, which the multi-match tiebreak in
// `check_for_existing_container` reads directly from real
// `docker inspect` output.
let given_config = r#"
{
"Id": "sha256:abc123",
"Config": {
"Labels": {
"com.docker.compose.project": "devcontainer-compose-test_devcontainer",
"devcontainer.local_folder": "/path/to/project"
}
}
}
"#;

let inspect: DockerInspect = serde_json_lenient::from_str(given_config).unwrap();
assert_eq!(
inspect.config.labels.compose_project.as_deref(),
Some("devcontainer-compose-test_devcontainer")
);
assert!(inspect.config.labels.metadata.is_none());
}
}