fix(app): surface stream delete failures to operators

[cursor]

Summary

Bug-scan pass on app.py found that the async background delete introduced in c29362a always showed "Stream deletion started…" and only logged Lrtmp2ApiError from the daemon thread. Operators could not see failed revokes during incident response.

Bug

Scenario: Operator deletes a compromised stream; API is unreachable or the 35s RTMP drain times out. Panel flashes "deletion started" but the stream and keys remain listed with no error.

Impact: Failed delete appears successful — publish/play keys stay valid when the operator believes they are being removed.

Fix

Restore synchronous client.delete_stream() so failures surface via flash_error. Gunicorn --timeout 60 still covers the client's 35s drain window.

Validation

• python -m pytest -m "not integration" — 81 passed

  

---

^{Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.}

[qodo-code-review]

PR Summary by Qodo

Surface stream delete failures to operators by making deletes synchronous

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 10-20 Minutes

AI Description

• Make stream deletes synchronous so API failures surface via UI flash errors.
• Remove background delete thread and misleading “deletion started” messaging.
• Update tests and bug-scan notes to validate error surfacing behavior.

Diagram

graph TD
  U(["Operator"]) --> R["POST /streams/{id}/delete"] --> A["app.py delete_stream()"] --> C["Lrtmp2Client.delete_stream()"] --> S{{"librtmp2-server API"}}
  S -- "HTTP/timeout" --> E["Lrtmp2ApiError"] --> A
  A --> V["index.html shows flash_error"]

  subgraph Legend
    direction LR
    _user(["User"]) ~~~ _svc["Service/Code"] ~~~ _ext{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep background delete, persist outcome for next page load

• ➕ Non-blocking HTTP request; avoids tying up a Gunicorn worker for up to ~35s
• ➕ Still lets operators see failures if status is stored in Redis/db and rendered on redirect
• ➖ More moving parts (state storage, expiry, multi-worker correctness)
• ➖ Harder to guarantee the operator sees the final result without polling/UI changes

2. Job queue (RQ/Celery) + status/progress UI

• ➕ Robust async processing; scalable for longer operations
• ➕ Clear operator feedback (queued/running/failed/succeeded) and audit trail
• ➖ Operational overhead (broker, workers) and more code
• ➖ Overkill if deletes are usually quick and need immediate confirmation

3. Client-side polling after delete request

• ➕ Keeps server request short while still confirming completion
• ➕ Can show a live “deleting…” state until stream disappears
• ➖ More frontend complexity and edge cases (auth, retries, timeouts)
• ➖ Still needs a way to surface API/network errors cleanly

Recommendation: Given delete_stream() already intentionally blocks to confirm drain completion and can raise meaningful Lrtmp2ApiError messages, the synchronous route is the simplest way to make failures immediately visible during incident response. Consider an async job + status only if delete durations frequently exceed worker timeouts or if you need an audit trail/progress UI.

Files changed (3) +38 / -52

Bug fix (1) +4 / -22

app.pyMake stream deletion synchronous and surface API errors via flash_error +4/-22

Make stream deletion synchronous and surface API errors via flash_error

• Removes the daemon background delete thread and calls client.delete_stream() in-request. Catches Lrtmp2ApiError and stores the message in session["flash_error"] so the index page renders the failure to the operator.

app.py

Tests (1) +10 / -25

test_app.pyUpdate delete-stream tests to assert UI error surfacing and sync API call +10/-25

Update delete-stream tests to assert UI error surfacing and sync API call

• Replaces the background-thread logging assertion with checks that delete failures appear on the index page. Updates the success-path test to assert delete_stream is called before redirect and that “deletion started” is no longer shown.

tests/test_app.py

Documentation (1) +24 / -5

bug-scan-progress.mdDocument app.py bug-scan findings and update scan checklist +24/-5

Document app.py bug-scan findings and update scan checklist

• Updates the last-scanned date, resets checklist items not yet scanned, and records the delete_stream background-thread error-reporting bug and its fix rationale.

.cursor/bug-scan-progress.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 6 rules

✅ Cross-repo context

  Explored: repo: OpenRTMP/librtmp2-server (sha: 40e5850f)

1. Sync delete blocks workers ✓ Resolved 🐞 Bug ☼ Reliability

Description

delete_stream() now runs client.delete_stream() inline; when librtmp2-server returns 202, the client
can poll for up to 35s before returning, keeping the request open. With the Dockerfile’s default
Gunicorn command (no workers/threads specified), that can monopolize the single worker and stall the
entire admin panel during deletes.

Code

app.py[R383-387]

+        try:
+            client.delete_stream(stream_id)
+        except Lrtmp2ApiError as exc:
+            session["flash_error"] = str(exc)
        return redirect(url_for("index"))

Evidence

The route now calls delete_stream synchronously; the client’s delete_stream can poll until its 35s
deadline; and the Docker image starts Gunicorn without specifying workers/threads, so a long-running
request can block the panel in the default container deployment.

app.py[377-387]
lrtmp2_client.py[110-141]
Dockerfile[26-29]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DELETE /streams/<id>` is now synchronous and may spend up to ~35s polling the API; with the default Gunicorn invocation this can block the only worker and make the panel unresponsive during deletes.

### Issue Context
`Lrtmp2Client.delete_stream()` explicitly polls on HTTP 202 to wait for draining RTMP sessions. The Docker image starts Gunicorn without configuring additional workers/threads.

### Fix Focus Areas
- Dockerfile[26-29]
- lrtmp2_client.py[110-141]

### Suggested fix
- Update the Gunicorn command to use a threaded worker model (e.g., add `--threads 4` or `--worker-class gthread --threads 4`) so one long delete does not stall all operator requests.
- Keep `--timeout 60` as-is (or adjust if you later change the client wait window).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Brittle Dockerfile test ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New

Description

tests/test_delete_runtime.py hard-codes exact quoted substrings and reads "Dockerfile" relative to
the current working directory, so semantically-equivalent CMD formatting changes (or running pytest
from a different CWD) can fail tests even when the container config is correct.

Code

tests/test_delete_runtime.py[R6-11]

+def test_container_uses_threaded_gunicorn_for_long_running_deletes():
+    dockerfile = Path("Dockerfile").read_text(encoding="utf-8")
+
+    assert '"--worker-class", "gthread"' in dockerfile
+    assert '"--threads", "4"' in dockerfile
+    assert '"--timeout", "60"' in dockerfile

Relevance

⭐⭐⭐ High

Team regularly accepts making tests less brittle/CI-dependent (env isolation, timeouts,
order-independence).

PR-#46
PR-#19
PR-#12

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test currently depends on relative-path file lookup and exact quoted substrings, while the
Dockerfile CMD is a JSON-array that can be parsed and validated semantically instead of by
formatting.

tests/test_delete_runtime.py[6-11]
Dockerfile[29-29]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`test_container_uses_threaded_gunicorn_for_long_running_deletes()` is brittle because it:
- uses `Path("Dockerfile")`, which depends on the test runner’s working directory
- asserts exact string snippets including quotes/spacing, which can fail on harmless formatting changes to the Dockerfile CMD

### Issue Context
The Dockerfile CMD is a JSON-array form and can be extracted/parsed to assert semantics (presence of `--worker-class gthread`, `--threads 4`, `--timeout 60`) rather than exact substrings.

### Fix Focus Areas
- tests/test_delete_runtime.py[6-11]
- Dockerfile[29-29]

### Suggested approach
1. Resolve the Dockerfile path relative to the repository/test file (e.g., `Path(__file__).resolve().parents[1] / "Dockerfile"`).
2. Extract the `CMD [...]` JSON array from the Dockerfile text (regex on `^CMD\s+(\[.*\])$` with `re.M`) and `json.loads(...)` it.
3. Assert membership/ordering in the parsed list (e.g., `"--worker-class"` followed by `"gthread"`, etc.), rather than raw substring matches.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Delete errors not logged ✓ Resolved 🐞 Bug ◔ Observability

Description

When client.delete_stream() raises Lrtmp2ApiError, delete_stream() only sets session['flash_error']
and redirects, producing no server-side log entry. This reduces auditability/diagnosability of
failed revocations if an operator reports “delete failed” after the fact.

Code

app.py[R383-387]

+        try:
+            client.delete_stream(stream_id)
+        except Lrtmp2ApiError as exc:
+            session["flash_error"] = str(exc)
        return redirect(url_for("index"))

Evidence

The updated delete_stream handler catches Lrtmp2ApiError and only sets a flash message; app.py
otherwise only logs startup warnings, so delete failures won’t appear in server logs.

app.py[105-114]
app.py[377-387]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`delete_stream()` surfaces failures to the UI, but it does not emit any log entry when the delete fails, which makes operational debugging and post-incident audit trails harder.

### Issue Context
Other routes also flash `str(exc)`, but stream deletion is a security-sensitive action (revoking keys) where having an error log with stream_id is especially useful.

### Fix Focus Areas
- app.py[377-387]

### Suggested fix
- In the `except Lrtmp2ApiError as exc:` block, add a structured log line including `stream_id` (e.g., `app.logger.warning("delete_stream(%s) failed: %s", stream_id, exc)`), while still setting `session["flash_error"]`.
- Ensure the logged message does not include secrets (current `Lrtmp2ApiError` strings appear user-safe).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit e267f0c

Results up to commit d25690c

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Sync delete blocks workers ✓ Resolved 🐞 Bug ☼ Reliability

Description

delete_stream() now runs client.delete_stream() inline; when librtmp2-server returns 202, the client
can poll for up to 35s before returning, keeping the request open. With the Dockerfile’s default
Gunicorn command (no workers/threads specified), that can monopolize the single worker and stall the
entire admin panel during deletes.

Code

app.py[R383-387]

+        try:
+            client.delete_stream(stream_id)
+        except Lrtmp2ApiError as exc:
+            session["flash_error"] = str(exc)
        return redirect(url_for("index"))

Evidence

The route now calls delete_stream synchronously; the client’s delete_stream can poll until its 35s
deadline; and the Docker image starts Gunicorn without specifying workers/threads, so a long-running
request can block the panel in the default container deployment.

app.py[377-387]
lrtmp2_client.py[110-141]
Dockerfile[26-29]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DELETE /streams/<id>` is now synchronous and may spend up to ~35s polling the API; with the default Gunicorn invocation this can block the only worker and make the panel unresponsive during deletes.

### Issue Context
`Lrtmp2Client.delete_stream()` explicitly polls on HTTP 202 to wait for draining RTMP sessions. The Docker image starts Gunicorn without configuring additional workers/threads.

### Fix Focus Areas
- Dockerfile[26-29]
- lrtmp2_client.py[110-141]

### Suggested fix
- Update the Gunicorn command to use a threaded worker model (e.g., add `--threads 4` or `--worker-class gthread --threads 4`) so one long delete does not stall all operator requests.
- Keep `--timeout 60` as-is (or adjust if you later change the client wait window).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Delete errors not logged ✓ Resolved 🐞 Bug ◔ Observability

Description

When client.delete_stream() raises Lrtmp2ApiError, delete_stream() only sets session['flash_error']
and redirects, producing no server-side log entry. This reduces auditability/diagnosability of
failed revocations if an operator reports “delete failed” after the fact.

Code

app.py[R383-387]

+        try:
+            client.delete_stream(stream_id)
+        except Lrtmp2ApiError as exc:
+            session["flash_error"] = str(exc)
        return redirect(url_for("index"))

Evidence

The updated delete_stream handler catches Lrtmp2ApiError and only sets a flash message; app.py
otherwise only logs startup warnings, so delete failures won’t appear in server logs.

app.py[105-114]
app.py[377-387]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`delete_stream()` surfaces failures to the UI, but it does not emit any log entry when the delete fails, which makes operational debugging and post-incident audit trails harder.

### Issue Context
Other routes also flash `str(exc)`, but stream deletion is a security-sensitive action (revoking keys) where having an error log with stream_id is especially useful.

### Fix Focus Areas
- app.py[377-387]

### Suggested fix
- In the `except Lrtmp2ApiError as exc:` block, add a structured log line including `stream_id` (e.g., `app.logger.warning("delete_stream(%s) failed: %s", stream_id, exc)`), while still setting `session["flash_error"]`.
- Ensure the logged message does not include secrets (current `Lrtmp2ApiError` strings appear user-safe).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 297268a

[sonarqubecloud]

Quality Gate failed

Failed conditions
29.7% Duplication on New Code (required ≤ 3%)
D Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

💡 Need a hand with PR review? Try Gitar by Sonar!

[qodo-code-review]

Code review by qodo was updated up to the latest commit e267f0c