fix(dashmate)!: stop read-only commands rewriting config and clobbering concurrent updates - #4248
fix(dashmate)!: stop read-only commands rewriting config and clobbering concurrent updates#4248shumkov wants to merge 7 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>
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (8)
📝 WalkthroughWalkthroughDashmate configuration hydration now preserves clean state, configuration mutations use locked read-modify-write operations with atomic persistence, and templates render from pre-save change tracking. Config and group commands, dependencies, tests, and concurrency documentation were updated accordingly. ChangesDashmate configuration safety
Estimated code review effort: 4 (Complex) | ~45 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 |
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>
|
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>
|
🕓 Ready for review — 7 ahead in queue (commit dd795e9) |
thepastaclaw
left a comment
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.
| configFile.markAsSaved(); | ||
| configFile.getAllConfigs().forEach((config) => config.markAsSaved()); |
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']
| 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); |
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>
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>
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>
Issue being fixed or feature implemented
Closes #4242.
Config's constructor delegates tosetOptions(), which marks the config changed — so every config is dirty the moment it is 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 this 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. Callers that build a config which must reach disk (createConfigFile,createConfig) already mark it explicitly.The dirty flag now means one thing.
writeConfigTemplates()no longer callsconfig.markAsSaved()— rendering templates is not persistingconfig.json. The repository clears that state instead, and only after the write succeeds (previously it was cleared before the I/O, so a throwing write left configs claiming to be saved).Writes are atomic.
write-file-atomic, which preserves the existing file mode —config.jsonholds masternodeprivateKey, Core RPCpasswordand ZeroSSLapiKey.Stale writes are refused, not merged. The repository remembers what it last observed on disk and, under a short
proper-lockfilelock held only across compare-and-replace, refuses to write if that changed. The lock is deliberately not held for command duration — that would serialize every invocation behind a multi-minutestart. Because the lock lives at the repository seam, the helper daemon and the SSL renewal paths inherit it without going throughBaseCommand.Merging concurrent edits was considered and rejected: a silently mis-merged config is worse than the bug. On refusal the in-memory state is parked at
config.json.rejected-<stamp>-<pid>-<n>(mode 0600) and named in the error, so material generated during a command — operator keys, a spork key, a miner address — is never lost. Lock-release failures are contained so they can never replace that error with a message about lock bookkeeping.Service templates are rendered only for configs that actually changed. Previously every command re-rendered and rewrote all of them — 40 files for a four-config home directory, on
config getas much as onsetup. That was never intentional: hydration marked every config changed, so the existing "changed configs only" filter matched all of them.This is safe because upgrading Dashmate records the new version in the config file even when no migration applies —
getConfigFormatVersionreturnsmax(newestMigrationKey, packageVersion), andmigrateConfigFilestamps it regardless of whether any migration matched.read()then marks every config changed because the recorded version moved, so a release that edits a template still reaches every node on its next command. That property is load-bearing and was untested; it is pinned now.How Has This Been Tested?
183 dashmate unit tests pass; eslint clean on changed files. Every behavioural change was written test-first and observed failing against the unfixed code:
expected true to be falseexpected undefined to existexpected 13 to be at least 350expected '98.0.0' to equal '99.0.0'Two test-design traps were found and avoided:
Local E2E run —
packages/dashmate/test/e2e/localNetwork.spec.js, 5/5 passing in 30 minutes: setup → start → restart → stop → reset on a real local network. This is the job CI skips for a dashmate-only diff, and it is the coverage that matters most here: those are the commands that actually mutate config, sosetupgenerating spork/operator keys andresetrestoring defaults both exercised the new write path. No.rejected-*files were produced across a full lifecycle.The design and the diff were both reviewed by independent cross-model passes, which caught two P1 bugs in the first implementation (concurrent first-run writes still clobbering; a deleted config file being silently resurrected) and the lock-release masking issue. All are fixed and covered above.
Breaking Changes
A command that would previously have silently overwritten a concurrent configuration change now fails with a non-zero exit and
ConfigFileConflictError(code: 'DASHMATE_CONFIG_FILE_CONFLICT'). Automation that relied on last-writer-wins will now see failures where it previously saw silent data loss. The refused configuration is written to the.rejected-*file named in the error.Two documented limits (
packages/dashmate/docs/config/index.md):Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes