Repository navigation
feat(reliability): yt-dlp 3× exponential backoff + snapshot restore timeout - #850
Merged
LucasSantana-Dev merged 13 commits intoMay 14, 2026
Merged
Conversation
Build context was including `.worktrees/` (3.4GB), `worktrees/` (707MB), `.wt-specs/` (41MB), `.claude/` (115MB), and `.agents/` (183MB) — totalling ~4.5GB of duplicated trees and AI agent state shipped to the Docker daemon on every build. None of this is needed inside any image. Also exclude `archive/` and `downloads/` (host-only state).
The previous HEALTHCHECK was `node -e "console.log('Service is running')"`,
which always exits 0 regardless of bot state — orchestrators could never
detect a wedged or disconnected bot.
New check opens a raw TCP socket to Redis ($REDIS_HOST / $REDIS_PORT) and
sends the RESP PING command. Pass = +PONG within 3s; anything else fails.
This confirms (1) node can execute inside the container and (2) the bot's
critical Redis dependency is reachable from this container. No new deps.
Start period bumped 5s → 30s to account for `prisma migrate deploy` running
before the bot process starts.
`docker-compose.dev.yml` referenced `target: development` but no such stage existed in `Dockerfile` — `docker compose -f docker-compose.dev.yml up --build` would fail with 'failed to find target development'. New `development` stage derives from `base-runtime` (already has ffmpeg / opus / yt-dlp), adds native build tools, and runs `tsx watch` via `npm run dev --workspace=packages/bot`. Compose still bind-mounts host source over `/app`; node_modules installed on first run to populate the anonymous volume.
Switch `Dockerfile.nginx` and `Dockerfile.frontend` from `nginx:alpine`
(runs as root to bind port 80) to `nginxinc/nginx-unprivileged:1.27-alpine`
(UID 101, listens on 8080 by default — no NET_BIND_SERVICE capability
needed).
Changes:
- nginx confs (frontend + reverse proxy): `listen 80` → `listen 8080`
- nginx reverse-proxy upstream: `http://frontend:80` → `http://frontend:8080`
- Dockerfile.frontend + Dockerfile.nginx: pinned image, EXPOSE 8080, added
HEALTHCHECK via `wget --spider` (busybox wget ships in the base image).
- docker-compose.yml: port mapping `${NGINX_PORT:-8080}:80` → `:8080`.
DEPLOY ACTION REQUIRED: update `cloudflared/config-lucky.yml` on the
homelab so the tunnel ingress points at `http://nginx:8080` instead of
`http://nginx:80` before merging this PR. Host-side `NGINX_PORT`
default is unchanged (8080).
Compose previously had no memory or CPU limits on any service — a runaway bot or backend process could OOM-kill the homelab host. Adds tiered defaults via YAML anchors: small-svc (frontend, nginx, webhook, cloudflared) 128m / 0.25 cpu medium-svc (redis, backend) 512m / 0.5 cpu large-svc (postgres, bot) 1g / 1.0 cpu Bot + backend also get `env_file: .env` so any var missing from the explicit `environment:` block falls back to .env at startup. Explicit entries still take precedence, so behavior is unchanged for vars already listed. Cloudflared `user: root` removed — the official image's nonroot default is sufficient for `tunnel run` with a mounted config dir. `docker-compose config -q` passes.
The deploy webhook container has `/var/run/docker.sock` bind-mounted in `docker-compose.yml`, which gives anything inside it effective root on the host. Pulling `almir/webhook:latest` (last published 2026-02-12) every rebuild made that surface vulnerable to silent upstream changes. Pin to `2.8.3` (current latest, identical digest as `latest` at time of this change). Also fix the inline-comment placement on `USER root` — it was on the same line which is parsed differently across Docker versions.
… node 22
Three small but durable cleanups:
1. Drop the no-op `base-runtime-backend` stage. `production-backend` now
derives directly from `node:${NODE_VERSION}` — the intermediate stage
only set WORKDIR, which the production stage already does.
2. Replace `pip3 install --break-system-packages` for yt-dlp with a
proper PEP-668-compliant venv at `/opt/ytdlp`, symlinked into
`/usr/local/bin`. Same behavior, no warning suppression, easier to
audit and upgrade. /root/.cache is cleaned in the same layer.
3. `packages/frontend/Dockerfile.dev` was using `node:24-alpine` while
production frontend is pinned to `node:22-alpine` (PR #846). Aligned
to 22 to avoid silent native-module drift. Also removed
`npm cache clean --force` which defeated the BuildKit cache mount.
Captures the 8-commit Docker surface overhaul: motivation, decisions per commit, consequences (including the cloudflared config-lucky.yml port edit required on the homelab before merge), out-of-scope items, and revisit triggers.
- deploy/Dockerfile: pin almir/webhook:2.8.3 by manifest digest sha256:f77cc91c91d1527b48052280af38b50791e842ad8dd291e9b360a0c13c9ca991. Docker Hub tags are mutable by default; the digest makes the supply-chain story (this container holds docker.sock) actually defensible. - docs/decisions/2026-05-13-docker-overhaul.md: remove dangling reference to a separate audit doc that was never committed; ADR contains audit findings inline.
…imeout - Replace single-shot yt-dlp execution with 3-attempt retry loop using exponential backoff (30s → 45s → 60s). Each attempt has its own independent timeout; a settled flag prevents double-resolve when the process closes after a timeout kill. - Wrap restoreSnapshot() in Promise.race() with a 2-second deadline so a slow or hung DB call cannot block the entire queue restore path. Logs a warning and falls through to an empty queue on timeout.
…rategy - Add 4 retry-behavior tests: first-attempt success, retry-on-failure, all-attempts-exhausted (expects throw), timeout-kills-and-retries. Moved discord-player mock to file scope to eliminate jest.resetModules() worker crashes. - Add ADR 2026-05-14: defer in-CI voice integration testing; yt-dlp retry + lifecycle snapshot timeout + backend scaffolding are higher ROI.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
|
Failed to generate code suggestions for PR |
|
Size Change: 0 B Total Size: 369 kB ℹ️ View Unchanged
|
📝 WalkthroughWalkthroughThis PR standardizes Docker infrastructure across the application by migrating services to unprivileged images, moving from port 80 to 8080, isolating yt-dlp in a Python venv, and implementing yt-dlp retry logic with timeout guards alongside session restore resilience in the bot. ChangesInfrastructure Overhaul & Bot Reliability Improvements
Sequence Diagram(s)sequenceDiagram
participant executeYtDlp
participant spawnYtDlp
participant Process as child_process
participant Test as Test Framework
executeYtDlp->>spawnYtDlp: retry attempt 1 (timeout1)
spawnYtDlp->>Process: spawn yt-dlp
Process-->>spawnYtDlp: stdout/stderr + exit code
alt timeout elapsed
spawnYtDlp->>Process: kill process
spawnYtDlp-->>executeYtDlp: timeout error
executeYtDlp->>spawnYtDlp: retry attempt 2 (timeout2)
else success on close
spawnYtDlp-->>executeYtDlp: success result
end
executeYtDlp-->>Test: final result or exhausted retries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
Dockerfile (1)
116-120: ⚡ Quick winExtract the healthcheck to a standalone script for maintainability.
The Redis TCP PING implementation is correct and handles all expected cases appropriately (RESP protocol is properly formatted, socket cleanup is implicit via process.exit, timeout margins are adequate, and partial reads are negligible for this use case). However, the one-liner is difficult to debug and test locally. Extract to
scripts/healthcheck-bot.jsto enable standalone testing and improve readability without changing logic.🤖 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 `@Dockerfile` around lines 116 - 120, Replace the inline HEALTHCHECK node one-liner with a call to a new executable script (scripts/healthcheck-bot.js) that contains the same Redis TCP PING logic (uses net.createConnection, writes the RESP PING frame, listens for 'data' to check for "+PONG", handles 'error' by exiting non-zero, and enforces a 3s timeout before exiting 1); add the script to the image via COPY and ensure it is executable, then update the Dockerfile HEALTHCHECK line to run node /app/scripts/healthcheck-bot.js (or the container path you COPY to) so behavior and exit codes remain identical while making the logic maintainable and testable locally.packages/bot/src/handlers/player/lifecycleHandlers.ts (1)
52-59: 💤 Low valueConsider clarifying the timeout fallback behavior in the log message.
The log states "continuing with empty queue", but the timeout doesn't explicitly clear the queue—it simply continues with whatever queue state exists at that moment. If tracks were already present or partially restored, the queue might not actually be empty.
Consider rephrasing to: "Snapshot restore timed out, continuing with current queue state" for accuracy.
🤖 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 `@packages/bot/src/handlers/player/lifecycleHandlers.ts` around lines 52 - 59, The timeout fallback log is misleading; update the message emitted in the restoreDeadline handler so it accurately reflects that the process continues with the current queue state rather than an empty queue — locate the Promise.race block using restoreDeadline and musicSessionSnapshotService.restoreSnapshot and change the infoLog call (currently using queue.guild.name) to something like "Snapshot restore timed out, continuing with current queue state" or equivalent.packages/bot/src/utils/music/ytdlpExtractor/service.ts (1)
141-161: 💤 Low valueUnreachable fallback at line 160.
The loop at lines 145-158 always returns on the last iteration (line 155:
if (isLastAttempt) return result), making the fallback at line 160 unreachable. While this doesn't affect runtime behavior, it adds dead code.Consider removing line 160 or adding a TypeScript assertion to document that the loop guarantees a return:
🧹 Remove unreachable fallback
debugLog({ message: `yt-dlp attempt ${attempt + 1} failed (${result.error}), retrying…` }) } - - return { success: false, error: 'yt-dlp exhausted all retries' } }🤖 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 `@packages/bot/src/utils/music/ytdlpExtractor/service.ts` around lines 141 - 161, The final fallback return is dead code because executeYtDlp always returns inside the retry loop when isLastAttempt is true; remove the unreachable `return { success: false, error: 'yt-dlp exhausted all retries' }` at the end of executeYtDlp (or, if you prefer to be explicit, replace it with a strong assertion/throw like `throw new Error('unreachable')`) and keep the existing loop logic that returns `result` when `isLastAttempt` is true.packages/bot/tests/utils/music/ytdlpExtractor.test.ts (1)
54-127: 💤 Low valueAdd a test to verify the exponential timeout progression (30s → 45s → 60s).
The test suite comprehensively validates the retry behavior for success on first attempt, retry-and-recover after initial failure, exhausting all three attempts, and timeout triggering process kill and retry. Consider adding a test to explicitly verify the timeout values scale according to the implementation progression to improve test coverage clarity.
🤖 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 `@packages/bot/tests/utils/music/ytdlpExtractor.test.ts` around lines 54 - 127, Add a unit test in ytdlpExtractor.test.ts that constructs a YtDlpExtractorService with initial timeout 30000 and uses mockSpawn/createMockProcess to return three processes; advance timers and flush microtasks between attempts and assert that jest.advanceTimersByTime was called (or timers progressed) with 30000, then 45000, then 60000 before each retry and that proc.kill was invoked on each timed-out process; reference the existing test patterns (retry behavior block, createMockProcess, mockSpawn, extractor.handle, jest.advanceTimersByTime, and flushMicrotasks) to mirror the other retry tests and verify the exponential timeout progression (30s → 45s → 60s).
🤖 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.
Nitpick comments:
In `@Dockerfile`:
- Around line 116-120: Replace the inline HEALTHCHECK node one-liner with a call
to a new executable script (scripts/healthcheck-bot.js) that contains the same
Redis TCP PING logic (uses net.createConnection, writes the RESP PING frame,
listens for 'data' to check for "+PONG", handles 'error' by exiting non-zero,
and enforces a 3s timeout before exiting 1); add the script to the image via
COPY and ensure it is executable, then update the Dockerfile HEALTHCHECK line to
run node /app/scripts/healthcheck-bot.js (or the container path you COPY to) so
behavior and exit codes remain identical while making the logic maintainable and
testable locally.
In `@packages/bot/src/handlers/player/lifecycleHandlers.ts`:
- Around line 52-59: The timeout fallback log is misleading; update the message
emitted in the restoreDeadline handler so it accurately reflects that the
process continues with the current queue state rather than an empty queue —
locate the Promise.race block using restoreDeadline and
musicSessionSnapshotService.restoreSnapshot and change the infoLog call
(currently using queue.guild.name) to something like "Snapshot restore timed
out, continuing with current queue state" or equivalent.
In `@packages/bot/src/utils/music/ytdlpExtractor/service.ts`:
- Around line 141-161: The final fallback return is dead code because
executeYtDlp always returns inside the retry loop when isLastAttempt is true;
remove the unreachable `return { success: false, error: 'yt-dlp exhausted all
retries' }` at the end of executeYtDlp (or, if you prefer to be explicit,
replace it with a strong assertion/throw like `throw new Error('unreachable')`)
and keep the existing loop logic that returns `result` when `isLastAttempt` is
true.
In `@packages/bot/tests/utils/music/ytdlpExtractor.test.ts`:
- Around line 54-127: Add a unit test in ytdlpExtractor.test.ts that constructs
a YtDlpExtractorService with initial timeout 30000 and uses
mockSpawn/createMockProcess to return three processes; advance timers and flush
microtasks between attempts and assert that jest.advanceTimersByTime was called
(or timers progressed) with 30000, then 45000, then 60000 before each retry and
that proc.kill was invoked on each timed-out process; reference the existing
test patterns (retry behavior block, createMockProcess, mockSpawn,
extractor.handle, jest.advanceTimersByTime, and flushMicrotasks) to mirror the
other retry tests and verify the exponential timeout progression (30s → 45s →
60s).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb010c5b-b21c-4bab-b2a3-b69c3ed55a96
📒 Files selected for processing (14)
.dockerignoreDockerfileDockerfile.frontendDockerfile.nginxdeploy/Dockerfiledocker-compose.ymldocs/decisions/2026-05-13-docker-overhaul.mddocs/decisions/2026-05-14-discord-integration-testing-strategy.mdnginx/frontend.confnginx/nginx.confpackages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/utils/music/ytdlpExtractor/service.tspackages/bot/tests/utils/music/ytdlpExtractor.test.tspackages/frontend/Dockerfile.dev
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
LucasSantana-Dev
enabled auto-merge (squash)
May 14, 2026 17:31
|
Merged
Merged
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
spawnYtDlpnow retries up to 3 times with increasing timeouts (30 s → 45 s → 60 s). Network blips or CDN hiccups no longer cause permanent playback failures on the first transient error.restoreSnapshot()in theconnectionlifecycle handler is now wrapped inPromise.race(2 s)with a warn-and-continue fallback. A slow or hung DB call can no longer block the entire queue restore path indefinitely.ytdlpExtractor.test.ts.docs/decisions/2026-05-14-discord-integration-testing-strategy.mddocuments the DEFER decision for in-CI voice integration testing and the rationale for these higher-ROI reliability fixes instead.Test plan
pnpm test --filter=bot— all 24 ytdlp tests passlifecycleHandlers— session restore still works within 2 s under normal conditionsSummary by CodeRabbit
Release Notes
Chores
Bug Fixes
Documentation