[Bugfix] FrameLock: a background thread that re-acquires without yielding starves the main loop (31 frames in 2s) #410

New issue

Open

opened 2026-07-27 17:51:26 +00:00 by john · 0 comments

john commented

2026-07-27 17:51:26 +00:00

Owner

Found while building the #395 stress/measurement coverage. Not a deadlock — which is what makes it hard to find. It presents as "the engine is slow."

The defect

FrameLock::closeWindow() (src/GameEngine.cpp) waits for waiting == 0 before it sets safe_window = false:

void FrameLock::closeWindow() {
    std::unique_lock<std::mutex> lock(mtx);
    cv.wait(lock, [this]{ return waiting == 0; });
    cv.wait(lock, [this]{ return active  == 0; });
    safe_window = false;
}

safe_window is still true for the whole of that wait. So a background thread that releases and immediately calls acquire() again is served again inside the same window — its cv.wait(safe_window) predicate is already satisfied — and it bumps waiting back above zero before the main thread's predicate can be re-evaluated. The window never closes. The barrier places no bound on how many times one thread may be served per frame.

Measured

One background thread, while True: with mcrfpy.lock(): time.sleep(H), no yield between iterations, against a mcrfpy.step(0.016)-driven headless main loop for 2 seconds:

hold H (ms)	main-thread frames in 2 s	median frame (ms)	worst frame (ms)	acquires served per frame
0	376 883	0.005	0.5	4.0
1	31	32.1	683	56.6
2	18	40.8	438	54.6
5	10	118.7	568	39.2

A 1 ms hold, from one thread, reduced the main loop to 31 frames in two seconds. The engine still makes progress, so nothing times out, nothing logs, and no stack points anywhere. At H=0 the effect is invisible (376 883 frames) — you only see it once the hold is long enough that the main thread reliably loses the race to close the window.

Compare the well-behaved regime, one acquire per frame with a yield in between: frame time tracks hold time 1.005:1 with 0.06–0.15 ms of barrier overhead (tests/integration/frame_lock_hold_budget_test.py).

Why it matters

This is the failure mode a background message bus produces under a backlog — the topology CLAUDE.md documents for embedders: a handler per inbound event, each in its own with mcrfpy.lock():, drained as fast as events arrive. That is the intended usage, and it is exactly the shape that triggers this.

Suggested fix (not attempted here)

Close the window against late arrivals rather than waiting for the waiter count to reach zero — e.g. set safe_window = false first and let acquire() distinguish "already admitted" from "must wait for the next window", or admit a generation/epoch counter so a thread is served at most once per frame. Either changes the barrier's admission semantics, so it needs its own careful pass against #398 (the no-timeout condition variable) and against the nested-lock() case, which is why it is filed rather than fixed inside a coverage task.

Not fixed here, but now documented and covered

docs/frame-lock.md states the rule ("one bounded unit of work per acquire; do not re-acquire without yielding to a frame") and carries the table above.
tests/integration/frame_lock_hold_budget_test.py measures both regimes; the greedy one is liveness-gated only, deliberately, so the eventual fix here does not break it.

Related: #395 (the coverage work that found it), #398 (the no-timeout deadlock in the same barrier), #411.

Found while building the #395 stress/measurement coverage. Not a deadlock — which is what makes it hard to find. It presents as "the engine is slow." ## The defect `FrameLock::closeWindow()` (`src/GameEngine.cpp`) waits for `waiting == 0` **before** it sets `safe_window = false`: ```cpp void FrameLock::closeWindow() { std::unique_lock<std::mutex> lock(mtx); cv.wait(lock, [this]{ return waiting == 0; }); cv.wait(lock, [this]{ return active == 0; }); safe_window = false; } ``` `safe_window` is still `true` for the whole of that wait. So a background thread that releases and immediately calls `acquire()` again is **served again inside the same window** — its `cv.wait(safe_window)` predicate is already satisfied — and it bumps `waiting` back above zero before the main thread's predicate can be re-evaluated. The window never closes. The barrier places no bound on how many times one thread may be served per frame. ## Measured One background thread, `while True: with mcrfpy.lock(): time.sleep(H)`, no yield between iterations, against a `mcrfpy.step(0.016)`-driven headless main loop for 2 seconds: | hold H (ms) | main-thread frames in 2 s | median frame (ms) | worst frame (ms) | acquires served per frame | |---:|---:|---:|---:|---:| | 0 | 376 883 | 0.005 | 0.5 | 4.0 | | 1 | **31** | **32.1** | **683** | 56.6 | | 2 | 18 | 40.8 | 438 | 54.6 | | 5 | 10 | 118.7 | 568 | 39.2 | A **1 ms** hold, from **one** thread, reduced the main loop to **31 frames in two seconds**. The engine still makes progress, so nothing times out, nothing logs, and no stack points anywhere. At H=0 the effect is invisible (376 883 frames) — you only see it once the hold is long enough that the main thread reliably loses the race to close the window. Compare the well-behaved regime, one acquire per frame with a yield in between: frame time tracks hold time **1.005:1** with 0.06–0.15 ms of barrier overhead (`tests/integration/frame_lock_hold_budget_test.py`). ## Why it matters This is the failure mode a background message bus produces under a backlog — the topology CLAUDE.md documents for embedders: a handler per inbound event, each in its own `with mcrfpy.lock():`, drained as fast as events arrive. That is the intended usage, and it is exactly the shape that triggers this. ## Suggested fix (not attempted here) Close the window against *late* arrivals rather than waiting for the waiter count to reach zero — e.g. set `safe_window = false` first and let `acquire()` distinguish "already admitted" from "must wait for the next window", or admit a generation/epoch counter so a thread is served at most once per frame. Either changes the barrier's admission semantics, so it needs its own careful pass against #398 (the no-timeout condition variable) and against the nested-`lock()` case, which is why it is filed rather than fixed inside a coverage task. ## Not fixed here, but now documented and covered - `docs/frame-lock.md` states the rule ("one bounded unit of work per acquire; do not re-acquire without yielding to a frame") and carries the table above. - `tests/integration/frame_lock_hold_budget_test.py` measures both regimes; the greedy one is liveness-gated only, deliberately, so the eventual fix here does not break it. Related: #395 (the coverage work that found it), #398 (the no-timeout deadlock in the same barrier), #411.