Restore macOS Intel

[larsoner]

Hopefully this is all that's needed! 🤞

[larsoner]

@mscheltienne a different run is failing here, it's flaky. I'll keep looking to see if I can make it more reliable somehow (maybe get Claude to see if it has ideas...)

[larsoner]

@mscheltienne I threw Claude Opus 4.8 at the timeout problem with the suggestion of using pytest-rerunfailures and this is what it came up with. Hopefully it comes back green on the first try this time 🤞

[mscheltienne]

the suggestion of using pytest-rerunfailures

I had this suggestion several times, it's a workaround instead of digging through the root cause which is why I haven't implemented it yet (it potentially hides failures that we don't have figure out yet). In this case, I tend to push Claude to dig for the root cause by making hypothesis then creating debug scripts in e,g, /tmp/ that it can executes to validate or invalidate its hypothesis, potentially monkeypatching elements to inflate artificially race and issues. Throwing a bit Fable at it, I got the following points to fix/try. Have a look and let me know what you think, for now I reverted the rerun to see if those tests still fail.

A. Over-count (4 epochs, expected 3) — test_epochs_with_irregular_numerical_event_stream_and_event_id

Fixed in 89cd69c

A — over-count (4 vs 3) is a real library bug in _prune_events, not a test issue. The dedup key is wrong: _last_ts stores the absolute data-buffer timestamp a marker maps to (epochs.py:556), and _prune_events re-admits any event whose mapped timestamp is strictly > last_ts (epochs.py:966-967). But liblsl re-derives per-sample timestamps on every pull_chunk with a clock-sync correction that jitters by ~1e-11 s (1 ULP). So the same physical sample the last-accepted marker maps to drifts a hair above the _last_ts it itself set → the marker re-admits itself → a duplicate 4th epoch. The agent reproduced it deterministically at the real bufsize=10 (last_ts=2545.447142958006 vs later mapped_ts=2545.4471429580462, larger by ~4e-11). Slow/Intel runners hit it because the new test's 1.5s settle keeps calling acquire() through the drift window; the old test exited on first sighting of 3 and never saw the late dup.

B. test_epochs_single_event flaky

Fixed in ce8896e

B — A much larger ~-0.999 s backward jump in the data-stream timestamps once per file loop. It traced this to a PlayerLSL._stream loop-wrap bug (player_lsl.py:285-291): when file length is a multiple of chunk_size, start %= size turns start=1000 into 0, so it vstacks the whole file + next chunk (~1200 samples) but advances the timestamp by only one streaming_delay → liblsl back-dates that oversized chunk ~1 s into the past. B proved causation: forcing ts monotonic flips the count [2,2,2,2] → [0,0,0,0].

C. Fatal Python error: Aborted (SIGABRT / exit 134)

Maybe fixed in: sccn/liblsl#289

The abort happens on the main thread inside lib.lsl_destroy_inlet during stream.disconnect() — and faulthandler showed only one Python thread alive, which rules out every Python-level threading/teardown/numpy-race theory.

Chain: StreamInlet.del() does close_stream() then destroy on every disconnect. close_stream kicks liblsl's internal data-receiver thread into try_recover_from_error() → resolver resolve-waves (the "Stream transmission broke off ...; re-connecting..." line before every teardown). The destructor's disengage() → resolver.cancel() → cancel_all_registered() calls cancel() → shared_from_this() on a resolve-attempt that has no live shared ownership (mid-construction or mid-destruction) → throws std::bad_weak_ptr on the main thread → the unwind skips watchdog_thread_.join() → ~inlet_connection (defaulted, no destructor) destroys a still-joinable std::thread → std::terminate → abort (exit 134). The CI log's fingerprint matches exactly: WARN| Unexpected error during inlet shutdown: bad_weak_ptr, then 501 ms later terminate called without an active exception.

This is upstream sccn/liblsl#220 (reported by this repo's author, closed as "fixed by sccn/liblsl#246" — but sccn/liblsl#246 is unrelated; the race is still reachable in the pinned 1.17.7). Intermittent because the race windows are ~100 ns, widening to ms only under CPU-starved 2-vCPU CI (not reproduced in ~1300 local iters). 3.14t is not an amplifier here (native-thread race + one GIL-dropping ctypes call).

[larsoner]

But you are also using ./.github/actions/retry-step which seems like a less fine grained (and slower) way of retrying, no? Agreed it would be better to get rid of retry altogether, but if you can't do that then at least making it more targeted toward the tests that failed would make it retry faster.

[mscheltienne]

So yes.. ./.github/actions/retry-step but that's even a bit worst than that since it's mostly here to retry flakiness on the C-code and segfault/python fatal errors on which we have little control without changes upstream in liblsl (but they are coming, it's more actively developed than it used to be!).

Anyway, I think the fix above and one more improvement baking now should actually yield stable CIs, if not I'll re-introduce the pytest-rerunfailures to indeed keeps CIs moving.

[mscheltienne]

I'll trigger the CIs 5/6 times to check for remaining flakiness. For now: 2 / 2.

[larsoner]

Ahh yes I didn't think of segfault. But maybe pytest-xdist plus rerun would also work for this case (can't remember but it might) to stay more targeted

Hopefully it won't be needed at all! 🤞

[mscheltienne]

I definitely remember looking into that.. but can't remember what was the issue with pytest-xdist at the time that led to the custom action. Anyway, 🤞

[larsoner]

Coming back green again, time to remove the retry-step?

[mscheltienne]

The retry-step would be necessary at least until sccn/liblsl#289 is reviewed and merged - Claude flags that this error is currently caught by the retry-step and thus "safe".

[mscheltienne]

But at least, it looks like flakiness in our python side is down!

[larsoner]

Yeah once it looks good to go, just ask Fable about the possibility of replacing the action-retry with pytest-xdist (even with n=1, which seems necessary for all this network business) plus pytest-rerunfailures... I thought I saw that working in some repo recently but maybe not! But if it does work, would be nice to slot it in here as well. (Or I can do it if you need to move on to other stuff.)

[mscheltienne]

Go for it, thanks 🙏

[larsoner]

Looks like we're doing well, Fable had more ideas, but I'm going to have it pursue (1) and (2) because they seem low-risk:

Ideas

Yes — several, and the vendored liblsl source lets me ground them in the actual crash path. Recall the chain: close_stream() breaks the connection → receiver thread calls try_recover_from_error() → resolver launches resolve waves ("re-connecting…" logs) → lsl_destroy_inlet races with an in-flight resolve attempt → shared_from_this() throws bad_weak_ptr → unwind skips the thread join → std::terminate. Ordered by leverage:

1. Stop calling close_stream() before lsl_destroy_inlet in StreamInlet.del (stream_inlet.py:143). In the vendored source, try_recover_from_error() is gated on !shutdown (inlet_connection.cpp:281), and shutdown_ is set by the destructor's disengage() before it closes the sockets. So if you destroy directly, the connection breaks after the shutdown flag is up and recovery never engages — no resolve waves, no race. The explicit close_stream() inverts that ordering: it breaks the connection while shutdown_ is still false, which is exactly why every teardown logs "transmission broke off; re-connecting" and then destroys mid-resolve. The C++ destructor closes everything anyway, so the call looks redundant — it dates to the big ENH: Build and bundle liblsl in wheels #351 refactor with no sign it's load-bearing. This is a one-line change, but it needs CI-scale stress validation since the abort never reproduced locally.

2. Disable recovery (recover=False) on the inlets used in tests. Also verified in source: with recovery_enabled_ false, try_recover_from_error() throws lost_error and returns — the resolver and its cancellable resolve attempts (the objects whose cancel() triggers bad_weak_ptr) never exist at all. StreamInlet already exposes the flag, but StreamLSL doesn't plumb it (stream_lsl.py:183 uses the default True). Exposing recover on StreamLSL.connect()/EpochsStream and passing False in tests closes the crash path without touching the public default. Caveat: any test that stops a Player while a stream is still connected would then get lost_error in the acquisition thread instead of silent reconnection attempts, so the acquisition error handling would need a look.

3. Carry the upstream fix in the submodule. You build liblsl from src/liblsl (pinned at sccn/liblsl@64988c6a), so once inlet: fix bad_weak_ptr abort during inlet destruction sccn/liblsl#289 merges — or sooner, by pointing the submodule at a fork with the fix cherry-picked — the root cause disappears for wheel users too, not just CI. Reviewing [pre-commit.ci] pre-commit autoupdate #289 upstream is probably the highest-value use of effort since mscheltienne already has the relationship there.

4. Teardown hygiene as a frequency reducer: the second abort in today's log fired from a GC-triggered del, not an explicit disconnect. Making fixtures disconnect deterministically (and keeping disconnect-before-player-stop ordering) shrinks how often destruction happens at arbitrary moments, though it can't eliminate the race.

My recommendation: 1 and 2 are complementary and both Python-side-available today — 1 removes the provocation (recovery engaging at teardown), 2 removes the machinery (resolve attempts existing at all). I'd try 1 first since it's least invasive and arguably a correctness cleanup on its own, keep the xdist rerun as the safety net, and treat 3 as the durable fix.

So I'll push these, and then I think we should merge assuming it comes back green again!

[mscheltienne]

For the proposal (3), as soon as 1.18 stable is released (I think we are now at the 4th RC release), I will update the liblsl bundled to that new version; hopefully including the fix proposed.

[mscheltienne]

Can you also delete the now unused retry action?

[larsoner]

I think it's still used for doc building

[larsoner]

Looks like doc build has been stable, I'll remove the retry for it and the action

[larsoner]

Okay good to merge I think @mscheltienne !

[mscheltienne]

Thanks a lot!

[larsoner]

Hopefully things stay stable now!