fix: atomically preserve reconciliation snapshots - #42
codeforester merged 2 commits into
Conversation
| staged_path = Path(staged.name) | ||
| staged.write(serialized) | ||
| staged.flush() | ||
| os.fsync(staged.fileno()) |
There was a problem hiding this comment.
Correctness: file-permission regression on every reconcile
tempfile.NamedTemporaryFile creates the staged file with mode 0o600 (owner-only), and os.replace moves that file (with its own permission bits) onto last-reconciliation.json — it does not preserve whatever mode the destination previously had. Before this PR, state_path.write_text(...) created/kept the file at the umask-derived mode (typically 0o644) and that mode was stable across rewrites.
Reproduced locally:
existing file mode before second reconcile: 0o644
existing file mode after atomic replace: 0o600
So the very first reconcile after this change (and every one after it) silently narrows last-reconciliation.json to owner-only, breaking any other user/process/service that previously could read it (a monitoring sidecar, a different service account, a CI artifact collector, etc.), with no message or migration path.
Fix: either os.chmod(staged_path, 0o644) (or read+reapply the previous file's mode) before the _replace_state call, or make the intended permissions explicit/documented if 0600 is actually desired.
| def _replace_state(staged_path: Path, state_path: Path) -> None: | ||
| """Atomically publish a complete snapshot from the same filesystem.""" | ||
|
|
||
| os.replace(staged_path, state_path) |
There was a problem hiding this comment.
Durability gap: rename isn't fsynced, so the "atomic" guarantee is incomplete
os.fsync(staged.fileno()) (line ~113) only guarantees the staged file's content is durable before the rename. os.replace itself (here) updates the directory entry, but nothing calls os.fsync on state_path.parent afterward. On a crash/power-loss right after the rename but before the directory entry is flushed, some filesystems/mount options can still lose the rename and leave the previous (stale) snapshot — or in rarer cases an inconsistent directory entry — despite the docstring's claim to "atomically publish a complete snapshot."
Notably, this codebase's own base_cli._private_files.write_private_json (the framework's internal equivalent for private JSON state) does exactly this extra step via _sync_directory/os.fsync(parent_fd) after the rename — so the fuller pattern is already established practice here, just not applied in this new consumer-side implementation.
Suggested fix: after _replace_state, open and fsync state_path.parent (best-effort, ignoring OSError since directory fsync isn't supported everywhere).
| staged_path = None | ||
| except (OSError, TypeError, ValueError) as exc: | ||
| raise click.ClickException( | ||
| "Could not persist the reconciliation snapshot; the previous " |
There was a problem hiding this comment.
Minor: misleading error message on the very first reconcile
The ClickException unconditionally states "the previous snapshot was left unchanged," but on the first-ever reconcile in a fresh state_dir there is no previous snapshot — last-reconciliation.json simply doesn't exist yet. A user hitting a failure on first run (e.g. disk full, permission denied on the parent dir) gets a message implying a prior snapshot exists and was preserved, which is inaccurate and could confuse debugging.
Consider branching the message on state_path.exists() (checked before entering the try, or captured before mutating anything), e.g. "...; no previous snapshot existed" vs "...; the previous snapshot was left unchanged."
1 participant
Fixes #30