Collapse the target / build-host duplication - #15
Merged
Conversation
targets and build_hosts carry the same six host-key columns with the same
meaning — migration 0007 says so outright. The trust-on-first-use state
machine over them was written twice: row_to_host_key byte-identical in both
stores, four mutation methods each, and the connect-and-record path copied a
third and fourth time into the deploy engine and the build-host check RPC.
Collapse it onto one SshHost enum naming the table:
- store/host_keys.rs owns row_to_host_key and pin/record_pending/
clear_pending/forget, once each.
- set_target_status and set_build_host_status stop branching into two SQL
statements and use the COALESCE form set_ingress_status already used.
- Engine::connect and ssh_target_for take either kind of host through one
SshHostRef borrow, so connect_build_host and ssh_target_for_build_host
disappear and BuildHosts.Check stops open-coding what Targets.Check
already delegated.
Also derive the by-id selects from the list selects instead of hand-copying
24 column names, which retires the test that existed only to police the two
copies for drift.
…pers
tests/e2e.rs was 2358 lines covering three unrelated areas — deploying,
building on a build host, and ingress — over one shared container fixture,
with wait_for_async defined in the build-host section and used by the deploy
tests. It becomes tests/e2e/{main,fixture,deploy,build_hosts,ingress}.rs:
still one binary named e2e, so the CI invocation and the Makefile target are
unchanged, and all 14 tests keep their names.
Splitting it also put the file under clippy for the first time (CI lints
without the e2e feature), which surfaced three lints; fixed.
Separately, nudo-format is the canonical vocabulary for unit states, artifact
sources and log levels, but four callers still went through a one-line
wrapper around it. server::units::status_label was dead outside its own
tests and its doc claimed a sharing that no longer happened; cli::units_label
claimed to be a duplicate it no longer was; three of mcp::presentation's four
functions were pure delegation. All deleted in favour of calling nudo_format
directly, and the exhaustive label assertions moved to sit with the logic.
The dashboard opened four gRPC connections to the control plane one after another, then read two more things out of the database, all in series — statement order was the only thing saying which of the seven reads depended on another, and none of the first five did. Group them: one join for the independent reads, one for the two that need their results. Same on the deployments list and the build-host pages. unit_statuses deliberately stays sequential. Each call opens an ssh connection to the target, so making a dashboard refresh arrive at a latency-critical box as a burst of simultaneous connections would trade the property this tool exists to protect for a faster page.
server::logs::priority_label was a one-line call through to nudo_format::journal_priority_label with no caller outside its own test. Its assertions move to nudo_format, where the mapping actually lives, and gain the four severities neither copy covered. is_problem stays: it is real logic rather than delegation, and the dashboard's priority_class is a different question (three CSS classes, not a boolean). Its doc no longer claims a caller it does not have.
Loa212
added a commit
that referenced
this pull request
Jul 31, 2026
Comment-only. #15 justified batching the dashboard's independent reads with each one opening its own connection to the control plane; #16 made that false by giving all three clients one held, multiplexed channel. The batching is still right — sharing a connection is what makes asking at once cheap rather than contended.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
A thermo-nuclear-refactor pass over the whole repo. Behaviour is unchanged
throughout:
make checkpasses, and the full Docker-backed e2e suite passes14/14 — which matters here, because the largest change is to the ssh host-key
path those tests exercise directly.
The dominant finding is that the target / build-host parallel was implemented
by copy-paste at four layers. Everything below follows from pulling on that.
1. The host-key state machine was written twice
targetsandbuild_hostscarry the same six host-key columns with the samemeaning. Migration 0007 says so outright — "identical in shape and meaning to
the columns added to
targetsin 0006" — and then the code over those columnswas duplicated anyway:
row_to_host_keywas byte-identical in both stores.pin/record_pending/clear_pending/forgetexisted once per table,eight methods for four operations.
into the deploy engine's
connect_build_hostand intoBuildHosts.Check,while the target equivalents already delegated to a shared helper.
It now lives once in
store/host_keys.rs, keyed on anSshHostenum naming thetable.
Engine::connectandssh_target_fortake either kind of host throughone
SshHostRefborrow, soconnect_build_hostandssh_target_for_build_hostare gone and
BuildHosts.Checkstops open-coding whatTargets.Checkalreadydelegated.
This is the change that matters most: it is what makes "a host presenting the
pinned key clears a pending change" true of build hosts because it is true of
targets, rather than true of whichever copy was edited last.
2. Two selects per table, and a test to police them
Each host store hand-copied its ~24 column names into a list select and a by-id
select.
build_hostscarried a third copy as a test-only const plus a testasserting the two selects agreed — a test that existed only because the
duplication existed. The by-id reads now append
WHERE id = ?1to the oneselect, the same way the list reads already appended
ORDER BY. The guard testis retired with the thing it guarded.
3. A field mask meant one statement per field
update_targetandupdate_build_hostissued up to eightUPDATEs inside atransaction that existed to make the set of them atomic.
store/field_mask.rscollects the named columns and writes them in one statement, so the update is
atomic on its own and the transaction goes away with the round trips. Which
columns a blank value writes is still per-field policy, now visible in one list.
4.
set_*_statusbranched into two statements to do one thingBoth stores matched on whether the probe succeeded and ran one of two SQL
statements.
set_ingress_statusin the same file already expressed exactly this("only success advances the timestamp") as a single
COALESCE. Converged onthat form.
5. Five one-line wrappers around the canonical vocabulary
nudo-formatis the shared crate for unit states, artifact sources and loglevels, but five callers still went through a wrapper around it —
server::units::status_label,server::logs::priority_label,cli::units_label, and three ofmcp::presentation's four functions. Two weredead outside their own tests, and two carried doc comments describing a
duplication or a sharing that had stopped being true. Deleted; the exhaustive
assertions moved to sit with the logic, gaining cases neither copy covered.
6. The preflight-checks card was rendered twice
Identical markup in the target and build-host renderers, differing only in a
trailing warnings block. The two proto
Checkmessages are distinct Rust types,so they borrow into one
Probeview and share onepreflight_card. RenderedHTML is byte-identical — all 230 web tests pass untouched.
7. The dashboard asked for seven things in a row
Four gRPC reads, each opening its own connection, then two database reads — in
series, with statement order the only thing indicating which depended on which.
None of the first five did. They now go out together, with a second group for
the two that need their results.
unit_statusesdeliberately stays sequential. Each call opens an sshconnection to the target, and making a dashboard refresh arrive at a
latency-critical box as a burst of simultaneous connections would trade the
property this tool exists to protect for a faster page.
8.
tests/e2e.rswas 2358 linesThree unrelated areas — deploying, building, ingress — over one shared container
fixture, with
wait_for_asyncdefined in the build-host section and used by thedeploy tests. Split into
tests/e2e/{main,fixture,deploy,build_hosts,ingress}.rs:still one binary named
e2e, so the CI invocation and the Makefile target areunchanged, and all 14 tests keep their names.
Splitting it also put the file under clippy for the first time — CI lints without
the
e2efeature, so it never had been — which surfaced three lints. Fixed.Verification
make check— fmt, clippy (-D warnings), 999 tests, release scripts. Green.cargo test --features e2e --test e2e -- --test-threads=1— 14/14 againstreal Docker containers, 362s. Exercises host-key pinning on first use, a
refused changed key, accepting a reviewed key, build-host pinning, deploys and
rollback — i.e. precisely the code section 1 rewrote.
main: the only two tests that no longerexist are the drift guard from §2 and the label test from §5, both retired with
the duplication they existed for. Every other test on
mainstill runs.Deliberately left alone
unit_statuses— see §7. The obvious parallelisation is not safe here.accept_host_key/forget_host_keyin the API layer. They look like nearduplicates across targets and build hosts, but the audit strings and the
validation differ deliberately, and the target path requires a non-empty
fingerprint where the build-host path does not. Collapsing them would have to
pick one behaviour; that is a semantic decision, not a refactor.
targets/build_hoststables themselves. Kept apart on purpose — itis what makes "a build host is never deployed to" structural. Only the host-key
column family, which is genuinely one concept, was unified.
logs::is_problem— unused, but real logic rather than delegation, so it iskept and its doc corrected to stop claiming a caller it does not have.