Skip to content

Quiet expected teardown logs and lock capability reads - #4010

Open
vavo wants to merge 2 commits into
livepeer:masterfrom
vavo:notrius/fix-3922-3885
Open

Quiet expected teardown logs and lock capability reads#4010
vavo wants to merge 2 commits into
livepeer:masterfrom
vavo:notrius/fix-3922-3885

Conversation

@vavo

@vavo vavo commented Aug 2, 2026

Copy link
Copy Markdown

What does this pull request do?

Reduces expected teardown noise and removes unsynchronized external capability map reads.

Specific updates

  • Demote cancellation-driven FFmpeg, segmentation, subscriber preconnect, and trickle client-disconnect messages from error-shaped output to debug-level logging.
  • Add locked external capability lookup and name snapshots.
  • Replace production direct map reads and protect mutable capability fields used by those readers.
  • Add regression coverage for canceled trickle clients and concurrent capability access.
  • Add pending changelog entries.

How did you test each of these updates?

  • go test ./trickle -race -count=1
  • Hosted CI is configured to run ./test.sh and ./test_e2e.sh, but the fork workflow runs currently require upstream maintainer approval.
  • A Linux Go 1.25 container successfully built the pinned Livepeer FFmpeg libraries and reached ./test.sh; the Docker engine became unresponsive during the cold-cache Go matrix before reporting a result. No repository files were changed by that verification attempt.

Does this pull request close any open issues?

Fixes #3922
Fixes #3885

Checklist

  • I have read the contributing guide
  • I have run the relevant tests
  • I have added a pending changelog entry

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability when accessing and updating external capabilities concurrently.
    • Prevented capacity over-allocation during simultaneous reservations.
    • Added safer handling for missing or exhausted capabilities.
    • Improved authorization token handling during concurrent operations.
    • Downgraded expected cancellation and shutdown messages to debug-level logging, reducing unnecessary error noise.
  • Tests

    • Added coverage for concurrent capability access, atomic reservations, and canceled client connections.

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4bc4ed8f-228b-47f3-a31d-d409b82938db

📥 Commits

Reviewing files that changed from the base of the PR and between d9bc575 and e8ab05c.

📒 Files selected for processing (5)
  • byoc/trickle.go
  • core/ai_orchestrator.go
  • core/external_capabilities_test.go
  • media/rtmp2segment.go
  • server/ai_live_video.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • media/rtmp2segment.go
  • core/ai_orchestrator.go
  • byoc/trickle.go
  • server/ai_live_video.go

📝 Walkthrough

Walkthrough

The changes synchronize external capability access across registries and orchestrators. They also classify expected stream-teardown cancellations as debug logs while preserving higher-severity logs for unrelated failures.

Changes

External capability synchronization

Layer / File(s) Summary
Capability registry accessors and mutation locking
core/external_capabilities.go, core/external_capabilities_test.go
The registry uses read-write locking. It provides synchronized capability lookup and name retrieval. Capability replacement locks mutable fields. Tests cover lookup and concurrent reads.
Synchronized capability consumers
core/ai_orchestrator.go, core/orchestrator.go, byoc/job_orchestrator.go, byoc/stream_orchestrator.go, core/external_capabilities_test.go
Capability URLs, tokens, capacity, and pricing use synchronized accessors and capability locks. Capacity reservation rejects exhausted capabilities. Contention tests verify atomic reservation.

Cancellation logging

Layer / File(s) Summary
Cancellation-aware teardown logging
byoc/trickle.go, server/ai_live_video.go, media/rtmp2segment.go, trickle/trickle_server.go, trickle/trickle_subscriber.go, trickle/trickle_test.go, CHANGELOG_PENDING.md
Expected cancellation during process termination, segmentation shutdown, preconnect, and client disconnect handling logs at debug level. Other failures retain their existing severity. A regression test verifies canceled client disconnects do not emit error-level logs. The changelog records both fixes.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: docs

Suggested reviewers: rickstaa

Sequence Diagram(s)

sequenceDiagram
  participant StreamContext
  participant FFmpeg
  participant RTMPSegmenter
  participant TrickleSubscriber
  participant TrickleServer
  StreamContext->>FFmpeg: Cancel context
  FFmpeg-->>TrickleSubscriber: Process termination error
  TrickleSubscriber->>TrickleSubscriber: Log cancellation at debug level
  StreamContext->>RTMPSegmenter: Cancel context
  RTMPSegmenter->>RTMPSegmenter: Log cancellation at debug level
  StreamContext->>TrickleServer: Cancel client request
  TrickleServer->>TrickleServer: Log disconnect at debug level
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the two primary changes: quieter expected teardown logs and locked capability reads.
Linked Issues check ✅ Passed The changes address cancellation-driven log demotion [#3922] and locked capability access with protected mutable fields [#3885].
Out of Scope Changes check ✅ Passed The code, tests, and changelog entries support the linked issue objectives without unrelated implementation changes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added go Pull requests that update Go code AI Issues and PR related to the AI-video branch. labels Aug 2, 2026
@vavo
vavo marked this pull request as ready for review August 2, 2026 00:56
@vavo
vavo marked this pull request as draft August 2, 2026 00:56

@coderabbitai coderabbitai 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.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@byoc/trickle.go`:
- Around line 407-411: Update the cmd.CombinedOutput logging branches in
byoc/trickle.go lines 407-411 and server/ai_live_video.go lines 453-457 to split
severity by error state: retain DEBUG logging for cancellation, use clog.Errorf
for non-cancellation err != nil failures, and call clog.Infof only when err ==
nil.

In `@core/ai_orchestrator.go`:
- Around line 1152-1175: Make ReserveExternalCapabilityCapacity perform the
capacity check and Load increment under the same cap.Mu write lock, returning a
capacity-exhausted error when Load is already at Capacity; update callers to
rely on this atomic reservation instead of the separate
CheckExternalCapabilityCapacity result, and add a concurrent reservation
regression test proving Load never exceeds Capacity.

In `@media/rtmp2segment.go`:
- Around line 73-77: The FFmpeg error path after cmd.CombinedOutput() still logs
cancellation-driven shutdowns at error level. Update the err != nil branch in
the segmentation flow to check ctx.Err() and use clog.V(common.DEBUG).Infof for
cancelled contexts, while retaining clog.Errorf for active-context failures,
matching the existing retry handling.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 399de4ad-377f-48f3-bf64-907b68b56e96

📥 Commits

Reviewing files that changed from the base of the PR and between bafb22c and d9bc575.

📒 Files selected for processing (13)
  • CHANGELOG_PENDING.md
  • byoc/job_orchestrator.go
  • byoc/stream_orchestrator.go
  • byoc/trickle.go
  • core/ai_orchestrator.go
  • core/external_capabilities.go
  • core/external_capabilities_test.go
  • core/orchestrator.go
  • media/rtmp2segment.go
  • server/ai_live_video.go
  • trickle/trickle_server.go
  • trickle/trickle_subscriber.go
  • trickle/trickle_test.go

Comment thread byoc/trickle.go Outdated
Comment thread core/ai_orchestrator.go
Comment thread media/rtmp2segment.go
@vavo

vavo commented Aug 2, 2026

Copy link
Copy Markdown
Author

Reviewed the remaining CodeRabbit docstring-coverage warning. It reports repository-wide coverage rather than a regression in this patch, and this change does not add a new docstring policy or require a broad comment-only sweep, so I left it out of scope.

@vavo vavo left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ready for review .. I think 👀

Comment thread byoc/trickle.go Outdated
Comment thread core/ai_orchestrator.go
Comment thread media/rtmp2segment.go
@vavo
vavo marked this pull request as ready for review August 2, 2026 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Issues and PR related to the AI-video branch. go Pull requests that update Go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BYOC: spurious ERROR-level logs at every clean /stream/stop Race condition in external capability access (medium)

1 participant