fix(dashmate)!: stop concurrent commands reverting each other's config changes - #4248
Open
shumkov wants to merge 10 commits into
Open
fix(dashmate)!: stop concurrent commands reverting each other's config changes#4248shumkov wants to merge 10 commits into
shumkov wants to merge 10 commits into
Conversation
…ng concurrent updates `Config`'s constructor delegated to `setOptions()`, which marks the config changed, so every config was dirty the moment it was read off disk. `BaseCommand.finally()` persists the whole config file whenever anything is dirty, so every successful command — including pure readers like `config get` and `core cli` — rewrote `config.json` from its own load-time snapshot. That turns any overlapping pair of commands into a lost update: a reader loads at T0, `config set` writes and exits 0 at T1, and the reader writes its stale snapshot at T2. The setting silently reverts even though `config set` reported success. It is invisible in single-process use because re-serializing an unchanged config is byte-identical. - Hydrating a config no longer marks it changed. Callers that build a config which must reach disk already mark it explicitly. - Rendering service templates no longer clears persistence state; the repository clears it, and only after the write actually succeeds. - Writes are atomic (`write-file-atomic`, preserving file mode — `config.json` holds masternode keys, RPC passwords and SSL API keys). - A write is refused when the file changed on disk since it was read, under a short `proper-lockfile` lock held only across compare-and-replace, never for the duration of a command. The refused state is parked next to `config.json` so material generated during the command is never lost. The lock is cooperative and scoped to local filesystems; both limits are documented for operators. BREAKING CHANGE: a command that would have silently overwritten a concurrent configuration change now fails with a non-zero exit and `ConfigFileConflictError`. Automation that relied on the previous last-writer-wins behaviour will now see failures where it previously saw silent data loss. Tests would have caught this in CI: hydrated config is clean ✖ before → ✔ after stale write refused, not clobbering ✖ before → ✔ after concurrent first-run write refused ✖ before → ✔ after deleted config file not resurrected ✖ before → ✔ after saved flags cascade only after a write ✖ before → ✔ after waits for a lock held by another process ✖ before → ✔ after The lock test spawns a real second process and fails without the lock (write completes in 13ms instead of waiting 700ms), so it pins the lock rather than the staleness comparison. Closes #4242 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDashmate configuration hydration now preserves clean state, mutations use locked read-modify-save operations with atomic persistence, and templates render from saved state. Commands, certificate and miner workflows, tests, dependencies, and concurrency documentation were updated. ChangesDashmate configuration locking
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js (3)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded format version will drift.
'4.1.0'duplicates the production config format version; when it bumps, these tests seed a file that no longer matches the schema/migration expectations. Prefer importing the same constant/source the repository uses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js` at line 9, Replace the hardcoded CURRENT_FORMAT_VERSION in ConfigFileJsonRepository.spec.js with the production config format version constant or source used by ConfigFileJsonRepository, so tests always seed files using the current schema version.
297-300: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHot busy-wait spins a core for up to 10s on failure.
fs.existsSyncin a tight loop pegs a CPU in CI when the child fails to start. Reuse anAtomics.wait-based sleep between polls.♻️ Suggested tweak
const deadline = Date.now() + 10000; while (!fs.existsSync(lockPath) && Date.now() < deadline) { - // busy-wait + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 10); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js` around lines 297 - 300, Replace the tight polling loop around lockPath in the ConfigFileJsonRepository test with an Atomics.wait-based sleep between fs.existsSync checks, preserving the existing 10-second deadline and success condition. Ensure the polling still exits promptly when the lock appears while avoiding continuous CPU usage.
273-319: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueChild process may leak if an assertion throws.
Any failing
expectbetween Lines 302-316 aborts the test before theexitlistener path completes, leaving the spawned child (and the lock dir) around until its own timer fires. Achild.kill()in anafterEach/try-finallymakes the failure mode deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js` around lines 273 - 319, Ensure the spawned child in the “should wait for a lock held by another process before writing” test is always cleaned up when an assertion or repository operation fails. Wrap the synchronous test flow in try/finally or add equivalent teardown that kills the child and removes any remaining lock directory, while preserving the existing exit callback and success assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dashmate/docs/config/index.md`:
- Around line 192-197: Add the `text` language identifier to the fenced code
block containing the configuration overwrite warning in the documentation, while
preserving the warning text unchanged.
In `@packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js`:
- Around line 174-198: Update the lock acquisition and release flow around
`#acquireLock` so proper-lockfile uses a non-throwing onCompromised handler and
release failures are caught in finally. Preserve the original operation or
ConfigFileConflictError when release() reports ERELEASED, and prevent
compromised-lock refresh errors from becoming uncaught asynchronous exceptions.
---
Nitpick comments:
In
`@packages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js`:
- Line 9: Replace the hardcoded CURRENT_FORMAT_VERSION in
ConfigFileJsonRepository.spec.js with the production config format version
constant or source used by ConfigFileJsonRepository, so tests always seed files
using the current schema version.
- Around line 297-300: Replace the tight polling loop around lockPath in the
ConfigFileJsonRepository test with an Atomics.wait-based sleep between
fs.existsSync checks, preserving the existing 10-second deadline and success
condition. Ensure the polling still exits promptly when the lock appears while
avoiding continuous CPU usage.
- Around line 273-319: Ensure the spawned child in the “should wait for a lock
held by another process before writing” test is always cleaned up when an
assertion or repository operation fails. Wrap the synchronous test flow in
try/finally or add equivalent teardown that kills the child and removes any
remaining lock directory, while preserving the existing exit callback and
success assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d46df355-b215-4071-93e5-830b41f4f559
⛔ Files ignored due to path filters (3)
.yarn/cache/proper-lockfile-npm-4.1.2-a140a3c928-000a4875f5.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/write-file-atomic-npm-5.0.1-52283db6ee-648efddba5.zipis excluded by!**/.yarn/**,!**/*.zipyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
.pnp.cjspackages/dashmate/docs/config/index.mdpackages/dashmate/package.jsonpackages/dashmate/scripts/helper.jspackages/dashmate/src/config/Config.jspackages/dashmate/src/config/configFile/ConfigFileJsonRepository.jspackages/dashmate/src/config/errors/ConfigFileConflictError.jspackages/dashmate/src/oclif/command/BaseCommand.jspackages/dashmate/src/templates/writeConfigTemplatesFactory.jspackages/dashmate/test/unit/config/Config.spec.jspackages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js
💤 Files with no reviewable changes (1)
- packages/dashmate/src/templates/writeConfigTemplatesFactory.js
`proper-lockfile`'s `release()` reports ERELEASED/ENOTACQUIRED when the lock was compromised or already gone. Calling it bare in `finally` let that replace the outcome the caller needs — an operator hitting a concurrent write would have been told "Lock is already released" instead of being given the conflict and the path their configuration was parked at. Release failures are now contained; a lock that cannot be released goes stale and is reclaimed by the next writer. The library's default `onCompromised` rethrows, and it runs from a refresh timer rather than the caller's stack, so a lock compromised mid-write would have taken the whole process down. Replaced with a handler that does not throw: the critical section lasts milliseconds, and the byte comparison against the baseline — not the lock — is what actually prevents a lost update. Also labels a fenced block in the config docs (MD040). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing its child The poll waiting for the other process to take the lock was a tight `fs.existsSync` loop, which pegs a CI core for the full ten-second timeout whenever the child fails to start. It now sleeps between polls. The spawned lock holder was only cleaned up on the success path, so a failing assertion left it running — still owning the lock directory — after the test that created it had gone. It is tracked and killed in `afterEach` instead, so the failure mode is deterministic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
|
Dispositions for the three nitpicks from the CodeRabbit review, for the record. Fixed in
Declining, with reasoning:
|
Every command re-rendered and rewrote all service templates — 40 files for a four-config home directory, on `dashmate config get` as much as on `setup`. That was never intentional: hydration marked every config changed, so the existing "changed configs only" filter matched all of them. Fixing hydration left the filter correct but the rendering unconditional, which kept half of the original side effect alive. Rendering only what changed is safe because upgrading Dashmate records the new version in the config file even when no migration applies, and `ConfigFileJsonRepository.read()` marks every config changed when that recorded version moved. So a release that edits a template still reaches every node on its next command, without paying for it on every unrelated command. That property was load-bearing and untested, so it is pinned now: removing the version stamp from `migrateConfigFile` fails the new test with `expected '98.0.0' to equal '99.0.0'`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
🕓 Ready for review — 1 ahead in queue (commit aae3308) |
thepastaclaw
suggested changes
Jul 29, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The compare-and-replace write path prevents stale writers from overwriting config.json, but the new dirty-state lifecycle is not safely integrated with existing SSL and template workflows. Intermediate SSL saves can discard pending render work, completed JSON writes can permanently consume the only template-upgrade signal, and the long-lived helper cannot recover after another process updates the file. These are three blocking regressions.
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
1 additional finding(s) omitted (not in diff).
🤖 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/config/configFile/ConfigFileJsonRepository.js`:
- [BLOCKING] packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js:194-195: Intermediate SSL saves discard pending template renders
Clearing every nested `Config` flag here breaks callers that save during a command and rely on `BaseCommand.finally()` to render afterward. The ZeroSSL pipeline updates `enabled`, `provider`, and the certificate ID and writes the `ConfigFile` at `obtainZeroSSLCertificateTaskFactory.js:145-150`; its remaining tasks only write certificate files, so nothing marks the config changed again. `BaseCommand.finally()` consequently sees a clean file and skips rendering, which can leave `config.json` switched from self-signed to ZeroSSL while Envoy still has the old listener configuration. The Let's Encrypt save task re-dirties the selected config, but its intermediate write still clears migration-triggered flags on every other config, suppressing their required first-command-after-upgrade renders. Preserve pending-template state across intermediate persistence, or explicitly render the captured pending configs after these writes.
In `packages/dashmate/src/oclif/command/BaseCommand.js`:
- [BLOCKING] packages/dashmate/src/oclif/command/BaseCommand.js:119-130: A template failure permanently consumes the upgrade marker
`BaseCommand.finally()` commits the new config format version and clears all dirty flags before rendering any service template. If rendering or writing a template throws, or the process terminates after the config write, `config.json` remains stamped with the current version while one or more generated files remain stale. The next process hydrates every config as clean, and migration no longer marks them because the recorded version already matches, so the failed and unattempted templates are not retried automatically. This defeats the upgrade invariant on which render-only-changed behavior relies. Keep a durable pending-render indication until all affected templates have been written successfully, or provide an equivalent recovery mechanism.
In `packages/dashmate/scripts/helper.js`:
- [BLOCKING] packages/dashmate/scripts/helper.js:93: The helper cannot recover after another process updates config.json
The helper reads `config.json` only once and retains that repository, `ConfigFile`, and selected `Config` for its entire lifetime. Under the new compare-and-replace contract, any later Dashmate command that changes the shared file invalidates the helper's baseline, including changes to an unrelated config. Certificate renewal then mutates the stale objects and writes through the stale repository, producing `ConfigFileConflictError`. Both renewal schedulers catch failures and schedule subsequent work with the same objects without re-reading, so every future renewal write remains unable to commit. Because the error is swallowed, the `unless-stopped` helper container stays alive rather than restarting and reloading, allowing gateway certificate renewal to remain broken until an external restart. Reload and reselect the current config before renewal mutations, or explicitly abandon and reload the helper state after a conflict.
Comment on lines
+194
to
+195
| configFile.markAsSaved(); | ||
| configFile.getAllConfigs().forEach((config) => config.markAsSaved()); |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Intermediate SSL saves discard pending template renders
Clearing every nested Config flag here breaks callers that save during a command and rely on BaseCommand.finally() to render afterward. The ZeroSSL pipeline updates enabled, provider, and the certificate ID and writes the ConfigFile at obtainZeroSSLCertificateTaskFactory.js:145-150; its remaining tasks only write certificate files, so nothing marks the config changed again. BaseCommand.finally() consequently sees a clean file and skips rendering, which can leave config.json switched from self-signed to ZeroSSL while Envoy still has the old listener configuration. The Let's Encrypt save task re-dirties the selected config, but its intermediate write still clears migration-triggered flags on every other config, suppressing their required first-command-after-upgrade renders. Preserve pending-template state across intermediate persistence, or explicitly render the captured pending configs after these writes.
source: ['codex']
Comment on lines
119
to
+130
| configFileRepository.write(configFile); | ||
|
|
||
| /** | ||
| * @var {writeConfigTemplates} writeConfigTemplates | ||
| */ | ||
| const writeConfigTemplates = this.container.resolve('writeConfigTemplates'); | ||
|
|
||
| configFile.getAllConfigs() | ||
| .filter((config) => config.isChanged()) | ||
| .forEach(writeConfigTemplates); | ||
| // Re-rendering only what changed is safe because upgrading Dashmate | ||
| // stamps the new version into the config file even when no migration | ||
| // applies, which marks every config changed - so a release that edits | ||
| // a template still reaches every node on its next command. | ||
| changedConfigs.forEach(writeConfigTemplates); |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: A template failure permanently consumes the upgrade marker
BaseCommand.finally() commits the new config format version and clears all dirty flags before rendering any service template. If rendering or writing a template throws, or the process terminates after the config write, config.json remains stamped with the current version while one or more generated files remain stale. The next process hydrates every config as clean, and migration no longer marks them because the recorded version already matches, so the failed and unattempted templates are not retried automatically. This defeats the upgrade invariant on which render-only-changed behavior relies. Keep a durable pending-render indication until all affected templates have been written successfully, or provide an equivalent recovery mechanism.
source: ['codex']
Reading the config file when a command starts and saving it when the command
exits means the state written is a snapshot that may already be out of date, so
an overlapping command's change is reverted. Detecting that afterwards — refusing
the write, parking the rejected state in a file and asking the operator to
reconcile — treated the symptom and put the burden on them.
`ConfigFileJsonRepository.update(mutator)` instead takes the lock, reads the
current state, applies the change and saves, all in one step. The state handed to
the mutator is read after the lock is held, so there is nothing stale to write and
nothing to detect: two commands changing different options now both succeed.
`dashmate config set` uses it, against the config name resolved for that command
rather than re-resolving the default, which another process may have changed.
Removed, because none of it has anything left to do:
- `ConfigFileConflictError`
- the `config.json.rejected-*` snapshots and their naming
- the observed-state baseline and the compare-before-write
- the operator recovery procedure in the config docs
Demonstrated on a real config file — read-early/write-late loses the other
command's edit, `update()` keeps both:
OLD read-early/write-late: other command edit survived => false
NEW update(): other command edit survived => true
this command edit survived => true
`setup`, `reset` and `ssl obtain` still load at start and save at exit, so
changing config with `config set` while one of them runs can still lose the
change. Documented rather than solved: they are exclusive operations, and the
mechanisms that would cover them cost more than the case is worth.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
shumkov
force-pushed
the
fix/dashmate/4242-readonly-config-clobber
branch
from
July 30, 2026 12:45
6702178 to
7abb4c3
Compare
…command `config set` already read, changed and saved as one step; `create`, `remove`, `default` and `group default` still mutated the config file loaded at startup and relied on it being saved on exit, so they could revert a change another command saved while they ran. `config remove` also deleted the service directory before that save, so a failed save left a config listed in config.json with nothing on disk behind it. The directory is now deleted only once the removal is saved, and removing a config another command already removed fails before anything is written. `config create` renders the new config's service files itself. It used to get that from the generic save-on-exit, which no longer sees a change because the config file handed to the command is deliberately not the one that was modified. Verified end to end for each command: the change reaches disk and the config file loaded at startup is left untouched, which is what stops the generic save from writing a pre-command snapshot afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/dashmate/src/commands/config/create.js (1)
21-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the new injected parameters.
configFileRepositoryandwriteConfigTemplatesare resolved by name from the DI container; leaving them out of the JSDoc makes the injection contract easy to break on rename. Same applies todefault.js,remove.js,set.js, andgroup/default.js.♻️ Proposed doc update
/** * `@param` {Object} args * `@param` {Object} flags * `@param` {ConfigFile} configFile + * `@param` {ConfigFileJsonRepository} configFileRepository + * `@param` {writeConfigTemplates} writeConfigTemplates * `@return` {Promise<void>} */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashmate/src/commands/config/create.js` around lines 21 - 45, Update the JSDoc for runWithDependencies in create.js to document the injected configFileRepository and writeConfigTemplates parameters, including their expected types. Apply the same dependency-parameter documentation to the corresponding runWithDependencies methods in default.js, remove.js, set.js, and group/default.js, preserving the existing injection names and behavior.packages/dashmate/test/unit/commands/config/mutatingCommands.spec.js (1)
114-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the command actually rejects.
.catch(() => {})swallows the outcome, so the test would still pass ifconfig removesilently succeeded on a missing config and simply skipped the delete. Pin the failure explicitly.♻️ Proposed test tightening
- await new ConfigRemoveCommand().runWithDependencies( - { config: 'does-not-exist' }, - flags, - loadedConfigFile, - { has: () => false }, - homeDir, - configFileRepository, - ).catch(() => {}); + let error; + try { + await new ConfigRemoveCommand().runWithDependencies( + { config: 'does-not-exist' }, + flags, + loadedConfigFile, + { has: () => false }, + homeDir, + configFileRepository, + ); + } catch (e) { + error = e; + } + + expect(error).to.exist();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashmate/test/unit/commands/config/mutatingCommands.spec.js` around lines 114 - 126, Update the test around ConfigRemoveCommand.runWithDependencies to explicitly assert that the command rejects when removing the missing “does-not-exist” configuration. Replace the empty catch that swallows the outcome with an assertion on the rejected promise, while preserving the existing checks that the service directory and configuration remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/dashmate/src/commands/config/create.js`:
- Around line 21-45: Update the JSDoc for runWithDependencies in create.js to
document the injected configFileRepository and writeConfigTemplates parameters,
including their expected types. Apply the same dependency-parameter
documentation to the corresponding runWithDependencies methods in default.js,
remove.js, set.js, and group/default.js, preserving the existing injection names
and behavior.
In `@packages/dashmate/test/unit/commands/config/mutatingCommands.spec.js`:
- Around line 114-126: Update the test around
ConfigRemoveCommand.runWithDependencies to explicitly assert that the command
rejects when removing the missing “does-not-exist” configuration. Replace the
empty catch that swallows the outcome with an assertion on the rejected promise,
while preserving the existing checks that the service directory and
configuration remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 18ab5426-a856-46b8-bfa7-fbcbbc673ca0
📒 Files selected for processing (10)
packages/dashmate/docs/config/index.mdpackages/dashmate/src/commands/config/create.jspackages/dashmate/src/commands/config/default.jspackages/dashmate/src/commands/config/remove.jspackages/dashmate/src/commands/config/set.jspackages/dashmate/src/commands/group/default.jspackages/dashmate/src/config/configFile/ConfigFileJsonRepository.jspackages/dashmate/test/unit/commands/config/mutatingCommands.spec.jspackages/dashmate/test/unit/commands/config/set.spec.jspackages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js
`setup`, `reset`, `group reset` and `ssl obtain` change configuration repeatedly while doing long, partly irreversible work. They still loaded config when they started and saved it when they finished, so a `config set` running in between was reverted — the one case the locked read-change-save step could not cover, since their changes are not a single self-contained edit. They now take the lock before reading, via `static mutatesConfig = true`, and hold it for the run. Reading inside the lock is what makes their state current: there is no window left for another writer. Read-only commands take no lock, so a node can still be inspected while it is being set up. Three things this needs to be safe: - The lock is reused, not re-taken, by `update()` and `write()` during the command. Taking it again would leave the command waiting on itself until the acquire timeout — verified: removing the guard fails the new test with "Lock file is already being held" after 15s. - Release covers every way out: normal finish, command failure, a failure before the command body runs, and graceful shutdown. It is idempotent so those paths need not coordinate. - A lock lost while we believed we held it now blocks the next save instead of being ignored. Ignoring it was right when the lock only narrowed a window; it is not right when the lock is what provides exclusivity. The stale threshold moves to 60s so a long command cannot have its live lock stolen during a synchronous stretch, while the acquire wait drops to 15s so a waiting command reports quickly rather than stalling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…wait Losing a held lock and giving up waiting for someone else's were both reachable only after seconds of real time, so neither was tested — the compromise handling in particular rested on reading the code. Lock timings are now injectable, which lets both run in about a second. They are real regression tests: with the previous no-op compromise handler, saving after losing the lock succeeds and the test fails. Waiting too long now reports in dashmate's terms — which command is holding configuration and what to do — instead of surfacing the locking library's "Lock file is already being held". Worth recording, since it is not obvious and it sets the floor on how fast a lost lock can possibly be noticed: proper-lockfile clamps `stale` to at least 2s and the refresh that detects loss to at least 1s. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
shumkov
changed the title
Converting the config commands left two mechanisms writing configuration, and the older one could still overwrite the newer. A cross-model review found five ways that showed up; all of them come from the generic save-on-exit persisting a copy loaded before the command ran. - `BaseCommand.finally()` now saves only for commands that hold the lock for their whole run. They read inside the lock, so their copy is current. Every other command changes configuration through `update()`, and saving its startup copy afterwards would revert whatever that produced — most visibly on a config file needing migration, where `config set` would have reverted its own change. - Migrating is saved where it happens, at startup, instead of being carried to the end of the command as a pending change. - `update()` creates the config file when there is none, so `config create` and friends work on first run. They previously threw `ConfigFileNotFoundError` after `BaseCommand` had built defaults that only the old save-on-exit knew about. - Rendering service files and removing a config's directory happen inside the lock, so two commands cannot save in one order and take effect in the other, and a removal cannot delete files a concurrent re-creation just wrote. - `ssl obtain` renders its own templates. Saving marks the configs clean, so nothing later knew the new certificate had to reach the gateway config. - Local group start and restart save the generated miner address as its own locked step rather than leaving it for exit, which would have written a copy from before a start that runs for tens of minutes. - Helper certificate renewal re-applies its result onto current state. It holds its copy for the life of the process — months — so saving that copy reverted everything changed on the node since it started. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js (1)
317-350: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
#compromisedis never reset on the standalone lock path — a single compromise permanently disables saves.
#compromisedis only cleared inacquire()(line 210).#locked()'s standalone branch (used by every plainupdate()/write()call, e.g. fromscheduleRenewLetsEncryptCertificateFactory/scheduleRenewZeroSslCertificateFactory) reaches#acquireLock()directly and never resets the flag after a fresh, successful acquisition. OnceonCompromisedfires once (line 332-334) on any instance — including a long-lived singleton used by the helper daemon — every future save via that instance throws "Lost the lock" forever, even though each call re-acquires the lock legitimately, until the process restarts.🔒 Proposed fix: reset compromise state on every successful acquisition
`#acquireLock`() { const deadline = Date.now() + this.lockAcquireTimeoutMs; for (;;) { try { - return lockfile.lockSync(this.configFilePath, { + const release = lockfile.lockSync(this.configFilePath, { lockfilePath: this.lockFilePath, realpath: false, stale: this.lockStaleMs, onCompromised: () => { this.#compromised = true; }, }); + + // A fresh, successful acquisition re-establishes exclusivity, so a + // compromise recorded during an earlier session must not keep + // failing saves made under this new lock. + this.#compromised = false; + + return release; } catch (e) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js` around lines 317 - 350, Reset `#compromised` after every successful lock acquisition in `#acquireLock`(), including the standalone path used by `#locked`() for plain update()/write() calls. Set it only after lockfile.lockSync succeeds, while preserving the existing compromised callback and error-handling behavior.packages/dashmate/src/oclif/command/BaseCommand.js (1)
145-183: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftWrite-before-render ordering loses the upgrade/migration retry marker on template failure (two sites, one root cause). In both places,
configFileRepository.write(configFile)commits and clears dirty/version flags before templates are rendered from the (now stale) capturedchangedConfigs; if rendering throws partway through, the config file is durably marked clean/current while some generated service files remain stale, with no future migration-triggered retry. This matches a previously-raised, apparently still-unaddressed review finding on this pattern.
packages/dashmate/src/oclif/command/BaseCommand.js#L145-L183: insaveConfigAndStopContainers, renderchangedConfigsviawriteConfigTemplates(or otherwise confirm success) before callingconfigFileRepository.write(configFile), or retain a separate pending-render marker that survives a partial rendering failure.packages/dashmate/src/oclif/command/BaseCommand.js#L73-L85: apply the same reordering/marker fix to the migration-triggered write ininit().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashmate/src/oclif/command/BaseCommand.js` around lines 145 - 183, In packages/dashmate/src/oclif/command/BaseCommand.js lines 145-183, update saveConfigAndStopContainers to render changedConfigs with writeConfigTemplates successfully before configFileRepository.write(configFile), or preserve a pending-render marker across partial failures. Apply the same ordering or marker fix to the migration-triggered write in init at lines 73-85; both sites must prevent the configuration from being marked clean/current until all template rendering completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dashmate/src/helper/scheduleRenewLetsEncryptCertificateFactory.js`:
- Around line 78-91: The certificate renewal schedulers must update only
renewal-produced fields on freshly loaded configuration, rather than replacing
stale SSL snapshots or persisting stale task inputs. In
packages/dashmate/src/helper/scheduleRenewLetsEncryptCertificateFactory.js:78-91
and
packages/dashmate/src/helper/scheduleRenewZeroSslCertificateFactory.js:82-95,
change the fresh-state callbacks to patch individual renewed fields; in
packages/dashmate/src/listr/tasks/ssl/letsencrypt/obtainLetsEncryptCertificateTaskFactory.js:257-261
and
packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js:151-155,
skip the direct write(configFile) when invoked by the background schedulers so
the scheduler updates own persistence.
In `@packages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.js`:
- Around line 96-101: Remove the subsequent minerConfig.set(...) assignment in
the group-node startup flow after configFileRepository.update. Use the existing
minerAddress value for the downstream logic, leaving minerConfig as the original
snapshot so command-exit persistence cannot overwrite concurrent configuration
changes.
---
Outside diff comments:
In `@packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js`:
- Around line 317-350: Reset `#compromised` after every successful lock
acquisition in `#acquireLock`(), including the standalone path used by `#locked`()
for plain update()/write() calls. Set it only after lockfile.lockSync succeeds,
while preserving the existing compromised callback and error-handling behavior.
In `@packages/dashmate/src/oclif/command/BaseCommand.js`:
- Around line 145-183: In packages/dashmate/src/oclif/command/BaseCommand.js
lines 145-183, update saveConfigAndStopContainers to render changedConfigs with
writeConfigTemplates successfully before configFileRepository.write(configFile),
or preserve a pending-render marker across partial failures. Apply the same
ordering or marker fix to the migration-triggered write in init at lines 73-85;
both sites must prevent the configuration from being marked clean/current until
all template rendering completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8dda59f3-48b6-4f14-a47b-e495ca1ed7c1
📒 Files selected for processing (20)
packages/dashmate/docs/config/index.mdpackages/dashmate/scripts/helper.jspackages/dashmate/src/commands/config/create.jspackages/dashmate/src/commands/config/remove.jspackages/dashmate/src/commands/config/set.jspackages/dashmate/src/commands/group/reset.jspackages/dashmate/src/commands/reset.jspackages/dashmate/src/commands/setup.jspackages/dashmate/src/commands/ssl/obtain.jspackages/dashmate/src/config/configFile/ConfigFileJsonRepository.jspackages/dashmate/src/createDIContainer.jspackages/dashmate/src/helper/scheduleRenewLetsEncryptCertificateFactory.jspackages/dashmate/src/helper/scheduleRenewZeroSslCertificateFactory.jspackages/dashmate/src/listr/tasks/ssl/letsencrypt/obtainLetsEncryptCertificateTaskFactory.jspackages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.jspackages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.jspackages/dashmate/src/oclif/command/BaseCommand.jspackages/dashmate/test/unit/commands/config/mutatingCommands.spec.jspackages/dashmate/test/unit/commands/config/set.spec.jspackages/dashmate/test/unit/config/configFile/ConfigFileJsonRepository.spec.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/dashmate/scripts/helper.js
- packages/dashmate/docs/config/index.md
Comment on lines
+78
to
+91
| // This process has held its copy of the config file since it started, | ||
| // possibly for months, so saving that copy would revert everything | ||
| // changed on the node since. Re-apply just what the renewal produced | ||
| // onto the current state instead. | ||
| const renewedOptions = config.get('platform.gateway.ssl'); | ||
|
|
||
| configFileRepository.update((freshConfigFile) => { | ||
| freshConfigFile.getConfig(config.getName()) | ||
| .set('platform.gateway.ssl', renewedOptions); | ||
| }, { | ||
| onSaved: (savedConfigFile) => writeConfigTemplates( | ||
| savedConfigFile.getConfig(config.getName()), | ||
| ), | ||
| }); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Renewal still writes stale configuration snapshots. The schedulers retain config for potentially months, while the certificate tasks directly save the injected configFile; either write can discard unrelated concurrent changes. The later full platform.gateway.ssl replacement can also overwrite newer SSL settings.
packages/dashmate/src/helper/scheduleRenewLetsEncryptCertificateFactory.js#L78-L91: patch only renewal-produced fields onto freshly loaded state; do not replace the stale SSL subtree.packages/dashmate/src/helper/scheduleRenewZeroSslCertificateFactory.js#L82-L95: apply the same field-level fresh-state update.packages/dashmate/src/listr/tasks/ssl/letsencrypt/obtainLetsEncryptCertificateTaskFactory.js#L257-L261: avoidwrite(configFile)when invoked by the background scheduler; let its fresh-state update own persistence.packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js#L151-L155: avoid the equivalent direct stale-file write.
📍 Affects 4 files
packages/dashmate/src/helper/scheduleRenewLetsEncryptCertificateFactory.js#L78-L91(this comment)packages/dashmate/src/helper/scheduleRenewZeroSslCertificateFactory.js#L82-L95packages/dashmate/src/listr/tasks/ssl/letsencrypt/obtainLetsEncryptCertificateTaskFactory.js#L257-L261packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js#L151-L155
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/dashmate/src/helper/scheduleRenewLetsEncryptCertificateFactory.js`
around lines 78 - 91, The certificate renewal schedulers must update only
renewal-produced fields on freshly loaded configuration, rather than replacing
stale SSL snapshots or persisting stale task inputs. In
packages/dashmate/src/helper/scheduleRenewLetsEncryptCertificateFactory.js:78-91
and
packages/dashmate/src/helper/scheduleRenewZeroSslCertificateFactory.js:82-95,
change the fresh-state callbacks to patch individual renewed fields; in
packages/dashmate/src/listr/tasks/ssl/letsencrypt/obtainLetsEncryptCertificateTaskFactory.js:257-261
and
packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js:151-155,
skip the direct write(configFile) when invoked by the background schedulers so
the scheduler updates own persistence.
Comment on lines
+96
to
+101
| // Saved as its own locked step rather than carried to the end of the | ||
| // command: starting a group runs for many minutes, and the address is | ||
| // needed by the miner right now. | ||
| configFileRepository.update((configFile) => { | ||
| configFile.getConfig(minerConfig.getName()).set('core.miner.address', minerAddress); | ||
| }); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Do not re-dirty the stale minerConfig snapshot.
The following minerConfig.set(...) at Line 103 defeats this locked update: the command-exit dirty save can overwrite concurrent changes with its startup snapshot. Remove that assignment; minerAddress already supplies the value needed below.
Suggested fix
configFileRepository.update((configFile) => {
configFile.getConfig(minerConfig.getName()).set('core.miner.address', minerAddress);
});
-
- minerConfig.set('core.miner.address', minerAddress);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Saved as its own locked step rather than carried to the end of the | |
| // command: starting a group runs for many minutes, and the address is | |
| // needed by the miner right now. | |
| configFileRepository.update((configFile) => { | |
| configFile.getConfig(minerConfig.getName()).set('core.miner.address', minerAddress); | |
| }); | |
| // Saved as its own locked step rather than carried to the end of the | |
| // command: starting a group runs for many minutes, and the address is | |
| // needed by the miner right now. | |
| configFileRepository.update((configFile) => { | |
| configFile.getConfig(minerConfig.getName()).set('core.miner.address', minerAddress); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.js` around lines
96 - 101, Remove the subsequent minerConfig.set(...) assignment in the
group-node startup flow after configFileRepository.update. Use the existing
minerAddress value for the downstream logic, leaving minerConfig as the original
snapshot so command-exit persistence cannot overwrite concurrent configuration
changes.
The previous commit extended the locked read-change-save to SSL issuance, the helper's certificate renewal and local group start. Review found each of those introduced a new defect: templates rendered before a certificate was actually usable, a renewal that reverted a provider change made since the helper started, and a miner address chosen from stale state then written over fresh state. None of them is needed to fix the reported bug, and they touch certificate issuance, where being wrong costs an operator a working gateway. They are reverted to their previous behaviour and left for separate changes with their own tests. Migration is now read and saved under one lock rather than read unlocked and saved after. Splitting them left the same window this change exists to remove: a change saved in between would have been reverted by the migrated copy. Service files are re-rendered for whatever the migration changed, which the config file's own save no longer implies. Also fixes a test that passed for the wrong reason: `config default` set the config that was already the default, so it would have passed with the save deleted. It now points at a different config. 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
Closes #4242.
Config's constructor delegates tosetOptions(), which marks the config changed — so every config was dirty the moment it was read off disk.BaseCommand.finally()persists the whole config file whenever anything is dirty, so every successful command, including pure readers likedashmate config getanddashmate core cli, rewroteconfig.jsonfrom its own load-time snapshot.That turns any overlapping pair of commands into a lost update:
dashmate config set …docker.image Xloads, writes, prints success and exits 0 at T1.The setting silently reverts even though
config setreported success. Reported from a real rolling masternode image-pin update, where some nodes kept the old image digest.Two things made it hard to spot:
config.json), so it never shows up outside real concurrency.scripts/helper.jsreads config once at startup and writes it back on certificate renewal months later. Its guard is even commented "Persist config if it was migrated", and this bug made it fire on every startup.What was done?
Hydration is no longer a mutation.
Config's constructor resets the flag after the initialsetOptions().ConfigFilealready did exactly this;Configwas just missing it. This alone means read-only commands write nothing — they cannot clobber anyone.Changing configuration reads and saves as one locked step. Reading when a command starts and saving when it exits means the state written may already be out of date.
ConfigFileJsonRepository.update(mutator)takes the lock, reads the current state, applies the change and saves — the state handed to the mutator is read after the lock is held, so there is nothing stale to write:Used by
config set,create,remove,defaultandgroup default. Two commands changing different options now both succeed.Commands that reconfigure a node hold the lock for their run.
setup,reset,group resetandssl obtainchange configuration repeatedly while doing long, partly irreversible work, so a single locked step does not fit. They declarestatic mutatesConfig = true;BaseCommand.init()takes the lock before reading and releases it infinally(). Reading inside the lock is what makes their state current.Read-only commands never take the lock, so a node can still be inspected while it is being set up.
Supporting details that matter:
update()/write()rather than re-acquiring — otherwise the SSL task's mid-command save (obtainZeroSSLCertificateTaskFactory.js:150) would wait on itself.config.jsonholds masternodeprivateKey, Core RPCpasswordand ZeroSSLapiKey.config removedeletes the service directory only after the removal is saved, so a failed save cannot leave a config listed with nothing behind it.Merging concurrent edits was considered and rejected: in-memory holds the whole config rather than a diff, so "our copy wins" reverts every field we merely loaded — the original bug, reintroduced by the merge.
How Has This Been Tested?
193 unit tests pass; eslint clean on changed files. Behaviours were proven by breaking them, not by assertion:
expected true to be falsebefore the fixother command edit survived => false;update()→ bothtrueLock file is already being heldafter 15sexpected '98.0.0' to equal '99.0.0'config removekeeps service files if the save failsLock timings are injectable purely so the paths that only occur after seconds of waiting can be tested in about a second. (
proper-lockfilefloorsstaleat 2s and its refresh at 1s, which sets the limit on how fast a lost lock can be noticed.)Local E2E — 5/5 passing,
packages/dashmate/test/e2e/localNetwork.spec.js: setup → start → restart → stop → reset on a real local network. CI skips this job for a dashmate-only diff, and it is the coverage that matters most here, sincesetupandresetare exactly the commands that now hold the lock across their whole run.Design and code were reviewed by independent cross-model passes throughout, which caught several defects in earlier iterations — including two concurrency bugs and an error path that would have hidden the message an operator needs.
Breaking Changes
A command that changes configuration can now report that another dashmate command is modifying configuration and exit non-zero, after waiting ~15s. Previously it would have silently overwritten that other command's work. Automation that changes configuration while
setup,resetorssl obtainis running will see this; retrying once the other command finishes succeeds.The guarantee is between cooperating dashmate versions on a local filesystem: a pre-fix process does not honour the lock, so upgrade all instances and restart the node (which restarts the helper) before relying on it.
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation