chore: modernize Recorder framing, CI coverage, and dependency floors#28
Merged
Conversation
- Reframe the Recorder docstring: it is OpenAdapt's original, hand-built, battle-tested recorder (the engine behind openadapt-flow's desktop record backends), not "legacy" code. Documents the multiprocessing listener/writer architecture and the headless-import invariant. - CI: add a windows-latest job that runs the unit suite plus the live Recorder integration tests (tests/test_performance.py, marked slow) -- the live recording path previously ran in NO CI anywhere because those tests skip off-Windows and CI was ubuntu-only. Add a macos-latest job running the unit suite for platform coverage. - Dependency floors: pynput>=1.7.6, av>=12.0.0, Pillow>=10.1.0, numpy>=1.26.0 (previous floors predate cp312 wheels, so they were unsatisfiable on the newest supported Python). Full suite verified green against current stable resolutions (av 18, Pillow 12, numpy 2.5, SQLAlchemy 2.0.51). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The new windows-latest CI leg surfaced a real bug invisible on POSIX: CaptureSession.close() closed the SQLAlchemy Session but never disposed the engine, so the pool kept the recording.db file handle open. On Windows that leaves the capture directory undeletable (WinError 32) -- teardown of every test using a TemporaryDirectory failed, and any consumer deleting a capture dir after close() would hit the same lock. - CaptureSession.close() now disposes the session's bind engine. - tests/test_highlevel.py: dispose the helper-created engines in the temp_capture_dir fixture teardown (before the tmpdir is removed), and dispose the engine leaked in test_pixel_ratio_round_trips_through_model. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ssions Second round of Windows CI findings (all invisible on POSIX, all WinError 32 file locks on recording.db): - CaptureSession.load() error paths (corrupt query, no recording row) closed the session but never disposed its engine -- the pool kept the file handle open. Both paths now dispose the bind as well. - get_session_for_path() leaked its engine when migrate_missing_columns raised on a corrupt/unreadable db (the sqlite connection to the bad file stayed pooled). Now disposed before re-raising. - tests/test_highlevel.py: crud.insert_recording ends with session.refresh(), which leaves the helper session's connection checked out until close -- engine.dispose() alone cannot reclaim it. The temp_capture_dir fixture now closes helper sessions before disposing helper engines, and test_session_leak_on_no_recording no longer discards the setup engine it creates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Third Windows CI finding: the live roundtrip test hung for the full 300s pytest timeout inside Recorder.__exit__. process_events() used a bare blocking event_q.get(): when terminate_processing is set while the queue is empty and the reader threads have already exited, no event ever arrives, the loop condition is never re-checked, and join_tasks() waits on the event_processor thread forever. The get is now bounded (timeout=1) so the loop re-checks terminate and drains cleanly. Also make the integration tests wait on the recorder's own readiness signal (rec.wait_for_ready()) instead of a fixed 0.5-1s sleep: on a cold CI runner startup takes ~3.5s, so the synthetic input was being injected before the listeners were up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Hosted Windows runners execute jobs in a non-interactive session, so SendInput-injected events never reach the low-level hooks pynput uses: the three event-capture assertions (roundtrip, reuse, throughput) can never hold there -- a runner-environment limitation, not a recorder bug. The CI step now sets OPENADAPT_CI_NO_INPUT_INJECTION=1 and those three tests skip with a precise reason, while the live pipeline tests that do not depend on captured input (recorder startup + clean bounded shutdown, per-capture db creation, bounded memory) keep running the real recorder on windows-latest. Run the full set on an interactive Windows desktop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The founder's call: the live
Recorderin this repo is the recorder for OpenAdapt's desktop path — the original, hand-built, battle-tested recording code in the org. This PR makes the code and CI reflect that, and fixes the real bugs the new CI legs immediately surfaced.1. Recorder reframed (docstring, not behavior)
openadapt_capture/recorder.pyno longer self-describes as "Copied from legacy OpenAdapt record.py". The new docstring states what it is — the production recording engine behindopenadapt-flow record --backend windows|rdp— and documents the multiprocessing listener/writer architecture and the headless-import invariant (enforced bytests/test_headless_import.py).tests/test_performance.pyheader/skip-reasons updated to match.2. The live recording path now runs in CI (it previously ran in NO CI anywhere)
test-windows(windows-latest): full unit suite + the live Recorder integration tests (-m slow). The live tests start the real multiprocessing pipeline (pynput listeners → writer processes → per-capture SQLite + PyAV video) and prove clean startup, bounded shutdown, recording.db creation, and bounded memory on real Windows. The three listener-dependent tests (roundtrip / reuse / throughput) skip there viaOPENADAPT_CI_NO_INPUT_INJECTION=1with a precise reason: hosted runners execute jobs in a non-interactive session, so SendInput-injected events never reach pynput's low-level hooks — an environment limitation, not a recorder bug. Follow-up: run the full event-capture set on an interactive Windows machine (e.g. the Parallels VM rig or a self-hosted interactive runner).test-macos(macos-latest): the unit suite (event processing, storage, PyAV video encode/decode, headless-import invariant, Recorder API surface). Theslowtests stay skipped there — pynput injection needs Accessibility permissions a hosted runner cannot grant, and the multiprocessingspawnwriter path is not yet validated end-to-end on macOS.3. Real bugs found and fixed by the new CI legs
All invisible on POSIX/ubuntu-only CI:
recorder.py):process_events()used a bare blockingevent_q.get(); when terminate is set with an empty queue and the readers gone, the loop condition was never re-checked andRecorder.__exit__hung forever (the live test burned its full 300s timeout). The get is now bounded (timeout=1).CaptureSession.close()and bothload()error paths closed the Session but never disposed the engine, so the pool keptrecording.dbopen — on Windows the capture directory could not be deleted.get_session_for_path()also leaked its engine when opening a corrupt db raised. All now dispose properly.tests/test_highlevel.py): helper-created sessions/engines are now released before temp-dir teardown (crud.insert_recordingends withsession.refresh(), which keeps the connection checked out until close).rec.wait_for_ready()instead of a fixed 0.5–1s sleep (cold-runner startup takes ~3.5s, so input was being injected before the listeners were up).4. Dependency floors refreshed (conservative)
pynput>=1.7.6,av>=12.0.0,Pillow>=10.1.0,numpy>=1.26.0. The old floors (av>=10,Pillow>=9,numpy>=1.20) predate cp312 wheels, so they were unsatisfiable metadata on the newest supported Python. No majors chased: suite verified green against current stable resolutions (av 18.0, Pillow 12.3, numpy 2.5, SQLAlchemy 2.0.51, pynput 1.8.2).Verification
ruff checkclean;pytest -m "not slow"→ 153 passed.Notes
LICENSEorREADME.md(open PRs chore: add MIT LICENSE file #26/docs: note openadapt-flow desktop record on-ramp in status banner #27 own those).🤖 Generated with Claude Code