BUG: make a failing Monte Carlo worker say so

[thc1006]

Six ways a failing Monte Carlo worker got away without saying so. Three of them came out of review of the first three, which is why this is longer than it started.

Was stacked on #1181, which is the parallel run being unable to start at all. That has landed and this branch now sits on top of it, so it no longer shows in the diff here. What is left is this pull request's own work: 317 lines of monte_carlo.py and five test files.

Pull request type

• Code changes (bugfix, features)

Checklist

• Tests for the changes have been added (if needed)
• Docs have been reviewed and added / updated
• Lint (ruff check / ruff format --check, pylint) has passed locally
• All tests have passed locally, other than ten that need optional dependencies this machine does not have. They fail the same way on develop, listed below.
• CHANGELOG.md: one line for this one. The workflow that writes them has not run since CI: run the changelog job for pull requests from forks #1112, which is BUG: the changelog workflow stopped running, and CHANGELOG.md is 37 merged pull requests behind #1173.

Current behavior

A worker that fails early dies inside its own handler. __sim_producer binds sim_idx and inputs_json inside the simulation loop:

while sim_monitor.keep_simulating():
    sim_idx = sim_monitor.increment() - 1
    inputs_json, outputs_json = "", ""

and its except block reads both. A worker that fails in the seeding above the loop, or in the claim that opens it, arrives there with neither name assigned:

UnboundLocalError: cannot access local variable 'inputs_json'

That happens after mutex.acquire() and before mutex.release(), so a manager lock is left held by a process that no longer exists. The next worker blocks on acquire() and the run waits forever. error_event.set() is the handler's last line, so the parent is never told either.

A worker that is killed is not noticed at all. The parent joins each worker and reads the event. A signal runs no handler, so the event stays clear and the join returns because the process is gone. With a SIGKILL in the second of six simulations across two workers:

simulate() returned normally
asked for 6, rows on disk: 2

I would rather have the hang than that one. A hang is at least visible.

New behavior

Both names are bound before the try, and the message says worker startup when no index was claimed instead of naming one that does not exist. The handler now finishes, releases the mutex and sets the event, so a worker failure surfaces as a RuntimeError naming the error file rather than as a wait.

After the join, _refuse_a_worker_that_did_not_finish reads the exit codes. Anything other than zero is refused. It sits before the event check because the two are disjoint: a worker that reports through its handler exits cleanly, and one that was killed only leaves its exit code behind.

The handler holds the shared mutex while it reports, so a failure in the reporting used to leave the lock held by a process that had already gone. The event is set first and from outside the lock, the lock is released from a finally, and each reporting step is separate so an unwritable log cannot replace the failure being reported. A startup failure writes a row of its own now, since the caller is told to read that file and a traceback a worker printed is not there once its output is redirected.

The lock belongs to the manager and is not released when its holder is killed, so a sibling can block on a lock nobody owns while an unbounded join waits with it. That is the case the exit-code check exists for and the one it could not see. _join_the_workers polls instead, and acts only once a worker has actually ended badly. A run that is merely slow is never bounded: an exit code, not a duration, decides. This is deliberately not a general worker-lifecycle change; it is the smallest thing that makes the check above reachable.

Scope is the parallel producer. __run_in_serial has the same unbound inputs_json and belongs to #1177, whose tests cover it already; I have deliberately not touched it.

Breaking change

• No

A run that used to hang now raises, and a run that used to return with missing rows now raises. Both were already failures.

Additional information

Verification, on a clean tree:

pytest tests/                          2518 passed, 10 failed
  the same on develop at bc3fe735      2455 passed, 10 failed
pytest rocketpy --doctest-modules        48 passed
ruff check . / ruff format --check .    clean
pylint rocketpy/ tests/ docs/           10.00/10, exit 0

The ten are the same ten on both, which is why I ran develop at the commit this branch sits on. Each of them is an optional dependency this machine does not have: statsmodels and prettytable for the sensitivity tests, imageio for the ellipses one, and timezonefinder, windrose, ipywidgets and jsonpickle for the environment analysis ones.

Mutations, each leaving a control standing:

undone	goes red
the two names are not bound before the try	14
only inputs_json is bound, which is what the traceback points at	the same 14
the parent stops reading exit codes	1, test_a_worker_that_leaves_early_does_not_pass_as_a_finished_run
if process.exitcode in place of != 0	1, the None case
the reporting is not wrapped in try/finally	3, the lock tests
the join goes back to being unbounded	12
the join acts on whether a worker has ended rather than on how	4, the slow-run test among them
the join stops reading the event	1, test_a_reported_failure_ends_the_wait
the outer handler joins unbounded again	1, test_no_failure_path_waits_on_a_worker_without_a_bound
the run is judged on exit codes alone	1, test_a_worker_that_leaves_cleanly_without_recording_is_not_a_success

Counts are from this branch's own test files, with a green control before each. The last two rows include test_monte_carlo_parallel_runs.py and the rest do not, so those two are over five files and the others over four. They were measured before the branch was squashed and rebased onto bc3fe735, which touches nothing this pull request changes. The later runs take their baseline from git show HEAD: rather than from a copy, after one run left a file mutated and the next took that as its baseline; the file was checked clean against HEAD after every run.

Two rows there are worth pointing at. Fixing only the name the traceback names looks complete and leaves the message raising on the other one, and the same 14 tests catch either version. And ending the wait as soon as any worker has finished, rather than on how one finished, takes down the test that says a slow run must be left alone. That is the failure mode that would hurt real users most, since it stops a run that was only taking its time.

Three more, each of which let a broken run still look finished. They were raised in review of the commits above and all three reproduced.

The join waited only on exit status. A worker that fails the ordinary way is caught by the producer, reports through the event and returns, so it exits cleanly. With a sibling stuck, the parent saw one clean exit and one live process and never stopped. A reported failure ends the wait now too. A manager that cannot be asked is not taken as evidence either way, which has its own control test.

The bounded shutdown was undone one level up. After the exit-code check raised, the outer handler joined every process again with no timeout, so the stubborn worker the bound exists for was waited on anyway. That handler uses the same teardown now, and a test walks the parallel path's syntax to keep an unbounded join out of it.

An exit code says how a process ended, never whether the index it claimed reached the logs. The monitor counts claims, not rows, so it cannot stand in either. Measured, six simulations across two workers leaving through os._exit(0):

before   simulate(): returned normally     rows on disk: 0
after    RuntimeError: The run is incomplete: the input log does not hold
         every simulation that was asked for, 6 of 6 are missing

The run is checked against its own logs at the end: both must hold exactly the simulations asked for, none twice, none unreadable, none numbered past the run.

Still not covered, and I would rather say so than imply otherwise: a worker killed between mutex.acquire() and the write it is guarding leaves a torn record behind. The run is stopped and reported now, but the rows it was midway through are not repaired.

_refuse_a_worker_that_did_not_finish is a module-level function rather than a method because the run paths are driven by stub objects in the tests, where self.__helper() would not resolve.

The stand-in worker in the join tests gives up after two hundred polls. Reproducing a real hang there would take a CI job down with it rather than report, and requirements-tests.txt has no pytest-timeout; I found that out by hanging my own mutation run for the full fifteen minutes.

The Monte Carlo objects in these tests are built on tmp_path rather than retargeted after construction. filename is a plain attribute and the three log paths are settled in __init__, so assigning it leaves a test writing the fixture's own files into the working directory, which is what these were doing.

#1169 and #1170 have landed since I first ran that, so it is not a throwaway tree any more. This branch is rebased onto current develop and the suite ran on the result. The fixed-seed baseline came out the same: stochastic_calisto under seed 42 reads mass=14.906007947 radius=0.063501935.

Every row above was measured again on the current tree, since a mutation table is a claim about the tests and the tests had moved. Two of the counts had been written as five and as eight and are fourteen and twelve now, because the branch gained tests after the table was first written. Two anchors were wrong on the first attempt and gave nothing: bool(exitcode) is the same function as not in (None, 0) for an int or None, and shortening the join poll does nothing to a stand-in worker. Both were redone against the real shape.

[codecov]

Codecov Report

❌ Patch coverage is 99.16667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.50%. Comparing base (bc3fe73) to head (81711b8).

Files with missing lines	Patch %	Lines
rocketpy/simulation/monte_carlo.py	99.16%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1182      +/-   ##
===========================================
+ Coverage    91.24%   91.50%   +0.26%     
===========================================
  Files          131      131              
  Lines        17619    17723     +104     
===========================================
+ Hits         16076    16217     +141     
+ Misses        1543     1506      -37     

☔ 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.

[Copilot]

Pull request overview

Fixes parallel Monte Carlo seed handling and ensures failed workers cannot silently hang or produce incomplete successful runs.

Changes:

• Normalizes SeedSequence values without collapsing worker streams.
• Adds worker failure reporting, exit-code validation, and bounded shutdown polling.
• Adds regression coverage for parallel execution and worker failure modes.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
rocketpy/tools.py	Adds deterministic SeedSequence conversion.
rocketpy/stochastic/stochastic_model.py	Accepts worker seed sequences.
rocketpy/simulation/monte_carlo.py	Improves worker reporting and lifecycle handling.
tests/unit/stochastic/test_seed_types.py	Tests seed compatibility and stream independence.
tests/unit/simulation/test_monte_carlo_parallel_runs.py	Exercises real serial and parallel runs.
tests/unit/simulation/test_monte_carlo_worker_reporting.py	Tests worker failure diagnostics.
tests/unit/simulation/test_monte_carlo_worker_join.py	Tests polling and shutdown behavior.
tests/unit/simulation/test_monte_carlo_worker_exit.py	Tests abnormal worker exit detection.

---

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

[thc1006]

Thanks for the approve, @Gui-FernandesBR. Flagging that the branch has moved a fair way since you looked at it, so you can decide whether you want another pass.

TL;DR: four commits landed after your approve (6fb180f4 to cad1281a, +497/-33). One of them undoes a change an earlier round of feedback asked for, because measuring it showed it made things worse. This repo does not dismiss a review on push, so the PR is sitting mergeable with those four unreviewed.

What landed, and why:

• f5b992c7: a worker that exits 0 without recording the index it claimed was treated as a success. Six simulations across two workers leaving through os._exit(0) returned normally with nothing written. The run is now checked against its own logs at the end.
• 8962cc52: the failure report named the wrong simulation. A worker kept sim_idx and inputs_json after committing a row, so a failure in the next claim was blamed on the simulation that had just succeeded, and that simulation's inputs were written to the error log as well. Also: shutdown shares one deadline across the fleet instead of one per worker, uses a monotonic clock, and kills what outlives terminate.
• c9a8a9d7: a blank line in a log, which is what an interrupted write leaves.
• cad1281a: a data_collector key called index replaced the number of the simulation the row belonged to, because the collector's values are merged over the record after the index is written. A two-simulation run with such a collector wrote both rows as 999. Refused now where the collector is handed over.

The part worth your eye is the reversal. I had made a reported failure end the parent's wait, which reads sensibly and turns out to be wrong: a worker that reports has left nothing behind, and its siblings stop on their own once they finish the simulation in hand. With the event treated as a reason to stop, a sibling that needed forty polls was terminated after three. It was not stuck, it was mid-simulation, and it would have exited cleanly with its rows intact. Only a worker that died can hold the shared lock for good, so only that ends the wait now. There is a test pinning it, and reverting the line turns that test red.

One thing I have not covered and would rather say than imply: the monotonic clock is a judgement call, not a tested one. Pinning it would mean moving the system clock, which I do not think belongs in CI.

Nothing here needs action from you if you are happy with it as is. If you would rather the extra work went somewhere separate, I am glad to move those four out into their own PR and put this back to what you approved.

Apologies for the moving target: I kept finding things while reviewing my own answer to the last round, and pushed each as I went rather than waiting. It is settled now.

[thc1006]

Heads up that I pushed 75b902e after this was approved, so the approval no longer covers the head. Three things changed, all in the failure path.

The reporting lock. acquire() sat outside the try and release() inside the finally, so a lock a dead sibling never gave back could hold the reporting worker for good, and either call could replace the simulation failure with a proxy error. It is asked for with a bound now, released only if it was taken, and neither call can hide what actually failed. Two tests cover a lock that raises on acquire and one that raises on release; putting the old shape back turns both red.

Index types. True and 1.0 compare equal to 1, so either could stand in for a simulation that never ran, and an unhashable index escaped set() as a raw TypeError. An index is a row that can be read only if it is a non-negative int.

Paired logs. The two were validated independently. A record goes into both under one lock and the inputs write rolls back if the outputs write fails, so their index sequences have to match. They are compared now.

Full tests/ on this branch: 2393 passed, plus the four test_sensitivity failures my machine has from optional dependencies it does not have installed. Happy to split any of the three out if you would rather take them separately.

[thc1006]

Another push after the approval, e2e0b64, so the approval no longer covers the head. It answers a liveness gap that I had described wrongly above, which is worth being explicit about.

The description was wrong. It said a reported failure ends the wait. The code said the opposite, in the join docstring, and the code is what ran: __report_a_failed_simulation() returns cleanly, the producer swallows the exception, and the worker exits 0, so the join loop had nothing to act on. With a sibling that never returns from the simulation in hand, the parent waited for good. That was my error in the description rather than a late regression.

The event ends the wait now, on a longer grace than the exit-code path, since a sibling that only read the event is working rather than blocked on a lock nobody owns. Slowness alone still ends nothing: with no failure reported a healthy worker is given as long as it needs, which the existing control test pins.

This does change a decision the branch had made deliberately, and the test that recorded it. That test said a sibling mid-simulation should be left alone. I think the trade goes the other way: once a failure is reported the run is short a simulation and the completeness check refuses it, so that sibling's row belongs to a run that has already failed, while the hang costs the caller any error at all. The sixty seconds is a judgement call with nothing principled behind it, and I would happily take a different number.

Three smaller things in the same commit.

• Starting a worker moved inside the cleanup scope, and a process is recorded only once it has started. A later start failing left the earlier ones running, and an unstarted one could still be joined.
• The error file keeps the stage and the traceback as well as what the simulation had drawn. Writing the input row on its own left neither, for every failure past the point the inputs were built.
• A log's indices have to be as many as its rows. [0, 1, 99] passed for a target of two. A checkpoint that is legitimately longer still passes, and completion order stays free, both with tests.

Full tests/ here: 2400 passed, plus four test_sensitivity failures my machine has from optional dependencies it does not have installed.

[thc1006]

Conflicts are gone. The branch is squashed to one commit and rebased onto current develop, so the history here is one change rather than twenty plus two merges of develop.

On the stacking: this one sat on #1181, which has landed, so the diff here is its own work again. 317 lines of monte_carlo.py and five test files. Nothing is waiting on anything else.

I ran the suite against a develop worktree at the same commit for comparison. Same ten failures on both, all of them optional dependencies my local environment does not have, and this branch adds 63 passing tests on top.