fix(cameras): contain a wedged camera read — bounded locks, first-frame deadline

[Mokuroh54]

Important

Stacked PR — layer 2 of 2 on #70 (fix/camera-hotplug-pump). Merge #70 first, then retarget this to main (automatic if #70's branch is deleted on merge). Do not squash-merge #70 while this is open — that duplicates its commits into this diff and forces a manual rebase. Merge order: #70 → this.

What

Defense-in-depth for the same-port-replug camera wedge. #70's runloop pump prevents stale devices (the list heals, replug self-recovers). This layer survives the case the pump cannot reach: a cap.read() already bound to a dead handle — isOpened() stays True, the read blocks forever while holding the per-index lock (verified live 2026-08-07), and one wedged read then stacks every preview client, freezes recording/inference start, and holds uvicorn's graceful shutdown hostage via the in-flight MJPEG response.

• Every lock acquisition that contends with a reader is bounded (LOCK_ACQUIRE_TIMEOUT = 2.0s). Timeout ⇒ the reader is wedged: leak the capture, log loudly, deregister, move on. Leaking is deliberate — releasing a capture under an in-flight read segfaults cv2.
• A stream's first read gets a deadline (FIRST_FRAME_TIMEOUT = 5.0s), so a wedged device becomes a visible stream error instead of an eternal blank tile. The wedge signal is precise: the watchdog thread still alive after the join. A read that raises is a fast failure and releases normally (tested, mutation-verified).
• Because a wedged entry is deregistered, a retry after the pump heals the device gets a fresh capture and succeeds — the pump makes recovery possible, this layer makes the retry reachable without a hang.

Known tradeoff (deliberate)

Each wedged attempt against a genuinely dead device leaks one cv2.VideoCapture + one daemon thread. Retries are bounded by the frontend's capped backoff; a leak beats a process-wide hang, and the only true fix for an already-wedged handle is a restart.

Rebased onto #70's identity-keyed base

#70 now keys the preview capture registry by camera uniqueID rather than by cv2 index, so this layer's wedge flag is scoped to the device that wedged instead of to an index slot — previously a renumber left a wedged entry fast-failing whatever camera had since taken that index, while the genuinely dead device answered fresh under its old number. The bounded-lock and first-frame-deadline logic is unchanged; every registry lookup now goes through the identity key, and the "per-index lock" is a per-camera lock throughout.

Checks

ruff check + ruff format --check clean; pytest tests/test_camera_preview.py tests/test_camera_identity.py → 44 passed (23 preview + 21 identity, all hardware mocked; verified under both random and fixed test ordering); scoped pre-commit (typos, bandit, mypy et al.) fully green.

test_wedge_does_not_fast_fail_a_different_camera_at_the_same_index is the one that earns its place here: it passes only if the containment and the identity keying are both present and interacting correctly.

Deferred / out of scope

• Stale camera_runloop_experiment.py at repo root is fix(cameras): hotplug/replug self-recovers via main-runloop pump; preview tile releases on remove #70's, untouched here.
• camera_identity.py change is docstring-only: documents the residual hole this layer contains, without weakening fix(cameras): hotplug/replug self-recovers via main-runloop pump; preview tile releases on remove #70's pump claims.

🤖 Generated with Claude Code

[IsaacSinn]

1. Only the first read is protected — a camera that wedges later still hangs shutdown. The read loop is if first_frame: _read_first_frame(entry) (watchdog) else: entry.cap.read() — the else branch is a raw, unbounded OpenCV call made while holding the per-camera lock. So: open a preview, see frames, unplug and replug in the same port. The next read wedges with no deadline, the thread sits in OpenCV forever holding the lock, and the MJPEG response never completes. A --reload or graceful shutdown still hangs until you force-kill — which is the exact failure this PR's own commit message says it contains. The added test covers a blocking first read, not a blocking later one. This one is pre-existing, but it blocks because the PR claims to close it.

2. Retries leak a thread and a capture, without bound. After a 5s wedge the code abandons a daemon thread plus its capture and deregisters the index, so the frontend retries — opening another capture that can wedge another thread. BackendCameraStream.tsx caps the retry interval at 12s but never caps the attempt count. Leave the tile open on a genuinely dead camera and threads and handles accumulate until restart. Introduced: the deregister-and-reopen behavior is new, and it turns the existing retry loop into an unbounded leak. (The good news: an abandoned read writes only into its own stale local list, so it can't inject a garbage frame into the new owner.)

3. The 2s lock timeout undercuts the 5s deadline. LOCK_ACQUIRE_TIMEOUT = 2.0, FIRST_FRAME_TIMEOUT = 5.0. A real webcam can legitimately take 2–3s for its first frame. Open the same preview in two tabs: the first gets 5s, the second waits only 2s for the shared lock, declares a perfectly healthy camera wedged, and tells that user to restart the app — possibly moments before the first request succeeds. Then the entry stays flagged wedged, so everyone after fails fast too.

[Mokuroh54]

All three confirmed and fixed in 551289e1 — thank you, these were all reachable exactly as described, and each one now has a regression test. I reproduced each against the branch before touching anything, so the notes below are measurements rather than readings.

1. Only the first read was protected. Confirmed. Reproduced with a capture that streams one frame and then blocks: the generator never returns, so the MJPEG response never completes and graceful shutdown waits on it forever — the hang the commit message claimed to contain.

One scoping note in the PR's favour: the lock acquisitions on the other three paths (_acquire, _release, stop_all) were already bounded, so a late wedge never froze Start or stacked new clients. What survived was specifically the stream generator itself, which is the one that holds shutdown hostage. Narrower than "the containment doesn't work", but you're right that it's the headline case.

_read_first_frame is now _read_with_deadline(entry, timeout) and every read goes through it — FIRST_FRAME_TIMEOUT (5s) for the first, a new READ_TIMEOUT (2s, ~30x a healthy read at 15fps) for the rest. I benchmarked the watchdog-per-frame cost before committing to it: 25us to start and join, ~0.04% of one core per camera at TARGET_FPS, which is noise next to the JPEG encode already in that loop. It's also the only way to bound an uncancellable blocking call in CPython.

2. Retries leak a thread and a capture. Confirmed, and the mechanism is slightly worse than "the deregister-and-reopen behavior is new": _release deletes the entry from _captures before the wedge flag is set, so the flag landed on an object nobody could look up again. _acquire's fast-fail check and the class docstring's promise that "new clients get a fast CameraOpenError" were both already there — they were simply unreachable, because the entry was always gone before the next client arrived.

Measured on the unfixed branch: 10 retries against a dead camera opened 10 captures and stranded 10 threads.

Fixed by keeping the entry as a tombstone (_mark_wedged, guarded so it can't overwrite a live entry that has since taken the key). The cost is now capped at one thread, one capture and one lock per physically wedged camera, no matter how long the tile retries. stop_all also skips tombstones now — otherwise it re-waited LOCK_ACQUIRE_TIMEOUT on each of them on every recording/inference Start, to re-learn what it already knew.

I did not add the frontend attempt cap, and I want to flag that rather than quietly skip it. Once the leak is fixed at the source, a retry is an instant 503 with no allocation and no log line, so the cap would be cosmetic — and BackendCameraStream's unbounded retry is deliberate: most preview failures are transient (a recording session holding the cameras, a restarting server), and a blind attempt cap would leave the tile dead after a long recording. If you'd still rather bound it, I'd want it conditional on the unrecoverable reason rather than on attempt count, which needs a machine-readable signal the endpoint doesn't currently emit. Happy to do that as a follow-up if you disagree.

3. The 2s lock timeout undercuts the 5s deadline. Confirmed, and this one was the most interesting. Reproduced with a healthy camera that takes 3s for its first frame (well inside the 5s budget): the second client is refused with "Restart MakerMods Lab", and entry.wedged is set on a working device.

LOCK_ACQUIRE_TIMEOUT is now derived rather than picked:

LOCK_ACQUIRE_TIMEOUT = max(FIRST_FRAME_TIMEOUT, READ_TIMEOUT) + 1.0

The derivation is really the point. That timeout is the sole evidence for "the holder is wedged", and the inference only holds if no healthy reader can still hold the lock when it expires — i.e. if it sits strictly above every read deadline. At 2.0 it sat below the 5.0 first-frame budget, so a warming-up camera and a dead one were indistinguishable. There's now a test asserting the ordering directly, since inverting it silently re-arms this.

Two consequences worth naming:

• stop_all's worst case on the Start path goes 2s -> 6s, but only when a camera is genuinely mid-first-frame or wedged. That's the price of removing the false positives, and with tombstones a false wedge is now permanent-until-restart, so removing them stopped being optional.
• The test fixture that shrinks these timeouts now preserves the production ordering rather than setting them equal, so it can still tell a slow read from a wedge.

Known limit, unchanged: the tombstone makes recovery restart-only, which is what every message on this path already says. #70's runloop pump means a replugged camera could in principle revive in-process, but telling "it came back" from "still dead" needs a re-enumeration signal — the uniqueID reads identically either way. Worth a follow-up, not this PR.

Tests added/updated in tests/test_camera_preview.py: mid-stream wedge ends the response; 10 retries open one capture and strand no extra threads; a slow-but-healthy first read is not mistaken for a wedge by a second client; the timeout ordering invariant; stop_all skips tombstones; and the two existing wedge tests updated to assert the tombstone instead of deregistration.

Full suite: 1276 passed. The 7 test_models.py failures are pre-existing on this base (real-user-state leakage, fixed separately by #76) — I verified them against a pristine checkout of the branch tip.