[None][fix] reject nested control_action() instead of corrupting its handshake - #17346
[None][fix] reject nested control_action() instead of corrupting its handshake#17346lowsfer wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesControl action re-entrancy
Estimated code review effort: 3 (Moderate) | ~30 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CallingThread
participant PyExecutor
participant ExecutorBarrier
participant ControlActionBody
CallingThread->>PyExecutor: control_action()
PyExecutor->>PyExecutor: acquire lock and check owner thread
alt same thread already owns control_action
PyExecutor-->>CallingThread: raise RuntimeError
else first caller or waiting thread
PyExecutor->>ControlActionBody: enqueue and execute action
PyExecutor->>ExecutorBarrier: poll barrier until completion or deadline
alt shutdown, dead executor, or lost barrier edge
ExecutorBarrier-->>PyExecutor: raise RuntimeError
else barrier handshake completes
ExecutorBarrier-->>PyExecutor: return control completion
ControlActionBody-->>PyExecutor: complete action
end
PyExecutor->>PyExecutor: clear owner and release lock
PyExecutor-->>CallingThread: return or raise
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/_torch/pyexecutor/py_executor.py`:
- Around line 4451-4464: Make control-action admission atomic in the
control_action flow by acquiring a non-reentrant threading.Lock before the
existing re-entrancy check, request enqueue, and control_request_barrier.wait().
Hold the lock through the outer finally cleanup, release it on every exit path,
and ensure a second thread cannot enter while the first action is waiting or
executing. Add a two-thread test that releases the barrier only after both
callers attempt entry.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 331be719-1b8e-406f-89db-9988bf09f0f6
📒 Files selected for processing (3)
tensorrt_llm/_torch/pyexecutor/py_executor.pytests/integration/test_lists/test-db/l0_cpu.ymltests/unittest/executor/test_control_action_reentrancy.py
| handshake. | ||
| """ | ||
|
|
||
| if self._control_action_in_progress: |
There was a problem hiding this comment.
Agreeing with CodeRabbit's thread below, with the evidence that I think settles it: this codebase already documents exactly that race, and already works around it by hand. base_worker.py:719-723:
Serialise concurrent sleep/wakeup calls.
control_action()uses an Event-based barrier, not a mutex, so two concurrent callers can both pass the barrier and then interleave sends/recvs on_sleep_wakeup_comm, consuming the wrong ACKs or resuming the event loop prematurely.
That workaround is _sleep_wakeup_lock, and it covers only the three base_worker sites (:725, :923, :961). The @control_action_decorator path takes no lock at all — _ray_utils.py:51 is a bare with self.engine.control_action(drain=drain) — so ray_gpu_worker.py:263,284 and rlhf_utils.py:36,162 can race a sleep()/wakeup() or each other, and land in the corruption this PR's docstring says it prevents.
Which is really an argument for your framing, not against it. You already say the right place is control_action() itself rather than any individual caller — a non-reentrant threading.Lock acquired before the enqueue and released in the finally gives you the nesting rejection (owner thread re-entering → deadlock-free reject via a stored owner ident) and subsumes _sleep_wakeup_lock, so the hand-rolled workaround can go. As written, the bool narrows one of the two ways this handshake gets corrupted and leaves the other looking handled.
Not blocking on scope if you'd rather keep this PR to nesting — but then the docstring and the RuntimeError text should say "same thread", because a concurrent caller currently gets "call the undecorated operation instead", which is the wrong advice for that case.
The rest reads well: rejecting before barrier.wait() and before the rank-0 enqueue is the right placement, clearing the flag ahead of control_action_done.set() is the right order, and the four tests cover the outer-handshake-intact and body-raises cases without needing a GPU.
There was a problem hiding this comment.
Thanks — the base_worker.py:715 pointer is what settled it, and it changed my mind. Fixed in f9fa918.
Confirmed your reading: _sleep_wakeup_lock covers only :719, :917, :955, while _ray_utils.py:51 is a bare with self.engine.control_action(drain=drain), so the five decorated methods (rlhf_utils.py:93/211/221, ray_gpu_worker.py:263/284) take no lock at all. As you say, the bool narrowed one route into the corruption and left the other looking handled — which is worse than not touching it.
Also checked the concurrency is real rather than theoretical, since it was reasonably questioned: rpc_server.py:69 creates a ThreadPoolExecutor(max_workers=num_workers) with num_workers=4 and dispatches registered methods via run_in_executor (:476), so two RPC calls genuinely land on different threads. (The Ray actor path is separately serialised — RayWorkerWrapper.options(...) sets no max_concurrency, so Ray's default of 1 applies. The RPC path is where it bites.)
Implemented as you suggested: threading.Lock acquired before the enqueue and released in the finally, with a stored owner ident so a nested call from the holding thread is rejected deadlock-free rather than blocked. Concurrent callers serialise, matching what _sleep_wakeup_lock already gives its three sites.
One deliberate omission: I have not removed _sleep_wakeup_lock. It is now redundant, but deleting it touches the multi-rank sleep/wakeup paths this PR otherwise does not, and those are hard to exercise locally. Happy to do it as a follow-up, or in this PR if you would rather see it closed out here.
Tests: the two-thread cases are added — one proving concurrent callers serialise rather than raise, one proving the owner check keys on thread ident so a sibling thread is not mistaken for nesting. Mutation-verified: drop the lock and the concurrency test fails; drop the owner pre-check and nesting deadlocks rather than raising.
|
/bot run --disable-fail-fast |
|
PR_Github #64312 [ run ] triggered by Bot. Commit: |
45a6345 to
f9fa918
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unittest/executor/test_control_action_reentrancy.py`:
- Around line 38-55: Add precise return annotations to all newly added test and
worker functions in this file, including the deferred-import _make_executor()
function, which should use a quoted PyExecutor return type. Annotate procedural
helpers and tests with None, and ensure every function covered by the comment
has an explicit return type.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 59ddf5cc-a3b0-49c0-97db-3029b278e86e
📒 Files selected for processing (3)
tensorrt_llm/_torch/pyexecutor/py_executor.pytests/integration/test_lists/test-db/l0_cpu.ymltests/unittest/executor/test_control_action_reentrancy.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/test_lists/test-db/l0_cpu.yml
|
PR_Github #64312 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64367 [ run ] triggered by Bot. Commit: |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
The guard is correct and well placed. Raising before enqueue_control_request() means a refused attempt leaves no orphaned sentinel, and clearing the owner inside the lock's finally is right. Two things to address.
The description doesn't match the diff. It describes a re-entrancy flag ("the flag is cleared in the existing finally"), but the code also adds _control_action_lock and holds it for the whole action. That is a mutual-exclusion change that affects every existing caller, including base_worker._multi_rank_sleep_wakeup, which already carries _sleep_wakeup_lock for the same reason (base_worker.py:714). It's the bigger half of the PR and should get its own paragraph: what it now guarantees, and whether _sleep_wakeup_lock is still needed or should be documented as covering only the wider send/recv critical section.
[None] in the title. Fine here, since this is review-driven hardening for a hazard with no reported failure. But if #17163's discussion produced a tracking ticket, cite it.
Inline comments cover the untimed wait under the lock and a gap in the tests.
| finally: | ||
| self.control_action_done.set() | ||
| self.control_request_barrier.clear() | ||
| self.control_request_barrier.wait() |
There was a problem hiding this comment.
The lock is now held across an untimed control_request_barrier.wait(). That changes the failure mode of an existing narrow race: _handle_control_request does barrier.set(); barrier.clear(); return on the aborted-control-request path (py_executor.py:4327-4329) without waiting for control_action_done. A caller that hasn't reached wait() yet when that pulse happens misses the edge and blocks forever. Before, that stalled one caller; now it wedges every later control action behind the lock, silently and with no diagnostic.
Since you're adding the lock anyway, consider bounding the wait: if not self.control_request_barrier.wait(timeout=...): raise RuntimeError(...), so a lost barrier fails loudly on the caller responsible instead of deadlocking sleep/wakeup/weight-update forever. Same argument for shutdown, where the executor loop may already be gone.
There was a problem hiding this comment.
Confirmed and fixed in aac7f28afa — you are right that the lock turns a single stalled caller into a silently wedged executor.
I verified the path you describe: _handle_control_request really does set(); clear(); return on the aborted path (py_executor.py:4324-4329) with no control_action_done wait, so a caller not yet parked in wait() misses the edge permanently.
I did not take wait(timeout=...) verbatim, because a single deadline would have been wrong here: with drain=True the sentinel is parked until active_requests and waiting_queue drain, so a legitimate wait is bounded only by in-flight request duration. A tight timeout would trade your deadlock for spurious failures on long batches. Instead _wait_for_control_barrier() splits the two failure modes:
- shutdown / dead executor loop — polls
shutdown_eventandworker_thread.is_alive(), so it fails in one poll interval (~0.5s) rather than waiting out the deadline. This covers your shutdown point directly. - lost barrier edge — backstopped by
_CONTROL_BARRIER_TIMEOUT_S = 1800.0.
The 30 min is deliberately generous and documented as a wedge-breaker rather than a latency budget; because shutdown is caught by the liveness poll, it is only ever paid in a genuine lost-edge case. Happy to make it env-configurable if you would prefer.
Both modes are tested. test_lost_barrier_edge_fails_loudly_instead_of_wedging_the_lock reproduces your abort pulse and asserts the lock is released; mutation-checked by reverting to the bare wait(), which makes it hang rather than fail — so it is testing the real thing. test_shutdown_fails_fast_without_waiting_out_the_timeout sets a 300s timeout and asserts failure in under 5s, so it would hang if the liveness check regressed.
| action; same-batch requests fetched after the sentinel | ||
| are parked until the ``with`` block exits. | ||
|
|
||
| Mutually exclusive and not re-entrant: the two events are a broadcast |
There was a problem hiding this comment.
"Callers therefore serialise on _control_action_lock" claims more than holds for dist.rank != 0. Only rank 0 enqueues; other ranks just wait on the barrier that the broadcast sentinel sets. The lock gives local exclusion, but the order in which local callers on rank N acquire it isn't tied to rank 0's enqueue order, so two concurrent control actions can still pair different bodies with the same barrier cycle on different ranks. Worth one sentence saying the lock provides local mutual exclusion only, and that cross-rank ordering still depends on rank 0 being the single enqueue point (i.e. don't issue concurrent collective control actions).
There was a problem hiding this comment.
Agreed — the docstring claimed more than the lock delivers. Fixed in aac7f28afa:
The lock provides local mutual exclusion only. It does not order control actions across ranks: only rank 0 enqueues, and the order in which callers on rank N acquire the lock is not tied to rank 0's enqueue order, so concurrent control actions could still pair different bodies with the same barrier cycle on different ranks. Cross-rank correctness relies on rank 0 being the single enqueue point - do not issue concurrent collective control actions.
|
|
||
| def _make_executor() -> "PyExecutor": | ||
| """Build a PyExecutor shell exercising only control_action()'s state. | ||
|
|
There was a problem hiding this comment.
dist.rank = 1 plus executor_request_queue = None means the rank-0 path is never exercised, so the property the PR description leads with ("the check raises before enqueue_control_request(), so a refused nesting attempt leaves no orphaned control request") has no test. Moving the guard below the enqueue would still pass all five tests.
Cheap to close: add a variant with rank=0 and a recording stub queue (SimpleNamespace(enqueue_control_request=lambda **kw: calls.append(kw))), then assert one call for the outer action and none for the rejected nested one.
There was a problem hiding this comment.
You were right, and I confirmed it by mutation before fixing rather than taking it on faith: I hoisted enqueue_control_request() above the guard — introducing exactly the orphaned-sentinel bug the description claims to prevent — and all 6 tests still passed. The property was genuinely unverified.
Fixed in aac7f28afa with essentially your suggestion: _make_executor() now takes rank/queue, plus a _RecordingQueue, and test_rejected_nesting_enqueues_no_control_request runs at rank=0 and asserts the queue still holds exactly one call after the nested rejection.
Re-running that same mutation now fails exactly one test — the new one — and nothing else, so it closes the gap precisely without over-constraining the rest.
(Worth noting for anyone reading later: an earlier mutation I tried, moving the guard inside the lock, was not a faithful test of your point — nested calls then deadlock on the non-reentrant lock and fail for an unrelated reason.)
|
PR_Github #64367 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
PR_Github #64402 [ run ] triggered by Bot. Commit: |
|
PR_Github #64402 [ run ] completed with state
|
Review follow-ups from brnguyen2 on NVIDIA#17346. The lock is held across control_request_barrier.wait(), which was unbounded. _handle_control_request pulses set(); clear() on the aborted-control-request path without waiting for control_action_done, so a caller that has not reached the wait yet misses the edge. Previously that stalled one caller; with the lock it wedged every later control action silently. Wait in bounded slices instead: shutdown or a dead executor loop fails within one poll interval, and a lost edge fails after _CONTROL_BARRIER_TIMEOUT_S. The timeout is deliberately generous because drain=True is unbounded by design - it is a wedge-breaker, not a latency budget. Also note in the docstring that the lock is local mutual exclusion only and does not order control actions across ranks, and add a rank=0 test with a recording queue asserting a rejected nested call enqueues nothing - the previous tests all ran at rank=1, where the enqueue is skipped, so moving the guard below it went undetected. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unittest/executor/test_control_action_reentrancy.py`:
- Around line 275-294: Extend
test_shutdown_fails_fast_without_waiting_out_the_timeout to stub the executor’s
worker_thread with an object whose is_alive() returns False while shutdown_event
remains clear. Keep the long barrier timeout and assert control_action() raises
before the timeout, then verify both _control_action_lock and its owner state
are released.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0d20d368-0523-4225-a8d8-784d8e1c2ec8
📒 Files selected for processing (2)
tensorrt_llm/_torch/pyexecutor/py_executor.pytests/unittest/executor/test_control_action_reentrancy.py
Review follow-ups from brnguyen2 on NVIDIA#17346. The lock is held across control_request_barrier.wait(), which was unbounded. _handle_control_request pulses set(); clear() on the aborted-control-request path without waiting for control_action_done, so a caller that has not reached the wait yet misses the edge. Previously that stalled one caller; with the lock it wedged every later control action silently. Wait in bounded slices instead: shutdown or a dead executor loop fails within one poll interval, and a lost edge fails after _CONTROL_BARRIER_TIMEOUT_S. The timeout is deliberately generous because drain=True is unbounded by design - it is a wedge-breaker, not a latency budget. Also note in the docstring that the lock is local mutual exclusion only and does not order control actions across ranks, and add a rank=0 test with a recording queue asserting a rejected nested call enqueues nothing - the previous tests all ran at rank=1, where the enqueue is skipped, so moving the guard below it went undetected. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
aac7f28 to
a2e45bb
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run --disable-fail-fast |
|
PR_Github #64472 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #64476 [ run ] triggered by Bot. Commit: |
|
PR_Github #64472 [ run ] completed with state |
|
PR_Github #64476 [ run ] completed with state
|
|
The 1800 second deadline raises after the sentinel is enqueued, so the barrier is never cleared and every rank parks in the untimed wait with no caller left, and the hang detector hard kills the job a few minutes later. BTW, not from this PR: the drain gate ignores in-flight transfers, so it can report done while a context send is still reading the pool. |
…handshake control_request_barrier and control_action_done are single-slot events shared by every control action. A control action whose body opened another one would have the inner exit clear the barrier and set done, releasing the executor loop while the outer body was still running -- the drain it asked for silently no longer holding. Nothing nests today: update_weights() reaches the reuse state through the undecorated self.engine.reset_prefix_cache(), the Ray worker's sleep()/wakeup() call no decorated sibling, and the base-worker control blocks only touch CUDA and memory. The guard is there so that stays true, and so a future caller that wraps one @control_action_decorator method in another fails at the point of the mistake rather than at whatever breaks later. Raise before enqueue_control_request() so a refused nesting attempt leaves no orphaned control request, and clear the flag in the existing finally so a body that raises does not wedge the executor. Register the new test file in l0_cpu.yml -- test-db lists files individually, so an unregistered unit test never runs in CI. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
Review of the first cut pointed out the flag was not atomic, and BowenFu found the evidence that settles it: base_worker.py:715 already documents this exact race and works around it by hand with _sleep_wakeup_lock -- "control_action() uses an Event-based barrier, not a mutex, so two concurrent callers can both pass the barrier". That workaround covers three call sites; the @control_action_decorator path takes no lock at all, so the five decorated methods can race each other or a sleep()/wakeup(). An Event is a broadcast, not a token: set() releases every waiter. So two threads could both clear control_request_barrier and both set control_action_done, releasing the executor loop while one body still ran -- the corruption the guard is supposed to prevent, reachable by the other of its two routes. Replace the bool with a real mutex, which is the primitive the handshake was missing. Concurrent callers serialise rather than fail: that is what the existing _sleep_wakeup_lock sites already rely on, and refusing them would report contention as re-entrancy. Only a nested call from the thread already holding the lock raises, since blocking there would deadlock; it is distinguished by thread ident rather than by taking the lock again. _sleep_wakeup_lock is now redundant but left in place -- removing it touches multi-rank sleep/wakeup, which deserves its own change. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
CODING_GUIDELINES.md requires a return annotation on every function. Add -> None to the tests and their worker closures, and a TYPE_CHECKING-quoted PyExecutor return type to _make_executor() so the deliberately deferred runtime import stays inside the function body. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
Review follow-ups from brnguyen2 on NVIDIA#17346. The lock is held across control_request_barrier.wait(), which was unbounded. _handle_control_request pulses set(); clear() on the aborted-control-request path without waiting for control_action_done, so a caller that has not reached the wait yet misses the edge. Previously that stalled one caller; with the lock it wedged every later control action silently. Wait in bounded slices instead: shutdown or a dead executor loop fails within one poll interval, and a lost edge fails after _CONTROL_BARRIER_TIMEOUT_S. The timeout is deliberately generous because drain=True is unbounded by design - it is a wedge-breaker, not a latency budget. Also note in the docstring that the lock is local mutual exclusion only and does not order control actions across ranks, and add a rank=0 test with a recording queue asserting a rejected nested call enqueues nothing - the previous tests all ran at rank=1, where the enqueue is skipped, so moving the guard below it went undetected. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
_wait_for_control_barrier fast-fails on two conditions: shutdown_event being set, or worker_thread having died. Only the first was covered -- _make_executor never assigns worker_thread, so getattr(...) returned None in every test and the liveness half of the disjunct always evaluated False. Removing it entirely kept the suite green. Add a test that stubs worker_thread with is_alive() -> False while shutdown_event stays clear, so the branch is reachable, and assert the call fails well inside the (monkeypatched, 300s) barrier timeout rather than waiting it out. Also assert _control_action_owner is released in the existing shutdown test, matching the other cleanup tests. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
The deadline stranded the sentinel. Rank 0 enqueues before waiting, so raising while the executor loop is still alive left the loop to pop that sentinel, set the barrier and block in the untimed control_action_done wait with no caller to answer - hanging the executor until the hang detector killed the job. That is worse than the stall the deadline was meant to avoid. Keep polling, but only report a loop that cannot serve the request at all. Both surviving conditions are safe because each implies no consumer is left to strand: shutdown_event is set in _executor_loop_cleanup(), i.e. only once the loop has exited, and a dead worker_thread cannot pop anything either. An orphaned sentinel is inert in both cases. A lost barrier edge therefore still blocks, as it did before this PR. Fixing that needs the abort routed back to the caller, not a timeout; the docstring says so to stop a deadline being reinstated later. Drop the test asserting the removed raise - without the deadline it would hang rather than fail. The two fast-fail tests now run off-thread and join with a timeout so a regression fails instead of hanging CI, and a new test pins the property that matters: with the loop alive the caller keeps waiting instead of stranding the sentinel. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
a5f4f8a to
a6b2aaf
Compare
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
PR_Github #64596 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #64596 [ run ] completed with state
|
|
PR_Github #64659 [ run ] triggered by Bot. Commit: |
|
PR_Github #64659 [ run ] completed with state
|
6 participants
Description
Follow-up to #17163, per the review discussion there (thread): @shuyixiong raised the risk of nested
control_action_decoratorcalls, and we agreed the right place to address it isPyExecutor.control_action()itself rather than any individual caller.control_request_barrierandcontrol_action_doneare single-slot events shared by every control action. If a control action's body opened another one, the inner exit would clear the barrier and setdone— releasing the executor loop while the outer body was still running, so the drain the outer caller asked for would silently no longer hold.Nothing nests today:
WorkerExtension.update_weights()reaches the reuse state through the undecoratedself.engine.reset_prefix_cache(), not the decorated wrapper.sleep()/wakeup()call no decorated sibling.base_workercontrol blocks only touch CUDA and memory.The hazard is real enough that it already had to be worked around by hand —
tests/unittest/_torch/ray_orchestrator/multi_gpu/test_inflight_weight_update.pycallsWorkerExtension.update_weights.__wrapped__(...)with the comment "Invokingsuper().update_weights(...)directly would re-enter the drain=True control_action context manager and nest control actions." Without a guard, getting that wrong corrupts the handshake silently instead of failing at the mistake.Two details of the implementation:
enqueue_control_request(), so a refused nesting attempt leaves no orphaned control request and does not touch the events the outer action still owns.finally, so a body that raises does not wedge the executor.Test Coverage
tests/unittest/executor/test_control_action_reentrancy.py— 4 CPU-only tests (no GPU, MPI or weights; the executor is built withobject.__new__and only the attributescontrol_action()touches, following the pattern intest_sleep_collective_rpc_guards.py):donenot set, which is the corruption the guard exists to prevent;Verified by mutation, not just by passing:
DID NOT RAISE <class 'RuntimeError'>);finallyreset → 3 failed;The new file is registered in
tests/integration/test_lists/test-db/l0_cpu.yml;test-dblists test files individually, so an unregistered unit test never runs in CI.PR Checklist
Dev Engineer Review
PyExecutor.control_action()rejects same-thread re-entrant calls withRuntimeError.QA Engineer Review
tests/integration/test_lists/test-db/l0_cpu.yml.