fix(sim): start/stop_cameras_recording run off the blanket action lock their recorder renders under

[cagataycali]

Through Robot("so101", mode="sim") — the agent path — start_cameras_recording burned its entire 6 s readiness timeout and stop_cameras_recording returned did not stop within 5.0s … Nothing was encoded. The same calls on a bare Simulation worked. _dispatch_action holds the blanket self._lock around both verbs; the recorder thread's render() takes that same lock for its mjData read. Start waited for a readiness the warmup render could not reach, and stop joined a thread blocked on the lock stop held. A self-inflicted deadlock, resolved only by timeouts.

Measured through _dispatch_action, clean upstream/main fa7d21b vs this PR

scenario	before	after
1 camera, steps in between	start 6.00 s not ready, stop 0.35 s / 14 frames	start 0.82 s ready, stop 0.35 s / 17 frames
stop right after start	start 6.02 s, stop ERROR 5.00 s, nothing encoded	start 0.22 s, stop 0.17 s, 1 frame on disk
the next start after that stop	REFUSED Already recording — the world's recorder is wedged	0.23 s ready, 25 frames
2 cameras (added wrist_view)	start 7.00 s not ready, 18 frames/cam	start 0.85 s ready, 21 frames/cam, both MP4s on disk
camera that buffered nothing	names rec_…__default.mp4 — file_exists: false	0 frames - no clip written, path=None

The wedge in row 3 is a consequence worth naming: a stop whose join expires leaves the recording registered, so every later start in that world is refused.

Change

• Both verbs join _SELF_LOCKING_ACTIONS, with the reason recorded beside the existing entries.
• start_cameras_recording takes self._lock itself around validation + registration + thread start (_start_cameras_recording_under_lock) and waits for readiness with the lock released.
• stop_cameras_recording touches the registration and the frame buffers, never mjData, so it takes no lock.
• A camera that buffered nothing says 0 frames - no clip written with path=None instead of naming a file that was never written.
• The recorder samples wall time, which nothing said. Measured: step(n_steps=500) returns in 0.025 s, advances the world 1.0 s, and records 1 frame, not 500. Start's success text and the tool spec now say so and point at start_recording for one frame per control step.

Verification

• New test file, 4 cells. Pre-fix control on clean main: 3 failed, 1 passed (the passer is the unchanged-refusal pin). Post-fix: 4 passed.
• 6/6 mutants caught. The strongest keeps both verbs in _SELF_LOCKING_ACTIONS but re-acquires the lock across the readiness wait — it restores the exact 6.00 s burn, proving the lock-released wait is the load-bearing part rather than set membership alone.
• tests/simulation/mujoco (4 538 items): 46 failed on this branch and 46 on a clean upstream/main worktree, and the two-way comm of the failing-ID sets is empty in both directions — zero regressions. All 46 are the pre-existing No module named 'lerobot.utils.feature_utils' drift on the start_recording/LeRobotDataset path; none reference either verb here.
• Gate: ruff check + ruff format --check clean on 2 125 files; mypy 28 errors in 12 files, identical to the clean-main baseline, none in the touched files. Fragment/overlap/assemble guards all pass.

A regression this PR fixes in its own first commit

The original split carried the Args: block onto the private helper. tests/test_args_docstring_completeness.py walks public methods only (a leading underscore is skipped), so the public verb dropped out of _SURFACES entirely and all seven of its parameters silently lost the documentation guarantee — clean main 567 passed, the split 1 failed. The block is back on the public method (the documented surface) with the wall-clock note and a Returns:; the helper points at it rather than carrying a second copy that could drift. That grader is the pin, so no new test is owed.

Overlap

scripts/check_merge_base_overlap.py names #3343, #3743, #3784 and #3791 on simulation.py. None of the four touches _SELF_LOCKING_ACTIONS, cameras_recording, or rendering.py at all — no duplicate fix and no shared hunk.

LOC: +248 / −34 across 4 files. The additions are the lock split, the 111-line regression pin, and the docstring the grader requires; the deletions are the readiness wait moving out of the locked region.

Measured on Thor (Jetson AGX Thor, MuJoCo 3.5.0, MUJOCO_GL=egl). The frame in the figure is decoded out of an MP4 this PR recorded.

[cagataycali]

Merged main (now e12fe59d7) to clear Detect an untested overlap with the base branch — #3743 had landed on simulation.py after this branch diverged. The overlap script now reports "No overlap … 0 path(s) changed on upstream/main in that span", and _SELF_LOCKING_ACTIONS survived the merge with both new entries intact. Re-verified on the merged tree: my 4 cells pass, test_args_docstring_completeness.py 567 pass, ruff check/format --check clean on 2 133 files, mypy 28 errors in 12 files (the clean-main baseline).

Heads-up unrelated to this branch: main is currently red in two places, so Test and Lint here will report failures this PR does not own. Measured on a clean git worktree at e12fe59d7, with the identical result on this branch (two-way comm of the failing-ID sets empty in both directions):

red cell on clean main e12fe59	result	touches this PR's diff?
tests/test_module_scope_imports_are_not_repeated_in_functions.py — dataset_recorder.py binds import inspect at lines 1381 and 1492	1 failed, 307 passed	no — dataset_recorder.py is not in this diff
tests/simulation/mujoco/test_stop_policy_resolves_the_only_rollout.py — test_an_empty_name_stops_the_one_policy_running, test_an_explicit_name_is_never_overridden	2 failed, 105 passed	no — arrived with #3743

The first is the grader #3787 introduced meeting a duplicate import already in the tree; the second is the "green apart, red together" shape — #3743 was green on its own base. Both are one-line-ish and neither is bundled here, since this PR is about the recorder lock. Files changed by this PR: rendering.py, simulation.py, one new test, one changelog fragment.

[yinsong1986]

Summary

Moves start_cameras_recording / stop_cameras_recording into _SELF_LOCKING_ACTIONS so the dispatched agent path no longer holds the blanket action lock that the recorder thread's render() needs: start now takes self._lock only around validation + registration (_start_cameras_recording_under_lock) and waits for the recorder's readiness event with the lock released, while stop takes no lock (it touches only the registration and frame buffers, never mjData, and joins a thread that renders under the lock). This resolves the self-deadlock where start burned its full readiness timeout and stop's join expired with nothing encoded and the recording left wedged-registered. Also fixes the zero-frame artifact to report path=None / "no clip written" instead of naming a file that was never created, and documents the wall-clock sampling semantics in the success text, docstring, and describe() surface.

What's good

• The lock-split is correct at each seam I traced: registration-before-thread-start invariant preserved, thread-start RuntimeError still deregisters, the _refuse_replacing_cams_recording guard runs under the lock, and the flush-only-deregisters contract is untouched.
• The rationale for set membership is recorded inline next to _SELF_LOCKING_ACTIONS, matching the existing convention for remove_robot / step / stop_policy.
• The Args: block stays on the public verb (docstring-completeness grader: 567 passed on the branch) and the private helper defers to it instead of carrying a copy that could drift.
• Regression pin covers all four behaviors (set membership, fast dispatched start/stop round-trip with frames on disk, unchanged refusal, empty-buffer path=None); ran it on the head SHA: 4/4 pass, plus the neighboring test_cameras_recording_preflight_guards.py + test_action_controller_dispatch_contract.py (66 passed) and test_camera_flush_encoder_refusal.py + test_sim_engine_describe_discovery.py (51 passed).
• No non-ASCII additions, no host paths in the test file, changelog fragment included.

The artifacts[].path -> None change for zero-frame cameras is a behavior change on a returned envelope, but the prior value named a file that did not exist, so any consumer reading it got garbage; this is a bug fix, not a one-way door.

Reviewed at the merged head 088330ea2 (a plain merge of upstream/main into the branch after 0b279712); the effective diff over the merge base is byte-identical to the pre-merge diff, and the regression pin passes 4/4 at the merged head.

[yinsong1986]

Summary

Moves start_cameras_recording / stop_cameras_recording into _SELF_LOCKING_ACTIONS so the dispatched agent path no longer self-deadlocks against the recorder thread: render() serializes its mjData read under self._lock, and the blanket dispatch lock meant start burned its full readiness timeout waiting for a warmup render that was waiting on the lock start held, while stop's join expired against a thread blocked the same way, leaving the recording wedged-registered. Start now takes self._lock only around validation + registration (_start_cameras_recording_under_lock) and waits for the readiness event with the lock released; stop takes no lock, which is sound because it touches only the registration and frame buffers (never mjData) and only flushes after the recorder thread is observed to have exited. I traced the invariants across the split: registration-before-thread-start is preserved, the thread-start RuntimeError path still deregisters, _refuse_replacing_cams_recording runs under the lock so concurrent starts stay serialized, and the flush-only-deregisters contract (including the expired-join retention path) is unchanged. The zero-frame artifact reporting path=None / "no clip written" instead of naming a never-written file corrects an envelope that previously handed callers a path to nothing. Diff is ASCII-clean with no host paths in the test file; changelog fragment included. No blocking concerns found.

[yinsong1986]

Summary

Fixes the self-deadlock where start_cameras_recording / stop_cameras_recording, dispatched through the agent path, held the blanket action lock that the recorder thread's render() needs for its mjData read: start burned its full readiness timeout waiting on a warmup render that was waiting on start's own lock, and stop's join expired against a thread blocked the same way, leaving the recording wedged-registered so every later start was refused. Both verbs now sit in _SELF_LOCKING_ACTIONS; start takes self._lock only around validation + registration (_start_cameras_recording_under_lock) and waits for the readiness event with the lock released, and stop takes no lock, which is sound because it only flips the running flag, joins, and flushes buffers after the thread is observed to have exited - it never touches mjData, and the thread=None registration window it tolerates predates this PR (the synchronous variant leaves it None for good). The invariants survive the split: registration-before-thread-start, deregistration on the thread-start RuntimeError path, the replace-guard running under the lock, and the flush-only-deregisters contract including the expired-join retention path. The zero-frame artifact change (path=None / "no clip written") corrects an envelope that handed callers a path to a file that was never written; no in-tree consumer reads that field assuming a string. Diff is ASCII-clean, the test file carries no host paths, the changelog fragment is present, and the _SELF_LOCKING_ACTIONS rationale is recorded inline beside the existing entries.

What's good

• The regression pin covers the four distinct behaviors (set membership, fast dispatched start/stop round-trip with frames on disk, unchanged refusal + no leaked registration, empty-buffer path=None).
• Scope discipline: the deletions are exactly the readiness wait moving out of the locked region; nothing unrelated moved.
• The Args: block stays on the public verb so the docstring-completeness grader keeps its guarantee, with the helper deferring instead of carrying a copy that could drift.

[cagataycali]

Pushed ae2a539. The public/private split moved name_list_error out of the public verb, and the backend sweep reads public surfaces only. Asking the cameras= shape in the public method - and ahead of with self._lock, since no world state answers it - fixes the sweep and serves this branch's thesis: a mistyped list is refused without contending for the lock the recorder renders under. Pinned behaviourally, because the sweep cannot see placement; putting it back inside the locked section leaves the sweep green and fails that cell. 5371 passed.

[yinsong1986]

Summary

Resolves the self-deadlock where start_cameras_recording / stop_cameras_recording, dispatched through _dispatch_action's blanket self._lock, waited on a recorder thread whose render() needs that same lock: start burned its full readiness timeout and returned success over an empty recorder, and stop's join expired and left the recording wedged-registered. Both verbs move into _SELF_LOCKING_ACTIONS with the reason documented beside the existing entries; start now takes self._lock only around validation + registration (_start_cameras_recording_under_lock) and waits for the readiness event with the lock released, while stop takes no lock at all - it touches only the registration and the frame buffers, and reads buffers only after the join has observed the thread exit. The registration write discipline stays single-writer: start refuses while any state is registered, stop writes None only after a successful flush, and a thread-start failure deregisters before returning its error envelope. A camera that buffered nothing now reports path=None / "no clip written" instead of naming an MP4 that was never encoded, and the success text plus describe() disclose that the recorder samples wall time.

What's good

• The regression pin exercises the real agent path (_dispatch_action) with time bounds on both verbs, plus a lock-contention test proving the cameras= refusal answers without queueing on self._lock.
• Cleanup-on-failure and refuse-don't-clobber invariants are each pinned (_cams_rec_state is None after every refusal), per AGENTS.md #86 resource-cleanup learnings.
• Both docstrings state the concurrency contract explicitly (who holds which lock, and why), per the #85 thread-safety learnings.
• Changelog fragment present, ASCII-clean strings, no host paths in the test file.

Verification suggestions

The new test file is gated on pytest.importorskip("mujoco") and needs a working GL backend, so it can skip silently on CPU-only CI. On a GPU box: MUJOCO_GL=egl pytest tests/simulation/mujoco/test_cameras_recording_runs_off_the_blanket_action_lock.py -v and confirm all 4 pass (not skip).