Skip to content

BUG: write MonteCarlo input and output rows atomically (#1110) - #1125

Merged
Gui-FernandesBR merged 2 commits into
RocketPy-Team:developfrom
thatrandomasiandev:bug/1110-montecarlo-atomic-writes
Aug 15, 2026
Merged

BUG: write MonteCarlo input and output rows atomically (#1110)#1125
Gui-FernandesBR merged 2 commits into
RocketPy-Team:developfrom
thatrandomasiandev:bug/1110-montecarlo-atomic-writes

Conversation

@thatrandomasiandev

Copy link
Copy Markdown

Pull request type

  • Code changes (bug fix, features)

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/ / make lint) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally
  • CHANGELOG.md — no action needed; an LLM workflow auto-updates it after merge

Current behavior

MonteCarlo appends inputs then outputs in two separate open(..., "a") blocks (serial and parallel). If the second write fails, the inputs file keeps an orphan row and the files no longer line up by index.

Fixes #1110

New behavior

_append_simulation_record records the inputs file size, writes inputs then outputs, and truncates inputs back on outputs failure so either both rows land or neither does. Serial and parallel paths both use the helper.

Breaking change

  • No

Additional information

Focused unit test mocks a failing outputs append and asserts the inputs file was rolled back. Full slow suite not run in this contribution pass.

@thatrandomasiandev
thatrandomasiandev requested a review from a team as a code owner August 11, 2026 01:33
@Gui-FernandesBR

Copy link
Copy Markdown
Member

what is the impact in the final simulation time?

@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.77778% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.42%. Comparing base (60ed40f) to head (88bbbd3).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
rocketpy/simulation/monte_carlo.py 77.77% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1125      +/-   ##
===========================================
+ Coverage    84.39%   84.42%   +0.03%     
===========================================
  Files          130      130              
  Lines        17281    17291      +10     
===========================================
+ Hits         14584    14598      +14     
+ Misses        2697     2693       -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@thatrandomasiandev

Copy link
Copy Markdown
Author

no effect on each flight's simulated t_final. Wall-clock time only adds one os.path.getsize per sim on the happy path. I didnt change the physics and the ODE, and the truncate only runs on write failure.

@thatrandomasiandev

Copy link
Copy Markdown
Author

Lint CI failure was not a Pylint config/fatal problem. Pylint exit code `4` is the warning bitflag (score can still print `10.00/10` with `fail-under=10`).

From the job log the PR-introduced warning was:

```
tests/unit/simulation/test_monte_carlo.py:107:4: W1113: Keyword argument before variable positional arguments list in the definition of failing_output_open function (keyword-arg-before-vararg)
```

Fixed by rewriting the mock as `def failing_output_open(*args, **kwargs)` so it no longer puts a keyword arg before `*args`. Pushed in a follow-up commit on this branch.

(The `I0021` useless-suppression notes on `motor.py` / `sensor.py` are pre-existing on develop and did not set this exit code.)

@ting-hong-shieh

Copy link
Copy Markdown

The direct output-open failure covered by this PR passes on head b9ad9b5587f3eaf193765335066d293aaaef8d2a; the complete Monte Carlo unit file reports 36 passed.

I also exercised writes that append four bytes and then raise, which exposes two remaining inconsistent states:

Injected failure Input file after the call Output file after the call
partial input write original row plus {"in original row
partial output write original row original row plus {"in

The output-side case rolls the input file back but leaves the partially written output row. The input-side case is outside the current try block, so no rollback runs.

The smallest complete form I can see is to record both original sizes, wrap both appends in one exception boundary, and truncate both files back to those sizes after any open/write/flush failure. If rollback itself fails, preserving or chaining the original write exception would keep the initiating failure visible. Tests for partial input, partial output, and rollback failure would cover those paths.

I have not opened a replacement PR or changed this branch. Environment: Python 3.12.6; pytest 9.1.1; macOS 26.5.2 arm64. Current develop: cb6106a717207dd8fc2dfe1446d80ff75022f21b.

@Gui-FernandesBR

Copy link
Copy Markdown
Member

Answering my own question about simulation time: the added cost is one os.path.getsize() per simulation — a single stat, microseconds — against a flight simulation that is orders of magnitude slower, and it happens in a region that already opens and writes two files. Not measurable.

I also checked the thing that worried me more about a rollback-by-truncation approach: whether two parallel producers could interleave a getsize / write / truncate sequence and have one worker's rollback eat another worker's row. They cannot — in __sim_producer both writes are inside mutex.acquire() / finally: mutex.release(), so _append_simulation_record always runs with the lock held. Serial mode is trivially fine.

Good to merge.

@Gui-FernandesBR
Gui-FernandesBR force-pushed the bug/1110-montecarlo-atomic-writes branch from b9ad9b5 to 88bbbd3 Compare August 15, 2026 14:42
@Gui-FernandesBR
Gui-FernandesBR merged commit 8ba1f2f into RocketPy-Team:develop Aug 15, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MonteCarlo writes the inputs and outputs of one simulation non-atomically

3 participants