Skip to content

perf(deploy): cut a quarter of a deploy's SSH round trips - #159

Merged
mhenrixon merged 3 commits into
mainfrom
feat/issue-154-pr5-reduce-ssh-overhead
Sep 11, 2026
Merged

perf(deploy): cut a quarter of a deploy's SSH round trips#159
mhenrixon merged 3 commits into
mainfrom
feat/issue-154-pr5-reduce-ssh-overhead

Conversation

@mhenrixon

@mhenrixon mhenrixon commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

PR 5 of #154: fix the overhead the deploy report's per-phase command counts revealed. Every change here is a reduction the table measured, and the table is how each was verified afterwards — no fix landed that the numbers did not justify.

Measured on the integration harness (3 hosts, 2 app hosts), same stack before and after. The Acquire server lock rows nest inside the phase above them, so they are not summed twice:

Phase before after
Build and push app image 19 ssh 15
Acquire deploy lock 4 4
Ensure dash-proxy 32 29
  └ Acquire server lock 5 2
Detect stale containers 4 4
Boot 30 24
  └ web vm1 / vm2 9 each 7 each
Prune 21 10
  └ Acquire server lock 5 2
distinct round trips 110 86 (−22%)

A redeploy goes 67 → 57 (−15%): Build and push 25 → 21, Boot 34 → 28.

What changed

  • Dash::Commands::App#boot_state — the container-clash check and the running-version read become one command, chained with ; and split on a separator. Boot −1 per host.
  • Dash::Commands::Auditor#record_then — an audit line and the action it describes in one shell string, still in that order. Used by the boot, the latest-image tag, the image pull and both prune sweeps.
  • Dash::Commands::Builder::Base#clean_then_pull — drop the stale image and pull in one command; the removal stays best-effort (|| true).
  • Dash::Cli::Base#ensure_run_directory — sweep each host once per process instead of once per lock acquire. Both Acquire server lock rows 5 → 2.

Nothing new is executed and no ordering semantics change: the audit is still written before the action it describes, the health barrier is untouched, and no capture is folded into anything.

One user-visible change: folded audit lines now print at INFO as part of the command they lead, where they used to run at verbosity: :debug and were invisible. The audit log content and dash audit are unchanged.

Measured, not fixed — follow-up

Ensure dash-proxy is the single largest row (32 ssh, 12.8s, ahead of Boot and Prune) and is deliberately left alone. Its round trips are the staged-rename migrations — LegacyRename, copy_legacy_config_volume, the kamal-proxy container and kamal-proxy-net teardown — four executes per host of pure legacy checking on every boot, plus drift detection. Folding them changes migration semantics that CLAUDE.md freezes until stage 3c, and the issue's candidate list does not include them. Worth its own issue once 3c lands.

Test plan

  • bundle exec ruby -Itest -e 'Dir["test/**/*_test.rb"].grep_v(/integration/).each { |f| require File.expand_path(f) }' — 1873 runs, 0 failures
  • bundle exec rubocop --parallel — no offenses
  • bin/test (full suite, Docker + published proxy image) — 1892 runs, 6046 assertions, 0 failures
  • Real deploy + redeploy through the integration harness, before and after, tables above
  • Clash path verified against a real deploy: a same-version redeploy renames the clashing container and stops the renamed one, not the container the boot just started
  • New round-trip-count assertions in prune_test, build_test and app_test pin the reductions
  • The cost-guard sequence in test/cli/main_test.rb is unchanged

Deviations & judgment calls

Deviations

  • Proxy boot is the largest row and is left alone. See "Measured, not fixed" above. Reported rather than changed, because the round trips there are frozen migration logic.
  • The cost-guard sequence is unchanged. It pins only the commands deploy issues itself (subcommands are stubbed), and every reduction here lands in a subcommand. Round-trip-count assertions were added to the suites that do own those commands, so the reductions are pinned where the guard cannot see them.
  • validate_image keeps its own round trip. The issue asked for clean + pull in one command; folding validate_image in as well would put the pull under that command's trailing || (echo "... missing the 'service' label" && exit 1), so a failed pull would report a missing label. 4 → 2 per host rather than 4 → 1.

Discoveries

  • old_version_renamed_if_clashing had an ordering dependency the issue did not name. Combining the two captures means the running version is read before the clashing container is renamed. When the clash IS the running container, the old version to stop afterwards is the name it was renamed to — stopping the name that was read would stop the container the boot just started. Handled by substituting renamed_version when the read version equals the version being deployed, which is exactly what the second capture used to return. Unit-tested, and verified against the harness with a same-version redeploy.
  • ensure_run_directory had to memoise per host, not per process. The issue said "memoise per process (DASH.run_directory_ensured)". A boolean is wrong: dash upgrade narrows DASH.hosts with with_specific_hosts between lock acquires, so a flag would skip the sweep for a host that was never in scope when it was set. Tracks the host set instead.
  • test/cli/build_test.rb:142 failed on any clean checkout, and has since feat(report): measure every build step from the buildx stream dash already reads #156. stdouted strips, eating the two-space indent of Build when that header is the first line printed. CI only passes because the workflow runs rm Gemfile.lock before the suite, so Dash::Git.uncommitted_changes prints a line above it — exactly the host-dependence .claude/rules/testing.md forbids. In path (this PR edits the pull_on_hosts expectations in the same file), so fixed here: the capture no longer strips, and the two report tests stub uncommitted_changes so the checkout's state cannot decide the output.

Judgment calls

  • Auditor#record_then(line, *commands) rather than a combine at each call site: five call sites wanted the same shape, and it keeps shell building in the Commands layer where .claude/rules/coding-style.md puts it.
  • Folded audits print at INFO now. Called out above; the alternative (keeping them at debug) would mean not folding them at all.
  • BOOT_STATE_SEPARATOR = "--%--": % is not legal in a docker tag and container ids are hex, so neither answer can forge the separator line.

Refs #154

https://claude.ai/code/session_01MKyeeYAuG4fR11sBmJa8dm


Summary by cubic

Cuts a deploy's SSH round trips by 22% (110 → 86), the reductions #154's report exposed, by folding independent commands into single shell strings and sweeping the run directory once per host per process. A redeploy goes 67 → 57.

Notes for review

  • The clash check and running-version read now share a round trip, so the running version is read before the rename; when they're the same container, the boot stops the renamed one.
  • Folded audit lines now print at INFO as part of the command they lead; the audit log and dash audit are unchanged.
  • clean_then_pull groups the best-effort clean in parentheses so a failed audit write cannot fall into the || true and pull anyway.
  • validate_image keeps its own round trip so a failed pull doesn't report a missing service label.
  • Ensure dash-proxy is left alone deliberately — its round trips are frozen migration logic.
  • Fixes test issues: the build_test report assertions only held on a dirty checkout, and recorded_commands now stops recording where its block ends.

Written for commit e137dfb. Summary will update on new commits.

Review in cubic

The deploy report's per-phase command counts (#155-#158) showed where dash
was paying for connections it did not need. Every reduction here is one the
table measured, and the table is how each was verified afterwards.

A three-host integration deploy, before -> after (distinct round trips; the
"Acquire server lock" rows nest inside the phase above them):

  Build and push app image   19 -> 15
  Acquire deploy lock         4 ->  4
  Ensure dash-proxy          32 -> 29   (server lock 5 -> 2)
  Detect stale containers     4 ->  4
  Boot                       30 -> 24
  Prune                      21 -> 10   (server lock 5 -> 2)
  total                     110 -> 86   (-22%)

A redeploy goes 67 -> 57.

What changed:

- Dash::Commands::App#boot_state asks the clash check and the running-version
  read in one command. The running version is therefore read before a clashing
  container is renamed, so when the clash IS the running container the version
  to stop afterwards is the name it was renamed to - otherwise the boot would
  stop the container it just started.
- Dash::Commands::Auditor#record_then puts an audit line and the action it
  describes in one shell string, still in that order. Used by the boot, the
  latest-image tag, the image pull and both prune sweeps.
- Dash::Commands::Builder::Base#clean_then_pull drops the stale image and pulls
  in one command; the removal stays best-effort. validate_image keeps its own
  round trip, or a failed pull would report a missing service label.
- Dash::Cli::Base#ensure_run_directory sweeps each host once per process rather
  than once per lock acquire. Tracked per host, not as a flag: `dash upgrade`
  narrows the host set between acquires.

Folded audit lines now print at INFO as part of the command they lead; the
audit log itself and `dash audit` are unchanged.

Also fixes test/cli/build_test.rb's build-report assertions, which only held on
a dirty checkout: `stdouted` strips, eating the indent of a `  Build` header
printed first, and CI hid it by running `rm Gemfile.lock` before the suite.

Refs #154

Claude-Session: https://claude.ai/code/session_01MKyeeYAuG4fR11sBmJa8dm

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 17 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread test/cli/app_test.rb Outdated
Comment thread lib/dash/commands/builder/base.rb Outdated
Comment thread test/cli/prune_test.rb Outdated
Comment thread test/cli/build_test.rb Outdated
@mhenrixon mhenrixon self-assigned this Sep 11, 2026
@mhenrixon mhenrixon added the dx Developer/operator experience label Sep 11, 2026
Review findings on #159.

`clean_then_pull` returned `clean || true && pull`. `&&` and `||` bind
equally and associate left, so once record_then prepended the audit the
chain read `(((run_dir && audit) && clean) || true) && pull` - a FAILED
audit write fell into the same `|| true` and the pull ran anyway, exit
status 0, where two separate executes would have raised. Parenthesise the
best-effort clean so it confines the `|| true` to itself.

Also from the review, all test-only:

- The "single round trip" boot assertion counted Printer#execute_command,
  but both reads are captures and a stubbed capture_with_info never reaches
  that layer - it passed on the unfolded code too. Count capture_with_info
  instead; it now fails when the two reads are split.
- Assert the pull's exact total command count rather than an integer-divided
  per-host average, which rounded away one extra command on a single host.
- Move recorded_commands to CliTestCase; main_test keeps the lock-details
  redaction as recorded_deploy_commands on top of it.

Refs #154

Claude-Session: https://claude.ai/code/session_01MKyeeYAuG4fR11sBmJa8dm

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread test/cli/cli_test_case.rb Outdated
Review finding on #159. The Printer stub returns nil instead of printing,
and mocha leaves it standing until the end of the test - so every command
issued after the block was silently invisible, while the helper's comment
promised recording only "during the block".

Probed on a prune run: after the block, 0 of 4 `Running ...` lines printed;
with the ensure, all 4 do. No caller ran commands after the block today, so
this was a trap for the next one rather than a live bug.

Refs #154

Claude-Session: https://claude.ai/code/session_01MKyeeYAuG4fR11sBmJa8dm

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 1 file (changes from recent commits).

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@mhenrixon
mhenrixon merged commit 9111dbe into main Sep 11, 2026
10 checks passed
mhenrixon added a commit that referenced this pull request Sep 12, 2026
…ip (#165)

`dash build pull` paid four SSH round trips per app host: a `docker login`, a
`docker info` mirror probe, the audited clean-and-pull (#159), and
`validate_image`. The first two carried no work the other could not carry — the
probe runs before the pulls it shapes, and nothing between the login and the
probe depends on ordering across hosts.

`Dash::Commands::Registry#login_then` composes them into one command per host:

    docker login <server> -u … -p … > /dev/null && docker info --format '…'

The login's stdout is redirected away so the capture returns the mirror answer
alone. Mirror seeding is unchanged — a host with no mirror still fails the
`docker info` half with docker's index error, which is what "no mirror" means,
and a rejected login short-circuits the `&&` and raises as before. The two
single-command cases stay single: one app host issues only the login (nothing to
seed), a local registry only the probe (`Registry#login` returns nil).

Four app hosts: 16 round trips → 12. This is a fold, not a skip; every command
that ran before still runs, in the same order.

Refs #161
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dx Developer/operator experience

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant