fix(deploy): add timeout to whole-tree rollback up; correct its comment - #83
Merged
owine merged 2 commits intoAug 25, 2026
Conversation
Reviewer's GuideThe whole-tree rollback now times out each docker compose startup independently, reports actionable manual recovery guidance, and continues processing later stacks instead of allowing one hung container to cancel the job. The step comments now accurately describe its fleet-wide scope and the consistency invariant that scope preserves. Sequence diagram for resilient whole-tree rollbacksequenceDiagram
participant Workflow
participant Rollback as Rollback step
participant Timeout as timeout
participant Compose as docker compose
participant Next as Next stack
Workflow->>Rollback: Redeploy stacks at previous SHA
loop Each existing or removed stack
Rollback->>Timeout: timeout SERVICE_STARTUP_TIMEOUT
Timeout->>Compose: docker compose up -d --quiet-pull --wait --remove-orphans
alt Startup succeeds
Compose-->>Timeout: success
Timeout-->>Rollback: continue
else Startup times out or fails
Compose-->>Timeout: failure
Timeout-->>Rollback: non-zero status
Rollback-->>Workflow: warning with manual recovery command
Rollback->>Next: continue to next stack
end
end
Flow diagram for fleet-wide rollback scopeflowchart TD
A[Whole-tree rollback] --> B[existing_stacks]
A --> C[removed stacks]
B --> D[All discovered stacks minus new stacks]
C --> E[Stacks deleted by the deploy]
D --> F[Recreate fleet from previous SHA]
E --> F
F --> G[Restore stacks pinned by prior per-stack rollback]
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 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path=".github/workflows/deploy.yml" line_range="1025" />
<code_context>
+ op run --no-masking --env-file="$LIVE_REPO_PATH/compose.env" -- \
docker compose up -d --quiet-pull --wait --remove-orphans \
- || echo "::warning::rollback up failed for $stack"
+ || echo "::warning::rollback up failed for $stack; recover manually with: cd $LIVE_REPO_PATH/$stack && op run --no-masking --env-file=$LIVE_REPO_PATH/compose.env -- docker compose up -d --wait"
done
</code_context>
<issue_to_address>
**issue (bug_risk):** When `live-repo-path` contains whitespace, the manual recovery command in the warning is not shell-safe: `cd $LIVE_REPO_PATH/$stack` splits the path and `--env-file=$LIVE_REPO_PATH/compose.env` is also parsed incorrectly, so copying the command fails instead of recovering the stack.
**Triggers:** When a deployment uses a live repository path containing whitespace.
**Suggested fix:** Quote the displayed path components, for example `cd "$LIVE_REPO_PATH/$stack"` and `--env-file="$LIVE_REPO_PATH/compose.env"`.
```suggestion
|| echo "::warning::rollback up failed for $stack; recover manually with: cd \"$LIVE_REPO_PATH/$stack\" && op run --no-masking --env-file=\"$LIVE_REPO_PATH/compose.env\" -- docker compose up -d --wait"
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and the rollback docker compose up --wait is now forcibly stopped after SERVICE_STARTUP_TIMEOUT; if a valid stack needs longer, it can remain down while rollback continues to later stacks, causing a production outage that requires a manual rerun or recovery. Reverting removes the timeout, but it cannot undo any outage that already occurred.
Blocking findings: .github/workflows/deploy.yml:1025
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
owine
added a commit
that referenced
this pull request
Aug 25, 2026
The warning emitted when a per-stack rollback up fails prints a command for an operator to copy and paste. With an unquoted $LIVE_REPO_PATH, a deploy path containing whitespace produced a command that word-splits: cd /opt/my compose/termix && op run --env-file=/opt/my compose/compose.env so pasting it fails instead of recovering the stack. Since this is the one rollback path that leaves the job green, the hint is likely to be the operator's first action during an incident — it needs to work. Now emits quoted components, verified to parse as valid shell: cd "/opt/my compose/termix" && op run ... --env-file="/opt/my compose/compose.env" ... Found by Sourcery on the stacked PR #83; the same pattern was present here and is fixed in each PR separately.
Two pre-existing issues in `Redeploy stacks at previous SHA`, left untouched by the scoped-rollback work because that change deliberately did not modify this step's body. 1. Missing `timeout`. Every other `docker compose up` in this workflow is wrapped in `timeout "$SERVICE_STARTUP_TIMEOUT"`; this one was not. `--wait` blocks indefinitely on a container stuck in `starting` — exactly what a bad image produces — until the job's timeout-minutes cancels the whole job, stranding every stack after it with no further fallback. Verified with exec-able stubs: a 30s hang now aborts at the 2s budget, emits a warning, and the loop continues. The failure message also now carries the manual recovery command, since a failed rollback up leaves the job green. 2. Inaccurate comment. It claimed this step reverts "only the stacks this deploy actually touched" and that skipping untouched stacks "avoids needlessly recreating the whole fleet". Both are false: detect-stack-changes.sh:401 computes existing_stacks as (all discovered stacks - new stacks), so this loop covers the whole fleet on every run. The comment now says so, and records that the fleet-wide scope is load-bearing — it is what pulls a stack pinned by a prior per-stack rollback back into line with the tree.
Same fix as the per-stack hint on the base branch, applied to the whole-tree step's warning. An unquoted $LIVE_REPO_PATH produced a copy-paste command that word-splits on a deploy path containing whitespace, so pasting it fails instead of recovering the stack. Reported by Sourcery on this PR.
owine
force-pushed
the
fix/whole-tree-rollback-timeout-and-comment
branch
from
August 25, 2026 14:54
782aaf1 to
7e3b0d2
Compare
Owner
Author
|
Both Sourcery points addressed. 1. Unquoted paths in the recovery hint — fixed in 7e3b0d2. Good catch, and it applied more broadly than the diff showed: the same pattern was in the per-stack hint added by #82, which was outside this PR's diff so Sourcery could not see it. Fixed in each PR separately — a5ab6ac on the base branch, 7e3b0d2 here. Both hints now emit quoted components, verified to parse as valid shell against a path containing whitespace: 2. "A valid stack needing longer than The timeout does not stop containers. The command is The budget is the same one the deploy path already enforces. And the alternative is strictly worse. Before this commit, a container stuck in If the budget ever proves too tight in practice, |
owine
merged commit Aug 25, 2026
6fddd75
into
feat/scoped-rollback-and-image-quarantine
2 checks passed
owine
added a commit
that referenced
this pull request
Aug 25, 2026
#82) * docs: add scoped rollback and image quarantine design spec * docs: add scoped rollback and image quarantine implementation plan * feat(deploy): add rollback scope classifier Decides per-stack vs whole-tree rollback from the changed-file list. Uncertainty always resolves to whole-tree. * fix(deploy): close 3 unsafe-fallthrough gaps in rollback classifier Uncertainty must always resolve to whole-tree, never per-stack: a wrong whole-tree is merely wasteful, a wrong per-stack leaves a broken deploy partially un-rolled-back. Three input shapes violated that asymmetry: - Non-string array elements (numbers, null, nested arrays) reached the unguarded split()/index() jq pipeline and crashed the script under set -euo pipefail before any output was written (rc=5, no output). - An empty-string element (e.g. from a trailing-newline git diff pipeline) vacuously satisfied the "no paths outside a stack dir" check, landing on the unsafe per-stack side instead of whole-tree. - A flag given as the final argument (no value) tripped set -e in the shift 2 parsing and exited 1 with no output, instead of falling through to whole-tree. Tighten both array guards to require every element be a non-empty string, make argument parsing tolerate a missing trailing value, and correct a comment that inaccurately described the root-level-file fallthrough behavior. Added 6 test cases (17 total, 11 original unchanged). * fix(deploy): rollback classifier code-quality review fixes Addresses 6 findings from code-quality review of 7c3d300: - Comment both empty-list guards explicitly as load-bearing vs cosmetic: deleting the changed-files-empty guard inverts the safe default (jq's filter is vacuously true over []), the stack-dirs-empty guard only buys a clearer reason string. - Close the mid-argv missing-value hole for both flags (--changed-files --stack-dirs '[...]' no longer exits 1) by checking $# instead of shift 2, which also removes the `shift; [[ $# -gt 0 ]] && shift` construct a maintainer could "simplify" back into the exact bug already fixed. Distinguishing "missing value" from "explicit empty string" value requires checking argument count, not ${2:-} content -- the latter can't tell unset from empty. - Rework the test harness so every case asserts an exit code via a new expect_case helper; expect_scope becomes a thin 2-flag wrapper so all existing call sites stay unchanged. Verified the new rc assertion can actually fail: temporarily injected `exit 3` on the per-stack success path, confirmed 3 cases went red, reverted. - Add coverage for the unknown-flag exit-1 path, the most opinionated behavior in the file (invocation errors fail loudly; malformed data degrades to whole-tree) and previously untested. - Extract the duplicated is_string_array predicate so tightening one guard can't accidentally miss its twin. - Use log_warning (not log_info) for genuine data anomalies, with a truncated echo of the offending input so an operator doesn't have to dig through the upstream step's output. 19 test cases total (11 original unchanged, 8 new). Uncertainty still always resolves to whole-tree, never the reverse. * feat(deploy): expose rollback_scope from prepare Wire classify-rollback-scope.sh into the prepare job and expose its result as a job output for the (not-yet-wired) rollback job to consume. Also set escape_json: false on the tj-actions/changed-files step. That input defaults to true, which backslash-escapes every quote in the JSON outputs (e.g. all_changed_files becomes [\"x\"] instead of ["x"]) -- invalid JSON that jq can't parse. Every consumer of these outputs in this job (detect-stack-changes.sh and the new classifier step) pipes them through jq, so the escaped form silently broke input validation: the classifier's strict is_string_array guard rejected the malformed value and fell back to whole-tree every time, with no error surfaced anywhere. * feat(deploy): emit failed_stacks from health-check * feat(deploy): scope rollback to failed stacks when safe Per-stack rollback runs only when the change set is confined to stack directories AND a culprit stack was identified. Every other case keeps the existing whole-tree reset. The governing principle for this job: in a recovery job, malformed input changes the SCOPE of the rollback, never whether one happens. `Resolve rollback plan` is the job's first step, so any hard failure there skips every subsequent step and no rollback runs at all — neither per-stack nor whole-tree — leaving production broken until a human intervenes. The tradeoff is therefore not "loud failure vs. silently wrong rollback" but "loud failure with production still down vs. whole-tree rollback with production restored". So every unusable input forces the conservative whole-tree path and raises a ::error:: plus a step-summary entry: recovery still runs, the regression still screams. That covers three classes of bad input, all validated in the plan step rather than at their point of use, because a list we cannot trust should keep us off the per-stack path entirely: - a stack list that is not an array of non-empty strings - NEW_STACKS specifically, since without it we cannot tell a new stack from an existing one - a culprit name failing the stack-name pattern. Degrading is also the safer security response: the whole-tree path never uses these names (it resets the tree and iterates prepare's own existing/removed lists, a different producer), so it discards the poisoned name instead of acting on it. The per-stack loop also tolerates an unrevertable culprit. `git checkout <sha> -- <stack>/` for a directory absent at that SHA is an unmatched pathspec and exits non-zero, which under `set -e` aborted the whole loop and stranded every remaining culprit. It now warns and continues, as does its defence-in-depth name check. * feat(deploy): report rollback scope and culprits in notification * docs: record no-live-test decision and residual risk Task 7 live validation declined; per-stack path ships enabled. Documents what remains unverified and the accepted swallowed-failure risk. * fix(deploy): close 4 gaps in the per-stack rollback path C1 — culprits were never intersected with this deploy's change set. health-check iterates the *critical* stacks (detected from labels across all discovered stacks), not the changed ones, so failed_stacks could name a stack that is byte-identical at PREVIOUS_SHA and TARGET_REF. A commit touching only termix/ that knocked swag over produced mode=per-stack culprits=[swag]; the `git checkout $PREVIOUS_SHA -- swag/` was a no-op, swag stayed broken, the ::warning:: was swallowed, the job went green — and termix, the only thing that actually changed, was never reverted. Whole-tree would have caught it. prepare now emits `changed_stacks` (first path segment of every changed file that names a known stack dir) and `Resolve rollback plan` requires every culprit to be a member. A culprit outside that set means the failure cannot be attributed to a stack this deploy touched, so the plan degrades to whole-tree. Note this deliberately does NOT use `existing_stacks`: detect-stack-changes.sh defines it as (all discovered stacks - new stacks), so it names the whole fleet on every run and the check would be vacuous. I2a — the skip-gate left the live tree dirty indefinitely. `git checkout <sha> -- <dir>/` moves index and worktree but not HEAD, so after a per-stack rollback HEAD still equals TARGET_REF with a dirty tree. The skip-gate's SHA comparison saw equality and set skipped=true, so the `git reset --hard "$TARGET_REF"` never ran. "Re-run failed jobs" at the same target-ref reported a green "Repository already at target commit" while a stack sat pinned at the previous SHA. The gate now checks `git status --porcelain` ahead of the SHA comparison and forces a deploy on a dirty tree. This is what makes the deliberate absence of a cleanup step in the rollback job safe — the dirt survives for an operator to inspect, and is cleared by the next deploy's reset rather than by the recovery job. I3 — the per-stack `up` had no timeout, unlike every deploy-path `up`. `docker compose up --wait` waits indefinitely on a container stuck in `starting`, which is exactly what a bad image produces. The job's timeout-minutes then cancelled the run, stranding every remaining culprit un-rolled-back — and a cancelled job takes no whole-tree fallback. Wrapped in `timeout "$SERVICE_STARTUP_TIMEOUT"` to match the deploy path. Minors: - M8: the classifier wrapper's `jq -cn --argjson` aborted `prepare` on a malformed upstream list, where the script it calls would have degraded. Falls back to `[]`, which reaches the script's dirs_count guard and yields whole-tree — the same disposition the script itself would pick. - M9: the new-stack teardown branch skipped a missing compose file silently; now warns, matching the pre-existing whole-tree teardown step. - M10: the per-stack `up` failure warning now carries the manual recovery command, since this is the one path where the job still goes green. Every degradation added here routes to whole-tree; none exits non-zero. A recovery job must never abort and leave production down. Design doc §A4 rewrote: it asserted "no cleanup step is required ... no drift accumulates across runs", which was false — it did not account for the skip-gate, and it did not account for the re-`up` needed after the reset. Both dependencies are now stated explicitly, including the warning that the second one rests on `existing_stacks` naming the whole fleet. * fix(deploy): quote paths in the per-stack rollback recovery hint The warning emitted when a per-stack rollback up fails prints a command for an operator to copy and paste. With an unquoted $LIVE_REPO_PATH, a deploy path containing whitespace produced a command that word-splits: cd /opt/my compose/termix && op run --env-file=/opt/my compose/compose.env so pasting it fails instead of recovering the stack. Since this is the one rollback path that leaves the job green, the hint is likely to be the operator's first action during an incident — it needs to work. Now emits quoted components, verified to parse as valid shell: cd "/opt/my compose/termix" && op run ... --env-file="/opt/my compose/compose.env" ... Found by Sourcery on the stacked PR #83; the same pattern was present here and is fixed in each PR separately. * fix(deploy): add timeout to whole-tree rollback up; correct its comment (#83) * fix(deploy): add timeout to whole-tree rollback up; correct its comment Two pre-existing issues in `Redeploy stacks at previous SHA`, left untouched by the scoped-rollback work because that change deliberately did not modify this step's body. 1. Missing `timeout`. Every other `docker compose up` in this workflow is wrapped in `timeout "$SERVICE_STARTUP_TIMEOUT"`; this one was not. `--wait` blocks indefinitely on a container stuck in `starting` — exactly what a bad image produces — until the job's timeout-minutes cancels the whole job, stranding every stack after it with no further fallback. Verified with exec-able stubs: a 30s hang now aborts at the 2s budget, emits a warning, and the loop continues. The failure message also now carries the manual recovery command, since a failed rollback up leaves the job green. 2. Inaccurate comment. It claimed this step reverts "only the stacks this deploy actually touched" and that skipping untouched stacks "avoids needlessly recreating the whole fleet". Both are false: detect-stack-changes.sh:401 computes existing_stacks as (all discovered stacks - new stacks), so this loop covers the whole fleet on every run. The comment now says so, and records that the fleet-wide scope is load-bearing — it is what pulls a stack pinned by a prior per-stack rollback back into line with the tree. * fix(deploy): quote paths in the whole-tree rollback recovery hint Same fix as the per-stack hint on the base branch, applied to the whole-tree step's warning. An unquoted $LIVE_REPO_PATH produced a copy-paste command that word-splits on a deploy path containing whitespace, so pasting it fails instead of recovering the stack. Reported by Sourcery on this PR.
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.
Stacked on #82 — base is that branch, so this diff shows only its own commit. GitHub will retarget it to
mainwhen #82 merges.Two pre-existing issues in the whole-tree
Redeploy stacks at previous SHAstep. #82 deliberately left this step's body untouched (its own invariant), so they are split out here.1. Missing
timeouton theupEvery other
docker compose upin this workflow is wrapped intimeout "$SERVICE_STARTUP_TIMEOUT". This one was not.docker compose up --waitblocks indefinitely on a container stuck instarting— exactly what a bad image tends to produce. The job'stimeout-minutes: 15then cancels the whole job, so every stack after the hung one is never redeployed, and a cancelled job takes no further fallback. This is the rollback path, so the result is a half-recovered fleet during an incident.Verified with exec-able stubs on
PATH(a shell-function stub does not work here —timeoutexecsop, bypassing functions):The warning now also carries the manual recovery command, since a failed rollback
upleaves the job green.SERVICE_STARTUP_TIMEOUTis already in this job'senv:as of #82, so no new plumbing.2. The comment is factually wrong
It claimed:
Both halves are false.
detect-stack-changes.sh:401:existing_stacksis all discovered stacks minus new ones — the whole fleet, every run. Nothing is skipped and the fleet is recreated regardless.The new comment says so, and records why that matters: the fleet-wide scope is load-bearing, not an oversight. It is what brings a stack pinned by a prior per-stack rollback back into line with the tree. Narrowing
existing_stacksto the real change set would let such a stack drift — containers on the old image while the tree claims the new one — until it next changed.That dependency is the one open concern flagged in #82's review, and it was previously recorded only in the design doc. Now it is at the call site too.
Testing
yamllint --strictcleanactionlint— only the pre-existingjob.workflow_shawarningbash -non the extracted step body parsesdocker compose upsites in the file are nowtimeout+op runwrapped (was 4 of 5)Summary by Sourcery
Make whole-tree rollback resilient to stuck stack startups and document why it redeploys the entire fleet.
Bug Fixes:
Enhancements: