[wagenet]

Opened by Claude (AI assistant) on behalf of @wagenet.

Problem

POST /productions/:id/deactivate reads the production doc, runs the teardown, then writes with the _rev it read at the start. On a local rig with recording on and object storage unreachable, each request spent a long time in the VOD upload step, so three deactivates sent within about 25 s overlapped. The first succeeded. The other two hit a CouchDB 409 on the final write and returned 500 Deactivation failed, although the production was already deactivated. Any other write to the doc during the teardown, such as a tally update, has the same effect.

Change

Deactivate is now idempotent. A repeated request returns 200 with the production's resulting state and does not run the teardown again.

• One run per production. A deactivate that arrives while another one for the same production is running waits for it and returns the same response. If that run fails, every waiting request gets the 500, and the next request starts a new run. The map is per process, like activationAbortControllers.
• 409 on the final write re-reads the doc. It retries only if the stored doc names no Strom flow or intercom grouping other than the ones this run tore down:
  • same status: only unrelated fields changed (tally, name), so the deactivation is applied on top and retried (up to MAX_DB_WRITE_RETRIES);
  • already inactive/ended with no flow (idle watchdog, another instance): that status is kept, and the refs the other writer left behind (such as the intercom grouping this run deleted) are cleared;
  • otherwise, e.g. a new activation or an intercom grouping a guest created mid-teardown: still a 500, and the doc keeps the resource for the next deactivate to tear down. Main behaves the same way here.

Most of the productions.ts diff is the handler body moving into deactivate() with new indentation, so review with whitespace hidden (?w=1). If #403 lands, its status change goes in deactivatedDoc().

Not in this PR: the upload still blocks the response

With storage down, deactivate still takes tens of seconds to respond. Moving the upload after the response needs two checks this PR does not make: that Strom keeps recordings after the flow is deleted, and a new signal for the #366 isStillRecording guard, which compares _rev and would always skip deletion. Left as a follow-up.

Tests

src/__tests__/deactivate-concurrency.test.ts covers:

• overlapping requests share one teardown and one write;
• a failed shared run fails every request waiting on it, and the next request starts fresh;
• a later request runs again;
• each 409 outcome, including a guest's new intercom grouping and a watchdog stop that left a stale grouping;
• giving up after 3 conflicts.

Each case fails when its behaviour is reverted on its own. Against main's productions.ts, 5 of the 8 tests fail. The three that pass ("runs again", the new activation, and the guest's grouping) are cases main already handles.

pnpm typecheck passes. pnpm vitest run: 773 tests pass in 74 files.

🤖 Generated with Claude Code

[birme]

Reviewed by the daily-backlog-pr housekeeping pass (Phase 3.5). Verified against main: deactivate is made idempotent via a per-productionId in-flight Promise map (cleared in finally) plus a writeDeactivatedDoc conflict-retry that re-reads and only re-applies when the stored doc still names no flow/intercom other than those this run tore down — correctly refusing to clobber a reactivation or a guest's new intercom grouping. Teardown, the #366 isStillRecording rev-guard, and RecordingDoc persistence are preserved verbatim in the extracted deactivate(). MAX_DB_WRITE_RETRIES=3 matches the "gives up" test; concurrency/conflict coverage is strong. No fabrication, no security impact.

Note: this rewrites the deactivate handler in src/routes/productions.ts, which PR #407 (the startedAt line) also edits — no code dependency, but whichever merges second will need to resolve a conflict. Branch is behind main after #406 merged; it will be refreshed server-side so a required check runs against current main, then merged on a subsequent pass.

[birme]

This PR now conflicts with main and can't be refreshed by automated update-branch (that only fixes a branch that is merely behind, not one with a real conflict). The conflict is in src/routes/productions.ts: PR #403 ("restore ended after deactivating an active production") merged into main and overlaps this PR's change to the same deactivate handler. The review verdict here was otherwise APPROVE. Please merge main into your branch and resolve the deactivate-handler conflict, taking care to preserve both the idempotency fix here and the ended/endedReason: "deactivated" contract #403 restored.

[birme]

VERDICT: APPROVE

Reviewed at db760d6 (head of wagenet/fix-deactivate-conflict), diffed against main (95c00ee). Correctness-focused review; I did not author this.

Correctness — verified

• Root cause & fix match. The old handler read the doc, ran a teardown that can take tens of seconds (VOD upload with object storage down), then wrote with the stale _rev, so overlapping deactivates 409'd and surfaced as 500. The fix adds (a) a per-process inFlightDeactivations map so a concurrent request joins the running teardown and shares its result, and (b) a 409-aware final write (writeDeactivatedDoc) that re-reads and re-applies. Both are sound.
• In-flight dedup is leak-free and race-safe. The run promise is created eagerly, stored, and removed in a finally. Every consumer (creator and joiners) awaits run inside a try/catch, so a rejected run cannot produce an unhandled rejection, and a failed shared run correctly fails all waiters while the next request starts fresh (deactivate called twice in that test). Mirrors the existing activationAbortControllers per-process pattern, as claimed.
• writeDeactivatedDoc retry predicate is correct for each case I traced:
  • unrelated field changed (same status + same flow) → re-apply deactivation on the fresh rev;
  • already inactive/ended with no flow (idle watchdog / other instance) → keep that writer's status/endedReason and clear the stale refs this run already tore down;
  • a resource this run did not tear down (new activation with a different flow; a guest's new intercom grouping) → tornDown is false, so it rethrows the 409 → 500, leaving the resource for the next deactivate. The tornDown guard (stromFlowId/intercomProductionId either undefined or equal to what this run saw) correctly distinguishes these.
  • give-up bound honored: attempt >= MAX_DB_WRITE_RETRIES - 1 → at most 3 inserts.
• #403 (ended/endedReason: "deactivated") contract preserved. deactivatedDoc() reproduces status = stoppedStatus(doc.status) and endedReason = status === 'ended' ? 'deactivated' : undefined exactly as main, and when keeping another stop path's status it preserves that writer's endedReason (e.g. idle). The teardown body (abort, state clears, relay force-stops, VOD upload + RecordingDoc persist, intercom teardown, guest sweep) is moved verbatim into deactivate() with no behavioral change; response shape {id,name,status,_rev} is unchanged.

Merge-conflict question

No real conflict remains. src/routes/productions.ts is unchanged on main since the merge-base (bfcdefa), which already carries the #403 contract; git merge-tree produces no conflict markers. mergeStateStatus: BEHIND reflects only unrelated new commits on main. The overlap a prior comment flagged is resolved: the branch sits on top of the #403 change and preserves it. A rebase would be clean.

Fabrication / guesswork check

None found. All referenced symbols (stoppedStatus, MAX_DB_WRITE_RETRIES, EndedReason, teardownIntercomProduction, sweepGuestsOnProductionEnd, uploadRecordings, etc.) exist and type-check; EndedReason includes 'deactivated'/'idle'.

Security

No new surface. No auth/route changes, no secrets, best-effort teardown contracts and the guest-invite sweep (#325 security boundary) are retained unchanged.

Quality gates (local, Node 22 / pnpm 10)

• pnpm install --frozen-lockfile: OK
• pnpm typecheck: PASS
• pnpm build: PASS
• npx vitest run: PASS — 823/823 tests in 80 files (incl. the 8 new deactivate-concurrency cases and the existing production-lifecycle deactivate contract)
• gh pr checks 405: build-and-test PASS

Clean fix, well-tested, idempotency and the #403 contract are consistent. Approving.