Infrastructure, configuration, and core improvements

[biodrone]

Changes proposed

CI / Workflows

• Pin snyk/actions/docker from @master to @v1 in both deploy workflows
• Lower Snyk severity threshold from critical to high in staging
• Remove stale Snyk exception from action version validator
• Quote $GITHUB_STEP_SUMMARY and group writes per shellcheck guidance
• Fix validator self-match on @master|@main grep pattern

Concurrency / Safety

• Add sync.RWMutex to protect shared urls map from concurrent read/write panics
• Guard all 7 access sites (3 reads, 3 writes, 1 delete) with appropriate locks

Security

• Sanitize raw FFmpeg command log that was leaking redacted stream URLs
• Stop logging full signed stream URLs in both Go client and Python server
• Bind health endpoint to 127.0.0.1 instead of 0.0.0.0

Resilience

• Move control channel listener outside FFmpeg retry loop to prevent goroutine leaks
• Warn instead of log.Fatalf on config reload failures, keep last known good config
• Return error instead of log.Fatalf on gRPC dial failure
• Add default values for gRPC addr/port env vars (server:50051)
• Validate gRPC port binding before declaring startup success

Docker / Entrypoints

• Fix Alpine apk add version pin syntax (== to =)
• Add chown for bind-mounted dirs before dropping privileges
• Use getent checks before user/group creation in both entrypoints
• Use exec in server entrypoint to hand PID 1 to the Python process

Code Quality

• Use errors.As for proper syscall.Errno unwrapping in cross-device detection
• Prevent double-close of temp file in cross-device move path
• Rename log_message parameter to avoid shadowing Python builtin

Documentation

• Fix README: clarify runtime user switching vs build-time
• Fix FFmpeg resilience table markdown and correct default values

Testing

• Add end-to-end integration test that discovers a live Twitch stream, downloads ~15 seconds of video via the full server+client pipeline, and validates the output mp4 with ffprobe

Summary by CodeRabbit

• New Features

  • Active HTTP health checks, integration Docker Compose and end-to-end test script; workflow to validate pinned GitHub Actions

• New Features / FFmpeg

  • FFmpeg: retry with configurable attempts/backoff, optional stream-copy and extra input/output options

• Documentation

  • Expanded Docker security guidance and PUID/PGID usage; documented FFmpeg resilience settings

• Bug Fixes

  • Safer config reloads (non-fatal), improved concurrent URL/cache handling, robust cross-device file moves, tighter gRPC error/timeouts/logging

• Chores

  • Dependabot grouping/labels updated, GitHub Actions SARIF and scan behavior adjusted, CI/tooling version pins and Docker base image updates

[coderabbitai]

Warning

Rate limit exceeded

@biodrone has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 21 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 21 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ea4038c1-f93f-4e32-8d28-908c95a3693a

📥 Commits

Reviewing files that changed from the base of the PR and between 35ab99a and 1bd5ae1.

📒 Files selected for processing (4)

• .github/workflows/deploy_master.yml
• .github/workflows/deploy_staging.yml
• .github/workflows/test.yml
• go.mod

📝 Walkthrough

Walkthrough

Updated CI/dependabot/workflows; added actions validator; bumped toolchain/linter pins; enabled Docker healthchecks and embedded HTTP health endpoints; switched entrypoints to numeric PUID:PGID and exec Python; added FFmpeg retry/backoff, URL-cache mutexing, cross-device-safe moveFile (+tests), and gRPC/client/server logging tweaks.

Changes

Cohort / File(s)	Summary
CI / Dependabot / Workflows /.github/dependabot.yml, /.github/workflows/update-actions.yml, /.github/workflows/deploy_master.yml, /.github/workflows/deploy_staging.yml	Dependabot group edits (reviewers→assignees, labels, commit-message prefix); added github-actions update group; new workflow validates unpinned uses: refs; Snyk action pinned to @v1, adds --app-vulns --sarif, staging severity→high, SARIF category labels.
Toolchain / Linters / Modules /.trunk/trunk.yaml, go.mod	Bumped Trunk CLI/plugin/runtime and Node runtime; added/updated linter/tool pins (added golangci-lint2); updated Go/toolchain and bumped several module versions (grpc, protobuf, golang.org/x/* and indirect deps).
Dockerfiles & Entrypoints Dockerfile.client, Dockerfile.server, streamdl_server_entrypoint.sh, entrypoint_client.sh, entrypoint_server.sh	Updated base images and pinned packages; enabled active HEALTHCHECK probing /health; entrypoints now create numeric ${PUID}:${PGID} when missing, attempt best-effort chown, drop privileges by numeric IDs, and server entrypoint execs Python directly (removed uv wrapper).
Docs & Gitignore README.md, .gitignore, tests/integration/.gitignore	Added Security Best Practices, PUID/PGID and FFmpeg resilience docs; added .dccache and docs/ to .gitignore; integration .gitignore ignores output/ and config/.
Config Reader & Tests config_reader.go, config_reader_test.go	Added parseConfig(data []byte) ([]Config,error); introduced package-level fatalf var to allow test overrides; check now calls fatalf; added tests for valid read, missing-file fatal path (stubbed), and malformed YAML.
Download stream resilience download_stream.go	Added dir-exists check in createDirWithUmask; deferred live-URL removal; FFmpeg retry loop with configurable max retries and exponential backoff; rebuilds cmd per attempt, injects reconnect flags, optional stream-copy, extra input/output opts, ensures -y, sanitizes/logs FFmpeg tails, and exits early on dir creation errors.
File move & tests move_file.go, move_file_test.go	Added stub-able renameFunc, isCrossDeviceLink() detection, fast-path rename, cross-device fallback (copy→fsync→rename), improved resource/error handling, and tests covering same-fs, simulated EXDEV, and failure/cleanup scenarios.
Concurrency & Orchestration streamdl.go	Switched to parseConfig(readConfig(...)) with conditional assignment to avoid fatal on parse failure; added sync.RWMutex to guard urls map; added debug/trace logs and captured urls length under lock for shutdown logic.
gRPC client grpc_client.go	Replaced blocking Dial with non-blocking client using insecure creds, added defaults for addr/port, increased RPC deadline (10s→30s), added WaitForReady(true), improved connection and error logging, and logged connection-close errors.
Python server & health streamdl_proto_srv.py, streamdl_server_entrypoint.sh	Logging driven by LOG_LEVEL; added embedded HTTP health server on 127.0.0.1:8080; added debug logs in GetStream and Streamlink/yt-dlp paths; adjusted error mapping; entrypoint now execs Python directly.
Integration tests / Compose tests/integration/docker-compose.integration.yml, tests/integration/run.sh	Added Docker Compose integration with server+client and healthchecks; new integration script discovers a live Twitch stream, runs end-to-end download, waits for completed MP4, and validates via ffprobe (local or containerized).

Sequence Diagram(s)

sequenceDiagram
    participant Worker as StreamDL Worker
    participant FFmpeg as FFmpeg
    participant FS as Filesystem
    participant Move as moveFile
    rect rgba(200,200,255,0.5)
    Worker->>Worker: build cmd.Args (inject reconnect flags, extra opts, ensure -y)
    end
    Worker->>FFmpeg: spawn FFmpeg (attempt N)
    FFmpeg-->>Worker: exit code + logs
    alt success
        FFmpeg->>FS: write temp output file
        Worker->>Move: renameFunc(temp -> dest)
        Move-->>FS: rename or copy+sync+rename (cross-device fallback)
        Worker->>Worker: remove live URL, mark done
    else failure & retries remain
        Worker->>Worker: log sanitized tail, backoff, rebuild cmd, retry
    else failure & max attempts reached
        Worker->>Worker: log error, cleanup, remove live URL
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐇 I hopped through lines with tiny feet,

Retries and health checks made things neat.
Moves across devices now behave,
Mutexes guard each streaming cave.
CI pins and logs — a tidy save!

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Infrastructure, configuration, and core improvements' is vague and broad, using generic terms that don't convey the specific primary change; it reads like a catch-all category rather than a focused summary.	Consider a more specific title highlighting the main change, such as 'Add concurrency safety for shared URLs map and improve config resilience' or 'Improve security, resilience, and add integration tests across CI/Docker/code'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description is comprehensive and well-structured, covering CI/workflows, concurrency, security, resilience, Docker, code quality, documentation, and testing with specific implementation details.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch staging

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 18

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/deploy_staging.yml:
- Around line 55-60: The PR scan currently restricts findings by using the
--severity-threshold=critical flag in the args block (the multi-line args value
containing --exclude-base-image-vulns, --policy-path=.snyk, --app-vulns,
--sarif); remove or relax that flag (e.g., set to high or remove it entirely) so
high and medium vulnerabilities are also reported during staging scans, and
ensure the args string still contains the other flags
(--exclude-base-image-vulns, --policy-path=.snyk, --app-vulns, --sarif).

In @.github/workflows/update-actions.yml:
- Around line 26-27: The workflow validation currently skips snyk/actions/docker
via the grep exclusion, allowing unpinned `@master/`@main usage; update the
condition that filters out "snyk/actions/docker" so it is removed (or replace
the exemption by enforcing a version tag), and ensure any uses: references to
"snyk/actions/docker" are pinned to a released tag such as "@v1" (i.e., change
the grep -v "snyk/actions/docker" behavior and update the workflow usages to use
"snyk/actions/docker@v1" rather than an unversioned branch).
- Around line 35-40: The step that appends GitHub Actions info to
$GITHUB_STEP_SUMMARY should quote the variable and group the multiple echo/grep
writes into a single redirected block to avoid repeated unquoted redirects;
update the block that currently echoes "## GitHub Actions in use:", the fenced
code markers, and the grep line so they are executed inside a single grouped
command (referencing the uses grep -h "uses:" .github/workflows/*.yml and the
echo lines) and redirect the whole group once to "$GITHUB_STEP_SUMMARY".

In `@Dockerfile.client`:
- Around line 13-14: The RUN line installing packages uses incorrect version-pin
syntax (`==`) which breaks apk; update the RUN apk add invocation in
Dockerfile.client to use single equals for pins (e.g., change su-exec==0.2-r3 to
su-exec=0.2-r3) and ensure curl is similarly pinned with single `=`
(curl=8.14.1-r2); keep the rest of the flags (--no-cache) unchanged.

In `@download_stream.go`:
- Around line 36-40: The early return in createDirWithUmask skips permission
normalization for existing directories; remove the immediate return when
os.Stat(path) succeeds so that if info.IsDir() is true you still call
os.Chmod(path, desiredMode) to enforce the umask-adjusted permissions, while
preserving the existing handling for non-directories or Stat errors (i.e.,
create the directory on error or return when not a dir). Update the logic around
the os.Stat check in createDirWithUmask to always apply permission normalization
for existing directories before returning.
- Around line 66-70: The goroutine in downloadStream performs delete(urls, user)
without synchronization which races with other writers/readers (the ticker loop
and writers in streamdl.go); introduce a single sync.Mutex (e.g., urlsMu) or
sync.RWMutex guarding the shared map and use it wherever urls is accessed: lock
for writing in downloadStream before the delete, and update the write/iteration
sites in streamdl.go (the ticker loop and any writers) to acquire the same mutex
(use RLock for iteration if RWMutex) to prevent concurrent map read/write
panics. Ensure the mutex variable and its use are colocated with the urls
declaration so all callers use the same lock.
- Around line 188-200: The goroutine that listens on control and forwards to
sigint is created inside the retry loop, leaking a listener per retry; move that
goroutine out of the retry loop so only a single listener is created
(referencing the control listener block and sigint channel in downloadStream),
or change its send to sigint to be non-blocking (use select { case sigint <-
true: default: }) so stray listeners won’t block when downloadStream has
returned; ensure the single listener is started once (before the retry loop) and
cleans up when control is closed or downloadStream exits.
- Around line 176-180: The debug log at the FFmpeg invocation re-exposes
sensitive data by printing cmd.Args verbatim; replace the raw log that prints
cmd.Path and strings.Join(cmd.Args, " ") with a call that uses the
already-sanitized output from sanitizeArgs (or otherwise sanitize cmd.Args
before logging) so only redacted arguments are logged; update the log statement
referencing cmd.Path/cmd.Args in download_stream.go (the block around the
existing log.Debugf calls and sanitizeArgs usage) to log the sanitized args and
avoid printing cmd.Args/raw command strings.

In `@entrypoint_client.sh`:
- Around line 11-13: Before dropping privileges in entrypoint_client.sh, ensure
/app/dl and /app/out are owned by the target user: after creating the
directories with mkdir -p, run chown -R "${PUID}:${PGID}" /app/dl /app/out
(suppressing non-fatal errors) so bind-mounted root-owned dirs are reclaimed for
the unprivileged process; keep this chown step immediately before the exec
su-exec "${PUID}":"${PGID}" /app/streamdl_client_entrypoint.sh "$@" call to
avoid write failures referenced by download_stream.go.

In `@entrypoint_server.sh`:
- Around line 7-9: The current groupadd/useradd fallback unconditionally treats
any failure as "Group/User exists" which masks invalid IDs and collisions;
change the logic to explicitly check for the existing group/user and for ID
collisions before assuming success: after attempting groupadd for group name
streamdl with PGID, if it fails, use getent group streamdl and getent group
"${PGID}" to decide whether the group truly exists with the expected ID or
whether there is an ID collision (exit non-zero on collision/other errors); do
the same for useradd and the user streamdl with PUID using getent passwd
streamdl and getent passwd "${PUID}" (or check UID/GID fields) and only treat
the operation as OK when the existing entry matches the requested IDs, otherwise
fail loudly. Ensure the checks reference the groupadd/useradd commands and the
streamdl user/group plus PUID/PGID variables so the script stops on real errors
instead of masking them.

In `@grpc_client.go`:
- Around line 18-21: The gRPC dial uses raw os.Getenv values so an unset
STREAMDL_GRPC_ADDR or STREAMDL_GRPC_PORT yields an invalid target like ":";
replace the direct getenv usage by applying the documented defaults (use
"server" for addr and "50051" for port) when the env vars are empty (check addr
and port after os.Getenv or use os.LookupEnv), then use the resolved addr and
port in the log.Debugf and in the grpc.NewClient/ grpc.Dial call so the final
dial target is "server:50051" when envs are not provided.

In `@move_file.go`:
- Around line 68-88: The code closes tempFile twice: once explicitly before
rename and again in the deferred cleanup, causing noisy "already closed" errors
on success; update the cleanup to close only if not already closed by
introducing a flag (e.g., closed bool) in the defer closure that checks !closed
before calling tempFile.Close(), and set closed = true after the explicit
tempFile.Close() (the one after tempFile.Sync()) so the deferred func
(referencing tempFile and tempPath) becomes a best-effort remover/closer only
when needed.

In `@README.md`:
- Around line 48-53: The README currently claims a build-time non-root user but
the containers actually create and switch the streamdl user at runtime; update
the "Default User" and related bullets to state that the image performs runtime
user creation/switching (entrypoint_client.sh and entrypoint_server.sh
create/switch the streamdl user using PUID/PGID at container start) rather than
creating the user at build-time or setting USER. Also clarify that su-exec
(client) and gosu (server) are used for runtime user switching and that
directory ownership/permissions may be ensured at startup rather than baked into
the image.
- Around line 158-167: The README table rows are malformed (they start with
"||") and the documented defaults for FFMPEG_MAX_RETRIES and
FFMPEG_RETRY_BASE_DELAY_SECONDS do not match the code; update the Markdown table
to use single-pipe columns (e.g., "| Variable | Description | Default |" and
each row with leading "|" not "||") and change the Default values for
FFMPEG_MAX_RETRIES to 3 and FFMPEG_RETRY_BASE_DELAY_SECONDS to 2 so they match
the defaults used in download_stream.go (symbols: FFMPEG_MAX_RETRIES,
FFMPEG_RETRY_BASE_DELAY_SECONDS); keep other FFmpeg env var defaults unchanged.

In `@streamdl_proto_srv.py`:
- Around line 154-158: The health HTTP server is bound to all interfaces
("0.0.0.0") but Docker health checks use localhost; change the bind address to
loopback so the HealthHandler/health_server listens only on "127.0.0.1" (keep
health_port unchanged) and start the server thread (health_thread) the same way;
update the HTTPServer(("0.0.0.0", health_port), HealthHandler) instantiation to
use ("127.0.0.1", health_port) to restrict exposure.

In `@streamdl_server_entrypoint.sh`:
- Line 3: Replace the current one-line launcher that sets a hard-coded
PYTHONPATH and runs "python streamdl_proto_srv.py" so the Python process becomes
PID 1 and we don't embed a fixed site-packages path: use exec to replace the
shell (so the Python process receives container signals) and run the
module/interpreter from the virtualenv when available (e.g. exec
"$VIRTUAL_ENV/bin/python" -m streamdl_proto_srv) or fall back to exec python -m
streamdl_proto_srv; remove the hard-coded PYTHONPATH assignment and ensure you
reference "$VIRTUAL_ENV" or rely on the venv interpreter to provide correct
site-packages.

In `@streamdl.go`:
- Line 91: The current log.Debugf call emits the full signed playback URL (url)
which leaks tokens; modify the log to avoid logging the full URL by removing the
query string or omitting it entirely—e.g., log only streamer.User and a
non-sensitive identifier (host/path) instead of url. Locate the log.Debugf usage
(the "Discovered live stream: user=%s url=%s" call that references streamer.User
and url) and replace it so it either logs streamer.User with a sanitized URL
(strip RawQuery) or logs no URL at all.
- Around line 72-78: The current tick loop overwrites config or calls log.Fatalf
on parse errors; change it to preserve the last known good config by first
reading raw := readConfig(*confLoc) and attempting parsed, confErr :=
parseConfig(raw) but only assign config = parsed if confErr == nil; on confErr,
log the error (e.g., log.Printf or log.Error) and continue without calling
log.Fatalf so the service retains the prior working value; ensure
parseConfig/readConfig usage and the confLoc/config variables remain unchanged
elsewhere.

🪄 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

Run ID: 4a03b42f-6160-4c08-bd6a-1d7e721ed9af

📥 Commits

Reviewing files that changed from the base of the PR and between 112a5e5 and d6ef9e5.

⛔ Files ignored due to path filters (2)

• go.sum is excluded by !**/*.sum
• uv.lock is excluded by !**/*.lock

📒 Files selected for processing (21)

• .github/dependabot.yml
• .github/workflows/deploy_master.yml
• .github/workflows/deploy_staging.yml
• .github/workflows/update-actions.yml
• .gitignore
• .trunk/trunk.yaml
• Dockerfile.client
• Dockerfile.server
• README.md
• config_reader.go
• config_reader_test.go
• download_stream.go
• entrypoint_client.sh
• entrypoint_server.sh
• go.mod
• grpc_client.go
• move_file.go
• move_file_test.go
• streamdl.go
• streamdl_proto_srv.py
• streamdl_server_entrypoint.sh

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

grpc_client.go (1)

40-45: WaitForReady(true) + 30s timeout can stall the main poll loop.

Given the sequential loop in streamdl.go (Lines 72-128), each unavailable server call can block up to 30s per streamer (plus batch sleep/retries). Consider making this behavior configurable (or disabling WaitForReady by default) to avoid long scan latency under backend outages.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@grpc_client.go` around lines 40 - 45, The call to c.GetStream currently uses
grpc.WaitForReady(true) with a fixed 30s context timeout (timeout := time.Second
* 30 / ctx, cancel := context.WithTimeout(...)) which can stall the main polling
loop; change GetStream invocation to make WaitForReady and the timeout
configurable (e.g., add a grpcWaitForReady bool and dialTimeout/streamTimeout
value read from config/env) or default to not using grpc.WaitForReady (pass no
option) and use a shorter, configurable timeout; update the call site where
GetStream is invoked (c.GetStream) to use the new config values and ensure
context.WithTimeout uses the configured duration and grpc.WaitForReady is only
applied when enabled.

Dockerfile.client (1)

32-33: Privilege drop is implemented correctly via su-exec, but consider defense-in-depth improvements.

The entrypoint script properly executes the application as a non-root user (PUID:PGID, default 1000:1000) using su-exec with the exec keyword, which replaces the shell process and fails closed if privilege drop fails. However, for improved clarity and robustness:

• Add an explicit USER directive to the Dockerfile to declare privilege boundaries at the image level rather than relying solely on entrypoint logic.
• Consider adding set -e or set -euo pipefail to the entrypoint script for faster failure on unexpected errors.
• The silent logging of chown failures (2>/dev/null) should be reviewed to ensure permission issues don't silently degrade functionality.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Dockerfile.client` around lines 32 - 33, Add an explicit USER directive to
the Dockerfile to declare the non-root user boundary (complementing the
ENTRYPOINT ["/bin/sh", "/app/entrypoint_client.sh"]) and update the
entrypoint_client.sh script to enable strict failure modes (add set -e or set
-euo pipefail at top) and remove or surface the silent chown failure redirection
(remove 2>/dev/null or log the error) so permission problems are not silently
ignored; ensure the entrypoint still uses su-exec to drop privileges and the
exec invocation remains intact.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@grpc_client.go`:
- Around line 27-30: In getStream, don't call log.Fatalf on a dial error (the
grpc.NewClient / err check) because it kills the process; instead propagate the
error to the caller by returning it (update getStream's signature/return values
as needed) and remove the log.Fatalf call. Update callers (e.g., streamdl.go) to
handle the returned error and continue polling other streamers; ensure any
connection cleanup still happens when returning an error.

In `@move_file.go`:
- Around line 110-123: The isCrossDeviceLink function incorrectly type-asserts
the top-level error to syscall.Errno and relies on string matching; instead
import "errors" and use errors.As to unwrap the error chain (or unwrap to
*os.LinkError then to syscall.Errno) so you detect syscall.EXDEV correctly for
os.Rename failures; update isCrossDeviceLink to call errors.As on the error to
extract a syscall.Errno (or check a wrapped *os.LinkError.Err) and compare
against syscall.EXDEV, keeping a string fallback only as a last resort.

---

Nitpick comments:
In `@Dockerfile.client`:
- Around line 32-33: Add an explicit USER directive to the Dockerfile to declare
the non-root user boundary (complementing the ENTRYPOINT ["/bin/sh",
"/app/entrypoint_client.sh"]) and update the entrypoint_client.sh script to
enable strict failure modes (add set -e or set -euo pipefail at top) and remove
or surface the silent chown failure redirection (remove 2>/dev/null or log the
error) so permission problems are not silently ignored; ensure the entrypoint
still uses su-exec to drop privileges and the exec invocation remains intact.

In `@grpc_client.go`:
- Around line 40-45: The call to c.GetStream currently uses
grpc.WaitForReady(true) with a fixed 30s context timeout (timeout := time.Second
* 30 / ctx, cancel := context.WithTimeout(...)) which can stall the main polling
loop; change GetStream invocation to make WaitForReady and the timeout
configurable (e.g., add a grpcWaitForReady bool and dialTimeout/streamTimeout
value read from config/env) or default to not using grpc.WaitForReady (pass no
option) and use a shorter, configurable timeout; update the call site where
GetStream is invoked (c.GetStream) to use the new config values and ensure
context.WithTimeout uses the configured duration and grpc.WaitForReady is only
applied when enabled.

🪄 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

Run ID: 99273301-e06a-4188-af16-2d9e7818c28b

📥 Commits

Reviewing files that changed from the base of the PR and between d6ef9e5 and d96852d.

📒 Files selected for processing (10)

• .github/workflows/deploy_master.yml
• .github/workflows/deploy_staging.yml
• .github/workflows/update-actions.yml
• Dockerfile.client
• download_stream.go
• entrypoint_client.sh
• entrypoint_server.sh
• grpc_client.go
• move_file.go
• streamdl.go

🚧 Files skipped from review as they are similar to previous changes (7)

• .github/workflows/deploy_master.yml
• .github/workflows/deploy_staging.yml
• streamdl.go
• .github/workflows/update-actions.yml
• entrypoint_client.sh
• entrypoint_server.sh
• download_stream.go

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

streamdl_proto_srv.py (1)

169-174: Use finally for deterministic dual-server shutdown.

Current cleanup is only in the KeyboardInterrupt branch. A finally block is safer for consistent health/gRPC shutdown paths.

Proposed refactor

-    try:
-        server.wait_for_termination()
-    except KeyboardInterrupt:
-        logger.info("Shutting down servers...")
-        health_server.shutdown()
-        server.stop(0)
+    try:
+        server.wait_for_termination()
+    except KeyboardInterrupt:
+        logger.info("Shutting down servers...")
+    finally:
+        health_server.shutdown()
+        health_server.server_close()
+        server.stop(0)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@streamdl_proto_srv.py` around lines 169 - 174, The try/except currently only
shuts down health_server and server on KeyboardInterrupt; move the cleanup into
a finally block so health_server.shutdown() and server.stop(0) are executed
deterministically whether wait_for_termination() exits normally or via
exception. Update the block around server.wait_for_termination() to catch
KeyboardInterrupt if you need to log, but ensure both health_server.shutdown()
and server.stop(0) are invoked from a finally section (referencing
server.wait_for_termination(), health_server.shutdown(), and server.stop(0)).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@streamdl_proto_srv.py`:
- Around line 101-105: The debug log currently prints the full resolved stream
URL (logger.debug("GetStream success user=%s url=%s", request.user,
res["url"])), which may expose signed tokens; change the logging to avoid
emitting res["url"] verbatim — either remove the URL from the message or log a
redacted form (for example only the scheme+host+path or a constant like
"<redacted-url>") by parsing res["url"] and stripping the query/fragment before
calling logger.debug; update the GetStream logging call so it references
request.user and the safe/redacted value instead of res["url"].
- Around line 77-79: The log_message method currently uses a parameter named
format which shadows the built-in format() and triggers lint A002; rename the
parameter to fmt in the log_message method signature and update any references
inside the method body (e.g., replace format with fmt) so the method still
suppresses logs but no longer shadows the builtin.
- Around line 165-167: The code calls server.add_insecure_port(...) and then
server.start() and logger.info(...) without validating the env var or
add_insecure_port() result; update the startup sequence to (1) read and validate
STREAMDL_GRPC_PORT from environment (ensure it's present and a valid integer),
(2) call server.add_insecure_port(f"[::]:{port}") and check its return value
(bind_result) is non-zero, (3) if bind_result == 0 or the env var is invalid,
log an error via logger.error and abort (raise/exit) instead of calling
server.start(), otherwise call server.start() and then logger.info that includes
the actual port; reference server.add_insecure_port, server.start,
STREAMDL_GRPC_PORT, and logger.info/logger.error when making the change.

---

Nitpick comments:
In `@streamdl_proto_srv.py`:
- Around line 169-174: The try/except currently only shuts down health_server
and server on KeyboardInterrupt; move the cleanup into a finally block so
health_server.shutdown() and server.stop(0) are executed deterministically
whether wait_for_termination() exits normally or via exception. Update the block
around server.wait_for_termination() to catch KeyboardInterrupt if you need to
log, but ensure both health_server.shutdown() and server.stop(0) are invoked
from a finally section (referencing server.wait_for_termination(),
health_server.shutdown(), and server.stop(0)).

🪄 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

Run ID: 9bbe4adf-f3bf-4950-aaf7-a9fced94b245

📥 Commits

Reviewing files that changed from the base of the PR and between e314026 and 5269d40.

📒 Files selected for processing (4)

• README.md
• streamdl.go
• streamdl_proto_srv.py
• streamdl_server_entrypoint.sh

✅ Files skipped from review due to trivial changes (1)

• README.md

🚧 Files skipped from review as they are similar to previous changes (2)

• streamdl_server_entrypoint.sh
• streamdl.go

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/integration/docker-compose.integration.yml`:
- Line 12: The healthcheck command array under the service healthcheck uses
redundant quoted strings which tripped YAMLlint; update the healthcheck "test"
array (the CMD list) to use unquoted scalars instead of quoted strings (e.g.,
replace "CMD", "curl", "--fail", "--max-time", "5",
"http://localhost:8080/health" with CMD, curl, --fail, --max-time, 5,
http://localhost:8080/health) so the YAML lint rule 'quoted-strings' is
satisfied while keeping the same command semantics.

In `@tests/integration/run.sh`:
- Around line 79-92: The probe can block indefinitely on session.streams(...) so
wrap that call in a per-attempt timeout: in the inline Python snippet around
Streamlink() and session.streams(...) (the code that sets RESULT), use
signal.alarm or concurrent.futures with a timeout to limit how long
session.streams(...) may run, catch the timeout exception and print a distinct
error (e.g., "ERROR:TIMEOUT") before exiting; ensure you update the try/except
that currently prints f'ERROR:{e}' to also handle the timeout path so the shell
RESULT assignment never hangs.
- Line 68: The script currently always runs "$DC -f \"$COMPOSE_FILE\" up -d
--wait server" but legacy docker-compose (V1) doesn't support --wait; modify the
logic around the $DC invocation to detect whether the Compose binary supports
the --wait flag (e.g., probe "$DC --help" or inspect "$DC version" / whether $DC
is "docker compose" vs "docker-compose") and only append "--wait" to the "up -d"
invocation when supported, otherwise fall back to "$DC -f \"$COMPOSE_FILE\" up
-d server" (keep references to $DC, COMPOSE_FILE and the "up -d --wait server"
command so you update the exact call).

🪄 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

Run ID: 6c54c05c-085a-4359-be28-3459f7d67577

📥 Commits

Reviewing files that changed from the base of the PR and between 5269d40 and 8f817b8.

📒 Files selected for processing (3)

• tests/integration/.gitignore
• tests/integration/docker-compose.integration.yml
• tests/integration/run.sh

✅ Files skipped from review due to trivial changes (1)

• tests/integration/.gitignore

[coderabbitai]

🧹 Nitpick comments (2)

tests/integration/run.sh (1)

90-104: Consider adding an outer timeout for the channel probe.

While socket.setdefaulttimeout(15) was added, it may not cover all blocking scenarios (e.g., DNS resolution, SSL handshake). A stuck probe could still stall the integration run. Consider wrapping with a shell timeout:

Proposed fix

-  RESULT=$($DC -f "$COMPOSE_FILE" exec -T server \
+  RESULT=$(timeout 30s $DC -f "$COMPOSE_FILE" exec -T server \
     /app/.venv/bin/python -c "
 import socket
 socket.setdefaulttimeout(15)
 from streamlink import Streamlink
 session = Streamlink()
 try:
     streams = session.streams('https://twitch.tv/$channel')
     if streams:
         print('LIVE')
     else:
         print('OFFLINE')
 except Exception as e:
     print(f'ERROR:{e}')
-" 2>/dev/null) || RESULT="ERROR"
+" 2>/dev/null) || RESULT="TIMEOUT_OR_ERROR"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integration/run.sh` around lines 90 - 104, The channel probe can still
hang despite socket.setdefaulttimeout(15); wrap the entire docker-compose exec
invocation that sets RESULT (the command assigning RESULT using $DC -f
"$COMPOSE_FILE" exec -T server ...) with a shell-level timeout (e.g., the
timeout utility) so the probe is forcibly killed after a defined period; ensure
the failure path still sets RESULT="ERROR" when timeout exits nonzero and keep
existing stderr redirection and exit handling intact so any timed-out probe
yields a deterministic RESULT.

streamdl_proto_srv.py (1)

25-29: Stale comment references removed env var.

The comment mentions SERVER_LOG_LEVEL but the code actually reads LOG_LEVEL. This should be updated for consistency with .env.example which documents LOG_LEVEL=INFO.

Proposed fix

-# Configure root logger first to capture all logs, honoring SERVER_LOG_LEVEL
+# Configure root logger first to capture all logs, honoring LOG_LEVEL
 level_name = os.environ.get("LOG_LEVEL", "INFO").upper()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@streamdl_proto_srv.py` around lines 25 - 29, Update the stale top-of-file
comment that references SERVER_LOG_LEVEL to instead reference LOG_LEVEL so it
matches the actual variable used (level_name = os.environ.get("LOG_LEVEL", ...))
and the .env.example; ensure the comment clearly states that LOG_LEVEL controls
the root logger and that logging.basicConfig is configured with level_value and
the given format.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@streamdl_proto_srv.py`:
- Around line 25-29: Update the stale top-of-file comment that references
SERVER_LOG_LEVEL to instead reference LOG_LEVEL so it matches the actual
variable used (level_name = os.environ.get("LOG_LEVEL", ...)) and the
.env.example; ensure the comment clearly states that LOG_LEVEL controls the root
logger and that logging.basicConfig is configured with level_value and the given
format.

In `@tests/integration/run.sh`:
- Around line 90-104: The channel probe can still hang despite
socket.setdefaulttimeout(15); wrap the entire docker-compose exec invocation
that sets RESULT (the command assigning RESULT using $DC -f "$COMPOSE_FILE" exec
-T server ...) with a shell-level timeout (e.g., the timeout utility) so the
probe is forcibly killed after a defined period; ensure the failure path still
sets RESULT="ERROR" when timeout exits nonzero and keep existing stderr
redirection and exit handling intact so any timed-out probe yields a
deterministic RESULT.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 317d1421-2609-4a5c-bad0-f7edbbeb4217

📥 Commits

Reviewing files that changed from the base of the PR and between 5269d40 and ab2db99.

📒 Files selected for processing (7)

• .gitignore
• grpc_client.go
• move_file.go
• streamdl_proto_srv.py
• tests/integration/.gitignore
• tests/integration/docker-compose.integration.yml
• tests/integration/run.sh

✅ Files skipped from review due to trivial changes (3)

• tests/integration/.gitignore
• .gitignore
• tests/integration/docker-compose.integration.yml

🚧 Files skipped from review as they are similar to previous changes (1)

• move_file.go

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

streamdl_proto_srv.py (1)

262-266: ⚠️ Potential issue | 🟡 Minor

Handle missing width/height in format metadata.

Some formats (audio-only, or incomplete metadata) may have None for width or height. Accessing f['width'] and f['height'] directly could produce unhelpful output like resolution: Nonex720 or raise KeyError if keys are missing.

🛡️ Proposed fix

                     for f in formats:
-                        logger.critical(
-                            f"Format code: {f['format_id']}, resolution: {f['width']}x{f['height']}"
-                        )
+                        width = f.get('width', 'N/A')
+                        height = f.get('height', 'N/A')
+                        logger.critical(
+                            f"Format code: {f.get('format_id', 'unknown')}, resolution: {width}x{height}"
+                        )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@streamdl_proto_srv.py` around lines 262 - 266, The logging currently assumes
format entries always include 'width' and 'height' which can be None or missing;
update the loop that iterates formats (uses info_dict.get("formats", []) and
variable f) to safely access dimensions with f.get('width') and f.get('height'),
normalize None/missing to a clear placeholder (e.g., "audio-only" or "unknown")
and build the resolution string accordingly before calling logger.critical so it
never raises KeyError and prints meaningful resolution text for audio-only or
incomplete metadata.

🧹 Nitpick comments (1)

streamdl_proto_srv.py (1)

166-176: Prefer %-formatting over f-strings in logging calls.

Lines 166 and 176 use f-strings with logger.info(). While functional, %-formatting (%s) is preferred because it defers string interpolation until the log level is actually enabled, avoiding unnecessary computation when logging is disabled.

♻️ Suggested refactor

-    logger.info(f"Health server started on port {health_port}")
+    logger.info("Health server started on port %s", health_port)

-    logger.info(f"gRPC server started on port {bound_port}")
+    logger.info("gRPC server started on port %s", bound_port)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@streamdl_proto_srv.py` around lines 166 - 176, Replace the f-string
interpolations in the logging calls with %-formatting to defer interpolation
until the log level is enabled: change the health startup log (currently using
f"Health server started on port {health_port}") to use logger.info("Health
server started on port %s", health_port) and change the gRPC startup log
(currently using f"gRPC server started on port {bound_port}") to use
logger.info("gRPC server started on port %s", bound_port); update these calls
near where StreamServicer() is registered and the server is started so
bound_port and health_port are passed as separate arguments to logger.info.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@streamdl_proto_srv.py`:
- Around line 262-266: The logging currently assumes format entries always
include 'width' and 'height' which can be None or missing; update the loop that
iterates formats (uses info_dict.get("formats", []) and variable f) to safely
access dimensions with f.get('width') and f.get('height'), normalize
None/missing to a clear placeholder (e.g., "audio-only" or "unknown") and build
the resolution string accordingly before calling logger.critical so it never
raises KeyError and prints meaningful resolution text for audio-only or
incomplete metadata.

---

Nitpick comments:
In `@streamdl_proto_srv.py`:
- Around line 166-176: Replace the f-string interpolations in the logging calls
with %-formatting to defer interpolation until the log level is enabled: change
the health startup log (currently using f"Health server started on port
{health_port}") to use logger.info("Health server started on port %s",
health_port) and change the gRPC startup log (currently using f"gRPC server
started on port {bound_port}") to use logger.info("gRPC server started on port
%s", bound_port); update these calls near where StreamServicer() is registered
and the server is started so bound_port and health_port are passed as separate
arguments to logger.info.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e4eaf3ec-1c0a-40a9-a3f8-b42f4ff6d220

📥 Commits

Reviewing files that changed from the base of the PR and between ab2db99 and 98eea44.

📒 Files selected for processing (1)

• streamdl_proto_srv.py