Skip to content

Guard production deletion against active sessions - #367

Merged
birme merged 2 commits into
mainfrom
backend/fix-172-guard-active-sessions
Sep 23, 2026
Merged

birme merged 2 commits into
mainfrom
backend/fix-172-guard-active-sessions

Conversation

@birme

@birme birme commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The DELETE /production/:productionId endpoint deleted a production unconditionally, relying entirely on a client-side check for active participants before the request was even made.
  • Added ProductionManager.hasActiveSessions(productionId), which queries the db for non-expired sessions on the production that are still marked active.
  • The delete route now checks this first and returns 409 Conflict (without deleting) when active sessions exist, instead of silently removing an in-use production.

Test plan

  • Tests pass (npm test)
  • TypeScript compiles (npm run typecheck)
  • Lint clean (npm run lint)
  • Regression test: DELETE /production/:id returns 409 and does not call deleteProduction when hasActiveSessions resolves true, and still deletes (200) when it resolves false

Closes #172

🤖 Generated with Claude Code

The DELETE /production/:id endpoint deleted a production unconditionally,
relying entirely on a client-side check for active participants before
allowing the request. A direct API call bypassed that guard, so an
in-use production could be removed while sessions were connected.

Add ProductionManager.hasActiveSessions() to check for non-expired,
active sessions on a production, and have the route return 409 Conflict
instead of deleting when any are found.

Closes #172

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@birme

birme commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Self-review (daily-backlog-pr Phase 3) — Verdict: Needs Changes

Recorded as a marker comment because this PR is self-authored by the automation account, and GitHub blocks state-bearing self-review. The board item (#172) is being moved back to Ready so the next implementation pass addresses the feedback below.

The change is mechanically clean (409 wiring, schema updated, ./log mocked in every touched test file, style/lint/typecheck pass, no interface widening) — but the guard uses the wrong definition of "active", which undermines the fix.

Blocking

  • src/production_manager.ts hasActiveSessions: the query { productionId, isExpired: false, isActive: true } under-blocks. checkUserStatus flips a session to isActive: false after SESSION_INACTIVE_THRESHOLD (60s) of no heartbeat but keeps it isExpired: false until SESSION_EXPIRED_THRESHOLD (100s) — a routine ~40s window on every session lifecycle where a real, non-expired session (one a heartbeat would reactivate) exists, yet hasActiveSessions returns false and the production can be deleted out from under it.
  • This contradicts the codebase's own "in use" definition: getUsersForLine (which drives the participant list and the frontend's disabled delete button) keys on isExpired: false, not isActive. The frontend guard Bug: Can remove productions containing active sessions #172 asks the backend to backstop is isDeleteProductionDisabled={totalParticipants > 0}, backed by that isExpired-based long-poll data. So the guard as written can still be bypassed during the inactive-but-not-expired window — the exact gap Bug: Can remove productions containing active sessions #172 was filed to close.
  • Fix: base the query on isExpired: false (consistent with getUsersForLine / checkUserStatus) rather than requiring isActive: true. Update the hasActiveSessions JSDoc to match, and add a regression test for the isActive:false, isExpired:false boundary (the current two tests only cover the fully-active and no-session extremes, so they miss this).

Suggestions

  • Consider WHIP grace windows (WHIP_START_MS, WHIP_INACTIVE_ALLOW_MS, WHIP_ABSENCE_ALLOW_MS) for full parity with checkUserStatus — likely covered by construction once the query is isExpired-based, since WHIP sessions also stay non-expired through those windows.

Address self-review feedback on #172: keying the deletion guard on
isActive:true under-blocked the ~40s window where a session is past the
inactivity threshold (isActive:false) but not yet expired (isExpired:false)
and a heartbeat would reactivate it. Match the isExpired-based "in use"
definition used by getUsersForLine/checkUserStatus and add a regression
test for the isActive:false/isExpired:false boundary.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@birme

birme commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Self-review (daily-backlog-pr Phase 3) — Verdict: LGTM

Recorded as a marker comment because this PR is self-authored by the automation account (GitHub blocks state-bearing self-review). Reviewed by a separate code-reviewer invocation at bf5c632d2be32523c168b7563e0ade969b8e5a24.

The prior Needs-Changes feedback was addressed: hasActiveSessions now queries { productionId, isExpired: false }, matching the isExpired-based "in use" definition used by getUsersForLine/checkUserStatus, so the inactive-but-not-expired window no longer under-blocks. A regression test for the isActive:false, isExpired:false boundary was added; the shared manager mocks were updated; all 314 backend tests, lint, prettier and typecheck pass; CI is green.

No blocking items. Merging with --admin (satisfies the merge despite the unsatisfiable self-approval requirement; enforce_admins: false).

@birme
birme merged commit ba7d24c into main Sep 23, 2026
4 checks passed
@birme
birme deleted the backend/fix-172-guard-active-sessions branch September 23, 2026 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Can remove productions containing active sessions

2 participants