Repository navigation
fix(deploy): move compose_p into a tested lib; handle stale working_dir labels - #103
Merged
Merged
Conversation
…ir labels QA review of #101. compose_projects is now scripts/deployment/lib/compose-project.sh, checked out at job.workflow_sha and copied to $RUNNER_TEMP by the deploy, health-check and rollback jobs (replaces three inline "keep in sync" copies). - Stale working_dir: containers reused from an older checkout/manual `up` keep their original working_dir label, so a stack whose containers all came from another dir also named <stack> is now resolved by its normalised project name (with a ::warning::) instead of returning nothing. Previously a critical stack in that state failed the health check ("no containers") and triggered a rollback every deploy. - Foreign same-named project: still left alone, but now with a ::warning:: instead of a silent no-op down. - The fallback no longer passes the raw stack name to `docker compose -p` (rejected for uppercase/dots); the label lookup uses the normalised name. - health-check gains a compose-workflow checkout; rollback's checkout is now unconditional (its continue-on-error moves to the network step only). - New scripts/testing/test-compose-project.sh (17 cases, fake docker on PATH).
Reviewer's GuideThe PR replaces three duplicated inline Compose helper implementations with a pinned, tested shared library, and hardens project resolution against stale working_dir labels while warning before skipping foreign projects or using stale metadata. Sequence diagram for Compose project resolution and no-env commandssequenceDiagram
participant Job as Deploy or health-check or rollback job
participant Helper as compose-project.sh
participant Docker as Docker
participant Compose as docker compose
Job->>Helper: compose_p(stack, args)
Helper->>Helper: compose_projects(stack)
Helper->>Docker: docker ps -a by working_dir label
alt matching project labels found
Docker-->>Helper: project names
else no matching labels
Helper->>Helper: compose_normalize_name(stack)
Helper->>Docker: docker ps -a by normalized project label
alt all dirs end in stack name
Docker-->>Helper: stale project metadata
Helper-->>Job: warning and normalized project
else foreign project or no containers
Docker-->>Helper: foreign project or empty result
Helper-->>Job: warning or no project
end
end
Helper->>Compose: docker compose -p project args from /
Compose-->>Job: command result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/deployment/lib/compose-project.sh" line_range="53" />
<code_context>
+ [[ -n "$others" ]] || return 0
+
+ while IFS= read -r d; do
+ [[ "${d%/}" == */"$stack" ]] || stale=false
+ done <<<"$others"
+ if [[ "$stale" == true ]]; then
</code_context>
<issue_to_address>
**Foreign stacks get stopped**
When an unrelated Compose project has the normalized project name and a working directory whose basename matches the stack, `compose_projects` treats the basename match as proof that the project is a stale checkout, and `compose_p` can then run `down` against the unrelated project, stopping its containers.
Require a verified prior live-tree location or an explicit trusted-root policy before using this fallback.
</issue_to_address>
### Comment 2
<location path=".github/workflows/deploy.yml" line_range="348" />
<code_context>
+ # Scripts pinned to this workflow's commit, as in the prepare job. The
+ # target checkout below cleans the workspace, so the helper is copied out
+ # to $RUNNER_TEMP first (the shared-networks checkout re-fetches later).
+ - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+ if: steps.skip-gate.outputs.skipped == 'false'
+ with:
</code_context>
<issue_to_address>
**Rollback recovery gets skipped**
When a required compose-workflow checkout fails after deployment starts, the health-check checkout failure triggers rollback, while a rollback checkout failure prevents the plan and restore steps from running; GitHub skips those recovery steps and leaves the failed deployment unrestored.
Make the helper available to recovery without a required network fetch, or provide a fallback helper path.
Also at `.github/workflows/deploy.yml:829-833`, `.github/workflows/deploy.yml:1013`, `.github/workflows/deploy.yml:1022`, `.github/workflows/deploy.yml:1039`, `.github/workflows/deploy.yml:1081-1082`.
</issue_to_address>
### Comment 3
<location path="scripts/deployment/lib/compose-project.sh" line_range="38-39" />
<code_context>
- compose_projects() {
- local dir="${LIVE_REPO_PATH%/}/$1" p others
- p=$(docker ps -a --filter "label=com.docker.compose.project.working_dir=$dir" \
- --format '{{.Label "com.docker.compose.project"}}' 2>/dev/null \
- | sort -u) || p=""
- if [[ -z "$p" ]]; then
- # Captured first: piping docker into `grep -q` can SIGPIPE under pipefail.
</code_context>
<issue_to_address>
**Teardown failures go unnoticed**
When docker is unavailable or either `docker ps` lookup fails, `compose_projects` suppresses the lookup errors and converts failed queries to empty results, so `compose_p` returns success without running `down`; containers remain running and teardown callers see no failure.
Preserve and propagate lookup failures instead of treating them as successful empty results.
Also at `scripts/deployment/lib/compose-project.sh:47-50`.
</issue_to_address>
### Comment 4
<location path="scripts/deployment/lib/compose-project.sh" line_range="47" />
<code_context>
+
+ name=$(compose_normalize_name "$stack")
+ [[ -n "$name" ]] || return 0
+ others=$(docker ps -a --filter "label=com.docker.compose.project=$name" \
+ --format '{{.Label "com.docker.compose.project.working_dir"}}' 2>/dev/null \
+ | sort -u) || others=""
</code_context>
<issue_to_address>
**Running stacks appear empty**
When containers have stale `working_dir` labels and their project name comes from a custom Compose `name:` or `COMPOSE_PROJECT_NAME`, `compose_projects` falls back to searching only for the normalized stack name, so it misses those containers; `compose_p` skips Compose, health checks report no containers, and teardown leaves the project running.
Resolve the project name for stale-directory containers instead of limiting the fallback to the normalized stack name.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 4 findings to address first, and if project-label resolution or the pinned helper checkout is wrong, deployment teardown or rollback could target the wrong Compose project, causing a production outage or leaving a stack unrestored. Reverting prevents future occurrences but does not restore containers or undo an outage that already happened.
Blocking findings: scripts/deployment/lib/compose-project.sh:53, .github/workflows/deploy.yml:348, scripts/deployment/lib/compose-project.sh:39, scripts/deployment/lib/compose-project.sh:47
| [[ -n "$others" ]] || return 0 | ||
|
|
||
| while IFS= read -r d; do | ||
| [[ "${d%/}" == */"$stack" ]] || stale=false |
There was a problem hiding this comment.
🟠 High · Foreign stacks get stopped
When an unrelated Compose project has the normalized project name and a working directory whose basename matches the stack, compose_projects treats the basename match as proof that the project is a stale checkout, and compose_p can then run down against the unrelated project, stopping its containers.
Require a verified prior live-tree location or an explicit trusted-root policy before using this fallback.
Prompt for AI agents
In `scripts/deployment/lib/compose-project.sh` at line 53:
**Foreign stacks get stopped**
When an unrelated Compose project has the normalized project name and a working directory whose basename matches the stack, `compose_projects` treats the basename match as proof that the project is a stale checkout, and `compose_p` can then run `down` against the unrelated project, stopping its containers.
Require a verified prior live-tree location or an explicit trusted-root policy before using this fallback.| # Scripts pinned to this workflow's commit, as in the prepare job. The | ||
| # target checkout below cleans the workspace, so the helper is copied out | ||
| # to $RUNNER_TEMP first (the shared-networks checkout re-fetches later). | ||
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 |
There was a problem hiding this comment.
🟠 High · Rollback recovery gets skipped
When a required compose-workflow checkout fails after deployment starts, the health-check checkout failure triggers rollback, while a rollback checkout failure prevents the plan and restore steps from running; GitHub skips those recovery steps and leaves the failed deployment unrestored.
Make the helper available to recovery without a required network fetch, or provide a fallback helper path.
Also at .github/workflows/deploy.yml:829-833, .github/workflows/deploy.yml:1013, .github/workflows/deploy.yml:1022, .github/workflows/deploy.yml:1039, .github/workflows/deploy.yml:1081-1082.
Prompt for AI agents
In `.github/workflows/deploy.yml` at line 348:
**Rollback recovery gets skipped**
When a required compose-workflow checkout fails after deployment starts, the health-check checkout failure triggers rollback, while a rollback checkout failure prevents the plan and restore steps from running; GitHub skips those recovery steps and leaves the failed deployment unrestored.
Make the helper available to recovery without a required network fetch, or provide a fallback helper path.
Also at `.github/workflows/deploy.yml:829-833`, `.github/workflows/deploy.yml:1013`, `.github/workflows/deploy.yml:1022`, `.github/workflows/deploy.yml:1039`, `.github/workflows/deploy.yml:1081-1082`.
Comment on lines
+38
to
+39
| --format '{{.Label "com.docker.compose.project"}}' 2>/dev/null \ | ||
| | sort -u) || p="" |
There was a problem hiding this comment.
🟡 Medium · Teardown failures go unnoticed
When docker is unavailable or either docker ps lookup fails, compose_projects suppresses the lookup errors and converts failed queries to empty results, so compose_p returns success without running down; containers remain running and teardown callers see no failure.
Preserve and propagate lookup failures instead of treating them as successful empty results.
Also at scripts/deployment/lib/compose-project.sh:47-50.
Prompt for AI agents
In `scripts/deployment/lib/compose-project.sh` at lines 38-39:
**Teardown failures go unnoticed**
When docker is unavailable or either `docker ps` lookup fails, `compose_projects` suppresses the lookup errors and converts failed queries to empty results, so `compose_p` returns success without running `down`; containers remain running and teardown callers see no failure.
Preserve and propagate lookup failures instead of treating them as successful empty results.
Also at `scripts/deployment/lib/compose-project.sh:47-50`.|
|
||
| name=$(compose_normalize_name "$stack") | ||
| [[ -n "$name" ]] || return 0 | ||
| others=$(docker ps -a --filter "label=com.docker.compose.project=$name" \ |
There was a problem hiding this comment.
🟠 High · Running stacks appear empty
When containers have stale working_dir labels and their project name comes from a custom Compose name: or COMPOSE_PROJECT_NAME, compose_projects falls back to searching only for the normalized stack name, so it misses those containers; compose_p skips Compose, health checks report no containers, and teardown leaves the project running.
Resolve the project name for stale-directory containers instead of limiting the fallback to the normalized stack name.
Prompt for AI agents
In `scripts/deployment/lib/compose-project.sh` at line 47:
**Running stacks appear empty**
When containers have stale `working_dir` labels and their project name comes from a custom Compose `name:` or `COMPOSE_PROJECT_NAME`, `compose_projects` falls back to searching only for the normalized stack name, so it misses those containers; `compose_p` skips Compose, health checks report no containers, and teardown leaves the project running.
Resolve the project name for stale-directory containers instead of limiting the fallback to the normalized stack name.
owine
added a commit
that referenced
this pull request
Oct 10, 2026
) Sourcery review of #103. compose_projects swallowed docker ps failures (|| p=""), so a broken daemon made compose_p return 0 without running down: teardowns and failure cleanups looked successful while the containers kept running. Both lookups now capture docker's status before sorting (no reliance on the caller's pipefail), warn and return 1; compose_p captures the lookup result instead of reading a process substitution (whose exit status is lost) and returns 1 without calling compose. docker's own stderr is no longer discarded. Every call site already handles non-zero: teardowns log 'down failed', the health check reports 'docker compose ps failed', diagnostics use || true. Tests: 3 new cases (20/20); all 3 fail against the previous helper.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the QA review of #101. Three findings, fixed together.
Changes
compose_projects/compose_pmove toscripts/deployment/lib/compose-project.sh.deploy,health-checkandrollbackeach check out compose-workflow atjob.workflow_shaandinstallit to$RUNNER_TEMP/compose-project.sh, so the existingsourcelines are unchanged. This replaces three inline heredocs held together by "keep in sync" comments, and it's now shellcheck-able and unit-tested.deploy: the new checkout runs before the target checkout, whichgit cleans the workspace. The shared-networks checkout still re-fetches later.health-check: gains a checkout.rollback: the checkout is now unconditional and required.continue-on-errorstays on Ensure shared networks only. The runner gets this job from GitHub, so a git fetch from GitHub failing at this point is a narrow window.working_dirlabels. Reusing a container never refreshes itsworking_dirlabel, so a stack last brought up from an older checkout or a manualupwas invisible to the label lookup. A critical stack in that state failed the health check ("no containers") and rolled back on every deploy. Now, if every container of the stack's normalised project name came from a dir also named<stack>, that project is used, with a::warning::saying to recreate from the live tree.::warning::instead of a silentdownthat does nothing.docker compose -p <raw stack name>(rejected for uppercase or dots); the label lookup uses Compose's normalisation (compose_normalize_name).Verification
scripts/testing/test-compose-project.sh: 17/17, using a fakedockeron PATH. Covers name normalisation, working_dir match, top-levelname:, multiple projects, stale (used + warned), foreign and mixed (skipped + warned), andcompose_pper-project calls, no-op and rc propagation.yamllint --strictis clean. actionlint reports only the known, CI-ignoredjob.workflow_shaitems (now 6, up from 4, from the two new checkouts).docsandnonexistent-stackresolve to nothing. No stale labels exist there today, so this is hardening, not a live incident.Known limitation
The stale-vs-foreign check is by dir basename. A retired dockge running from
/opt/dockgealongside a repo stack nameddockgewould count as stale.has-dockgeis false in every caller and dockge isn't a repo stack, so I left that unhandled.Summary by Sourcery
Centralize and harden Compose project resolution across deployment jobs to recover stale labels safely and avoid acting on foreign projects.
Bug Fixes:
Enhancements:
Deployment:
Documentation:
Tests: