Merge staging: post_script, stream URL fixes, notice buffer, dep bumps - #581
Conversation
Pass site and postScript through the downloadStream signature and invoke runPostScript in a goroutine after a successful file move on the live stream path.
Add a marker-based hook script to the integration test that writes context to a file when fired. Both live stream and VOD phases now verify the post_script hook ran with correct env vars. Also add a manual test hook script for verifying on a live instance.
…hook feat: post-download script hook
Bare "best"/"worst" format selectors broke in newer yt-dlp versions for extractors that split streams into separate video/audio tracks. Map them to compound selectors (bestvideo*+bestaudio/best, etc.) so format selection works across all extractors. Also fix the format fallback path to return the resolved URL instead of erroring with 415 when the initial format selector isn't available.
When yt-dlp merges video+audio formats, the URL lives inside requested_formats rather than at the top level of the info dict. Add _extract_url helper that checks requested_formats and prefers the video stream URL, which FFmpeg can handle as an HLS manifest.
When yt-dlp returns merged formats, return the manifest_url (master HLS playlist) instead of individual chunklist URLs. FFmpeg needs the master playlist to mux both video and audio tracks into the output.
Move URL resolution from the tick loop into downloadStream so each FFmpeg attempt gets a fresh URL with a valid token. Previously the URL was resolved during the tick loop and reused across retries, causing failures when tokens expired before FFmpeg could connect. Rename the urls map to activeUsers since it now only tracks which users have an active download goroutine.
Remove the liveness probe from the tick loop — downloadStream now handles URL resolution with its own rate-limit retry logic. This halves the API calls per channel since we no longer resolve once to check liveness and again to get a fresh token for FFmpeg.
Add a per-site mutex so concurrent download goroutines for the same site resolve URLs one at a time. When one goroutine is resolving or backing off from a rate limit, others wait their turn rather than all hammering the API simultaneously. Different sites don't block each other.
Some CDNs bind manifest URL tokens to the originating session, so FFmpeg gets 403s when trying to use them. Return the video stream chunklist URL instead, which uses a simpler session parameter that FFmpeg can access directly.
Newer yt-dlp versions return separate video and audio format entries for some extractors instead of combined formats. Add audio_url to the gRPC StreamResponse proto and pass both URLs through to FFmpeg as separate inputs so recordings include both video and audio tracks.
…VOD parsing Cover the pure logic paths in streamdl_proto_srv.py: - Quality-to-format selector mapping (best/worst/passthrough) - DownloadError string classification (429/415/450/500) - VOD entry parsing (None filtering, missing fields, type coercion)
… users Only launch downloadStream goroutines for confirmed-live users. The tick loop probes each user sequentially with rate-limit retries, and only marks them active and launches a goroutine when the probe succeeds. This avoids misleading "Starting Download" messages for offline users and reduces unnecessary goroutine churn. downloadStream still resolves a fresh URL for FFmpeg so tokens are valid at connection time.
The liveness probe already has a valid URL with a fresh token. Pass it through to downloadStream for the first FFmpeg attempt instead of resolving a second time (which wastes an API call and risks rate limiting). Only re-resolve on retries where the token may have expired.
When video and audio are separate HLS inputs, FFmpeg needs -map 0:v -map 1:a -c copy to properly mux both streams. Without explicit mapping FFmpeg hangs trying to auto-select streams from two inputs.
The reconnect_streamed and reconnect_at_eof flags cause 403 loops on HLS streams — when a segment fetch reaches EOF, FFmpeg's reconnect logic retries the raw URL with an expired session token instead of letting the HLS demuxer naturally advance to the next segment. Only apply reconnect flags for single-input streams (e.g. Streamlink URLs). HLS split-format streams handle segment transitions natively.
Bumps [wheel](https://github.com/pypa/wheel) from 0.46.3 to 0.47.0. - [Release notes](https://github.com/pypa/wheel/releases) - [Changelog](https://github.com/pypa/wheel/blob/main/docs/news.rst) - [Commits](pypa/wheel@0.46.3...0.47.0) --- updated-dependencies: - dependency-name: wheel dependency-version: 0.47.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
…l-0.47.0 chore 🤖(deps): bump wheel from 0.46.3 to 0.47.0
Bumps [streamlink](https://github.com/streamlink/streamlink) from 8.3.0 to 8.4.0. - [Release notes](https://github.com/streamlink/streamlink/releases) - [Changelog](https://github.com/streamlink/streamlink/blob/master/CHANGELOG.md) - [Commits](streamlink/streamlink@8.3.0...8.4.0) --- updated-dependencies: - dependency-name: streamlink dependency-version: 8.4.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Add an in-memory notice buffer in the Go client that collects user-facing warnings and errors during each tick and flushes them after the wait line, so docker compose logs show actionable messages at the bottom of each cycle. Improve GetStream error propagation from the Python server with clear messages for quality mismatches (414), format failures (415), and offline channels (450). Format fallback warnings are sent via gRPC metadata. Includes Go and Python unit tests plus integration test phases 6a (Twitch) and 6b (Kick). Closes #573 Co-authored-by: Josh J <josh@joshjacobs.net>
Recreate only the client with --no-deps so Phase 6 notice checks still run after a VOD failure, and fix Compose v5 healthcheck quoting.
yt-dlp returns IDs like v2807766672; prepending videos/ produced twitch.tv/videos/v2807766672 which fails resolution.
…4c96 feat: tick-scoped notice buffer flushed after wait message
start_client() now passes --build only on the first call (or when INTEGRATION_FORCE_BUILD=1). Later phase transitions recreate the container without rebuilding, cutting run time and Docker disk use. Fixes #576 Co-authored-by: Josh J <josh@joshjacobs.net>
…build-43d2 fix: build integration client image once per run
…staging/actions/checkout-7 chore 🤖(deps): bump actions/checkout from 6 to 7
Document hook timeout/executable requirements, tick notice output, fix the example config path, and add post_script to config.yml.example. Co-authored-by: Cursor <cursoragent@cursor.com>
docs: fill README gaps for staging features
…amlink-8.4.0 chore 🤖(deps): bump streamlink from 8.3.0 to 8.4.0
…io-1.81.1 chore 🤖(deps): bump grpcio from 1.80.0 to 1.81.1
…lp-2026.6.9 chore 🤖(deps): bump yt-dlp from 2026.3.17 to 2026.6.9
Mirror dependabot #575 on staging; go test ./... passes locally. Co-authored-by: Cursor <cursoragent@cursor.com>
…ing/golang.org/x/net-0.55.0 chore(deps): bump golang.org/x/net from 0.49.0 to 0.55.0
Resolve pyproject.toml/uv.lock conflicts: take staging deps (grpcio, wheel, yt-dlp) plus urllib3 2.7.0 from master. Regenerated lockfile. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds per-site post-download hooks, tick-notice buffering, structured video/audio stream resolution, and associated config, proto, pipeline, test, and documentation updates. It also bumps CI workflow actions, a Docker base image, and dependency constraints. ChangesPost-script, tick notices, and split A/V streaming feature
Estimated code review effort: 4 (Complex) | ~75 minutes CI, Docker, and dependency updates
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant streamdl
participant downloadStream
participant getStream
participant tickNotices
participant runPostScript
streamdl->>downloadStream: start(site, quality, initialURLs, postScript)
downloadStream->>getStream: retry with site-scoped resolution
getStream-->>downloadStream: StreamURLs or error
downloadStream->>tickNotices: Warn/Error on retry or failure
downloadStream->>runPostScript: run hook after successful move
runPostScript-->>downloadStream: completion or error
sequenceDiagram
participant Client
participant StreamServicer
participant getStream
Client->>getStream: request site/user/quality
getStream->>StreamServicer: GetStream RPC with metadata
StreamServicer-->>getStream: StreamResponse(url, audio_url) and warning metadata
getStream-->>Client: StreamURLs{Video, Audio, Warning}
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Upgrade google.golang.org/grpc 1.80.0 → 1.82.0, pulling in github.com/go-jose/go-jose/v4@4.1.4 (was 4.1.3 via grpc). Co-authored-by: Cursor <cursoragent@cursor.com>
fix(deps): bump grpc and go-jose for Snyk CVE-2026-34986
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
streamdl_proto_srv.py (2)
372-386: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing validation that
video_urlis non-empty before returning success.
_extract_urlscan return("", "")(e.g., aninfo_dictlacking bothurlandrequested_formats). Unlike the retry fallback below (line 416,if video_url:else error 415), this primary success path returns{"url": video_url, "audio_url": audio_url}unconditionally, so a "successful"get_streamresult can carry an empty video URL through to the gRPC response, silently breaking downstream FFmpeg/client consumption instead of surfacing as a resolvable error at the source.🐛 Proposed fix
info_dict = ydl.extract_info(ytdlp_url, download=False) video_url, audio_url = _extract_urls(info_dict) - return {"url": video_url, "audio_url": audio_url} + if not video_url: + logger.error("No video URL resolved for %s", r.user) + return { + "error": 500, + "message": f"Unable to resolve stream URL for '{r.user}'", + } + return {"url": video_url, "audio_url": audio_url}🤖 Prompt for 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. In `@streamdl_proto_srv.py` around lines 372 - 386, The primary yt_dlp success path in get_stream returns the result from _extract_urls unconditionally, so add the same non-empty video_url validation used in the retry fallback before constructing the response. After calling _extract_urls(info_dict), check video_url and raise the same resolvable error path if it is empty, rather than returning {"url": video_url, "audio_url": audio_url}; keep the fix localized to get_stream and reference _extract_urls / the yt_dlp.extract_info block so the success response only happens when a valid video URL exists.
396-432: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrap the fallback yt-dlp call in its own error handling. The retry inside
except DownloadErrorcan still raise and bypass the surrounding handlers, so a second yt-dlp failure here will escapeget_stream()and surface as an unhandled gRPC error.🤖 Prompt for 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. In `@streamdl_proto_srv.py` around lines 396 - 432, The fallback yt-dlp retry inside get_stream()’s DownloadError handler is not protected, so a second failure can escape as an unhandled gRPC error. Add a separate try/except around the fallback YoutubeDL(...) / extract_info(...) block, catch DownloadError (and any yt-dlp-specific extraction errors if needed), log the failure with context for r.user and yt_dlp_format, and return the existing structured error response instead of letting it propagate.
🧹 Nitpick comments (5)
tests/manual/test_post_hook.sh (1)
21-28: 📐 Maintainability & Code Quality | 🔵 TrivialMinor: multiple individual redirects flagged by shellcheck (SC2129).
Could consolidate into a single
{ ... } >> "$LOGFILE"block, but this is a manual/non-CI helper script so the benefit is minimal.🤖 Prompt for 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. In `@tests/manual/test_post_hook.sh` around lines 21 - 28, The post-hook logging in the test_post_hook.sh helper uses multiple separate redirects that trigger shellcheck SC2129. Update the logging sequence around the post_script output to use a single grouped block redirected once to LOGFILE, keeping the same echoed fields and order while consolidating the writes.Source: Linters/SAST tools
download_stream.go (1)
148-186: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a shared rate-limit sentinel instead of
err.Error()checks
Both retry paths depend on the exact"rate limited"string fromgrpc_client.go. If that message is wrapped or tweaked later, rate-limited resolves fall into the fatal path instead of retrying. Use a shared sentinel (for exampleerrors.Is(err, ErrRateLimited)) and reuse it in bothdownload_stream.goandstreamdl.go.🤖 Prompt for 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. In `@download_stream.go` around lines 148 - 186, The retry logic in the stream resolution path is matching rate limits by comparing err.Error() to a hardcoded string, which is brittle. Introduce and use a shared sentinel such as ErrRateLimited in the resolution helpers, then update the retry handling in download_stream.go and the corresponding logic in streamdl.go to check errors with a shared predicate like errors.Is instead of string comparisons. Keep the existing getStream and retry flow intact, but route both failure paths through the shared rate-limit check so wrapped or reformatted errors still retry correctly.grpc_client.go (2)
165-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: inconsistent field access style.
msg.Urlis accessed directly whilemsg.GetAudioUrl()uses the generated getter on the same line. Prefermsg.GetUrl()for consistency with idiomatic protobuf usage elsewhere in this function (e.g.,msg.GetError()).Proposed fix
- return StreamURLs{Video: msg.Url, Audio: msg.GetAudioUrl(), Warning: warning}, nil + return StreamURLs{Video: msg.GetUrl(), Audio: msg.GetAudioUrl(), Warning: warning}, nil🤖 Prompt for 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. In `@grpc_client.go` at line 165, The return statement in grpc_client.go mixes direct protobuf field access and generated getters, which is inconsistent with the rest of the function. Update the StreamURLs construction in the relevant function to use the generated getter for the video URL as well, matching the existing msg.GetAudioUrl() and msg.GetError() style for idiomatic protobuf access.
144-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRate-limit detection relies on hardcoded string, matching pre-existing
getVodspattern.
codes.ResourceExhaustedreturns a fixederrors.New("rate limited"), discarding the server's actual detail message (e.g. "Rate limited by site"). This mirrors the existing convention ingetVods(Line 63) so it's consistent, but if calling code does string matching onerr.Error()for backoff logic (per PR objective's per-site serialization), a sentinel error type would be more robust than string comparison longer-term.🤖 Prompt for 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. In `@grpc_client.go` around lines 144 - 145, The `StreamURLs` handling for `codes.ResourceExhausted` currently returns a plain `errors.New("rate limited")`, which makes rate-limit detection depend on string matching and drops the server’s detail message. Update the `grpc_client` error path to use a dedicated sentinel error (or equivalent shared error value) for `ResourceExhausted`, and make the `getVods`-style callers compare against that symbol instead of `err.Error()` so per-site backoff logic is stable.tests/test_format_mapping.py (1)
9-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests validate a duplicated copy of the logic, not production code.
_get_format_map/_resolve_formatre-implementstreamdl_proto_srv.get_stream's quality-to-selector mapping locally instead of importing it. If the production map changes, these tests keep passing against a stale copy and won't catch a regression. Consider extracting the mapping into a small top-level function instreamdl_proto_srv.py(e.g._resolve_yt_dlp_format(quality)) thatget_streamcalls, and importing/testing that function directly here — the same pattern astest_extract_urls.pydoes for_extract_urls.🤖 Prompt for 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. In `@tests/test_format_mapping.py` around lines 9 - 20, The test helpers `_get_format_map` and `_resolve_format` are duplicating the production quality-to-format logic instead of exercising `streamdl_proto_srv.get_stream`. Move that mapping into a small top-level helper in `streamdl_proto_srv.py` (for example, `_resolve_yt_dlp_format(quality)`) and have `get_stream` call it. Then update `tests/test_format_mapping.py` to import and assert against that helper directly, matching the approach used by `test_extract_urls.py` with `_extract_urls`.
🤖 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 `@download_stream.go`:
- Around line 196-228: The split A/V path is skipping more than the problematic
reconnect flags because `buildReconnectArgs()` also includes network-hygiene
options like `-rw_timeout`, `-protocol_whitelist`, and `-user_agent`. Refactor
`buildReconnectArgs()` in `download_stream.go` into separate helpers such as
`buildNetworkHygieneArgs()` and a reconnect-only builder, then always apply the
hygiene args before inserting FFmpeg inputs while skipping only the
reconnect-specific flags when `streamURLs.Audio` is set.
In `@grpc_client_test.go`:
- Around line 76-84: The TestStreamResolveErrorIsUsableAsNotice test has an
empty errors.Is branch, so it does not actually assert anything and triggers
SA9003. Update the test to make the errors.Is compatibility check meaningful by
asserting the expected match on the value returned by streamResolveError, or
remove the branch entirely if this behavior is already covered elsewhere; keep
the err.Error() message assertion only if it still serves a distinct purpose.
In `@grpc_client.go`:
- Around line 168-175: The fallback handling in streamResolveError is too broad
because Contains(" - Unknown error") can collapse unrelated status messages into
the fallback. Tighten this check to only match the exact upstream producer
string you intend to normalize, or add a clear comment/docstring in
streamResolveError explaining the specific gateway/proxy message being handled.
Keep the existing preference for the trimmed gRPC statusMessage, but only route
to fallback for the narrowly defined unknown-error case.
In `@README.md`:
- Around line 183-194: The Tick Notices example in README.md uses a bare fenced
block, which triggers markdownlint MD040. Update the fenced code block in the
Tick Notices section to include an explicit language identifier such as text so
the example remains valid markdown while preserving the existing content.
In `@streamdl.go`:
- Around line 242-249: The shutdown drain in downloadStream is coupled to the
number of active users, but downloadStream may exit without sending on response,
so the current receive loop can hang. Replace the len(activeUsers)-based drain
with a synchronization mechanism that does not depend on response send counts,
such as a dedicated WaitGroup around downloadStream completion, or ensure every
exit path in downloadStream always sends exactly once on response. Keep the fix
localized around downloadStream and the shutdown wait logic that follows
activeUsersMu, vodWg, and postScriptWg.
- Around line 159-162: The initial VOD/live probe path in streamdl.go still
calls getStream directly, so it can overlap with retry resolution and violate
per-site serialization. Update the probe logic in the tick loop to acquire the
same getSiteResolveMu protection used by the retry path before calling getStream
for the first VOD/live checks, and release it after the request completes;
reference the getStream probe code and the existing getSiteResolveMu locking
pattern so both paths use the same per-site rate-limit guard.
---
Outside diff comments:
In `@streamdl_proto_srv.py`:
- Around line 372-386: The primary yt_dlp success path in get_stream returns the
result from _extract_urls unconditionally, so add the same non-empty video_url
validation used in the retry fallback before constructing the response. After
calling _extract_urls(info_dict), check video_url and raise the same resolvable
error path if it is empty, rather than returning {"url": video_url, "audio_url":
audio_url}; keep the fix localized to get_stream and reference _extract_urls /
the yt_dlp.extract_info block so the success response only happens when a valid
video URL exists.
- Around line 396-432: The fallback yt-dlp retry inside get_stream()’s
DownloadError handler is not protected, so a second failure can escape as an
unhandled gRPC error. Add a separate try/except around the fallback
YoutubeDL(...) / extract_info(...) block, catch DownloadError (and any
yt-dlp-specific extraction errors if needed), log the failure with context for
r.user and yt_dlp_format, and return the existing structured error response
instead of letting it propagate.
---
Nitpick comments:
In `@download_stream.go`:
- Around line 148-186: The retry logic in the stream resolution path is matching
rate limits by comparing err.Error() to a hardcoded string, which is brittle.
Introduce and use a shared sentinel such as ErrRateLimited in the resolution
helpers, then update the retry handling in download_stream.go and the
corresponding logic in streamdl.go to check errors with a shared predicate like
errors.Is instead of string comparisons. Keep the existing getStream and retry
flow intact, but route both failure paths through the shared rate-limit check so
wrapped or reformatted errors still retry correctly.
In `@grpc_client.go`:
- Line 165: The return statement in grpc_client.go mixes direct protobuf field
access and generated getters, which is inconsistent with the rest of the
function. Update the StreamURLs construction in the relevant function to use the
generated getter for the video URL as well, matching the existing
msg.GetAudioUrl() and msg.GetError() style for idiomatic protobuf access.
- Around line 144-145: The `StreamURLs` handling for `codes.ResourceExhausted`
currently returns a plain `errors.New("rate limited")`, which makes rate-limit
detection depend on string matching and drops the server’s detail message.
Update the `grpc_client` error path to use a dedicated sentinel error (or
equivalent shared error value) for `ResourceExhausted`, and make the
`getVods`-style callers compare against that symbol instead of `err.Error()` so
per-site backoff logic is stable.
In `@tests/manual/test_post_hook.sh`:
- Around line 21-28: The post-hook logging in the test_post_hook.sh helper uses
multiple separate redirects that trigger shellcheck SC2129. Update the logging
sequence around the post_script output to use a single grouped block redirected
once to LOGFILE, keeping the same echoed fields and order while consolidating
the writes.
In `@tests/test_format_mapping.py`:
- Around line 9-20: The test helpers `_get_format_map` and `_resolve_format` are
duplicating the production quality-to-format logic instead of exercising
`streamdl_proto_srv.get_stream`. Move that mapping into a small top-level helper
in `streamdl_proto_srv.py` (for example, `_resolve_yt_dlp_format(quality)`) and
have `get_stream` call it. Then update `tests/test_format_mapping.py` to import
and assert against that helper directly, matching the approach used by
`test_extract_urls.py` with `_extract_urls`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9305f89-5e05-4e5a-94ec-dfe475ffcf10
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumprotos/stream.pb.gois excluded by!**/*.pb.gostream_pb2.pyis excluded by!stream_pb2.pyuv.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.github/workflows/deploy_master.yml.github/workflows/deploy_staging.yml.github/workflows/test.yml.github/workflows/update-actions.ymlDockerfile.clientREADME.mdconfig.goconfig/config.yml.exampleconfig_reader_test.godownload_stream.gogo.modgrpc_client.gogrpc_client_test.gonotice.gonotice_test.gopost_script.gopost_script_test.goprotos/stream.protoprotos/stream_pb2.pyprotos/stream_pb2_grpc.pypyproject.tomlstreamdl.gostreamdl_proto_srv.pytests/integration/docker-compose.integration.ymltests/integration/run.shtests/manual/test_post_hook.shtests/test_error_classification.pytests/test_extract_urls.pytests/test_format_mapping.pytests/test_get_stream_messages.pytests/test_vod_parsing.pyvod_stream_test.go
| // streamResolveError prefers the gRPC status message when it is user-facing. | ||
| func streamResolveError(statusMessage, fallback string) error { | ||
| msg := strings.TrimSpace(statusMessage) | ||
| if msg == "" || strings.Contains(msg, " - Unknown error") { | ||
| return errors.New(fallback) | ||
| } | ||
| return msg.Url, nil | ||
| return errors.New(msg) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Python grpcio auto-generate a default status details string like "- Unknown error" when context.set_code(UNKNOWN) is called without set_details()?
💡 Result:
No, the Python gRPC library does not auto-generate a default status details string like "- Unknown error" when you call context.set_code(grpc.StatusCode.UNKNOWN) without calling set_details [1][2]. In the Python gRPC implementation, the set_code and set_details methods manage independent fields within the _RPCState object [1][2]. Specifically: - The set_details method stores the provided string (or None if not called) in the internal state [1][2]. - When an RPC completes, the gRPC runtime retrieves the values stored in these fields [1][2]. If set_details was never called, the details field remains None (or is treated as an empty byte string during serialization), rather than being populated with a default error message [1][2]. While the library includes internal logic to map invalid or missing status codes to UNKNOWN, this is distinct from the details string itself [1][3][4]. The responsibility for providing a human-readable error description in the details field lies entirely with the developer [5][6][7]. If you do not explicitly set the details, the client will typically receive an empty string for the status details [1][2].
Citations:
- 1: https://chromium.googlesource.com/external/github.com/grpc/grpc/+/HEAD/src/python/grpcio/grpc/_server.py
- 2: https://github.com/grpc/grpc/blob/cea03edc0b430e5c4734164a8336ae4b3b77fd16/src/python/grpcio/grpc/_server.py
- 3: Cannot return a non-standard GRPC status in Python grpc/grpc#13321
- 4: https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/_common.py
- 5: https://grpc.github.io/grpc/python/grpc.html?highlight=context
- 6: https://docs.ros.org/en/api/grpc/html/classgrpc_1_1ServicerContext.html
- 7: https://grpc.github.io/grpc/python/grpc%5Fasyncio.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant file and any local uses of the fallback string.
git ls-files | rg '(^|/)grpc_client\.go$|(^|/)grpc.*\.go$'
rg -n '" - Unknown error"|Unknown error|streamResolveError|set_details|set_code' -- grpc_client.go .Repository: dangeroustech/StreamDL
Length of output: 5629
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' grpc_client.go
printf '\n--- TESTS ---\n'
sed -n '1,140p' grpc_client_test.goRepository: dangeroustech/StreamDL
Length of output: 8134
🌐 Web query:
grpc-go status error string format "500 - Unknown error" UNKNOWN code message
💡 Result:
In grpc-go, the error string format produced by the status package is explicitly defined as: rpc error: code = desc = This format is generated by the Error method on the internal statusError type [1]: func (se *statusError) Error string { p:= (*spb.Status)(se) return fmt.Sprintf("rpc error: code = %s desc = %s", codes.Code(p.GetCode), p.GetMessage) } Regarding the string "500 - Unknown error" you mentioned: 1. Format Mismatch: The standard grpc-go error string does not include HTTP status codes (like "500") or a dash (" - ") in this format. If you are seeing "500 - Unknown error", it is likely being generated by a layer outside of the core grpc-go library, such as a gRPC-to-HTTP gateway (e.g., grpc-gateway), an API proxy, or custom error handling logic in your application that manually constructs this string. 2. UNKNOWN Code: The code "UNKNOWN" (gRPC code 2) [2][3] is the canonical gRPC status code for errors that do not have enough information or originate from an error space that the client does not recognize [2][3]. While "UNKNOWN" is semantically mapped to the HTTP 500 Internal Server Error status in many gRPC-to-HTTP mappings [4][5], the core gRPC framework itself uses the "rpc error: code =..." format described above. If you encounter "500 - Unknown error", you should inspect your infrastructure layers (like ingress controllers, proxies, or gateway services) or search your codebase for where this specific string is being formatted.
Citations:
- 1: https://github.com/grpc/grpc-go/blob/f4b523765c542aa30ca9cdb657419b2ed4c89872/status/status.go
- 2: https://github.com/grpc/grpc-go/blob/master/codes/codes.go
- 3: https://grpc.github.io/grpc/core/md_doc_statuscodes
- 4: https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto
- 5: https://cloud.google.com/service-infrastructure/docs/service-control/reference/rpc/google.rpc
Narrow the Unknown error fallback match
grpc-go and Python gRPC don’t emit "... - Unknown error" by default, so this looks like a compatibility shim for an upstream gateway/proxy. Match the exact producer string (or document it) instead of Contains(" - Unknown error"), so unrelated messages don’t get collapsed to the fallback.
🤖 Prompt for 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.
In `@grpc_client.go` around lines 168 - 175, The fallback handling in
streamResolveError is too broad because Contains(" - Unknown error") can
collapse unrelated status messages into the fallback. Tighten this check to only
match the exact upstream producer string you intend to normalize, or add a clear
comment/docstring in streamResolveError explaining the specific gateway/proxy
message being handled. Keep the existing preference for the trimmed gRPC
statusMessage, but only route to fallback for the narrowly defined unknown-error
case.
| activeUsersMu.RLock() | ||
| urlsLen := len(activeUsers) | ||
| activeUsersMu.RUnlock() | ||
| for i := 0; i < urlsLen; i++ { | ||
| <-response | ||
| } | ||
| vodWg.Wait() | ||
| postScriptWg.Wait() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm every exit path of downloadStream and whether each sends to response
rg -nP -B2 -A2 'response\s*<-' download_stream.goRepository: dangeroustech/StreamDL
Length of output: 426
🏁 Script executed:
#!/bin/bash
sed -n '270,340p' download_stream.goRepository: dangeroustech/StreamDL
Length of output: 2751
🏁 Script executed:
#!/bin/bash
rg -n -A6 -B6 'activeUsersMu|response|vodWg|postScriptWg|sigint' streamdl.go download_stream.goRepository: dangeroustech/StreamDL
Length of output: 13824
🏁 Script executed:
#!/bin/bash
sed -n '335,390p' download_stream.goRepository: dangeroustech/StreamDL
Length of output: 1915
Make the shutdown drain independent of response send counts. downloadStream can return on natural finish or after retries are exhausted without sending to response, so for i := 0; i < urlsLen; i++ { <-response } can block during shutdown if activeUsers is still counting a goroutine that already chose one of those paths. A WaitGroup or a guaranteed send on every exit path would avoid the hang.
🤖 Prompt for 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.
In `@streamdl.go` around lines 242 - 249, The shutdown drain in downloadStream is
coupled to the number of active users, but downloadStream may exit without
sending on response, so the current receive loop can hang. Replace the
len(activeUsers)-based drain with a synchronization mechanism that does not
depend on response send counts, such as a dedicated WaitGroup around
downloadStream completion, or ensure every exit path in downloadStream always
sends exactly once on response. Keep the fix localized around downloadStream and
the shutdown wait logic that follows activeUsersMu, vodWg, and postScriptWg.
- Split FFmpeg network hygiene from reconnect flags for split A/V streams - Serialize initial VOD/live getStream probes with getSiteResolveMu - Remove no-op errors.Is branch in grpc_client_test (SA9003) - Add text language tag to Tick Notices README example (MD040) Co-authored-by: Cursor <cursoragent@cursor.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Reject empty video_url on the primary yt-dlp path and wrap the format fallback extract_info call so second failures return structured errors. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
download_stream.go (1)
215-228: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSplit A/V audio input needs the same per-input options.
buildNetworkHygieneArgs()is only placed before the first-i, so the later audio input opens without-rw_timeout,-protocol_whitelist, or-user_agent. FFmpeg applies those flags to the next input only, so split streams that need them can fail to open or stall. Prepend the hygiene args before the audio-itoo.🤖 Prompt for 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. In `@download_stream.go` around lines 215 - 228, The split audio input in the FFmpeg argument assembly is missing the same per-input hygiene options that are applied for the first stream, so the audio URL may open without the needed network flags. Update the logic in the download stream argument construction around cmd.Args and streamURLs.Audio so the output of buildNetworkHygieneArgs() is inserted before the audio -i as well, ensuring -rw_timeout, -protocol_whitelist, and -user_agent apply to that input too.
🤖 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 `@streamdl_proto_srv.py`:
- Around line 410-452: The fallback yt-dlp path in get_stream only catches
DownloadError and ExtractorError, so unexpected exceptions from the
ydl_temp.extract_info fallback call can escape and crash the RPC. Add a generic
except Exception guard around the fallback block in streamdl_proto_srv.py,
alongside the existing fallback_err handlers, and return a 500-style error
payload with a clear message plus the exception details so all failures in the
fallback branch are handled consistently.
---
Outside diff comments:
In `@download_stream.go`:
- Around line 215-228: The split audio input in the FFmpeg argument assembly is
missing the same per-input hygiene options that are applied for the first
stream, so the audio URL may open without the needed network flags. Update the
logic in the download stream argument construction around cmd.Args and
streamURLs.Audio so the output of buildNetworkHygieneArgs() is inserted before
the audio -i as well, ensuring -rw_timeout, -protocol_whitelist, and -user_agent
apply to that input too.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b2e45ce-71ee-4888-b740-e4d2401318fa
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
README.mddownload_stream.gogo.modgrpc_client_test.gostreamdl.gostreamdl_proto_srv.pytests/test_get_stream_messages.py
💤 Files with no reviewable changes (1)
- grpc_client_test.go
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- streamdl.go
Prepend network hygiene args before the split A/V audio -i input, and catch unexpected exceptions in the yt-dlp format fallback path. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
Merges
stagingintomasterwith dependency conflicts pre-resolved.Features & fixes
post_script) for live and VOD downloadsDocs
Dependencies (conflicts resolved)
Release note: Pushing to
mastertriggers conventional-changelog + Docker release (expected ~v3.8.0).Test plan
go test ./...passes locallyuv run pytest tests/passes locallypyproject.toml/uv.lockconflicts resolved and lockfile regeneratedMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
post_scriptandSTREAMDL_POST_SCRIPT_TIMEOUT.Tests / Chores