fix(cli): stage generated config files before commit by yuanweize · Pull Request #318 · emqx/hocon

yuanweize · 2026-08-04T01:15:46Z

Problem

When disk space is exhausted (ENOSPC), writing generated app.<time>.config and vm.<time>.args directly can create or truncate final files before the write fails. A later startup may then pick up a 0-byte or incomplete generated configuration.

Failure-path review also found that earlier revisions could delete an existing target, publish only one of the two generated files, accumulate abandoned temp files, prune the just-written generation, or fail to reclaim excess history before an ENOSPC retry.

Solution

Stage both generated outputs in unique, exclusive temporary files in the destination directory.
Apply an existing regular target's permission bits to its staging/rollback file before writing content.
Write, sync, and close every temporary file before committing either output.
Keep a synced rollback copy only when replacing a pre-existing target.
Rename vm.args first and app.config last.
If a later rename returns an error, roll back earlier commits in reverse order:
- remove a target created by this generation; or
- restore the previous target contents from its rollback copy.
Clean up uncommitted temporary files and rollback copies on handled failure paths.
Remove stale temp files only when they match the exact generated target family and numeric temp suffix format.
Prune excess history before staging to reclaim space, excluding the current destination and reserving one slot for it; prune again after commit to enforce the final limit.
Match history by the complete destination base, extension, and timestamp shape so a dotted --dest_file cannot prune another config family.

This provides rollback for handled staging/rename errors. It does not claim a strict two-file ACID transaction across process crashes, concurrent generators, or power loss.

Regression tests

Coverage includes:

staging and backup-preparation failures preserving existing targets;
an injected second-rename failure removing a partial new generation;
the same failure restoring pre-existing target contents;
an older explicit timestamp retaining the current generation during pruning;
exact file-family pruning for dotted destination names;
stale temp cleanup without deleting fresh, unrelated-family, or non-generated temp files;
replacement preserving existing regular-file permission bits;
a failed backup restore preserving the current target and logging the rename error.

Testing

Erlang/OTP 23: production rebar3 compile passed
Erlang/OTP 26: Elvis passed; both changed files pass erlfmt
Erlang/OTP 27: Dialyzer, xref, full EUnit, and coverage passed — 584 tests
Erlang/OTP 28: Dialyzer, full EUnit, and coverage passed — 584 tests
Erlang/OTP 28: hocon_cli_tests passed — 25 tests
Real 1 MiB tmpfs ENOSPC injection:
- second staging write failed with both old targets intact and no temp files;
- rollback-backup preparation failed with both old targets intact and no temp files;
- pre-prune reclaimed enough space for generation to succeed with the final retention limit enforced.
git diff --check passed

Copilot

🟡 Not ready to approve

Current cleanup behavior can delete an existing valid target config file on write/rename errors, which risks data loss when overwriting an existing timestamped output.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR hardens hocon_cli config generation against disk-full (ENOSPC) scenarios by switching to temp-file + rename writes and adding cleanup logic so failed writes don’t leave corrupted config artifacts that can break subsequent node startups.

Changes:

Replace direct file:write_file/2 writes in hocon_cli:generate/1 with atomic_write_file/2 (write to *.tmp, then rename).
Add cleanup logic to remove temporary/failed-write artifacts.
Add a unit test covering write failure cleanup behavior.

File summaries

File	Description
`src/hocon_cli.erl`	Introduces `atomic_write_file/2` and cleanup behavior; updates generator to use atomic writes.
`test/hocon_cli_tests.erl`	Adds a regression test ensuring failed atomic writes don’t leave files behind.

Review details

Suppressed comments (1)

src/hocon_cli.erl:418

maybe_log_file_error/2 currently calls cleanup_file(Filename), which deletes the final target file on error. After switching to atomic temp-file writes, this can remove a previously valid config/vm.args file when an overwrite fails; it should only try to remove the temp file left behind by atomic_write_file/2.

maybe_log_file_error(Filename, {error, Reason}) ->
    cleanup_file(Filename),
    log(error, "Error writing ~s: ~s", [Filename, file:format_error(Reason)]),
    ok.

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

yuanweize · 2026-08-04T09:03:09Z

Follow-up commit f975484 addresses the review feedback and hardens the full generation failure path:

final targets are never deleted on failure;
both outputs are staged and synced before commit;
history is pruned only after successful commit;
temporary filenames are unique;
the regression test now verifies that a failed generation preserves old app/vm history and publishes no new app config.

Local OTP 27/28 xref + full EUnit pass (563 tests each), OTP 27 Dialyzer passes, and OTP 26 style checks pass.

The three GitHub Actions runs are currently marked action_required, so they appear to need maintainer approval before they can execute. Please re-review when convenient.

zmstone · 2026-08-06T21:38:55Z

+            _ = file:delete(Filename),
+            maybe_log_rollback_error(Filename, file:rename(BackupFilename, Filename))


just log the error.
file:rename/1 atomically rename and overwrite existing.
attempting to delete will fail wit the same error, and retry won't help.

Agreed. The delete/retry path was removed in 2883fc7. Follow-up 1939178 also separates delete cleanup from restore failures, so an enoent from the backup rename is logged instead of being silently treated like an already-deleted new target. The regression test leaves the current target untouched and now exercises the logged error path.

yuanweize · 2026-08-07T00:36:16Z

Follow-up commit 29d6e88 addresses all three latest maintainer review findings:

Pruning: the just-written generation is explicitly kept; only max_history - 1 older files are eligible for deletion, including the max_history=0 boundary.
Stale staging files: old app.*.config.*.tmp / vm.*.args.*.tmp files are cleaned at generation start when older than one hour. Fresh temp files are left alone to avoid deleting another active generator's staging file.
Rollback: failed backup restoration now only logs the rename error; it no longer deletes the target or retries a filesystem operation that already failed.

Added regression coverage for older timestamps, stale temp cleanup, and rollback failure preservation. Verified in clean OTP 27 and OTP 28 containers: 22 hocon_cli_tests passed on each; OTP 28 erlfmt check passed; git diff --check passed.

Please re-review the latest commit when convenient.

yuanweize · 2026-08-07T00:41:56Z

The branch was updated to 2883fc7 (force-with-lease from the previously pushed 29d6e88) to add a direct regression test for the rollback error path. The implementation remains unchanged: failed backup restoration only logs the single failed rename and does not delete or retry the target.

Final clean OTP 28 verification: rebar3 fmt --check src/hocon_cli.erl passed; rebar3 as test eunit --module=hocon_cli_tests passed (22 tests, 0 failures). OTP 27 had the same 22/0 result.

Please review the latest head 2883fc7.

zmstone

thank you, last two comments.

zmstone · 2026-08-07T06:27:40Z

+    HistoryLimit =
+        case MaxHistory > 0 of
+            true -> MaxHistory - 1;
+            false -> 0
+        end,


Nit: this is HistoryLimit = max(MaxHistory - 1, 0).

yuanweize · 2026-08-07T10:24:15Z

Addressed the latest review feedback in c01c610 (rebased onto current master 97b0f22).

Prune excess history before staging, so a disk-full retry can reclaim space; the current destination remains excluded.
Keep the post-commit prune to enforce the final retention limit.
Simplified HistoryLimit to max(MaxHistory - 1, 0).
Added/updated regression coverage for failed generation with retained history.
Fixed the Style Check failure by removing duplicated test setup.

Validation after rebase:

OTP 26 Elvis: passed
OTP 26 erlfmt checks for changed files: passed
OTP 27 xref + Dialyzer + 22 hocon_cli_tests: passed
OTP 28 xref + full EUnit: 581 tests passed
git diff --check: passed

yuanweize · 2026-08-07T11:28:21Z

I found and fixed two additional filesystem edge cases in 1302f5f: pruning for a dotted --dest_file could match another config family, and replacing an existing target changed its permission bits (for example, 0600 to the process default). Stale-temp cleanup is now also restricted to the exact generated family and numeric temp suffix format.

The new tests cover those cases plus second-target preparation failure. I reran clean OTP 23/26/27/28 checks and three real 1 MiB tmpfs ENOSPC scenarios; OTP 27 and 28 full EUnit both pass with 584 tests. The PR description has the exact current flow and validation results.

zmstone · 2026-08-07T15:00:38Z

please rebase (and squash)

Stage and sync both generated outputs before publishing either final file. Preserve existing targets with rollback copies, and restore or remove partial commits on handled rename failures. Clean only matching stale staging files, and prune history before and after commit so ENOSPC retries can reclaim space without deleting the current generation. Add regression coverage for staging, rollback, pruning, cleanup, permissions, and replacement failures.

yuanweize · 2026-08-07T15:30:35Z

Rebased onto latest upstream/master (d2ffcc9) and squashed the PR into a single commit (936e64a), as requested.

Verification after the rewrite:

the PR patch is byte-for-byte identical to the pre-rebase patch;
git diff --check passes;
OTP 28 hocon_cli_tests passes: 25 tests, 0 failures.

No GitHub checks are reported yet for the rewritten head.

zmstone · 2026-08-07T18:51:18Z

thank you for the pr.
tagged 0.46.2

Two robustness fixes in emqx_config_backup_manager: 1. When reading the current file for backup fails with a non-enoent error, the staged file is now still renamed into place. The backup is best-effort (its write failures are already only warnings), while persisting the new config is the actual contract of backup_and_write; skipping one backup generation is preferable to silently dropping the new config while reporting success. 2. The staged tmp file is now synced to disk before the atomic rename, so a power loss right after the rename cannot leave an empty or truncated cluster.hocon. Mirrors the staging/sync/rename pattern adopted in emqx/hocon#318 for the generated app/vm files.

Copilot AI review requested due to automatic review settings August 4, 2026 01:15

Copilot started reviewing on behalf of yuanweize August 4, 2026 01:16 View session

Copilot AI reviewed Aug 4, 2026

View reviewed changes

Comment thread src/hocon_cli.erl

yuanweize changed the title ~~fix(cli): use atomic write and cleanup error files to prevent 0-byte config corruption on ENOSPC~~ fix(cli): stage generated config files before commit Aug 4, 2026

zmstone reviewed Aug 4, 2026

View reviewed changes

Comment thread src/hocon_cli.erl Outdated

zmstone reviewed Aug 6, 2026

View reviewed changes

zmstone mentioned this pull request Aug 6, 2026

fix(conf): reliably persist cluster.hocon updates emqx/emqx#18277

Merged

3 tasks

yuanweize force-pushed the fix/atomic-write-config-on-disk-full branch from 29d6e88 to 2883fc7 Compare August 7, 2026 00:40

zmstone reviewed Aug 7, 2026

View reviewed changes

yuanweize force-pushed the fix/atomic-write-config-on-disk-full branch from 1939178 to c01c610 Compare August 7, 2026 10:20

keynslug mentioned this pull request Aug 7, 2026

fix(config): bump hocon to 0.46.1 emqx/emqx#18289

Merged

3 tasks

yuanweize force-pushed the fix/atomic-write-config-on-disk-full branch from 1302f5f to 936e64a Compare August 7, 2026 15:30

zmstone approved these changes Aug 7, 2026

View reviewed changes

zmstone merged commit b737866 into emqx:master Aug 7, 2026
4 checks passed

		_ = file:delete(Filename),
		maybe_log_rollback_error(Filename, file:rename(BackupFilename, Filename))

Uh oh!

Conversation

yuanweize commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Regression tests

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Suppressed comments (1)

Uh oh!

Uh oh!

yuanweize commented Aug 4, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zmstone Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

yuanweize Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuanweize commented Aug 7, 2026

Uh oh!

yuanweize commented Aug 7, 2026

Uh oh!

zmstone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zmstone Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

yuanweize commented Aug 7, 2026

Uh oh!

yuanweize commented Aug 7, 2026

Uh oh!

zmstone commented Aug 7, 2026

Uh oh!

yuanweize commented Aug 7, 2026

Uh oh!

Uh oh!

zmstone commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yuanweize commented Aug 4, 2026 •

edited

Loading

yuanweize Aug 7, 2026 •

edited

Loading