fix(320): break troubleshoot segment-wait loop on ffmpeg failure #423

Merged

timothy merged 3 commits from fix/320-troubleshoot-segment-wait-isfailed into main 2026-07-17 20:43:08 +02:00

Conversation 1 Commits 3 Files Changed 2

+99

timothy commented 2026-07-17 19:52:08 +02:00

Owner

Copy Link

Copy Source

Summary

Fixes #320 — the segment-readiness poll in POST /api/v1/troubleshoot/playback/start did not check notifier.IsFailed, so if ffmpeg died after writing the HLS playlist but before the first segments appeared, the while (!hasSegments) loop spun until the client cancelled — tying up the request thread and holding the troubleshooting lock (impact grew once #301 moved the manifest fetch server-side).

Change

Mirror the first (playlist) wait loop's exit check in the second (segment) loop: break on cancellationToken.IsCancellationRequested || notifier.IsFailed(sessionId). The existing if (!notifier.IsFailed(...)) guard then falls through to the terminal NotFoundProblem, releasing the thread + lock promptly. Check is placed before the segment scan so a dead ffmpeg wins immediately.

Test

Adds TroubleshootPlayback_Should_Return_404_And_Not_Spin_When_Ffmpeg_Fails_Before_Segments: seeds no segments, IsFailed→true, and asserts a 404 plus !cts.IsCancellationRequested (a bounded 10s deadline). Negative control verified: reverting the fix makes this test fail in ~10s (loop spins to the deadline), so it is non-vacuous.

Scope / review

• API write-path handler touching the lock-hold window → independent review is mandatory per the skip-rubric; a cold review pass follows.
• No docs update: route + response codes unchanged, no new/changed endpoint contract or convention.

🤖 Generated with Claude Code

## Summary Fixes #320 — the segment-readiness poll in `POST /api/v1/troubleshoot/playback/start` did not check `notifier.IsFailed`, so if ffmpeg died **after** writing the HLS playlist but **before** the first segments appeared, the `while (!hasSegments)` loop spun until the client cancelled — tying up the request thread and holding the troubleshooting lock (impact grew once #301 moved the manifest fetch server-side). ## Change Mirror the first (playlist) wait loop's exit check in the second (segment) loop: break on `cancellationToken.IsCancellationRequested || notifier.IsFailed(sessionId)`. The existing `if (!notifier.IsFailed(...))` guard then falls through to the terminal `NotFoundProblem`, releasing the thread + lock promptly. Check is placed before the segment scan so a dead ffmpeg wins immediately. ## Test Adds `TroubleshootPlayback_Should_Return_404_And_Not_Spin_When_Ffmpeg_Fails_Before_Segments`: seeds no segments, `IsFailed`→true, and asserts a 404 plus `!cts.IsCancellationRequested` (a bounded 10s deadline). **Negative control verified**: reverting the fix makes this test *fail* in ~10s (loop spins to the deadline), so it is non-vacuous. ## Scope / review - API write-path handler touching the lock-hold window → independent review is mandatory per the skip-rubric; a cold review pass follows. - No docs update: route + response codes unchanged, no new/changed endpoint contract or convention. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

timothy added 1 commit 2026-07-17 19:52:09 +02:00

fix(320): break troubleshoot segment-wait loop on ffmpeg failure ... b2059bd2a6

The second (segment-readiness) poll in POST /api/v1/troubleshoot/playback/start
only checked !hasSegments. If ffmpeg died after writing the HLS playlist but
before the first segments appeared, the loop spun until the client cancelled —
tying up the request thread and holding the troubleshooting lock (worse since
#301 moved the manifest fetch server-side per #320's writeup). Mirror the first
(playlist) wait loop's exit check so a dead ffmpeg or client cancellation breaks
the wait; the existing !IsFailed guard then falls through to the terminal
NotFoundProblem, releasing the thread + lock.

Adds a non-vacuous regression test (verified it fails on the pre-fix spinning
loop via a bounded cancellation deadline).

fixes #320

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

timothy referenced this issue from a commit 2026-07-17 19:57:59 +02:00

fix(320): address cold-review nits — drop redundant cancel term, harden test

timothy added 1 commit 2026-07-17 19:57:59 +02:00

fix(320): address cold-review nits — drop redundant cancel term, harden test ... 5f8525eed7

Cold review (PR #423) found two LOW nits:
- The segment-wait break also checked cancellationToken.IsCancellationRequested,
  which could exit into the Ok gate and return a spurious 200 for a cancelled
  request with no segments. The term is redundant (Task.Delay already throws on
  cancel -> caught -> 404), so break on notifier.IsFailed only.
- The regression test's non-vacuousness was filesystem-state dependent (absent
  troubleshooting folder -> Directory.GetFiles throws -> false pass pre-fix).
  Seed an empty folder like the sibling success test so the spin path is
  deterministic regardless of NUnit run order.

Negative control re-verified: removing the IsFailed break fails the test in ~10s.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

timothy added 1 commit 2026-07-17 20:03:13 +02:00

test(320): don't blanket-delete foreign *.ts in the shared troubleshooting folder ... de63603aab

Re-review nit: the regression test deleted every *.ts in the machine-global
TranscodeTroubleshootingFolder, which could nuke a live troubleshooting session's
segments on a dev machine (reaping files it didn't create). Drop the sweep and
keep only Directory.CreateDirectory — the folder-exists guarantee is what closes
the false-pass hole; NUnit runs serially and no test leaves >= 2 stray .ts, so
determinism is unaffected (negative control re-verified: still fails in ~10s).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

timothy commented 2026-07-17 20:04:03 +02:00

Author

Owner

Copy Link

Copy Source

Independent review — MERGEABLE.

Two cold, review-only passes (fresh context, no implementation role):

1. Initial diff (b2059bd2) → MERGEABLE-WITH-NITS. Confirmed the core fix is correct (breaking with hasSegments=false + IsFailed=true falls through to the terminal NotFoundProblem; no lock leak or double-release — the worker's finally still owns release since startEnqueued is true). Two LOW nits: (a) the break's cancellationToken.IsCancellationRequested term could exit into the Ok gate and return a spurious 200 for a cancelled request; (b) the test's non-vacuousness was filesystem-state dependent.
2. Fix commit (5f8525ee) re-review → both nits fixed, cancel path traced to a clean 404 with correct lock release, test deterministic. One new LOW nit: the test blanket-deleted foreign *.ts in the machine-global troubleshooting folder.

Final commit de63603a resolves that last nit — a pure subtraction of the foreign-file deletion (keeps only Directory.CreateDirectory); no new logic. Negative control re-verified after every change: removing the IsFailed break makes the regression test fail in ~10s.

All findings addressed; nothing outstanding.

Review-verdict: MERGEABLE @ de63603aab

**Independent review — MERGEABLE.** Two cold, review-only passes (fresh context, no implementation role): 1. **Initial diff** (`b2059bd2`) → MERGEABLE-WITH-NITS. Confirmed the core fix is correct (breaking with `hasSegments=false` + `IsFailed=true` falls through to the terminal `NotFoundProblem`; no lock leak or double-release — the worker's `finally` still owns release since `startEnqueued` is true). Two LOW nits: (a) the break's `cancellationToken.IsCancellationRequested` term could exit into the Ok gate and return a spurious 200 for a cancelled request; (b) the test's non-vacuousness was filesystem-state dependent. 2. **Fix commit** (`5f8525ee`) re-review → both nits fixed, cancel path traced to a clean 404 with correct lock release, test deterministic. One new LOW nit: the test blanket-deleted foreign `*.ts` in the machine-global troubleshooting folder. **Final commit `de63603a`** resolves that last nit — a pure subtraction of the foreign-file deletion (keeps only `Directory.CreateDirectory`); no new logic. Negative control re-verified after every change: removing the `IsFailed` break makes the regression test fail in ~10s. All findings addressed; nothing outstanding. Review-verdict: MERGEABLE @ de63603aab658e5d254bbcc0213f439d377e1a01

timothy merged commit 1b355660c5 into main 2026-07-17 20:43:08 +02:00

timothy deleted branch fix/320-troubleshoot-segment-wait-isfailed 2026-07-17 20:43:08 +02:00

timothy referenced this issue from a commit 2026-07-17 20:43:09 +02:00

Merge pull request 'fix(320): break troubleshoot segment-wait loop on ffmpeg failure' (#423) from fix/320-troubleshoot-segment-wait-isfailed into main

timothy referenced this pull request 2026-07-17 20:43:33 +02:00

TroubleshootController playback segment-wait loop doesn't check notifier.IsFailed (spins until client cancel if ffmpeg dies) #320

timothy referenced this pull request 2026-07-17 20:44:10 +02:00

ChicoryTV rewrite — queue tracker (goal, arc, session protocol) #237

timothy referenced this pull request 2026-07-17 21:01:16 +02:00

ci: pre-push guard against pushing an uncommitted working-tree change (H13) #424

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#423