amtronics/platform-actions

Watch

Fork 0

fix: Wait for one-shots before a rolling swap #95

Merged

ahmad merged 1 commit from feature/94-wait-one-shots into main

2026-10-10 05:43:50 +00:00

ahmad commented

2026-10-10 05:31:50 +00:00

Owner

Issue

Closes #94

Problem

In rolling mode the swap runs with --no-deps. A new replica can therefore start while a migration it depends on is still running, or after that migration has failed. Without rolling, compose's service_completed_successfully prevents this. It blocks suhba/rihla#417.

Solution

Before any swap, every recreated container whose restart policy is no must exit 0. A timeout or non-zero exit fails the deploy with the one-shot's logs, and the old replicas are left untouched. The README states the rule, including that a long-running service must set a restart policy.

Review notes

Three behaviour cases: a failed migration (kill, checked against a mutant that ignores the exit code), a finished one, and a long-running service that is not waited for.
Tested on the VPS with a throwaway project: the migration runs once per deploy, the swap waits for it, and a failing migration stops the deploy before any new replica starts.
The alternatives were measured and rejected:
- rolling without --no-deps re-runs a migration that has already exited;
- up --wait reports a running one-shot as healthy.

Risks and trade-offs

In rolling mode, a non-rolled service with no restart policy (Docker's default is no) is now treated as a one-shot. If it is actually long-running, the deploy fails after health-timeout. No current consumer sets rolling-services with such a service: suhba-web's mail is always.

### Issue Closes #94 ### Problem In rolling mode the swap runs with `--no-deps`. A new replica can therefore start while a migration it depends on is still running, or after that migration has failed. Without rolling, compose's `service_completed_successfully` prevents this. It blocks suhba/rihla#417. ### Solution Before any swap, every recreated container whose restart policy is `no` must exit 0. A timeout or non-zero exit fails the deploy with the one-shot's logs, and the old replicas are left untouched. The README states the rule, including that a long-running service must set a `restart` policy. ### Review notes - Three behaviour cases: a failed migration (kill, checked against a mutant that ignores the exit code), a finished one, and a long-running service that is not waited for. - Tested on the VPS with a throwaway project: the migration runs once per deploy, the swap waits for it, and a failing migration stops the deploy before any new replica starts. - The alternatives were measured and rejected: - rolling without `--no-deps` re-runs a migration that has already exited; - `up --wait` reports a running one-shot as healthy. ### Risks and trade-offs In rolling mode, a non-rolled service with no `restart` policy (Docker's default is `no`) is now treated as a one-shot. If it is actually long-running, the deploy fails after `health-timeout`. No current consumer sets `rolling-services` with such a service: suhba-web's mail is `always`.

ahmad added 1 commit

2026-10-10 05:31:50 +00:00

fix: Wait for one-shot services before a rolling swap

PR / validate (pull_request) Successful in 30s

Details

406c972df1

The swap rolls with --no-deps, so a new replica could start while a
migration was still running, or after it had failed. Recreated
services whose restart policy is no now have to exit 0 first; a
failure stops the deploy with the old replicas untouched.

ahmad_bot requested changes

2026-10-10 05:39:21 +00:00

Dismissed

ahmad_bot left a comment

Verified on 406c972d: the wait sits inside the rolling block after OTHERS is recreated with --force-recreate --no-deps and before the first --scale, so a one-shot is always this deploy's container and the swap cannot start before it exits 0; a timeout or non-zero exit prints the container's logs and exits before any --scale or stop. OTHERS is built from config --services, so the non-rolling path is untouched (#94 AC3). Behaviour suite passes locally; the kill case fails against a mutant that ignores the exit code, and the long-running case fails against one that skips the restart-policy check. README states the rule. CI green.

Blocking:

PR title and commit subject are 53 chars; the bar is ≤ 50 and the squash commit takes the title. Example: fix: Wait for one-shots before a rolling swap (45).

Notes, not blocking:

No case covers the still-running branch: a mutant whose loop never waits (!= "exited" → never true) passes the whole suite, because the stub always answers exited. A case with State.Status running would need date stubbed, which the suite does not do yet, so this is a follow-up, not a fix here.
A migration with restart: on-failure is not waited for (policy is not no). The README rule is unambiguous, so fine; worth a word there if a consumer ever does that.

Verified on 406c972d: the wait sits inside the rolling block after `OTHERS` is recreated with `--force-recreate --no-deps` and before the first `--scale`, so a one-shot is always this deploy's container and the swap cannot start before it exits 0; a timeout or non-zero exit prints the container's logs and exits before any `--scale` or `stop`. `OTHERS` is built from `config --services`, so the non-rolling path is untouched (#94 AC3). Behaviour suite passes locally; the kill case fails against a mutant that ignores the exit code, and the long-running case fails against one that skips the restart-policy check. README states the rule. CI green. Blocking: - PR title and commit subject are 53 chars; the bar is ≤ 50 and the squash commit takes the title. Example: `fix: Wait for one-shots before a rolling swap` (45). Notes, not blocking: - No case covers the still-running branch: a mutant whose loop never waits (`!= "exited"` → never true) passes the whole suite, because the stub always answers `exited`. A case with `State.Status` `running` would need `date` stubbed, which the suite does not do yet, so this is a follow-up, not a fix here. - A migration with `restart: on-failure` is not waited for (policy is not `no`). The README rule is unambiguous, so fine; worth a word there if a consumer ever does that.

ahmad changed title from ~~fix: Wait for one-shot services before a rolling swap~~ to fix: Wait for one-shots before a rolling swap

2026-10-10 05:40:07 +00:00

ahmad force-pushed feature/94-wait-one-shots from 406c972df1

PR / validate (pull_request) Successful in 30s

Details

to 982bdba2ca

PR / validate (pull_request) Successful in 25s

Details

2026-10-10 05:40:08 +00:00

Compare

ahmad referenced this pull request

2026-10-10 05:40:58 +00:00

Cover a still-running one-shot in the rolling-swap tests #98

ahmad_bot approved these changes

2026-10-10 05:42:46 +00:00

ahmad_bot left a comment

Re-review of 982bdba2 against review 1703. The PR diff is byte-identical to the one reviewed on 406c972d; only the title and commit subject changed, both now 45 chars. CI green on this head. The two non-blocking notes (no still-running case, on-failure policies) stand as a follow-up.

Re-review of 982bdba2 against review 1703. The PR diff is byte-identical to the one reviewed on 406c972d; only the title and commit subject changed, both now 45 chars. CI green on this head. The two non-blocking notes (no still-running case, `on-failure` policies) stand as a follow-up.

ahmad merged commit a1086d18c2 into main

2026-10-10 05:43:50 +00:00

ahmad deleted branch feature/94-wait-one-shots

2026-10-10 05:43:50 +00:00

ahmad referenced this pull request from a commit

2026-10-10 05:43:50 +00:00

fix: Wait for one-shots before a rolling swap (#95)