Make owner-permission file writes atomic on POSIX by palas · Pull Request #1286 · IntersectMBO/cardano-api

palas · 2026-08-11T00:41:59Z

Context

Old: the target was opened with O_TRUNC, so a crash or a full disk mid-write destroyed the previous contents and left a truncated file.

New: contents go to a fresh 0600 temp file in the target directory, which is fsynced and renamed over the target - the same approach the Windows implementation already uses. Readers see the old file or the new file, never a partial one.

Behaviour changes on POSIX:

A pre-existing target no longer keeps its permission bits: the result is always owner-only (0600 filtered by umask).
A symlink at the target path is replaced by a regular file instead of being written through.
Overwriting needs a writable directory instead of a writable file, so writeSecrets can now overwrite its own read-only output.

Also: haddocks updated for all five affected functions, and a new test pins the overwrite semantics (contents replaced, permissions re-tightened to owner-only).

How to trust this PR

Same temp+rename pattern as the existing Win32.hs; temp file is a sibling of the target, so the rename is atomic.
Added new test that checks the permission changing behaviour works.
Behaviour changes (always 0600, symlinks replaced) are documented in haddocks and changelog.

Checklist

Commit sequence broadly makes sense and commits have useful messages
New tests are added if needed and existing tests are updated. See Running tests for more details
Self-reviewed the diff
Changelog fragment added in .changes/

Fixes #1254. Old: the target was opened with O_TRUNC, so a crash or a full disk mid-write destroyed the previous contents and left a truncated file. New: contents go to a fresh 0600 temp file in the target directory, which is fsynced and renamed over the target - the same approach the Windows implementation already uses. Readers see the old file or the new file, never a partial one. Behaviour changes on POSIX: - A pre-existing target no longer keeps its permission bits: the result is always owner-only (0600 filtered by umask). - A symlink at the target path is replaced by a regular file instead of being written through. - Overwriting needs a writable directory instead of a writable file, so writeSecrets can now overwrite its own read-only output. Also: haddocks updated for all five affected functions, and a new test pins the overwrite semantics (contents replaced, permissions re-tightened to owner-only).

Copilot

Pull request overview

This PR makes POSIX implementations of the “owner-permission” file-writing helpers atomic by switching from truncation-in-place to a temp-file + fsync + rename strategy (matching the existing Windows approach), and updates documentation/tests/changelog accordingly.

Changes:

POSIX: write to a temp sibling file, fsync, then rename over the target to avoid partially-written/truncated targets on failure.
Documentation updates for the affected public-facing “WithOwnerPermissions” functions and writeSecrets.
Adds a regression test pinning overwrite semantics (contents replaced; permissions tightened back to owner-only), and updates the test-suite config to depend on unix when available.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
cardano-api/src/Cardano/Api/IO/Internal/Compat/Posix.hs	Implements atomic write via temp file + sync + rename on POSIX.
cardano-api/src/Cardano/Api/Serialise/TextEnvelope/Internal.hs	Updates Haddock to describe the new atomic semantics and behavioural changes.
cardano-api/src/Cardano/Api/IO.hs	Adds/updates Haddock notes for the owner-permission write helpers.
cardano-api/src/Cardano/Api/IO/Internal/Compat.hs	Documents `writeSecrets` atomic/owner-only behaviour at the API boundary.
cardano-api/test/cardano-api-test/Test/Cardano/Api/IO.hs	Adds a property test covering overwrite + permission tightening behaviour.
cardano-api/cardano-api.cabal	Ensures the test-suite conditionally depends on `unix` (via `maybe-unix`).
.changes/20260810_120000_cardano-api_palas_atomic_owner_permission_writes.yml	Adds a changelog fragment describing the change.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

- Changelog fragment: also classify as `breaking` - the PR reverses the previously documented guarantee that pre-existing permission bits are kept, which can break consumers at runtime without any compile error. - Restore chowning the file to the real user before the rename, and the matching haddock wording. Real and effective user only differ in setuid programs, but real-user ownership was documented behaviour and this PR has no reason to change it.

- Replace the fail-on-Left case expressions in the overwrite test with H.leftFail, and the expect-Left precondition case with `unless (isLeft ...)`. The file removal stays on the success path only. - Rewrite handleFileForWritingWithOwnerPermissionImpl's result handling as `fmap (first $ FileIOError path)` over `try`: the removed case expression was only doing error wrapping. - Add parameter haddocks to writeSecrets. - Add a test pinning that writeSecrets overwrites its own read-only output when rerun. Runtime-skipped on Windows: the read-only attribute set on secret files blocks replacing them there, which is pre-existing behaviour unrelated to this PR. - Use unix-compat and a runtime isWin32 guard instead of CPP for the precondition chmod, so the whole test file stays formattable by fourmolu.

The temp file was fsynced but the directory entry was not: after a power failure shortly after a write returned, the target could still hold the old contents (never a partial file, but the completed write could be lost). Syncing the directory makes the rename itself durable. Uses the 3-argument openFd, so the unix dependency now carries a >=2.8 bound instead of reintroducing the CPP this PR removed. Haddock extended to document the stronger guarantee.

Suggested in review. handleFileForWritingWithOwnerPermission, writeSecrets, the three write*FileWithOwnerPermissions writers and writeFileTextEnvelopeWithOwnerPermissions now run in any MonadIO, matching the neighbouring write* functions. The platform-specific implementations stay in IO; only the shared wrappers lift. The writer callback stays Handle -> IO (): generalising it would need MonadUnliftIO and a new dependency, without a use case in sight. Covered by the `breaking` changelog kind this PR already carries; the fragment mentions the generalisation now.

unix-compat hardcodes `extra-libraries: msvcrt`, but GHC 9.12's mingw-w64 toolchain links against ucrt, so the Windows test executable failed to link with "multiple definition" errors between the two C runtimes. Replace it with a tiny platform-specific Test.Cardano.Api.IO.Compat module selected via hs-source-dirs in the cabal file: the POSIX variant does the chmod through the unix package, the Windows variant is a no-op that is never called (the precondition stays runtime-skipped there). Still no CPP anywhere, so fourmolu keeps checking all the test files.

palas self-assigned this Aug 11, 2026

Copilot AI lite review requested due to automatic review settings August 11, 2026 00:42

palas requested review from CarlosLopezDeLara, Jimbo4350, carbolymer, disassembler and erikd as code owners August 11, 2026 00:42

palas linked an issue Aug 11, 2026 that may be closed by this pull request

Make POSIX owner-permission file writes atomic via temp file + rename #1254

Closed

Copilot started reviewing on behalf of palas August 11, 2026 00:42 View session

palas force-pushed the make-posix-writes-atomic branch from 2d90980 to 8736bf2 Compare August 11, 2026 00:44

Copilot AI reviewed Aug 11, 2026

View reviewed changes

Comment thread .changes/20260810_120000_cardano-api_palas_atomic_owner_permission_writes.yml

Comment thread cardano-api/src/Cardano/Api/IO/Internal/Compat/Posix.hs

Comment thread cardano-api/src/Cardano/Api/IO/Internal/Compat/Posix.hs