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

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 merged commit e85b9ad into dash Aug 3, 2026
9 checks passed
@mhenrixon mhenrixon added bug Something isn't working and removed bugfix labels Oct 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

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