Skip to content

Clean up WebRTCSession resources after startup failures - #2808

Open
voropaevv wants to merge 1 commit into
roboflow:mainfrom
voropaevv:codex/fix-webrtc-startup-cleanup
Open

Clean up WebRTCSession resources after startup failures#2808
voropaevv wants to merge 1 commit into
roboflow:mainfrom
voropaevv:codex/fix-webrtc-startup-cleanup

Conversation

@voropaevv

@voropaevv voropaevv commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Why

A failed first connection attempt left WebRTCSession in the STARTED state while its owned asyncio loop and daemon thread remained alive. Failures after RTCPeerConnection construction could also leak the partial peer because self._pc was assigned only after later setup and signaling completed.

There was an additional interruption race in the first draft: canceling the concurrent.futures.Future returned by run_coroutine_threadsafe() only requests cancellation on the event loop. Teardown could read self._pc before the startup task reached a terminal state, then startup could publish a peer afterward.

Fixes #2807.

What changed

  • transition a failed startup to the terminal CLOSED state before releasing the state lock
  • reuse the existing single-flight close() path after releasing that lock
  • publish self._pc immediately after peer construction so partial initialization remains cleanable
  • retain the actual loop-owned startup Task, not only its concurrent future
  • run cancellation and async resource cleanup on the owner loop
  • cancel and await the startup task before reading and closing the peer and source
  • clear the retained startup task only after its terminal state has been observed
  • close an unsubmitted startup coroutine if scheduling fails
  • cancel and drain pending loop tasks, shut down async generators, and close the loop in its owner thread
  • avoid joining the current thread and report a loop-thread join timeout
  • preserve the original startup exception and HTTP diagnostics
  • add deterministic lifecycle coverage for startup, interruption, and teardown boundaries

No public API, retry semantics, callback behavior, queue behavior, or server-side protocol behavior changed.

Verification

  • parent regressions:
    • failure before peer creation left the session STARTED, with an open/running loop, a live loop thread, and no source cleanup
    • failure after peer creation left the peer unpublished and peer.close() uncalled
  • interruption regression against the previous draft head dbb7c1845d90f2ac316bd0384dd47894ea76b5fd:
    • a real background event loop and real run_coroutine_threadsafe() future requested cancellation before teardown
    • startup published the peer after teardown had already inspected self._pc
    • peer.close_calls == 0
  • fixed candidate:
    • the same deterministic race closes the late-published peer exactly once
    • source cleanup runs exactly once
    • the startup task is terminal and cleared before the loop is stopped
  • PYTHONASYNCIODEBUG=1 PYTHONPATH="$PWD" python -m pytest tests/inference_sdk/unit_tests/webrtc/test_session_lifecycle.py -q -W error::ResourceWarning -W error::RuntimeWarning
    • 46 passed
  • PYTHONASYNCIODEBUG=1 PYTHONPATH="$PWD" python -m pytest tests/inference_sdk/unit_tests/webrtc -q -W error::ResourceWarning -W error::RuntimeWarning
    • 97 passed, 14 existing deprecation warnings
  • PYTHONPATH="$PWD" python -m pytest tests/inference_sdk/unit_tests -q
    • 533 passed, 86 warnings
  • make check_code_quality
    • Black, isort, and fatal Flake8 checks passed
  • direct Black/isort checks for both changed files and git diff --check passed
  • mutation checks:
    • removing the terminal startup-task await makes the cancellation-boundary regression fail with peer.close_calls == 0
    • removing startup-task tracking makes the successful-start lifecycle contract fail
    • restoring each production line makes the corresponding test pass

Not verified

  • a live end-to-end WebRTC session against a signaling server
  • the upstream Python and platform CI matrix
  • the full inference server and optional model dependency suites

Risks / rollback

The change is limited to client-side WebRTC lifecycle ownership. Cleanup now waits for the loop-owned startup task to terminate before inspecting partial resources; successful startup observes an already-completed task, so normal close does not add a blocking startup wait.

Deterministic tests cover failures before and after peer creation, concurrent callers, an external interruption with real cross-thread future semantics, publication during cancellation, scheduling failures, pending tasks, cleanup failures, and successful startup.

Reverting commit 304308af0ce32af7123d9d6bc39aa1f8abec3ccc restores the previous behavior.

Migrations / external effects

None.

Screenshots / preview

Not applicable; this changes internal SDK lifecycle behavior.

Ensure failed startup attempts enter a terminal state and reuse the existing single-flight teardown path. Track the loop-owned startup task so cleanup waits for it to stop before reading partial resources.

Publish peer connections as soon as they are created, close the event loop in its owner thread, and preserve the original startup error.

Cover pre- and post-peer failures, interruptions, concurrent callers, scheduler failures, pending tasks, and teardown failures.
@PawelPeczek-Roboflow

Copy link
Copy Markdown
Collaborator

Hi there
Really sorry but we did not finish checking other PRs prior to todays release. We will try to make it reviewed next cycle.
Thanks for the contribution.

1 similar comment
@PawelPeczek-Roboflow

Copy link
Copy Markdown
Collaborator

Hi there
Really sorry but we did not finish checking other PRs prior to todays release. We will try to make it reviewed next cycle.
Thanks for the contribution.

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.

WebRTCSession startup failures leak event-loop resources

2 participants