Fix GUI freeze on stop and prevent corrupted XDF footers

[sappelhoff]

Problem

When stopping a recording, recording::~recording() executes synchronously on the Qt UI thread. Because record_offsets() used an uninterruptible 5-second sleep (std::this_thread::sleep_for(offset_interval)) and boundary_thread joined with a 15-second timeout, the Qt event loop blocked, causing Windows to mark the window as "(Not Responding)" for 5–15+ seconds. Forcefully terminating the application when frozen aborted before stream footers could be written, resulting in corrupted XDF footers and truncated files.

Solution

1. Interruptible Thread Teardown: Added a std::condition_variable shutdown_cv_ in recording and replaced std::this_thread::sleep_for / sleep_until across record_offsets(), record_boundaries(), and typed_transfer_loop() with condition variable waits. When shutdown is triggered, all worker threads wake up in < 1 ms.
2. Active Socket Teardown: Track active lsl::stream_inlet instances and invoke in->close_stream() on shutdown to abort any blocking TCP socket calls or unreachable remote network routes immediately.
3. Low-Latency First-Sample Check: Lowered the initial in->pull_sample() timeout from 4.0s to 0.1s so silent or late-starting streams check shutdown_ rapidly.
4. Automated Integration Test: Added scripts/test_recording_teardown.py to verify that LabRecorderCLI stops in < 0.5s and produces fully valid, uncorrupted XDF files with intact footers.

Verification

• Tested on Windows and Ubuntu via GitHub Actions CI and local automated integration tests.
• Confirmed teardown completes in ~0.5s and resulting .xdf parses cleanly with pyxdf.

[cboulay]

Thanks for tackling the stop latency. I recommend addressing these before merging:

1. Missed shutdown notifications (high priority). shutdown_ and offset_shutdown are changed without holding shutdown_mut_, while the condition-variable predicates use that mutex. A notifier can set the flag and call notify_all() after the waiter reads false but before it enters the wait. The offset thread can then sleep for the full five-second interval. I reproduced this scheduling pattern in a standalone C++ harness. Update the predicate state under the same mutex, then notify, including both the normal and exception paths.

2. The unreachable-stream stop hang is not fully resolved. close_stream() in liblsl v1.18.0.b3 stops the data receiver; it does not cancel the separate metadata receiver. record_from_streaminfo() still calls in->info() with its default infinite timeout, including after an interrupted open_stream(). With an unreachable metadata endpoint, Stop can therefore still block indefinitely. Also, the existing try_join_once() calls blocking std::thread::join(), so the reduced max_join_wait does not bound this wait. Please use bounded metadata waits that observe shutdown and explicitly handle stopping during subscription/header retrieval.

3. Validation does not cover these guarantees yet. The new integration script is not called by the checked-in CI workflow. It verifies only the EEG footer, not the silent marker stream's footer, and permits 1.5 seconds despite documenting a 500 ms limit. Please run it in CI, check both footers and process exit status, and add stop-during-connect/metadata and repeated shutdown cases.

Review scope: source inspection against the PR head and liblsl v1.18.0.b3, plus the isolated condition-variable reproduction; I did not run the full integration script.

[sappelhoff]

Thanks — all three held up, and chasing the second one turned up a data-loss regression in my own patch. Pushed as 54ffd28.

1. Missed shutdown notifications

Correct. requestStop() is now the single shutdown entry point and publishes the flag under shutdown_mut_ before notifying; the destructor calls it rather than assigning shutdown_ itself. The per-stream offset flags go through stop_offsets(), which does the same. They are now shared_ptr<atomic<bool>> rather than a reference into the transfer thread's frame, so an offset thread that has to be detached cannot outlive its flag.

The phase gates test shutdown_ under phase_mut_, so requestStop() takes and releases that mutex too before notifying ready_for_streaming_/ready_for_footers_. The empty critical section is deliberate and commented.

2. Unreachable-stream hang

Also correct, and it went further than the two points you named.

fetch_info() polls info() in 200 ms slices. The result is fetched once in the headers phase and reused for both the header XML and the nominal rate, so the second unbounded info() call in the streaming phase is gone along with its round trip. A stop does not abort it instantly: a still-reachable source gets a short grace period so its header, and therefore its footer, still reaches the file.

open_stream() is sliced the same way in open_inlet(), and so is time_correction(). The watchlist resolver was blocking for a full resolve_interval; it now resolves for 1 s and waits out the remainder interruptibly, keeping the same cadence. The phase gates could park a stream for max_headers_wait with no escape, so one hung stream could cost another its footer — they now also break on shutdown.

On the joins: right, try_join_once() called std::thread::join(), which has no timeout, so a single call against a hung thread never returned and max_join_wait bounded nothing. Threads are now paired with a future that becomes ready when the body returns (worker, timed_join); packaged_task rather than async because the latter blocks in its future destructor, and the task is kept alive by the thread's lambda so detaching stays safe.

3. Validation

Rewritten into five cases, run on all three platforms in CI, checking exit status and the footer of every stream (sample count against the data, timestamps parseable) rather than just the EEG one. The budget is one stated number, 1.0 s, in both the docstring and the assertion, overridable with --max-stop. The cases are: plain stop, stop before the first sample, stop while subscribing to a source that has gone away, three back-to-back start/stop cycles, and no buffered samples lost.

The regression this turned up

Closing every inlet the moment stop is pressed discards whatever is still buffered in them. The new case sends 40 markers and stops; against the previous head of this branch 2 of 40 were recorded. Teardown is now staged — ~recording() signals and gives the stream threads 300 ms to drain and write their footers, and only closes the inlets of threads that are still stuck — plus a final non-blocking pull on the way out of the transfer loop. That case now passes, and teardown measures 0.09–0.19 s locally.

Three unrelated defects fixed while in there: record_offsets() wrote uninitialised offset/now into the file and the offset list when the time-correction query timed out; the inlet registration leaked on every exception path; and a stream that failed mid-recording was left without a footer even though its header was already on disk.

What the tests do not show

Two limits worth being explicit about:

• The integration test does not cover point 1. The lost-wakeup window is too narrow to hit end to end; your standalone harness is still the only reproduction. It also does not reproduce a genuinely unreachable metadata endpoint — dropping a local outlet sends a clean disconnect. It does exercise the info_receiver ... retrying path, but the bound there is by construction, not measured.
• My local build is not a usable crash oracle. I have no MSVC here, so I built with MinGW; under it master itself dies with an access violation on 20/20 stops and hung once for 30 s, and liblsl's loguru warns that __declspec(thread) was ignored, leaving a thread-local buffer shared. I saw two sporadic startup access violations on my build early on and could not reproduce them in 82 targeted runs plus 12 clean suite runs afterwards. CI is the real check here.

Left alone, deliberately

• A detached thread can still write into a destroyed XDFWriter. Pre-existing, and much less likely now that the waits are bounded, but not eliminated; fixing it properly means giving the writer a lifetime independent of recording. Happy to do that here if you would rather not leave it.
• requestStop() still has no caller — the GUI stops by destroying the object on the UI thread. Harmless now that the destructor is bounded, but calling it before currentRecording = nullptr would overlap the wakeup with Qt's event processing.
• Breaking the phase gates on shutdown means a footer can be written before another stream's last chunk, so a file can end up unsorted in the case where a stream has hung. That seemed clearly better than dropping the footer.