Skip to content

Refactor Docker Compose teardown into a shared helper#3564

Merged
lpcox merged 2 commits into
mainfrom
copilot/fix-duplicate-docker-compose-invocation
May 22, 2026
Merged

Refactor Docker Compose teardown into a shared helper#3564
lpcox merged 2 commits into
mainfrom
copilot/fix-duplicate-docker-compose-invocation

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 21, 2026

The container stop path and the startup-retry cleanup path each inlined the same docker compose down -v -t 1 invocation, leaving teardown behavior split across two files. This change consolidates that logic so future compose-down changes stay consistent while preserving the retry path’s best-effort semantics.

  • What changed

    • Added a shared runComposeDown(workDir, opts?) helper in src/container-cleanup.ts
    • Moved the common docker compose down -v -t 1 arguments and standard execa options into that helper
    • Kept stopContainers() on the default rejecting behavior
    • Updated the startup-retry teardown path in src/container-lifecycle.ts to call the same helper with reject: false
  • Behavior preserved

    • Normal container shutdown still fails loudly on compose-down errors
    • Retry cleanup before a second startup attempt remains best-effort and does not block the retry if teardown fails
  • Call sites after refactor

    await runComposeDown(workDir);
    await runComposeDown(workDir, { reject: false });
  • Why this matters

    • Compose teardown flags, timeout, env handling, and output routing now live in one place
    • Reduces drift risk between the primary shutdown path and retry cleanup path

Copilot AI changed the title [WIP] Refactor duplicated docker compose down invocation Refactor Docker Compose teardown into a shared helper May 21, 2026
Copilot finished work on behalf of lpcox May 21, 2026 23:56
Copilot AI requested a review from lpcox May 21, 2026 23:56
@lpcox lpcox marked this pull request as ready for review May 22, 2026 00:08
Copilot AI review requested due to automatic review settings May 22, 2026 00:09
@github-actions
Copy link
Copy Markdown
Contributor

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 95.98% 96.05% 📈 +0.07%
Statements 95.81% 95.88% 📈 +0.07%
Functions 97.37% 97.38% ➡️ +0.01%
Branches 89.42% 89.48% 📈 +0.06%
📁 Per-file Coverage Changes (3 files)
File Lines (Before → After) Statements (Before → After)
src/container-lifecycle.ts 93.9% → 94.0% (+0.04%) 94.2% → 94.3% (+0.04%)
src/container-cleanup.ts 90.1% → 90.2% (+0.11%) 90.2% → 90.3% (+0.11%)
src/config-writer.ts 83.0% → 85.6% (+2.54%) 83.0% → 85.6% (+2.54%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR consolidates Docker Compose teardown logic into a shared helper so that both the normal shutdown path and the startup-retry cleanup path use the same docker compose down -v -t 1 invocation and execa options, reducing drift while preserving best-effort retry cleanup behavior.

Changes:

  • Added runComposeDown(workDir, opts?) to centralize Compose teardown arguments and standard execa options.
  • Updated stopContainers() to call the shared helper (default rejecting behavior preserved).
  • Updated the startup retry cleanup path to call the helper with { reject: false } and adjusted unit tests accordingly.
Show a summary per file
File Description
src/container-cleanup.ts Introduces runComposeDown() and refactors stopContainers() to use it.
src/container-lifecycle.ts Uses runComposeDown(workDir, { reject: false }) for best-effort teardown before retrying startup.
src/docker-manager-lifecycle.test.ts Adds an assertion verifying retry cleanup calls Compose down with reject: false and the expected options.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 0

@github-actions
Copy link
Copy Markdown
Contributor

✅ Smoke Test: Claude Engine Validation

  • ✅ GitHub API: 2 PR entries confirmed in recent-prs.json
  • ✅ GitHub check: playwright_check=✅ PASS
  • ✅ File verify: smoke-test file exists

Result: PASS

💥 [THE END] — Illustrated by Smoke Claude

@github-actions
Copy link
Copy Markdown
Contributor

🔬 Smoke Test Results

Test Result
GitHub MCP connectivity ✅ PR listed: "docs(spec): add port information to /reflect endpoint sections"
GitHub.com HTTP connectivity ⚠️ Pre-step template vars not expanded (${{ steps.smoke-data.outputs... }})
File write/read ⚠️ Pre-step template vars not expanded

PR: "Refactor Docker Compose teardown into a shared helper" — author: @Copilot, assignees: @lpcox, @Copilot

Overall: PARTIAL — MCP ✅, pre-step data unavailable (template vars unexpanded in workflow)

📰 BREAKING: Report filed by Smoke Copilot

@github-actions
Copy link
Copy Markdown
Contributor

docs(spec): add port information to /reflect endpoint sections
fix(dind-probe): address review feedback from #3554
GitHub PR review: ✅ | SafeInputs GH: ❌ | Playwright: ✅ | Tavily: ❌
File/Bash: ✅ | Discussion: ❌ | Build: ✅
Overall status: FAIL

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • registry.npmjs.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "registry.npmjs.org"

See Network Configuration for more information.

🔮 The oracle has spoken through Smoke Codex

@github-actions
Copy link
Copy Markdown
Contributor

🏗️ Build Test Suite Results

Ecosystem Project Build/Install Tests Status
Bun elysia 1/1 passed ✅ PASS
Bun hono 1/1 passed ✅ PASS
C++ fmt N/A ✅ PASS
C++ json N/A ✅ PASS
Deno oak N/A 1/1 passed ✅ PASS
Deno std N/A 1/1 passed ✅ PASS
.NET hello-world N/A ✅ PASS
.NET json-parse N/A ✅ PASS
Go color 1/1 passed ✅ PASS
Go env 1/1 passed ✅ PASS
Go uuid 1/1 passed ✅ PASS
Java gson 1/1 passed ✅ PASS
Java caffeine 1/1 passed ✅ PASS
Node.js clsx passed ✅ PASS
Node.js execa passed ✅ PASS
Node.js p-limit passed ✅ PASS
Rust fd 1/1 passed ✅ PASS
Rust zoxide 1/1 passed ✅ PASS

Overall: 8/8 ecosystems passed — ✅ PASS

Generated by Build Test Suite for issue #3564 · ● 5M ·

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test Results — FAIL

Check Result
Redis PING ❌ Timeout (no response)
PostgreSQL pg_isready ❌ No response
PostgreSQL SELECT 1 ❌ Not reached

host.docker.internal is unreachable from this runner. Service containers may not be configured or attached to this workflow run.

🔌 Service connectivity validated by Smoke Services

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test: Copilot BYOK (Offline) Mode

Test Result
GitHub MCP connectivity ✅ Listed merged PR #3561 successfully
GitHub.com HTTP ⚠️ Pre-step template vars not expanded (skipped)
File write/read ⚠️ Pre-step template vars not expanded (skipped)
BYOK inference (agent → api-proxy → api.githubcopilot.com) ✅ Responding confirms path works

Running in BYOK offline mode (COPILOT_OFFLINE=true) via api-proxy → api.githubcopilot.com

Overall: PARTIAL PASS (inference ✅, pre-step data unavailable)

Author: @Copilot | Assignees: @lpcox, @Copilot

🔑 BYOK report filed by Smoke Copilot BYOK

@github-actions
Copy link
Copy Markdown
Contributor

Chroot Smoke Test Results

Runtime Host Version Chroot Version Match?
Python Python 3.12.13 Python 3.12.3
Node.js v24.15.0 v20.20.2
Go go1.22.12 go1.22.12

Overall: ❌ FAILED — Python and Node.js versions differ between host and chroot.

Tested by Smoke Chroot

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test: Gemini Engine Validation

  • GitHub MCP Testing: ❌ (Tools missing)
  • GitHub.com Connectivity: ❌ (SSL error 35)
  • File Writing Testing: ✅
  • Bash Tool Testing: ✅

Overall status: FAIL

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • localhost

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "localhost"

See Network Configuration for more information.

💎 Faceted by Smoke Gemini

@lpcox lpcox merged commit 2163cb8 into main May 22, 2026
66 of 71 checks passed
@lpcox lpcox deleted the copilot/fix-duplicate-docker-compose-invocation branch May 22, 2026 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Duplicate Code] docker compose down invocation duplicated in container-cleanup.ts and container-lifecycle.ts

3 participants