Skip to content

fix(signaling): retain unacknowledged queueable requests - #1531

Open
allenrchan wants to merge 4 commits into
livekit:mainfrom
allenrchan:allenrchan/signal-fifo-upstream
Open

allenrchan wants to merge 4 commits into
livekit:mainfrom
allenrchan:allenrchan/signal-fifo-upstream

Conversation

@allenrchan

@allenrchan allenrchan commented Oct 17, 2026 •

Copy link
Copy Markdown

Problem

SignalInner::flush_queue drains the whole pending queue before its writes have succeeded. A failed write therefore loses both the failed request and the unattempted tail. Separately, a connected queueable send can be cancelled while waiting for its write acknowledgement before the request has entered the pending queue.

Change

  • Keep queueable requests in a private FIFO until the transport reports a successful write.
  • Stop flushing at the first failure, preserving the failed head and unattempted tail for the next confirmed connection.
  • Enqueue connected queueable sends before awaiting their write, so caller cancellation does not discard the unacknowledged request.
  • Keep the in-flight writer acknowledgement on the queue entry rather than in the sending caller. A later flush settles that attempt before deciding whether to remove or retry it, preventing a cancelled caller's successful write from being replayed.
  • Use the existing bounded writer channel's reservation to make admission and receipt transfer synchronous after capacity is available. The receipt holds only a weak channel identity, so it cannot keep a retired writer alive.
  • Preserve the existing connection state machine, pass-through classification and public API. No new dependencies or features.

This does not promise exactly-once delivery across ambiguous transport failures: an unacknowledged write may have reached the peer. A successfully acknowledged prefix is removed immediately and is not replayed.

Verification

Based on upstream main 5328bf6f9289939f7ec5d563765785f4918eddb1, using Rust 1.99.1 on macOS ARM64:

  • Before the production repair, the new failed-flush regression fails: the retained queue is empty instead of containing the failed head and unsent tail.
  • Before the cancellation follow-up, its same-connection regression fails with track-a, track-a, track-b instead of track-a, track-b.
  • cargo test --locked -p livekit-signaling --lib: 49 passed.
  • cargo test --locked -p livekit-signaling --lib --features native,rustls-tls-webpki-roots: 49 passed.
  • The new cases exercise the existing SignalStream through WsConnection, recover on a fresh stream, and verify that closing releases the connection tasks. Cancellation waits for the external write to begin before aborting the caller.
  • Two cancellation-success regressions release the original external write after cancelling its caller, then verify no duplicate on the same connection or after reconnecting. The existing stalled-write case verifies retry after the old writer is aborted and joined.
  • cargo fmt -p livekit-signaling --check: passed.
  • cargo clippy --locked -p livekit-signaling --all-targets --features native,rustls-tls-webpki-roots --no-deps: passed with existing upstream warnings. No lint suppression or unrelated cleanup is included.
  • Strict -D warnings is not clean on untouched upstream: the same six library/test diagnostics occur on both this branch and its pristine base; including dependencies additionally stops on two existing device-info dead-code diagnostics.

This is package-scoped qualification, not a full SDK/platform, native media, server-backed reconnect or application deployment acceptance claim.

Native CI follow-up

The Windows job on the previous head aborts in the existing libwebrtc::imp::peer_connection::tests::set_bitrate test. Its stack reaches cxxbridge1$exception / Cow<str>::into_owned and a ptr::copy_nonoverlapping precondition failure; signaling tests have not yet run in that job.

This PR does not change libwebrtc, webrtc-sys, workspace manifests/lockfile, Cargo config, or the test workflow. The same test passed on the exact upstream base, with the same Rust 1.97.1, CXX 1.0.198 and MSVC versions. Windows and Linux tests on 03737403 now pass; no native change was made to achieve that result, so the previous Windows root cause is not claimed fixed.

The remaining macOS job aborts with SIGSEGV in test_resume_failure_escalates_to_full_reconnect, just after successful recovery. Local runs against LiveKit 1.13.9 with CI's Rust 1.97.1, ARM64 target, no incremental compilation and debug info disabled pass all seven reconnect tests on both 03737403 and untouched base 5328bf6f. Ten additional exact-case LLDB diagnostic runs pass without reproducing the crash. An earlier Rust 1.99.1 run had a separate participant-reconciliation timeout and is not passing target proof. The SIGSEGV root cause remains unconfirmed; local results do not clear the hosted failure. No test is disabled and no speculative native repair is included. A maintainer rerun with a native crash report is needed because this contributor cannot rerun upstream jobs.

@CLAassistant

CLAassistant commented Oct 17, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

devin-ai-integration[bot]

This comment was marked as resolved.

@allenrchan

Copy link
Copy Markdown
Author

Follow-up on the current macOS CI failure: job 117340260244 exits with SIGSEGV in test_resume_failure_escalates_to_full_reconnect, immediately after the engine reports successful recovery. This is a native process crash, not a failed FIFO assertion. Windows and Linux tests on this head have passed.

I tried to reproduce against a dedicated local LiveKit 1.13.9 server, matching CI's Rust 1.97.1, ARM64 target, CARGO_INCREMENTAL=0 and CARGO_PROFILE_DEV_DEBUG=0:

CARGO_INCREMENTAL=0 CARGO_PROFILE_DEV_DEBUG=0 \
  cargo +1.97.1 test --locked --target aarch64-apple-darwin \
  -p livekit --features __lk-e2e-test --test reconnection_test \
  -- --nocapture --test-threads=1
  • PR head 03737403: all 7 reconnect tests passed.
  • Untouched upstream base 5328bf6f: all 7 passed under the same local fixture/settings.
  • Ten additional runs of the exact failing case under LLDB all passed. This was a bounded diagnostic that would stop on the first non-passing run and dump all thread backtraces, not a retry-until-green qualification.
  • An earlier Rust 1.99.1 run did not reproduce the SIGSEGV either, but had a different participant-reconciliation timeout. That run is not counted as passing target proof.

The reconnect test, engine/session and native WebRTC sources, manifests/lockfile and build config are unchanged by this PR. That does not rule out a timing-sensitive interaction. There is no confirmed crash root cause, and I have not disabled a test or added speculative native changes.

Could a maintainer rerun only the failed macOS job on this head, retaining the macOS native crash report/backtrace if it recurs? This account has read-only upstream access and the scoped rerun command was refused. The local pass does not clear the failed hosted job.

@allenrchan

Copy link
Copy Markdown
Author

Added failure-only macOS reconnection crash-report retention in 20b79f79.
The previous failing run has no retained artifacts. This reuses the repository's
pinned upload action and collects only reconnection_test OS reports, not
process-memory cores or unrelated host logs.

The test command, assertions, retries and failure status are unchanged.
actionlint and git diff --check pass. The native SIGSEGV's root cause remains
unconfirmed; this is diagnostic coverage, not a claimed repair. If macOS emits
a report on a repeated failure, the new artifact should let us inspect its
native stack; missing reports produce an explicit warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants