fix(app,server): guard stop/fail writes against a concurrent force-stop - #235
Merged
Merged
Conversation
Final review pass on the merged capture-stop-hang PR found two real
gaps left over from that work:
- interruptMeetingCapture was hardened against racing a concurrent
stop/force-stop (a conditional UPDATE ... WHERE status NOT IN
('stopped','error')), but stopMeetingCapture and failMeetingCapture —
the two functions most likely to actually race force-stop, since
force-stop exists specifically to be called while one of them looks
stuck — never got the same guard. A stop or fail that resolves after
a concurrent force-stop already finalized the session could silently
overwrite its ended_at/status. Both now use the same guarded write,
skipping the SSE announce too when they lose the race.
- teardownCaptureSession's single-flight silently dropped a joining
caller's flushRemainder preference in favor of whichever caller
happened to start the shared task. The flag is now mutable on the
shared entry, so a joiner requesting a flush can still get it as long
as it arrives before that step actually runs.
Also: recorder.ts's local withDeadline duplicated
with-runtime-message-timeout.ts's identical helper (just a hardcoded
null fallback) — consolidated into one shared withDeadline, with
withRuntimeMessageTimeout as a thin specialization. And CapturePage's
Force stop button used the mutation's isPending across the whole
component, so force-stopping session A also disabled the button while
viewing session B; now scoped via the mutation's own `variables`.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
❌ Tests failed — 17/2179 failed
Failed tests
|
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
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
Follow-up to #234 (already merged) — a final review pass on that PR now that the review session limit reset found two real gaps left over from it:
interruptMeetingCapturewas hardened against racing a concurrent stop/force-stop (UPDATE ... WHERE status NOT IN ('stopped','error')), butstopMeetingCaptureandfailMeetingCapture— the two functions most likely to actually race force-stop, since force-stop exists specifically to be called while one of them looks stuck — never got the same guard. A stop or fail that resolves after a concurrent force-stop already finalized the session could silently overwrite itsended_at/status. Both now use the same guarded write, skipping the SSE announce too when they lose the race.teardownCaptureSession's single-flight silently dropped a joining caller'sflushRemainderpreference in favor of whichever caller happened to start the shared task. The flag is now mutable on the shared entry, so a joiner requesting a flush can still get it as long as it arrives before that step actually runs.Also two cleanups from the same review:
recorder.ts's localwithDeadlineduplicatedwith-runtime-message-timeout.ts's identical helper (just a hardcodednullfallback) — consolidated into one sharedwithDeadline, withwithRuntimeMessageTimeoutas a thin specialization.CapturePage's Force stop button used the mutation'sisPendingacross the whole component, so force-stopping session A also disabled the button while viewing session B; now scoped via the mutation's ownvariables.Declined as accepted debt (non-blocking, cosmetic):
recorder.tsstill exceeds the 400-line pre-commit warning threshold (628 lines) — a candidate for splitting the start/stop race-tracking into its own module, but out of scope for this fix.Test plan
bun run typecheck(apps/app, apps/server) passesbun run lint/bun run fallow— no new warnings in changed filesapps/server:test:capture,test:api,test:agent,trust-invariantsall passapps/app: full test suite passes🤖 Generated with Claude Code