fix: write the scenario state file atomically by jeffcpullen · Pull Request #4667 · ansible/molecule

jeffcpullen · 2026-07-25T15:53:13Z

First step of the phased plan in #4666: make the scenario state.yml write atomic, which removes the partial-read crash. It does not attempt the larger shared-state design change (defect 2 in the issue); that stays tracked in #4666.

Problem

Molecule runs a collection's scenarios in parallel with --workers. The parallel-testing guide (docs/guides/parallel.md:30-32) pairs that with shared_state: true, so every scenario running at once reads and writes the same state.yml. That file is written non-atomically, so a scenario can read it mid-rewrite, get a truncated document, and abort with a YAML scanner error. It surfaces as the intermittent test_workers failure on the py314-milestone job (py3.14 timing just widens the window):

molecule.util.MoleculeError: while scanning a quoted scalar
  ...
yaml.scanner.ScannerError: could not find expected ':'

Root cause

The state write is a truncate-then-rewrite with no atomicity:

State._write_state_file (src/molecule/state.py:234) → util.write_file → Path.write_text(content) (src/molecule/util.py:222). write_text truncates the file, then writes — a window in which a concurrent reader sees an empty or partial document.
State._load_file (src/molecule/state.py:231) → util.safe_load_file → util.safe_load, which turns the resulting yaml.scanner.ScannerError into a MoleculeError (src/molecule/util.py:285-286) and the worker aborts.
A truncation that lands on an empty read is quieter but worse: safe_load returns {} (util.py:347, ... or {}), so State._get_data (state.py:214-218) falls back to _default_data() and the worker silently resets shared state (e.g. prepared → False) with no error at all. Atomicity removes both this silent reset and the ScannerError crash.

SharedState (src/molecule/state.py:241-249) points every opted-in scenario at one shared_ephemeral_directory/state.yml, and molecule's own test_workers fixture sets shared_state: true (tests/fixtures/integration/test_workers/extensions/molecule/config.yml:13) — so CI exercises exactly this path.

Fix

Add util.atomic_write_file: write the content to a temp file in the target's own directory, then Path.replace onto the target. The replace is a same-filesystem rename — POSIX guarantees rename(2) is atomic — so a concurrent reader always opens either the complete old file or the complete new file, never a truncated one. The temp file is removed if the write fails.
Point State._write_state_file at it. The file content is identical (safe_dump output with the same header); the only practically observable change is that state.yml is now created owner-only (0600) via mkstemp instead of umask-derived (0644) — harmless for this per-run ephemeral file, and if anything tighter.

write_file and atomic_write_file share a small _prepare_write helper for the header/Path prep, so the new function is not a copy-paste of the old; write_file's write path itself is unchanged. Scoped deliberately to the state write, not a global write_file swap: in the normal --workers layout only state.yml is written concurrently — every other write_file caller writes under a per-scenario ephemeral_directory — so widening it would add churn without fixing anything new.

Evidence

Reproduction of the race and its removal. Six workers (3 writers / 3 readers) hammer one shared state.yml through the exact functions State uses — util.safe_load_file to read, and util.write_file (before) vs. util.atomic_write_file (after) to write. Run against this branch on Python 3.10.20 (the race reproduces on every supported version; 3.14 timing widens the window):

===== BEFORE (util.write_file) =====
util.write_file -- py 3.10.20, 3000 iters x 6 workers
  reader outcome: ok  x8560
  reader outcome: TRUNCATED_DICT(missing keys)  x440
  CORRUPTION: 440 corrupt read(s)
===== AFTER (util.atomic_write_file) =====
util.atomic_write_file -- py 3.10.20, 3000 iters x 6 workers
  reader outcome: ok  x9000
  clean: 0 corrupt read(s)
summary: before=440 corrupt reads, after=0 corrupt reads

(The script is in #4666; earlier runs also caught the genuine ScannerError — which partial read you hit depends on where the truncation lands.)

Unit tests (tests/unit/test_util.py), three new, all deterministic:

test_atomic_write_file — same content as write_file, and no temp file survives in the directory.
test_atomic_write_file_never_truncates_target — spies on Path.replace to assert the live file still holds the complete old content right up to the atomic swap (proves the target is never truncated in place).
test_atomic_write_file_cleans_up_on_error — a failing replace leaves the original intact and drops no temp file behind.

The last two patch Path.replace — exactly the call the code makes — so they exercise the real swap on every supported interpreter (verified on 3.10 and 3.14).

tests/unit/test_util.py::test_atomic_write_file PASSED
tests/unit/test_util.py::test_atomic_write_file_never_truncates_target PASSED
tests/unit/test_util.py::test_atomic_write_file_cleans_up_on_error PASSED

Suite / lint:

tox -e py -- tests/unit:  full unit suite green (Fedora container, py3.14)
python3.10 -m pytest tests/unit/test_util.py tests/unit/test_state.py:  98 passed

The single-process state flow (tests/unit/test_state.py) is unchanged and green, and the new test_util.py cases are included above. tox -e lint — ruff, mypy, pydoclint, cspell all pass on the diff.

Regression / no-behavior-change checks

util.write_file's write path is unchanged (it now shares the _prepare_write header/Path helper); test_write_file still passes.
atomic_write_file reproduces write_file's output exactly (header prepend included), asserted by test_atomic_write_file — the on-disk content is byte-identical; the write path and the resulting file mode (0600, see Fix) are the only differences.
The single-process state flow is unaffected: tests/unit/test_state.py passes unchanged.

Notes for reviewers

This is intentionally step 1 of Shared state gets corrupted when running scenarios in parallel, flaking CI #4666, not the whole issue. It removes the crash (defect 1). The second defect in Shared state gets corrupted when running scenarios in parallel, flaking CI #4666 — under --workers, each worker rewrites the whole shared file from its own in-memory snapshot, so one can silently overwrite another's flags — is a design question (who owns prepare, per-worker state vs. one shared file) that atomic writes do not solve. I left Shared state gets corrupted when running scenarios in parallel, flaking CI #4666 open for it rather than closing it here.
Why scoped to the state write and not a global atomic_write_file swap: in the normal --workers layout only state.yml is written concurrently, so widening it would add churn without fixing anything new. atomic_write_file is left public (not _private) because it is a general helper worth reusing if another file ever needs the same guarantee.
Leftover temp files: if a process is killed between mkstemp and the replace, a hidden .<name>.*.tmp can remain in the ephemeral dir. The finally unlinks it on any in-process failure; only a hard kill mid-write could leave one, and it's inert.

Shared state gets corrupted when running scenarios in parallel, flaking CI #4666 — the issue; the full mechanism, both defects, and the phased plan.
feat: add --workers flag for concurrent scenario execution #4616 — added --workers; where the shared state.yml began being written concurrently.
fix: report a clear error for unparseable molecule.yml instead of a traceback #4664, fix: remove git-ignore-based scenario filter #4665 — where the py314-milestone flake was observed.

Summary by CodeRabbit

Bug Fixes
- Improved state-file updates to write changes atomically, preventing readers from seeing partially written content.
- Preserved existing state files and ensured temporary files are cleaned up if an update fails.
Reliability
- State persistence now safely handles concurrent updates and interruptions by using atomic write behavior.
Tests
- Added unit tests covering atomic write correctness, concurrency safety, and failure/cleanup scenarios.

The per-scenario state.yml is written by truncating the file and rewriting it (util.write_file -> Path.write_text). Under `--workers` with `shared_state: true` -- the parallel configuration the testing guide recommends -- several scenarios share one state.yml and write it at the same time. A reader that lands inside another worker's truncate/rewrite window sees a partial document, and molecule aborts with a YAML scanner error. This is the intermittent test_workers failure on the py314-milestone CI job. Add util.atomic_write_file (write a temp file in the target's own directory, then Path.replace onto it) and use it for the state write. The replace is a same-filesystem rename that POSIX guarantees is atomic, so a concurrent reader always sees the complete old or new file, never a truncated one. The temp file is removed if the write fails, and the state file is now owner-only (0600), since it is created via mkstemp. This is the first step of the phased plan in ansible#4666: it removes the partial-read crash. The remaining shared-state design work (workers concurrently overwriting each other's flags) is tracked there and is not in scope here. Refs: ansible#4666 Assisted-by: Claude (Anthropic) Signed-off-by: Jeff Pullen <9343691+jeffcpullen@users.noreply.github.com>

coderabbitai · 2026-07-25T15:53:36Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 247397d3-c653-471c-bf54-7a0d79220d0f

📥 Commits

Reviewing files that changed from the base of the PR and between a904b63 and 0c077bb.

📒 Files selected for processing (1)

src/molecule/util.py

🚧 Files skipped from review as they are similar to previous changes (1)

src/molecule/util.py

📝 Walkthrough

Walkthrough

The change adds atomic file writing with temporary-file replacement, refactors shared write preparation, updates state persistence to use atomic writes, and adds tests for content, replacement visibility, and cleanup after errors.

Changes

Atomic file writes

Layer / File(s)	Summary
Atomic write implementation `src/molecule/util.py`	Write preparation is centralized, while `atomic_write_file()` writes through a temporary file and replaces the target with cleanup on failure.
State integration and validation `src/molecule/state.py`, `tests/unit/test_util.py`	State files use atomic writes, with tests covering final content, non-truncated reads, and failed replacement cleanup.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant State._write_state_file
  participant util.atomic_write_file
  participant Filesystem
  State._write_state_file->>util.atomic_write_file: write state content
  util.atomic_write_file->>Filesystem: create and write temporary file
  util.atomic_write_file->>Filesystem: replace target file atomically
  Filesystem-->>util.atomic_write_file: replacement result or error

🚥 Pre-merge checks | ✅ 6

✅ Passed checks (6 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Description check	✅ Passed	The stated goal of atomic scenario-state writes matches the actual changes in state.py, util.py, and the new tests.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: atomic scenario state-file writes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Refs ansible#4666. First step of the phased plan in ansible#4666: make the scenario `state.yml` write atomic, which removes the partial-read crash. It does not attempt the larger shared-state design change (defect 2 in the issue); that stays tracked in ansible#4666. ## Problem Molecule runs a collection's scenarios in parallel with `--workers`. The parallel-testing guide (`docs/guides/parallel.md:30-32`) pairs that with `shared_state: true`, so every scenario running at once reads and writes the *same* `state.yml`. That file is written non-atomically, so a scenario can read it mid-rewrite, get a truncated document, and abort with a YAML scanner error. It surfaces as the intermittent `test_workers` failure on the `py314-milestone` job (py3.14 timing just widens the window): ``` molecule.util.MoleculeError: while scanning a quoted scalar ... yaml.scanner.ScannerError: could not find expected ':' ``` ## Root cause The state write is a truncate-then-rewrite with no atomicity: - `State._write_state_file` (`src/molecule/state.py:234`) → `util.write_file` → `Path.write_text(content)` (`src/molecule/util.py:222`). `write_text` truncates the file, then writes — a window in which a concurrent reader sees an empty or partial document. - `State._load_file` (`src/molecule/state.py:231`) → `util.safe_load_file` → `util.safe_load`, which turns the resulting `yaml.scanner.ScannerError` into a `MoleculeError` (`src/molecule/util.py:285-286`) and the worker aborts. - A truncation that lands on an *empty* read is quieter but worse: `safe_load` returns `{}` (`util.py:347`, `... or {}`), so `State._get_data` (`state.py:214-218`) falls back to `_default_data()` and the worker silently resets shared state (e.g. `prepared` → `False`) with no error at all. Atomicity removes both this silent reset and the `ScannerError` crash. `SharedState` (`src/molecule/state.py:241-249`) points every opted-in scenario at one `shared_ephemeral_directory/state.yml`, and molecule's own `test_workers` fixture sets `shared_state: true` (`tests/fixtures/integration/test_workers/extensions/molecule/config.yml:13`) — so CI exercises exactly this path. ## Fix - Add `util.atomic_write_file`: write the content to a temp file in the target's **own directory**, then `Path.replace` onto the target. The replace is a same-filesystem rename — POSIX guarantees `rename(2)` is atomic — so a concurrent reader always opens either the complete old file or the complete new file, never a truncated one. The temp file is removed if the write fails. - Point `State._write_state_file` at it. The file *content* is identical (`safe_dump` output with the same header); the only practically observable change is that `state.yml` is now created owner-only (`0600`) via `mkstemp` instead of umask-derived (`0644`) — harmless for this per-run ephemeral file, and if anything tighter. `write_file` and `atomic_write_file` share a small `_prepare_write` helper for the header/`Path` prep, so the new function is not a 8000 copy-paste of the old; `write_file`'s write path itself is unchanged. Scoped deliberately to the state write, not a global `write_file` swap: in the normal `--workers` layout only `state.yml` is written concurrently — every other `write_file` caller writes under a per-scenario `ephemeral_directory` — so widening it would add churn without fixing anything new. ## Evidence **Reproduction of the race and its removal.** Six workers (3 writers / 3 readers) hammer one shared `state.yml` through the exact functions `State` uses — `util.safe_load_file` to read, and `util.write_file` (before) vs. `util.atomic_write_file` (after) to write. Run against this branch on Python `3.10.20` (the race reproduces on every supported version; 3.14 timing widens the window): ``` ===== BEFORE (util.write_file) ===== util.write_file -- py 3.10.20, 3000 iters x 6 workers reader outcome: ok x8560 reader outcome: TRUNCATED_DICT(missing keys) x440 CORRUPTION: 440 corrupt read(s) ===== AFTER (util.atomic_write_file) ===== util.atomic_write_file -- py 3.10.20, 3000 iters x 6 workers reader outcome: ok x9000 clean: 0 corrupt read(s) summary: before=440 corrupt reads, after=0 corrupt reads ``` (The script is in ansible#4666; earlier runs also caught the genuine `ScannerError` — which partial read you hit depends on where the truncation lands.) **Unit tests** (`tests/unit/test_util.py`), three new, all deterministic: - `test_atomic_write_file` — same content as `write_file`, and no temp file survives in the directory. - `test_atomic_write_file_never_truncates_target` — spies on `Path.replace` to assert the live file still holds the complete *old* content right up to the atomic swap (proves the target is never truncated in place). - `test_atomic_write_file_cleans_up_on_error` — a failing replace leaves the original intact and drops no temp file behind. The last two patch `Path.replace` — exactly the call the code makes — so they exercise the real swap on every supported interpreter (verified on 3.10 and 3.14). ``` tests/unit/test_util.py::test_atomic_write_file PASSED tests/unit/test_util.py::test_atomic_write_file_never_truncates_target PASSED tests/unit/test_util.py::test_atomic_write_file_cleans_up_on_error PASSED ``` **Suite / lint:** ``` tox -e py -- tests/unit: full unit suite green (Fedora container, py3.14) python3.10 -m pytest tests/unit/test_util.py tests/unit/test_state.py: 98 passed ``` The single-process state flow (`tests/unit/test_state.py`) is unchanged and green, and the new `test_util.py` cases are included above. `tox -e lint` — `ruff`, `mypy`, `pydoclint`, `cspell` all pass on the diff. ## Regression / no-behavior-change checks - `util.write_file`'s write path is unchanged (it now shares the `_prepare_write` header/`Path` helper); `test_write_file` still passes. - `atomic_write_file` reproduces `write_file`'s output exactly (header prepend included), asserted by `test_atomic_write_file` — the on-disk *content* is byte-identical; the write path and the resulting file mode (`0600`, see Fix) are the only differences. - The single-process state flow is unaffected: `tests/unit/test_state.py` passes unchanged. ## Notes for reviewers - **This is intentionally step 1 of ansible#4666, not the whole issue.** It removes the crash (defect 1). The second defect in ansible#4666 — under `--workers`, each worker rewrites the whole shared file from its own in-memory snapshot, so one can silently overwrite another's flags — is a *design* question (who owns `prepare`, per-worker state vs. one shared file) that atomic writes do not solve. I left ansible#4666 open for it rather than closing it here. - **Why scoped to the state write** and not a global `atomic_write_file` swap: in the normal `--workers` layout only `state.yml` is written concurrently, so widening it would add churn without fixing anything new. `atomic_write_file` is left public (not `_private`) because it is a general helper worth reusing if another file ever needs the same guarantee. - **Leftover temp files:** if a process is killed between `mkstemp` and the replace, a hidden `.<name>.*.tmp` can remain in the ephemeral dir. The `finally` unlinks it on any in-process failure; only a hard kill mid-write could leave one, and it's inert. ## Related - ansible#4666 — the issue; the full mechanism, both defects, and the phased plan. - ansible#4616 — added `--workers`; where the shared `state.yml` began being written concurrently. - ansible#4664, ansible#4665 — where the `py314-milestone` flake was observed.  ## Summary by CodeRabbit * **Bug Fixes** * Improved state-file updates to write changes atomically, preventing readers from seeing partially written content. * Preserved existing state files and ensured temporary files are cleaned up if an update fails. * **Reliability** * State persistence now safely handles concurrent updates and interruptions by using atomic write behavior. * **Tests** * Added unit tests covering atomic write correctness, concurrency safety, and failure/cleanup scenarios.  Signed-off-by: Jeff Pullen <9343691+jeffcpullen@users.noreply.github.com> Co-authored-by: Jeff Pullen <9343691+jeffcpullen@users.noreply.github.com> Co-authored-by: tanwigeetika1618 <84617407+tanwigeetika1618@users.noreply.github.com>

jeffcpullen requested a review from a team as a code owner July 25, 2026 15:53

github-project-automation Bot added this to 🧰 devtools project board Jul 25, 2026

github-actions Bot added the fix label Jul 25, 2026

tanwigeetika1618 added 2 commits July 30, 2026 03:14

Merge branch 'main' into fix/atomic-state-write

0c077bb

Merge branch 'main' into fix/atomic-state-write

f7c081e

tanwigeetika1618 approved these changes Jul 31, 2026

View reviewed changes

8000
github-project-automation Bot moved this to Review in 🧰 devtools project board Jul 31, 2026

tanwigeetika1618 merged commit 8be0a96 into ansible:main Jul 31, 2026
20 checks passed

github-project-automation Bot moved this from Review to Done in 🧰 devtools project board Jul 31, 2026

This was referenced Sep 8, 2026

fix(state): reload before change_state so a stale snapshot can't revert a sibling State's write #4694

Merged

Shared state gets corrupted when running scenarios in parallel, flaking CI #4666

Open

jeffcpullen mentioned this pull request Sep 18, 2026

fix: shared_state - give each scenario its own working directory #4703

Draft

8 tasks done

Navigation Menu

Repository navigation

fix: write the scenario state file atomically - #4667

fix: write the scenario state file atomically#4667
tanwigeetika1618 merged 3 commits into
ansible:mainfrom
jeffcpullen:fix/atomic-state-write

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Development

Uh oh!

2 participants

Conversation

Uh oh!

Problem

Root cause

Fix

Evidence

Regression / no-behavior-change checks

Notes for reviewers

Related

Summary by CodeRabbit

Uh oh!

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Development

Uh oh!

2 participants