fix(cli): stage generated config files before commit

[yuanweize]

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]

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]

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.

[yuanweize]

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]

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]

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]

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

[yuanweize]

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]

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]

please rebase (and squash)

[yuanweize]

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]

thank you for the pr.
tagged 0.46.2