[Bugfix] mcrfpy.step()/advance() from a background thread (without the lock) silently runs a frame off the main thread #411

New issue

Open

opened 2026-07-27 17:55:30 +00:00 by john · 0 comments

john commented

2026-07-27 17:55:30 +00:00

Owner

Found while implementing the guard that makes step()/advance() raise when the calling thread holds mcrfpy.lock(). That guard closes the deadlock case. The adjacent case is still wide open and is filed rather than folded in, because deciding it is a semantics change rather than a bug fix.

Measured, on the current tree

background-thread step() WITHOUT the lock:
  returned: 0.5
  current_frame: (0, 1)
  simulation_time_ms: (0, 500)

A background thread that has not taken mcrfpy.lock() can call mcrfpy.step() and it just works: the simulation clock advances 500 ms, current_frame increments, and a full runSimFrame() body runs — currentScene()->update(), timer callbacks, Scene.update() Python hooks, animations, scene transitions — all on a thread that is not the clock's owner, with no synchronisation against the main loop doing the same thing.

If the main thread is also stepping, two frame bodies run concurrently over the same scene graph, timer map and animation manager. That is not a race the frame barrier protects against: mcrfpy.lock() synchronises holders against the frame, not two threads both driving frames.

Why this is not just theoretical

CLAUDE.md's own embedding story is a network client on a background thread inside the engine process, handing results back under mcrfpy.lock(). "Advance the world when an event arrives" is an obvious next thing for such a thread to try, and it appears to work — it returns a plausible dt and the clock moves. There is nothing to tell the author it is wrong until something corrupts intermittently under load.

There should be exactly one thing that moves simulation_time. Right now any thread can.

Options (not decided here)

Refuse off the main thread: step()/advance() raise unless Resources::game->isMainThread(). Simplest and matches the one-clock-owner model. Risk: an embedder that legitimately drives the world from a dedicated non-main thread (with no main-thread loop at all) breaks. That is a real pattern for a headless server.
Refuse only when the main thread is also driving — needs a notion of "the driving thread", latched on first step(). Handles both patterns and is still Fail-Early.
Document only. Weakest: this is a silent-corruption class, and it looks like it works.

Option 2 is probably right, but it is a semantics decision about who owns the clock, and it belongs with #395 rather than inside a coverage task.

What is already done

Holding the lock and calling step()/advance() raises RuntimeError — tests/regression/frame_lock_reentrancy_test.py.
The rule is documented in docs/frame-lock.md.
That test's section 2 asserts the legal case (a background thread outside any lock block) still returns, so whichever option is chosen here, that assertion is the one to revisit.

Related: #395, #398, #410.

Found while implementing the guard that makes `step()`/`advance()` raise when the calling thread *holds* `mcrfpy.lock()`. That guard closes the deadlock case. The adjacent case is still wide open and is filed rather than folded in, because deciding it is a semantics change rather than a bug fix. ## Measured, on the current tree ``` background-thread step() WITHOUT the lock: returned: 0.5 current_frame: (0, 1) simulation_time_ms: (0, 500) ``` A background thread that has *not* taken `mcrfpy.lock()` can call `mcrfpy.step()` and it just works: the simulation clock advances 500 ms, `current_frame` increments, and a full `runSimFrame()` body runs — `currentScene()->update()`, timer callbacks, `Scene.update()` Python hooks, animations, scene transitions — all on a thread that is not the clock's owner, with no synchronisation against the main loop doing the same thing. If the main thread is also stepping, two frame bodies run concurrently over the same scene graph, timer map and animation manager. That is not a race the frame barrier protects against: `mcrfpy.lock()` synchronises *holders* against the frame, not two threads both *driving* frames. ## Why this is not just theoretical CLAUDE.md's own embedding story is a network client on a background thread inside the engine process, handing results back under `mcrfpy.lock()`. "Advance the world when an event arrives" is an obvious next thing for such a thread to try, and it appears to work — it returns a plausible dt and the clock moves. There is nothing to tell the author it is wrong until something corrupts intermittently under load. There should be exactly one thing that moves `simulation_time`. Right now any thread can. ## Options (not decided here) 1. **Refuse off the main thread**: `step()`/`advance()` raise unless `Resources::game->isMainThread()`. Simplest and matches the one-clock-owner model. Risk: an embedder that legitimately drives the world from a dedicated non-main thread (with no main-thread loop at all) breaks. That is a real pattern for a headless server. 2. **Refuse only when the main thread is also driving** — needs a notion of "the driving thread", latched on first `step()`. Handles both patterns and is still Fail-Early. 3. **Document only.** Weakest: this is a silent-corruption class, and it looks like it works. Option 2 is probably right, but it is a semantics decision about who owns the clock, and it belongs with #395 rather than inside a coverage task. ## What is already done - Holding the lock and calling `step()`/`advance()` raises `RuntimeError` — `tests/regression/frame_lock_reentrancy_test.py`. - The rule is documented in `docs/frame-lock.md`. - That test's section 2 asserts the *legal* case (a background thread outside any lock block) still returns, so whichever option is chosen here, that assertion is the one to revisit. Related: #395, #398, #410.