Skip to content

fix(proxy): cancel restart policy before draining the old generation - #106

Merged
mhenrixon merged 1 commit into
dashfrom
fix/issue-105-handoff-restart-policy
Aug 3, 2026
Merged

fix(proxy): cancel restart policy before draining the old generation#106
mhenrixon merged 1 commit into
dashfrom
fix/issue-105-handoff-restart-policy

Conversation

@mhenrixon

Copy link
Copy Markdown
Collaborator

Closes #105

Problem

kamal proxy reboot on a host already on the port-holder architecture fails mid-handoff. Both generations run with --restart unless-stopped. drain makes the old proxy process exit on its own — unlike docker stop, that does not mark the container deliberately stopped, so Docker's restart policy resurrects it within its sub-second backoff. The un-forced docker container rm in handoff_generation races that restart and loses essentially every time. Result: both kamal-proxy (old gen, resurrected) and kamal-proxy-next serving via SO_REUSEPORT, promote_next_container never run, reboot exiting non-zero with the host half-promoted.

Fix

Cancel the restart policy BEFORE draining — the issue's preferred variant, because it leaves no timing window at all:

execute *proxy.disable_restart   # docker update --restart=no kamal-proxy
execute *proxy.drain(timeout: KAMAL.config.drain_timeout)
execute *proxy.wait_for_exit
execute *proxy.remove_stopped_container
execute *proxy.promote_next_container
  • Kamal::Commands::Proxy#disable_restart — new command builder, docker update --restart=no kamal-proxy (argv-array style, matching siblings)
  • Kamal::Cli::Proxy::Reboot#handoff_generation — runs it first

Sibling-path audit: handoff_generation is the only caller of drain on a proxy container. stop_and_replace and migrate_to_holder use docker container stop (marks the container deliberately stopped — no resurrection) plus container prune --force, so neither shares the race.

Test plan

  • test/commands/proxy_test.rb — new disable_restart test asserting the exact argv: docker update --restart=no kamal-proxy
  • test/cli/proxy_test.rb — the port-holder handoff test now asserts docker update --restart=no appears in the output AND, via index comparison, that it runs strictly before the drain command
  • TDD: both tests written first and confirmed failing (RED), then the implementation (GREEN)
  • bundle exec rubocop --parallel — clean (218 files)
  • Full unit suite: 1422 runs, 4396 assertions, 2 failures — both are the known Apple-Silicon arch-dependent builder tests, verified (via git stash) to fail identically on the unmodified baseline; they pass in CI

Deviations & judgment calls

  • Sibling-path audit: only handoff_generation drains a proxy container. stop_and_replace and migrate_to_holder use docker container stop (marks the container deliberately stopped, so the restart policy does not resurrect it) followed by container prune --force — no shared race. No other fix needed.
  • disable_restart takes no name: keyword (unlike wait_for_exit/remove_stopped_container) — only one caller exists and it targets the default container; avoided an unused parameter.
  • Used --restart=no as a single token (matching the issue text) rather than the two-token "--restart", "no" style used by the docker run builders — docker update accepts both; the single token reads unambiguously in printed/logged output.
  • CLI-level ordering asserted in the existing "boot with drifted port_holder proxy hands off with zero downtime" test (it exercises handoff_generation via the reboot path) using output index comparison, rather than adding a near-duplicate test.
  • Unit suite's 2 failures (CommandsBuilderTest#test_hybrid_builder_with_local_registry, CliBuildTest#test_push_with_remote_builder_checks_both_the_builder_and_the_remote_context) verified pre-existing on baseline via git stash — the two known Apple-Silicon host-arch failures (names differ from the older grep hint in .claude/rules/testing.md, but the failure mode — amd64/arm64 platform swap — matches exactly). Not regressions.
  • No rollback added for a failed handoff: handoff_generation had none before this change, and disable_restart failing before drain leaves the host fully intact (old gen still serving, restart policy merely disabled; the next reboot re-runs the handoff). Out of scope per the issue.

Both port-holder generations run with --restart unless-stopped. drain
makes the old proxy exit on its own - unlike docker stop, that does not
mark the container deliberately stopped, so Docker's restart policy
resurrects it within its sub-second backoff. The un-forced docker
container rm then races that restart and loses essentially every time,
leaving the host half-promoted: both generations bound via SO_REUSEPORT,
promote_next_container never run, reboot exiting non-zero.

docker update --restart=no BEFORE draining removes the race entirely -
there is no window in which the exited container can be restarted.

Closes #105
@mhenrixon mhenrixon self-assigned this Aug 3, 2026
@mhenrixon mhenrixon added the bugfix Fixes a defect in fork code label Aug 3, 2026
@mhenrixon
mhenrixon merged commit e85b9ad into dash Aug 3, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fixes a defect in fork code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

port_holder handoff: drained old generation is resurrected by its restart policy, so docker container rm fails and the next generation is never promoted

1 participant