Skip to content

fix: consume buffered final SIP responses after transaction completion - #923

Open
benjamin-di wants to merge 2 commits into
livekit:mainfrom
benjamin-di:fix/final-response-after-done-upstream
Open

benjamin-di wants to merge 2 commits into
livekit:mainfrom
benjamin-di:fix/final-response-after-done-upstream

Conversation

@benjamin-di

@benjamin-di benjamin-di commented Oct 25, 2026 •

Copy link
Copy Markdown

On TCP/TLS, a client transaction can finish immediately after queuing a final response. If both Done() and Responses() are ready, sipResponse can select completion first and report an incomplete transaction despite a buffered final response. This also loses authentication challenges before a retry can be constructed.

Drain queued responses when completion is observed, retaining normal cancellation and missing-final-response handling. Regressions cover buffered 200, 401, 403 and 407 responses after a provisional response, plus completion without a final response when the response channel is either closed or still open. The open-channel cases are bounded and release their owned channel on failure; they check that an incomplete transaction returns promptly and retains the provisional-response count.

Validation on upstream 1df341e plus this patch: focused race-enabled tests repeated ten times; go test -race -coverprofile=coverage.out ./pkg/... passed. sipResponse has 100% local statement coverage, including the previously untested completion-with-empty-open-channel return. Replacing the nonblocking drain with a blocking receive makes both new cases fail within their watchdog, without a race or global test timeout. The original buffered-final regression fails on the unchanged stable implementation and passes with the fix.

The latest commit is a test-only follow-up. The runtime patch carries no transport/configuration changes and adds no dial retries.

Rebase validation (2026-10-30)

Rebased onto ffd1a64b1755; current head 7c0255e79b00. go test -race ./pkg/... passes on this head. No live SIP call was made.

Rebase validation (2026-10-31)

Rebased onto main at e73272dedf3b; current head 1eca7b475a7d.

The complete go test -race ./pkg/... suite and git diff --check pass. No live SIP call or separate integration fixture was run after this rebase.

@benjamin-di
benjamin-di requested a review from a team as a code owner October 25, 2026 08:03
@CLAassistant

CLAassistant commented Oct 25, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

@codecov

codecov Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.42%. Comparing base (0460b40) to head (1eca7b4).
⚠️ Report is 435 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #923      +/-   ##
==========================================
+ Coverage   65.25%   70.42%   +5.16%     
==========================================
  Files          51       46       -5     
  Lines        6588     9149    +2561     
==========================================
+ Hits         4299     6443    +2144     
- Misses       1915     2191     +276     
- Partials      374      515     +141     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@benjamin-di

Copy link
Copy Markdown
Author

Added the test-only follow-up in 37a61ef for a completed transaction whose response channel is empty but still open, both without responses and after a provisional response. It verifies a prompt incomplete-transaction error with the correct response count; the watchdog releases its owned channel before reporting a stalled receive.

The focused race-enabled tests pass ten repeated runs, and go test -race -coverprofile=coverage.out ./pkg/... passes. Both new cases fail with a deliberately blocking receive. Hosted tests/integration pass, and Codecov now reports all modified lines covered. No runtime change.

@benjamin-di
benjamin-di force-pushed the fix/final-response-after-done-upstream branch 5 times, most recently from e81d5e2 to 7c0255e Compare October 30, 2026 05:58
Reliable transports can close the transaction immediately after queuing a
final response. When both channels are ready, preserve queued responses
before reporting an incomplete transaction. Cover authentication challenges,
success and rejection responses, and missing final responses.
Check empty and provisional-only open response channels after completion.
Both must return the incomplete-transaction error without blocking and
retain the response count. Bound the test and release its owned channel
before reporting a timeout. Runtime behavior is unchanged.
@benjamin-di
benjamin-di force-pushed the fix/final-response-after-done-upstream branch from 7c0255e to 1eca7b4 Compare October 30, 2026 23:17
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