Every ssh.exec opened its own connection: a TCP handshake, a key exchange and an
authentication round trip per command. A key rotation paid for that eight times,
a deployment six, and refreshing M profile states M times.
Connections are now kept per server. The three risks that made this worth doing
carefully are handled explicitly:
- Staleness. A pooled connection can be dead exactly when it matters. Liveness is
tracked through error, close and end, and a lease that finds a dead entry opens
a new one. The remaining race, where the connection dies between the check and
the command, is caught by the retry rule below.
- Retrying. Only a failure that proves the command never reached the server is
retried, and only once, and only on a connection that was already established
before this call. execClient marks exactly that case, when the channel fails to
open. A command that opened a stream is never repeated, because the server may
already be acting on it - repeating a deployment is not this layer's decision.
Two tests hold that line: widening the rule to any failure fails both.
- Lifetime. Idle connections close after a minute, the pool is reference counted
so a shared connection survives until its last user is done, closeAll runs
during quit, and every pooled client keeps a standing error listener so an
error while idle cannot reach the uncaughtException handler.
A trust-on-first-use connection is never pooled: it was established without
verifying the fingerprint, so it must not serve a later verified call. A change
to host, port, user, auth type, key path or trusted fingerprint invalidates the
pooled connection.
ssh-service coverage rises from 61% to 90% of lines and 97% of functions.
Also in this commit, the smaller items from the same review:
- Diagnostics batched records that queue up while a write is in flight into one
append, and chmod runs once per file instead of once per record. At the debug
level every IPC call writes a line, which is exactly when troubleshooting.
- The set that suppresses duplicate deployment notifications is trimmed instead
of growing for the lifetime of the process.
- The updater kept the same once('error') pattern on its spawned helper that
took the app down through the SSH client.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A key rotation verified the candidate with `git ls-remote`, then immediately ran
preflightCandidate, which threw that result away and ran the same command over a
second SSH connection. Nothing happens between the two calls that could change
the answer, and the proof was already being passed in.
preflightCandidate now uses a proof that established a remote commit and falls
back to verifying when it is handed nothing usable, so it still works as a
standalone gate. Every ssh.exec opens its own connection, so this removes a full
TCP, key exchange and authentication round trip from a rotation.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
views.js sat at the project's 750-line limit, so the diff cap in the previous
commit pushed it over and every further change would have meant shaving
comments elsewhere. That is the file asking for decomposition, which is what the
architecture audit says to do.
Diff rendering is self-contained: the line cap, the line classifier and the
change-map illustration depend on nothing in views.js beyond ui and escapeHtml.
They now live in src/renderer/diff-view.js and are registered in index.html and
in the three renderer file lists that scan the bridge surface, so anything added
there is covered by the existing contract tests.
views.js drops from 755 to 714 lines and no source file exceeds 750 again. The
nested ternary that classified a diff line became a named function with guard
clauses on the way.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two failure modes that only show up under conditions the tests never reached.
renderDiff built one span per diff line with no bound. A regenerated lock file
is an ordinary change: 50,000 lines produce 4 MB of markup and 50,000 elements
that then have to be parsed and laid out inside the full shell replacement, and
200,000 lines produce 16 MB. The rendered view now stops at 2,000 lines and says
how many were left out; ui.diff keeps the whole change, so Copy diff, the editor
and hunk staging are unaffected. The line scan also runs once now instead of
three times.
withClient registered the connection error handler with once(). A connection
that fails and then emits a second error while it is being torn down - a reset
during client.end() is the ordinary case - leaves that event unhandled, and an
unhandled 'error' on an EventEmitter reaches the uncaughtException handler,
which calls app.exit(1). The handler stays attached and ignores anything after
the first failure.
Both are covered by tests that were confirmed to fail without the fix, together
with the SSH paths that had none: host key mismatch reporting, the trusted
fingerprint requirement for exec and upload, and remote upload path validation.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NuklearRabbit
Jens