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>
deployment-service.cjs sat at 52% and unraid-deploy-key-host.cjs at 39% of its
functions, both hidden behind a healthy aggregate. They are now at 100% lines
and functions, tested through real HTTP endpoints and by intercepting the shell
script the key host sends, rather than by mocking the boundary away.
What is pinned down: a successful workflow run still fails when the server
cannot prove it runs that exact commit; a rollback ends as rolled-back rather
than success; an unreachable status endpoint is never treated as healthy; a
failed poll is recorded on the operation instead of losing it; deploy keys stay
repository-scoped under the server base path with a pinned host key; promotion
verifies the candidate before swapping atomically; and revocation moves key
material to recovery instead of deleting it.
Two assumptions turned out to be wrong and the tests follow the real behaviour:
the previous-SHA check runs before the already-live check, and a rollback
against an unreachable endpoint surfaces the underlying network error.
Covering clone-target exposed a real defect: a remote ending in "....git"
yielded the folder name "...". Windows strips trailing dots, so that resolves
back to the project root itself, past an escape guard that only looks for "..".
A dots-only name now falls back to "repository", consistent with how an empty
name was already handled. As a side effect "." and ".." resolve to a usable
folder instead of raising an error.
Coverage gates: the aggregate moves to 85/85/68, and a new per-module gate
(60 statements, 50 functions, 36 branches) stops a single module from silently
collapsing behind the total. It reuses the data from the first run, so the
suite is not executed twice.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three handlers referenced a dependency they were never given, which made them
throw a ReferenceError as soon as they ran:
- deployment:preflight for Gitea Actions profiles (`preflight` was passed to
registerOperationsIpc but not to registerDeploymentIpc)
- Unraid write-access repair (`safeRelativeRemoteFile` was missing from
createUnraidAccessMethods)
- a dead reference of the same name in unraid-state-methods
no-undef and no-unused-vars were disabled for every file, which is why none of
these were caught. Both are now enabled for src/main and src/shared, where the
dependency graph is explicit. The renderer keeps them off because its functions
are deliberately cross-script globals.
Performance:
- git.status() spawned three processes (rev-parse, status, remote get-url) per
call. A directory holding its own .git is by definition the work tree root, so
rev-parse is unnecessary, and the remote URL is cached against the mtime of
.git/config, including the failure for a repository without that remote.
- git status runs with --no-optional-locks so a read no longer rewrites the
index. That stops it fighting a concurrent Git command for the index lock, and
is what makes filesystem watching viable at all.
- One commit issued four `git status` reads; callers that already hold the
status now pass it on, leaving two.
- The repository monitor is event driven. A watched repository is read on
filesystem activity, with a 30s safety net for watchers that stop delivering
and a 1s floor so a busy tree cannot drive a read per event. Repositories that
cannot be watched keep using the interval. Idle cost for one repository over
35s: 24 git processes before, 3 after.
- Resolving one repository by name no longer refreshes the whole workspace.
- Concurrent configuration saves share a single write of the latest state.
- Repository discovery follows directory junctions again. The filter that
skipped them made the realpath cycle guard dead code, and hid any project
folder reached through a junction.
Renderer:
- render() replaced the whole shell on every poll, discarding focus, caret and
scroll position while the user was typing. Those are preserved now, and an
unchanged render leaves the DOM alone entirely.
- The four sections that enhanceRenderedUi() injected after render moved into
the views, so the rendered markup is the single source of truth.
- The monitor no longer keeps a repository paused forever when it is unlinked
mid-mutation, scheduleAutoRefresh honours its delay argument, the demo bridges
no longer block startup, and #app is no longer an aria-live region announcing
the entire UI on every render.
IPC channel plumbing moved to src/main/ipc/channel.cjs, replacing a module-level
mutable diagnostics singleton with an argument.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Had a real dependency-audit gate (npm audit --audit-level=high) but no
secret scan. trufflehog's Action is Docker-based and cannot run on this
repo's existing windows-latest runner (needed for the Windows desktop
app's own browser/quality tests), so this adds it as a separate,
parallel ubuntu-latest job rather than inserting it into the Windows
job where it would break CI - the last gap for this repo to count as
fully-authored.
Jens
NuklearRabbit