fix(dashmate): keep the node up when an image pull fails, and stop reporting success when it did not - #4283
Open
shumkov wants to merge 8 commits into
Open
fix(dashmate): keep the node up when an image pull fails, and stop reporting success when it did not#4283shumkov wants to merge 8 commits into
shumkov wants to merge 8 commits into
Conversation
Contributor
|
Warning Review limit reachedNext included review available in 16 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (33)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
🕓 Ready for review — 3 ahead in queue (commit ba2ec49) |
thepastaclaw
suggested changes
Aug 4, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The PR improves pull failure reporting and TLS key permissions, but three in-scope blockers remain. Group restart still performs local builds after stopping every node, replacement of a historically exposed key writes the new secret through the old inode, and registry-controlled error text is printed directly to the terminal.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/dashmate/src/commands/group/restart.js`:
- [BLOCKING] packages/dashmate/src/commands/group/restart.js:47-56: Build local images before stopping group nodes
This pre-stop phase only calls `pullMissingImages`, which intentionally excludes services marked `isBuiltLocally`. When local builds are enabled, `startGroupNodesTask` runs `buildServicesTask` as its first task, but that happens only after every node has been stopped. A missing local image or any build failure therefore still leaves the entire group down, despite the commit's stated guarantee that required images are prepared before anything stops. Run the shared local build before the stop phase and arrange for the subsequent group start to skip the duplicate build.
In `packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js`:
- [BLOCKING] packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js:35-39: Replace an exposed private key through a fresh restricted inode
The `mode` option only affects file creation. If `private.key` already exists with mode 0644, `writeFileSync` writes the replacement secret through that world-readable inode and restricts it only afterward. A local user can also open the old inode before this operation and retain access through that descriptor after `chmodSync`, allowing them to read the newly generated key. Write the key to a temporary file in the same directory created as 0600, then atomically rename it over `private.key`; descriptors for the old inode will then expose only the old key.
In `packages/dashmate/src/commands/update.js`:
- [BLOCKING] packages/dashmate/src/commands/update.js:64-70: Sanitize registry error text before terminal output
The `error` field can come directly from a Docker registry pull-stream `errorDetail.message`, but the stderr summary interpolates it unchanged. A registry response can therefore inject newlines, carriage returns, ANSI escapes, bidirectional controls, or an excessively long message into the operator's terminal. This path is unconditional, including when `--format=json` is selected. Strip unsafe control characters and bound the rendered length before writing remote error text to terminal streams, using protections equivalent to the new remote-diagnostic sanitization in `status/providers.js`.
…ts version
The lookup caught node-fetch's FetchError/AbortError names, but dashmate runs
on Node's native fetch, which rejects with TypeError('fetch failed') and a
TimeoutError DOMException. Neither matched, so the null branch was unreachable
and the lookup threw on essentially every network failure.
The returned tag_name was also unvalidated and one character was stripped
unconditionally, mangling tags without a "v" prefix. Anything the API returns
is now required to be a valid semver before it is returned, printed or cached,
which keeps package-manager specifiers (git+https:, file:, npm:) and terminal
control sequences out of a value the updater will act on.
Also enforces the declared response-size cap on JSON bodies, cancels bodies on
paths that skip reading them, treats rate limiting as unknown rather than as an
error, sends GITHUB_TOKEN when present, and rejects insight responses whose
shape would crash the status renderer.
Test would have caught this in CI: 14 of 18 new specs fail before the fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… node Docker reports pull failures such as registry rate limiting and a full disk as in-band error objects on an otherwise successful stream, so docker-modem's followProgress resolves and the old hand-rolled parser looked only for status lines. Failures were therefore reported as a coloured word in a table, with the reason printed only under DEBUG and an exit code of 0, so automation could not tell "updated everything" from "updated nothing". Restart compounded that: it stopped every service and only then let compose pull whatever was missing, so a pull that failed in that window left the node down. Required images are now confirmed present before anything is stopped, which for a masternode is the difference between a failed command and missed blocks. Pull-stream parsing moves to docker-modem's followProgress, which buffers across chunk boundaries and splits on the separator Docker actually emits. The previous parser split each chunk on CRLF and parsed every fragment, so an ordinary TCP boundary threw inside the stream handler where nothing could catch it. Services built from local sources are reported as such instead of being pulled, and group restart gains the same pre-stop guarantee across every node. Test would have caught this in CI: 11 new specs fail before the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ft exposed Keys written before Dashmate set a mode are group- and world-readable, and both certificate writers now create and repair their own. Neither covers a key that is never rewritten: nothing inspects the mode of an SSL file, and on the ZeroSSL path a renewal reuses the existing key and skips the write entirely, so a deployed host stays exposed indefinitely. Permissions are therefore also restricted when starting the node, which is the one action every operator performs regardless of certificate provider, and doctor reports a key other users can read so it is visible in the meantime. Only the group and world bits are dropped, so an owner that hardened the key further keeps what it chose. Neither creates the file if it is absent: an empty key would convince the certificate validators one exists. The two historical migrations that copy the key restrict it as well, since copyFileSync reproduces the source mode and would otherwise carry the old permissions forward. Also fixes config and group default printing the current name. An args default of null is never applied by oclif's parser, so the argument stayed undefined and both commands failed instead of reporting, which matters because a planned feature treats them as read-only. Test would have caught this in CI: 8 specs fail before the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Group restart confirms every node's images are present before the first node is stopped, but the confirmation is a pull, and a pull deliberately skips services built from local sources. Those were built by the group start instead, which runs once every node is already down - so a missing base image or a compile error still left the whole group stopped, which is the outage the pre-stop check exists to prevent. The build now runs before the pull, and the group start is told the images are already there so a restart does not pay for the build twice. Test would have caught this in CI: 2 of the 4 new specs fail when the build is left to the group start. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the terminal A failed pull is now reported with the reason the registry gave, and that text reaches the operator's terminal unchanged. The registry chooses it: an escape sequence in it can erase the screen, move the cursor back over lines already printed or address the terminal itself, and an unbounded message pushes the rest of the output out of the scrollback. The same text is also carried in the JSON output, where a caller may hand it on somewhere else. Control characters and bidirectional overrides are therefore removed, the message is collapsed onto one line and its length is bounded, at the two points where the text is taken: the failure a pull stream carries in band, and the message the pull callback reports. Test would have caught this in CI: the new update spec fails before the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s rewritten The replacement key is written into the inode the old one occupies, because the gateway's bind mount follows that inode rather than the path. Tightening the mode only after the write would leave the new secret behind the old, world readable permissions for as long as the write takes. The task already restricts the mode first, and the existing specs check only the mode the file is left with - which stays correct either way. This pins the order itself, so the window cannot be reopened by a refactor. Test would have caught this in CI: it fails when the tightening is moved after the write. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t behind The escape sequences that rewrite a terminal were already removed, but several characters that hide or reorder text were not: the Arabic letter mark, zero width space and joiner, the line and paragraph separators, and the byte order mark all survived into a message an operator reads to decide what to do next. Test would have caught this in CI: 6 of the new cases fail before the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
shumkov
force-pushed
the
fix/dashmate/update-path-bugs
branch
from
August 31, 2026 13:57
93fd1ee to
68d467e
Compare
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
Issue being fixed or feature implemented
Three independent defects found while working on dashmate's update path. Each is reachable today and none depends on the others.
A failed image pull can leave a masternode down.
restartstops every service and only then lets compose pull whatever is missing, so a pull that fails in that window leaves the node stopped. For a masternode that is missed blocks and PoSe-penalty exposure.dashmate updatereports success when it updated nothing. Docker reports pull failures — rate limiting, a full disk — as in-band error objects on an otherwise successful stream, and the old parser looked only for status lines. Failures surfaced as a coloured word in a table, with the reason printed only underDEBUG, and an exit code of 0. Automation could not distinguish "updated everything" from "updated nothing".The GitHub release lookup throws on almost any network failure. It caught node-fetch's error names, but dashmate runs on Node's native fetch, which rejects with
TypeError('fetch failed')and aTimeoutErrorDOMException. Neither matched, so itsnullbranch was unreachable. It also stripped one character fromtag_nameunconditionally, mangling any tag without avprefix.Plus two smaller ones: the gateway's TLS private key was written world-readable, and
dashmate config defaultwith no argument threw instead of printing the default, because oclif never applies an argsdefault: null.What was done?
Required images are confirmed present before anything is stopped. Pull-stream parsing moves to docker-modem's
followProgress, which buffers across chunk boundaries and splits on the separator Docker actually emits — the previous parser split each chunk on CRLF and parsed every fragment, so an ordinary TCP boundary threw inside the stream handler where nothing could catch it.group restartgains the same pre-stop guarantee across every node.The release lookup now returns unknown instead of throwing, enforces its declared response-size cap, and requires a valid semver before returning a version — which keeps package-manager specifiers and terminal control sequences out of a value callers may act on.
The TLS key is written
0600by both writers, the two historical migrations that copy it no longer carry the old mode forward, permissions are restricted when starting the node so existing installations are actually fixed rather than only new ones, anddoctorreports a key other users can read.How Has This Been Tested?
Failing-test-first throughout; each fix has a test shown red against the unfixed code. The suites were then mutation-tested — deliberately weakened implementations were written to confirm the tests catch them, which is how several of these defects were found in the first place. 257 unit tests pass.
Not executed: no live gateway or masternode was exercised (no Docker daemon available), so container-readability of the tightened key rests on the compose wiring —
ENVOY_UIDis the operator's own uid — rather than a running container.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only