From 20357abe358df1e89beb35dd49776c8ae3476002 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Sat, 12 Sep 2026 19:51:20 +0200 Subject: [PATCH 1/4] fix(proxy): route every container-creating path through the stage-3c bridge `Dash::Cli::Proxy::Reboot`, `Dash::Cli::Proxy::LoadbalancerReboot` and `dash proxy loadbalancer start` all create the renamed container without ever running the stage-3c bridge. `docker run --volume dash-loadbalancer-config:...` auto-creates the named volume empty when it does not exist, so a `dash proxy reboot` against a host that has never been through `dash proxy boot` brings the new volume into existence before the bridge has had any chance to copy the legacy routing table and ACME cache into it. The next boot then finds the new volume already there and skips the copy for good, via `copy_legacy_config_volume`'s own guard - silently. Since #167 that state also writes the `.legacy-renamed` marker, so recovery needs the marker deleted as well as the volume fixed. Fix it at the source rather than making the marker's heuristic smarter: every path that can create the container, the volume or the network now runs `prepare_boot` first. On both reboot paths this is round-trip neutral - they already spent a round trip on `ensure_apps_config_directory`, which `prepare_boot` carries. Refs #168 --- lib/dash/cli/proxy.rb | 6 ++ lib/dash/cli/proxy/loadbalancer_reboot.rb | 9 ++- lib/dash/cli/proxy/reboot.rb | 7 ++- lib/dash/commands/loadbalancer.rb | 22 ++++--- test/cli/proxy_test.rb | 70 +++++++++++++++++++++++ 5 files changed, 100 insertions(+), 14 deletions(-) diff --git a/lib/dash/cli/proxy.rb b/lib/dash/cli/proxy.rb index 8c7c2c95..242af63c 100644 --- a/lib/dash/cli/proxy.rb +++ b/lib/dash/cli/proxy.rb @@ -414,6 +414,12 @@ def loadbalancer(status) if DASH.config.proxy.load_balancing? on(DASH.config.proxy.effective_loadbalancer) do |host| execute *DASH.registry.login + # start_or_run falls through to `docker run` on a host with no + # container, so this is a volume-creating path too and gets the same + # bridge boot and reboot do (zoolutions/dash#168). It also makes the + # apps-config directory the bind mount would otherwise have docker + # create root-owned. + execute *DASH.loadbalancer.prepare_boot execute *DASH.loadbalancer.start_or_run end else diff --git a/lib/dash/cli/proxy/loadbalancer_reboot.rb b/lib/dash/cli/proxy/loadbalancer_reboot.rb index 47d741b2..e60fdab9 100644 --- a/lib/dash/cli/proxy/loadbalancer_reboot.rb +++ b/lib/dash/cli/proxy/loadbalancer_reboot.rb @@ -21,6 +21,14 @@ def run execute *DASH.registry.login ensure_network + # After the network the bridge attaches the legacy network's containers to, + # and before anything that could create the new container or let `docker + # run --volume` create dash-loadbalancer-config empty: adopt the legacy + # volume's routing table, dynamic domains and ACME cache. Carries the + # apps-config mkdir this reboot paid a round trip for anyway + # (zoolutions/dash#168, see Dash::Commands::Loadbalancer#legacy_rename). + execute *DASH.loadbalancer.prepare_boot + info "Stopping and removing #{DASH.loadbalancer.container_name} on #{host}, if running..." execute *DASH.loadbalancer.stop, raise_on_non_zero_exit: false execute *DASH.loadbalancer.remove_container @@ -32,7 +40,6 @@ def run execute *DASH.loadbalancer.remove_proxy_secrets_file, raise_on_non_zero_exit: false end - execute *DASH.loadbalancer.ensure_apps_config_directory Dash::Cli::Proxy::LoadbalancerClaim.new(host, sshkit).claim_run_config(replace: true) execute *DASH.loadbalancer.run diff --git a/lib/dash/cli/proxy/reboot.rb b/lib/dash/cli/proxy/reboot.rb index 583d125d..29c977d3 100644 --- a/lib/dash/cli/proxy/reboot.rb +++ b/lib/dash/cli/proxy/reboot.rb @@ -37,7 +37,12 @@ def pull_image def replace_container execute *proxy.ensure_proxy_directory - execute *proxy.ensure_apps_config_directory + # Before the new container - and the new config volume `docker run + # --volume` would otherwise create empty - exists: bring a host still on + # pre-rename identity across. Carries the apps-config mkdir this reboot + # paid a round trip for anyway, so the bridge costs none here either + # (zoolutions/dash#168, see Dash::Cli::Proxy::LegacyRename). + execute *proxy.prepare_boot sync_proxy_secrets if proxy.port_holder? diff --git a/lib/dash/commands/loadbalancer.rb b/lib/dash/commands/loadbalancer.rb index 2c592d83..7e41761b 100644 --- a/lib/dash/commands/loadbalancer.rb +++ b/lib/dash/commands/loadbalancer.rb @@ -199,18 +199,16 @@ def legacy_rename_marker # Verified on the volume existing, not on a container being gone - the loadbalancer # replaces no legacy container, so this is the only signal its bridge has. That makes # it foolable in one specific way: if `dash-loadbalancer-config` comes to exist before - # this bridge ever runs on a host - e.g. `dash proxy reboot` invoked directly against a - # dedicated LB host that has never been through `dash proxy boot`, since - # Dash::Cli::Proxy::LoadbalancerReboot#run boots the container without calling this - # bridge first - the marker is written despite the legacy volume's routing table and - # ACME cache never having been copied. That gap predates this fold: on `main`, - # `copy_legacy_config_volume`'s own guard (`volume_exists(new) || ...`) already skips - # the copy for good in that state, silently, every deploy - this only turns a - # per-deploy re-check into a cached one. Closing it means routing `reboot` through the - # same bridge, a `Dash::Cli::Proxy::LoadbalancerReboot` change, out of scope here - # (zoolutions/dash#160 is `boot`'s round trips) - tracked in zoolutions/dash#168. - # Recovery today is the same either way: copy the legacy volume's contents over by - # hand, then remove .legacy-renamed under this host's loadbalancer directory so this + # this bridge ever runs on a host, the marker is written despite the legacy volume's + # routing table and ACME cache never having been copied. The fix is upstream of the + # heuristic rather than in it - every path that can create the container, and with it + # the volume `docker run --volume` auto-creates empty, runs the bridge first: + # `boot`, both reboots (Dash::Cli::Proxy::Reboot, Dash::Cli::Proxy::LoadbalancerReboot) + # and `dash proxy loadbalancer start` (zoolutions/dash#168). + # + # If a host is somehow in that state anyway - an operator's own `docker run`, a + # volume created by hand - recovery is to copy the legacy volume's contents over, + # then remove .legacy-renamed under this host's loadbalancer directory so this # re-evaluates. def mark_legacy_renamed combine \ diff --git a/test/cli/proxy_test.rb b/test/cli/proxy_test.rb index ab544531..57839454 100644 --- a/test/cli/proxy_test.rb +++ b/test/cli/proxy_test.rb @@ -78,6 +78,34 @@ class CliProxyTest < CliTestCase end end + # zoolutions/dash#168, the loadbalancer half: a dedicated LB host reached by `reboot` + # before it was ever booted would get `dash-loadbalancer-config` created empty by + # `docker run`, and its routing table, dynamic domains and ACME cache would never be + # adopted from `kamal-loadbalancer-config`. + test "reboot routes the loadbalancer host through the stage-3c bridge before creating the container" do + Dash::Configuration::Proxy.any_instance.unstub(:load_balancing?) + + run_command("reboot", "-y", fixture: :with_loadbalancer).tap do |output| + create = output.index("docker network create dash on lb.example.com") + bridge = output.index("test -f .dash/loadbalancer/.legacy-renamed || (") + copy = output.index("docker volume inspect dash-loadbalancer-config > /dev/null 2>&1 || ! docker volume inspect kamal-loadbalancer-config") + run = output.index("docker run --name load-balancer") + + assert bridge, "reboot must send the loadbalancer host the bridge: #{output}" + assert create && copy && run, output + assert create < bridge, "the bridge attaches containers to the dash network, which has to exist first" + assert bridge < run, "the volume copy must land before docker run can create the volume empty" + end + end + + test "reboot carries the loadbalancer bridge in the apps-config round trip it already paid" do + Dash::Configuration::Proxy.any_instance.unstub(:load_balancing?) + + run_command("reboot", "-y", fixture: :with_loadbalancer).tap do |output| + assert_match(/test -f \.dash\/loadbalancer\/\.legacy-renamed \|\| \(.*\) && mkdir -p \.dash\/proxy\/apps-config on lb\.example\.com/, output) + end + end + # SSHKit prefixes the first word of every command with /usr/bin/env, and # `env !` is exit 127 — so no chain the gem emits may start with `!`. test "no command sent to a host starts with shell negation" do @@ -136,6 +164,32 @@ class CliProxyTest < CliTestCase end end + # zoolutions/dash#168. `reboot` creates the new container, and `docker run --volume` + # auto-creates the new config volume empty if it is not there yet - so a host rebooted + # before it was ever booted would take the bridge's volume-existence guard out from + # under it, adopting nothing and recording itself migrated. The bridge belongs on this + # path too, ahead of anything that can create the new identity. + test "reboot routes a host through the stage-3c bridge before replacing the container" do + run_command("reboot", "-y").tap do |output| + bridge = output.index("test -f .dash/proxy/.legacy-renamed || (") + stop = output.index("docker container stop --time 40 dash-proxy on 1.1.1.1") + run = output.index("| xargs docker run --name dash-proxy") + + assert bridge, "reboot must send the bridge: #{output}" + assert stop && run, output + assert bridge < stop, "the bridge decides before the running proxy is taken down: #{output}" + assert bridge < run, "the new volume must not exist before the legacy one is copied into it" + end + end + + # The bridge rides the apps-config mkdir the reboot already paid a round trip for, so + # closing the gap costs none - the same fold boot got in zoolutions/dash#167. + test "reboot carries the bridge in the apps-config round trip it already paid" do + run_command("reboot", "-y").tap do |output| + assert_match(/test -f \.dash\/proxy\/\.legacy-renamed \|\| \(.*\) && mkdir -p \.dash\/proxy\/apps-config on 1\.1\.1\.1/, output) + end + end + test "boot takes the server lock so concurrent destinations serialise on the shared proxy" do run_command("boot").tap do |output| assert_match "Acquiring the server lock...", output @@ -1053,6 +1107,22 @@ class CliProxyTest < CliTestCase end end + # `start_or_run` falls through to `docker run` on a host with no container, which is + # the same volume-creating path `reboot` takes - so this admin command gets the bridge + # too, the last thing that can create dash-loadbalancer-config (zoolutions/dash#168). + test "loadbalancer start routes through the stage-3c bridge before it can run" do + Dash::Configuration::Proxy.any_instance.unstub(:load_balancing?) + + run_command("loadbalancer", "start", fixture: :with_loadbalancer).tap do |output| + bridge = output.index("test -f .dash/loadbalancer/.legacy-renamed || (") + start = output.index("docker container start load-balancer || docker run --name load-balancer") + + assert bridge, "loadbalancer start must send the bridge: #{output}" + assert start, output + assert bridge < start, "the volume copy has to land before docker run could create the volume empty" + end + end + test "loadbalancer stop" do Dash::Configuration::Proxy.any_instance.unstub(:load_balancing?) From 80c8a21d9070b49eae0986ef29e32695fd84ef85 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Sat, 12 Sep 2026 20:01:13 +0200 Subject: [PATCH 2/4] docs(proxy): record why the bridge copies the config volume while it is live Both cubic and a human reader will ask whether `cp -a` over a volume the legacy container still mounts can capture a half-written routing table or certificate. It cannot: dash-proxy renames into place on every writer - the routing table via writeFileAtomic, the dynamic domain and redirect state via their own temp + rename, the response cache via CreateTemp + Rename, and the ACME cache via autocert.DirCache. Written at the shared copy rather than at one caller, since `boot` and both reboots all reach it. Refs #168 --- lib/dash/commands/proxy.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lib/dash/commands/proxy.rb b/lib/dash/commands/proxy.rb index e555876c..48d3aec8 100644 --- a/lib/dash/commands/proxy.rb +++ b/lib/dash/commands/proxy.rb @@ -101,6 +101,16 @@ def legacy_rename # first deploy. The subshell groups create-and-copy because `&&` and `||` # share precedence and associate left — without it a host that already has # the new volume would still run the copy over live state. + # + # The source volume is still mounted by the legacy container while this runs - + # deliberately, on every path (see Dash::Cli::Proxy::LegacyRename's step order), and + # safe because every writer into it renames into place: the routing table through + # writeFileAtomic, the dynamic domain and redirect state through their own temp + + # rename, the response cache through CreateTemp + Rename, and the ACME cache through + # autocert.DirCache. `cp -a` reads a complete file either way. Skew across files is + # possible and harmless - an unused certificate, or a route whose certificate reissues - + # and --recheck-targets-on-restore re-verifies the targets on the way back up + # (zoolutions/dash#169 review). def copy_legacy_config_volume(volume: Dash::Configuration::Proxy::CONFIG_VOLUME, legacy: Dash::Configuration::Proxy::LEGACY_CONFIG_VOLUME) copy_legacy_volume(legacy: legacy, volume: volume, image: proxy_image) end From 00c556cd7bf21c00433cba730dbdf872a1c092f4 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Sat, 12 Sep 2026 21:00:11 +0200 Subject: [PATCH 3/4] test: stop a --quiet CLI test deciding whether later tests see SSHKit output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `dash app stale_containers --quiet` in test/cli/app_test.rb leaves :error on both the DASH singleton and SSHKit's global output_verbosity: Cli::Base#initialize_commander sets the commander's verbosity and Commander#configure_sshkit_with mirrors it into SSHKit. Nothing restores either between tests — Commander#reset would, but only `dash alias` calls it — so from that point on every SSHKit.config.output.info in the process is dropped. Whether that mattered depended on the seed. CI run 34709935073 put the quiet test ahead of test/cli/healthcheck/progress_reporter_test.rb on Ruby 3.2 (seed 36230) and three of its assertions saw "", while Ruby 3.3, 3.4 and 4.0 drew seeds that passed the same commit. Reproduced locally with `bin/test --seed 36230`, three failures, same three tests. Pin both to :info in the suite's global setup, beside the Docker pins that answer the same class of problem — a test that wants another verbosity still sets it itself. Refs #166 --- test/test_helper.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/test_helper.rb b/test/test_helper.rb index 1ccb51b8..7d5d6045 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -74,6 +74,21 @@ class ActiveSupport::TestCase # when it is on PATH, so a developer who has it installed would see different advice # than CI does. Pin it off; test/dockerfile/hadolint_test.rb turns it back on. Dash::Dockerfile::Hadolint.stubs(:available?).returns(false) + + # Fourth path, this one internal to the suite: a CLI test that runs a command with + # --quiet or -v leaves that verbosity behind on BOTH the DASH singleton and SSHKit's + # global output_verbosity (Cli::Base#initialize_commander, then + # Commander#configure_sshkit_with). Nothing restores either between tests - + # Commander#reset would, but only `dash alias` calls it - so + # `dash app stale_containers --quiet` in test/cli/app_test.rb silences every later + # SSHKit.config.output.info in the process. Whether that mattered depended on the + # seed: CI seed 36230 put it ahead of + # test/cli/healthcheck/progress_reporter_test.rb and took three of its assertions + # down to "", while the other three Ruby versions' seeds passed the same commit. + # Pin the default so the order cannot decide; a test that wants another verbosity + # still sets it itself. + DASH.verbosity = :info + SSHKit.config.output_verbosity = :info end # Dash::Commands::Base#ensure_run_directory — the one-shot .kamal -> .dash From d5945bc7a0907386421632ae55aaf6248175a77d Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Sat, 12 Sep 2026 21:24:35 +0200 Subject: [PATCH 4/4] test: pin the proxy boot's round trips per host, not in one cross-host order `on` runs the proxy hosts in parallel threads, and the recorder behind "boot issues no round trip beyond the pinned per-host sequence" appended from both. The pin then spelled out host 1's sequence followed by host 2's, which held only while the two threads happened not to overlap. CI seed 59404 interleaved them (login, bridge, login, bridge, inspect, ...) - same commands, same count per host, different scheduling - and the test failed on a run that issued exactly what it pins. It reproduces standalone here too: 1 in 150. Tag every recorded round trip with the host it went to (the Printer command carries it; a capture reads it off SSHKit::Backend.current, the thread-local the backend sets for its run) and assert each host's own sequence. That is the claim the test was making - the count and the order the gem chooses - minus the one it never meant to: which thread the scheduler ran first. 0 in 300 after. Refs #167 --- test/cli/cli_test_case.rb | 9 +++++++-- test/cli/proxy_test.rb | 20 +++++++++++++------- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/test/cli/cli_test_case.rb b/test/cli/cli_test_case.rb index 253d4de5..c29f0e37 100644 --- a/test/cli/cli_test_case.rb +++ b/test/cli/cli_test_case.rb @@ -66,11 +66,16 @@ def recorded_captures # array between both stubs instead. Formats captures without their trailing options # hash (`raise_on_non_zero_exit: false` and friends), which reads as noise next to a # shell command; #recorded_captures keeps the hash for its own callers. + # Each round trip is tagged with the host it went to. `on` runs hosts in parallel + # threads, so the order ACROSS hosts is whatever the scheduler chose that run - a pin + # that spells out host A's sequence and then host B's held only while the threads + # happened not to overlap (CI seed 59404 interleaved them). Only the order within one + # host's list is the gem's to promise; group by host and assert there. def recorded_commands_and_captures round_trips = [] - SSHKit::Backend::Printer.any_instance.stubs(:execute_command).with { |cmd| round_trips << cmd.to_command; true } + SSHKit::Backend::Printer.any_instance.stubs(:execute_command).with { |cmd| round_trips << [ cmd.host.to_s, cmd.to_command ]; true } SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) - .with { |*args| round_trips << args.reject { |arg| arg.is_a?(Hash) }.join(" "); false } + .with { |*args| round_trips << [ SSHKit::Backend.current.host.to_s, args.reject { |arg| arg.is_a?(Hash) }.join(" ") ]; false } begin yield diff --git a/test/cli/proxy_test.rb b/test/cli/proxy_test.rb index 57839454..cf3536d3 100644 --- a/test/cli/proxy_test.rb +++ b/test/cli/proxy_test.rb @@ -143,8 +143,11 @@ class CliProxyTest < CliTestCase round_trips = recorded_proxy_round_trips { run_command("boot", fixture: :simple) } - assert_equal [ "docker network create dash" ] * 2 + PROXY_BOOT_ROUND_TRIPS_PER_HOST * 2, round_trips, - "deploy_simple has two proxy hosts: the network sweep, then the pinned sequence twice" + assert_equal %w[ 1.1.1.1 1.1.1.2 ], round_trips.keys.sort, "deploy_simple has two proxy hosts" + round_trips.each do |host, sequence| + assert_equal [ "docker network create dash" ] + PROXY_BOOT_ROUND_TRIPS_PER_HOST, sequence, + "#{host}: the network sweep, then the pinned sequence" + end end # Nothing changes for a host still running kamal-proxy: the three bridge steps travel @@ -1605,13 +1608,16 @@ def stub_proxy_drift(version: Dash::Configuration::Proxy::Run::MINIMUM_VERSION) stub_proxy_state("dash-proxy", "abc123 ghcr.io/zoolutions/dash-proxy:#{version} stale-digest") end - # Every round trip a proxy boot spends, executes and captures interleaved in issue - # order, with the deploy's own run-directory and lock commands filtered out and the - # long ones elided - the count and the order are what this pins, not the shell. + # Every round trip a proxy boot spends, per host: executes and captures interleaved in + # the order that host issued them, with the deploy's own run-directory and lock commands + # filtered out and the long ones elided - the count and the order are what this pins, + # not the shell. Keyed by host because hosts run in parallel and only each host's own + # order is deterministic (see CliTestCase#recorded_commands_and_captures). def recorded_proxy_round_trips recorded_commands_and_captures { yield } - .reject { |command| command.match?(/\.dash\/lock-|mv \.kamal \.dash/) } - .map { |command| elide(command) } + .reject { |_host, command| command.match?(/\.dash\/lock-|mv \.kamal \.dash/) } + .group_by(&:first) + .transform_values { |pairs| pairs.map { |_host, command| elide(command) } } end # Collapses the parts that carry a digest, a boot-config read or the whole stage-3c