fix(dashmate): keep the node up when an image pull fails, and stop reporting success when it did not - #4283
fix(dashmate): keep the node up when an image pull fails, and stop reporting success when it did not#4283shumkov wants to merge 3 commits into
Conversation
…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>
…owner The key was written with default permissions, so under the usual umask it landed world-readable. Both writers are fixed, and the two historical migrations that copy the key now restrict it as well, since copyFileSync reproduces the source mode and would otherwise carry the old permissions forward. Writing it correctly only helps new installations. Nothing in dashmate inspects the mode of any SSL file, and on the ZeroSSL path a renewal reuses the existing key and skips the write entirely, so a deployed host would have stayed 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. Neither creates the file if it is absent: an empty key would convince the certificate validators one exists. 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 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 59 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (32)
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 |
|
⛔ Blockers found — Sonnet deferred (commit 93fd1ee) |
thepastaclaw
left a comment
thepastaclaw
left a comment
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`.
| title: 'Pull missing images', | ||
| task: () => ( | ||
| new Listr(configGroup.map((config) => ({ | ||
| task: (ctx, task) => dockerCompose.pullMissingImages(config, { | ||
| onProgress: (message) => { | ||
| // eslint-disable-next-line no-param-reassign | ||
| task.output = message; | ||
| }, | ||
| }), | ||
| }))) |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['codex']
| // The private key must not be readable by other users on the host. | ||
| // chmod is required in addition to the write mode because the mode is | ||
| // applied only when the file is created, and the key is usually rewritten | ||
| fs.writeFileSync(keyFile, ctx.privateKeyFile, { encoding: 'utf8', mode: 0o600 }); | ||
| fs.chmodSync(keyFile, 0o600); |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['codex']
| const reasons = failedServices | ||
| .map(({ title, image, error }) => ` ${title} (${image}): ${error}`) | ||
| .join('\n'); | ||
|
|
||
| // Report to stderr to keep machine-readable output on stdout intact | ||
| // eslint-disable-next-line no-console | ||
| console.error(`\nFailed to update ${failedServices.length} of ${updateInfo.length} images:\n\n${reasons}\n`); |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['codex']
2 participants
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