Skip to content

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

Merged
antont merged 3 commits into
fix-compose-project-name-derivationfrom
prefer-canonical-compose-project
Apr 19, 2026
Merged

dev_container: Prefer canonical compose project on multi-match#7
antont merged 3 commits into
fix-compose-project-name-derivationfrom
prefer-canonical-compose-project

Conversation

@antont

@antont antont commented Apr 18, 2026

Copy link
Copy Markdown
Owner

Stacking

upstream/main
  └─ fix-docker-ps-multi-container        (PR #54068 — detection, unchanged)
     └─ fix-compose-project-name-derivation (PR #6 — derivation)
        └─ prefer-canonical-compose-project  (this PR — tiebreak)

Each PR is separately reviewable. The detection layer from zed-industries#54068 stays byte-identical (parse_find_process_output, the MultipleMatchingContainers variant, its Display, the pass-through arm in start_dev_container_with_config). The tiebreak is a new match arm inside check_for_existing_container, backed by a new pick_canonical_container helper.

How it works

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),
}

pick_canonical_container inspects each id, partitions into {canonical, others} by com.docker.compose.project, returns Ok(Some(canonical)) on unique canonical, falls through to MultipleMatchingContainers(ids) otherwise. Log message on recovery names both the canonical id and the orphans.

Prerequisite: DockerConfigLabels gained an Option<String> compose_project field serde-renamed to com.docker.compose.project. All existing inspect struct literals updated with compose_project: None.

Test plan

Live reproduction TODO (defer to approval): against /Users/antont/src/devcontainer-compose-test, manually create a legacy-named container under compose_duplicate_repro and a CLI-named container under devcontainer-compose-test_devcontainer, then open in Zed — expect the canonical one reused and the orphan id named in the log.

Release Notes:

  • Fixed Multiple dev container matches error when a workspace has two containers with identical dev container labels but different compose projects (e.g. one left by the devcontainer CLI and one by Zed's Rust implementation). Zed now reuses the container under the canonical ${folder}_devcontainer compose project and logs the orphan; two non-canonical matches still surface the error.

antont and others added 3 commits April 18, 2026 18:36
Scaffolds a test for the multi-match upgrade scenario where one of the
duplicate containers lives under the canonical (reference-CLI-matching)
compose project and the other under a legacy name. The tiebreak logic
that would prefer the canonical container does not exist yet, so
`check_for_existing_container_prefers_canonical_compose_project` fails
with `MultipleMatchingContainers`. The companion safety-net test
(`check_for_existing_container_errors_when_none_canonical`) passes
already and guards against regressions in the zero-canonical case.

This commit is deliberately test-only on the logic layer; the prod-code
changes are purely structural and required to let the tests compile:

- Extend `DockerConfigLabels` with `compose_project: Option<String>`
  serde-renamed to `com.docker.compose.project`. All existing inspect
  struct literals (3 in docker.rs tests, 8 in devcontainer_manifest.rs
  tests) get `compose_project: None`.
- Add `FakeDocker::inspect_overrides: Mutex<HashMap<String, DockerInspect>>`
  and `add_inspect_override` setter. `FakeDocker::inspect` consults
  overrides before its hardcoded pattern matching, so tests can control
  each multi-match candidate's `com.docker.compose.project` label
  without disturbing any existing test fixture.

RED output (`cargo test -p dev_container --lib check_for_existing_container`):

    running 3 tests
    test devcontainer_manifest::test::check_for_existing_container_errors_when_multiple_match ... ok
    test devcontainer_manifest::test::check_for_existing_container_errors_when_none_canonical ... ok
    test devcontainer_manifest::test::check_for_existing_container_prefers_canonical_compose_project ... FAILED

    ---- devcontainer_manifest::test::check_for_existing_container_prefers_canonical_compose_project stdout ----
    thread 'devcontainer_manifest::test::check_for_existing_container_prefers_canonical_compose_project' panicked at crates/dev_container/src/devcontainer_manifest.rs:5043:13:
    expected Ok(Some(canonical)), got Err(MultipleMatchingContainers(["canonical_id", "legacy_id"]))

    test result: FAILED. 2 passed; 1 failed

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Introduce `pick_canonical_container`, a thin recovery layer above
`find_process_by_filters`. When the multi-match detection from zed-industries#54068
trips, inspect each candidate and prefer the one whose
`com.docker.compose.project` label equals `self.project_name()`. Zero
or ≥2 canonical matches still fall through to the
`MultipleMatchingContainers` error, preserving the safety net; only the
unambiguous-recovery case is intercepted.

This makes the compose-project-name fix from PR #6 a transparent
upgrade: users migrating past v0.231.x to v0.232+ on an existing
Zed-managed project had one container under the legacy
`safe_id_lower(name)` project. After the derivation change new Zed
creates one under the canonical `${folder}_devcontainer`. Without the
tiebreak, the label-based lookup sees both and errors out; with it,
Zed reuses the canonical one and logs the orphan's id so users can
clean up on their own schedule.

The multi-match detection itself (`parse_find_process_output`,
`MultipleMatchingContainers`, its Display impl, the pass-through arm
in `start_dev_container_with_config`) stays byte-identical with
zed-industries#54068.

Updates the pre-existing
`check_for_existing_container_errors_when_multiple_match` test to
supply non-canonical inspect overrides, so it still exercises the
safety-net fall-through now that the path inspects each candidate.

    running 3 tests
    test devcontainer_manifest::test::check_for_existing_container_errors_when_multiple_match ... ok
    test devcontainer_manifest::test::check_for_existing_container_errors_when_none_canonical ... ok
    test devcontainer_manifest::test::check_for_existing_container_prefers_canonical_compose_project ... ok

    test result: ok. 78 passed; 0 failed (full crate)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Locks in the `com.docker.compose.project` serde rename on
`DockerConfigLabels`. The multi-match tiebreak in
`check_for_existing_container` reads this field from real
`docker inspect` output, but every tiebreak test plants the value via
`FakeDocker::add_inspect_override` — nothing exercises the
deserialization path. Without this test, breaking the rename fails
closed silently (tiebreak sees `None`, falls through to
`MultipleMatchingContainers`) and no existing test catches it.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@antont
antont merged commit 303ebca into fix-compose-project-name-derivation Apr 19, 2026
28 checks passed
@antont

antont commented Apr 19, 2026

Copy link
Copy Markdown
Owner Author

Auto-merged by fast-forwarding onto fix-compose-project-name-derivation (PR #6's branch). The three commits (RED test, GREEN tiebreak, serde coverage) are now part of #6. See #6 for the combined review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant