Repository navigation
feat(stream): drop missions the producer discards - #592
Conversation
The addon enforces its save limits, such as the minimum mission time, by not saving a recording. A streamed recording already lives on the server, though, and was saved as partial once its producer went away, so missions the addon chose not to keep showed up anyway. Producers now end such a mission with discard_mission instead of end_mission. The server removes the streaming row and its files, along with partial segments earlier attempts of the same mission id left behind, releases the session slot and acknowledges it like end_mission. The mission id is remembered as finished, so a producer whose acknowledgement was lost gets resume_completed. start_mission is now acknowledged with protocol version 3, which tells the extension that discard_mission is understood; older servers never receive it. Bumps the extension module for the protocol constants.
…r the session ended Review fixes: - discard_mission is acknowledged only after the recording is removed: progress acks stop short of its number, as for end_mission. A failed removal closes the connection unacknowledged and marks the row failed, so the producer repeats the discard. - discard_mission without a session, naming a mission id, removes what the server kept of that mission: a partial saved after the resume grace or a supersede, or a row failed by a restart. The extension sends it when it finishes an unconfirmed discard and resume answers resume_unknown. - Discards remove only recordings not saved through end_mission. - Resume replies carry protocolVersion 3, so a producer knows the server takes a discard without a session. - Bump github.com/OCAP2/extension/v5 to the review-fix commit of OCAP2/extension#217; docs updated.
…e missions to recovered starts Second review round: - A session-less discard reserves the mission id while it removes the mission's recordings, so a start of it cannot slip in. A session of the mission waiting for its producer (same producerId) is dropped in its place; one a connection feeds is left alone and the discard is not acknowledged. A session being finalized is waited for, not exempted. - endStream marks stream_ended before the session leaves the registry, so a discard never takes a just-saved recording for an unended one. - A start marked "recovered" (resent from the extension's spool) no longer supersedes the producer's live mission. - Nil repository guard; tests for the waiting and fed sessions, a discard losing to a supersede, a failed session-less removal, and recovered starts; docs updated. Bump the extension to OCAP2/extension#217's latest commit.
…ions from supersede Third review round: - dropSession removes the earlier segments before the session's own row, so a failure leaves that row to be marked failed. - A recovered session is not superseded by its producer's next start either. - Separate logs for a session another producer recorded, and for a session still finalizing after the retries. - Tests: another producer's waiting session, a session-less discard waiting for an expiry save, a failed segment removal, the recovered flag in both directions; the close helpers fail on a read timeout. Docs updated.
…discard race tests deterministic Fourth review round: - A session superseded or expired before it received anything after its start is removed instead of saved as an empty partial. - The session discard ack carries seq only for resumable sessions, as for end_mission; a failed stream_ended mark is logged as an error. - Session-less discard settles its reservation explicitly. - Tests: the supersede race calls discardStream after the supersede, the expiry race waits on a hook instead of sleeping, a session-less discard during an end save keeps the recording, a start waits for a session-less discard's reservation, empty missions are dropped; the harness retires leftover sessions so grace timers never fire into later tests. - Docs: protocol 2 "or later"; bump the extension to OCAP2/extension#217's latest commit.
…dable session-less discards Fifth review round: - An abandoned session counts as empty when it accepted no envelope after its start, not by its sequence number, which unnumbered envelopes do not advance. - A session-less discard_mission whose payload does not name a mission is not acknowledged, so the producer keeps its spool; an empty payload is still acknowledged, as for end_mission. - dropSession and the empty-session drop release the slot before leaving the registry, as saveStream does. - Tests: an unreadable session-less payload, a session discard keeping the mission's saved recording, a failed removal of an empty session; hooks are once-safe gates. Bump the extension to OCAP2/extension#217's latest commit.
…overed start next to a partial save Sixth review round (nits only): - Assert protocolVersion 3 on resume_ok and resume_completed, which the extension reads to decide whether a pending discard can still go through. - The recovered-start test records on the live mission, so its supersede saves a partial, and checks both recordings. - "discarding mission" is logged before the removal, so a failure no longer reads as a success. - Bump the extension to OCAP2/extension#217's latest commit.
Seventh review round: - A resume marked "recovered" (the extension's spool recovery) makes the session it continues exempt from its producer's next start, like a recovered start. The flag is atomic: the registry reads it unlocked. - Log "stream: mission discarded" after a successful session discard; a duplicate discard seq no longer moves lastSeq back. - Docs: a legacy session's failed discard stays failed; the recovered resume. Bump the extension to OCAP2/extension#217's latest commit.
…ed recovered starts Eighth review round: - end_mission marked interrupted on a session that received nothing after its start (recovery after a crash right after the start) removes the session instead of saving an empty partial, like grace expiry and supersede; the mission id is remembered as finished. - A recovered start that takes over an existing session marks it recovered, like a recovered resume. - dropSession's comment and the docs say how a failed or repeated discard arrives. Bump the extension to OCAP2/extension#217's latest commit.
|
Ratatoskr reviewed this pull request. Changes requested on Finished 2026-10-27 01:20 UTC. |
Merging this branch will increase overall coverage
Coverage detailsCompared to commit 3cc8f9d (run #40229552255)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files:
|
fank
left a comment
fank
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — 2 blocking correctness findings.
Review details
Reviewed head 8277bdc6a4f6d5414edd4ca893b51b36dafe0e56.
-
[P2] Do not acknowledge a save when its discard-protection marker fails —
internal/server/handler_stream.go:540–546.MarkStreamEndedfailure is only logged; the outererrremains nil, so the session is removed as completed andend_missionis acknowledged. Its persisted row still hasstream_ended = 0. A session-less discard waiting for this save then selects that row throughUnendedStreamSegmentsand deletes the supposedly kept recording and its files. This breaks the new protection precisely on a transient database-write failure. Make the ended marker part of successful finalization, and do not publish completion/acknowledge success unless it is durable. Add a marker-write failure test alongsideTestStreamDiscard_WithoutSessionKeepsRecordingSavedMeanwhile. An isolated SQL-driver fault-injection probe confirms thatendStreamcurrently returnsendSavedafter the marker fails. -
[P2] Recheck recovered status when executing a queued supersede —
internal/server/stream_registry.go:198–205, withhandler_stream.go:399–403. The exemption is checked only while buildingbyProducer's snapshot. A new live start can select an old session, then recovery can successfully resume it withrecovered: true, and finally the already-scheduledexpireStream(old, 0)runs.expireStreamnever rechecksrecovered, so it closes the just-resumed connection and saves/drops the recovery despite its exemption. There is also a window betweentakeOverunlocking and the recovered flag being stored. Serialize the recovered transition with takeover and recheck it under the session lock before a forced supersede. A deterministic probe (select session → recovered resume on a real WebSocket → execute selected expiry) reproduces removal of the resumed session. The existing recovery tests complete the resume before starting the next mission, so they miss this interleaving.
Verification
- Checkout left unchanged; regression probes were added only to an isolated copy at
/tmp/ocap592-review-copy. - Passed:
/tmp/go1272/go/bin/go test ./internal/ingestion ./internal/storage ./internal/conversion ./cmd/ocap-webserver -count=1. - Passed:
/tmp/go1272/go/bin/go test ./internal/server -run 'TestStreamRegistry' -count=1. - Both isolated regression probes failed their expected-correctness assertions, confirming the findings:
cd /tmp/ocap592-review-copy && /tmp/go1272/go/bin/go test ./internal/server -run '^TestReview(RecoveredResumeAfterSupersedeSelection|EndRequiresDurableEndedMarker)$' -count=1. The marker probe bypasses finalization IO to isolate the marker failure; it is not a SQLite integration test. - Full local suite unavailable:
go test ./...initially could not find Go;/tmp/go1272/go/bin/go test ./...reached SQLite tests but failed because CGO was disabled;CGO_ENABLED=1 /tmp/go1272/go/bin/go test ./...could not build becausegccis absent. These environment failures are not findings against the PR. GitHub's test and coverage checks passed. - No frontend changes; frontend/UI and extension-to-server E2E tests were not run.
- GraphQL review-thread enumeration returned no threads (no further pages): none to resolve, continue, or skip.
…recovery resumed after a supersede chose it A save whose ended marker cannot be written is not acknowledged: a discard of the mission id would take it for unended. A recovered resume sets its flag under the session lock, and a supersede scheduled before it rechecks the flag there.
|
Both findings were real; fixed in 99a9b88.
0270035 bumps |
|
Ratatoskr reviewed this pull request. Approved on Finished 2026-10-27 01:56 UTC. |
fank
left a comment
fank
left a comment
There was a problem hiding this comment.
APPROVE — 0 blocking findings.
Review details
Reviewed head 02700358a78490f9ed75cae11e38037944cf4ee7 against base 3cc8f9d625952b27444a1f5549c6d04b15dc1bbe, including surrounding stream lifecycle, registry locking/reservations, database segment selection, artifact removal, and the new regression tests. No actionable blocking defects found. In particular, finalization and detached discard serialize on the session lock, and the durable ended marker is written before unregistering the saved session.
Verification
PATH=/tmp/go1272/go/bin:$PATH go test ./internal/server -run '^(TestWithPartialTag|TestStreamRegistryCompletedExpires|TestStreamRegistryReserveWaitsForConcurrentStart)$' -count=1— passed.PATH=/tmp/go1272/go/bin:$PATH go test ./internal/ingestion ./internal/storage— passed (cached).PATH=/tmp/go1272/go/bin:$PATH go test ./...— could not validate the full suite locally: SQLite tests fail because this environment defaults toCGO_ENABLED=0.PATH=/tmp/go1272/go/bin:$PATH CGO_ENABLED=1 go test ./...— build blocked becausegccis unavailable. The new database-backed streaming tests therefore remain locally unverified; no claim of a local full-suite pass.- GitHub's Go
testcheck for this head succeeded; the coverage report was still running at the final status check. git diff --check 3cc8f9d625952b27444a1f5549c6d04b15dc1bbe HEAD— passed. Checkout unchanged. No frontend changes; no UI or companion-extension E2E tests run.
Earlier discussions: GraphQL enumeration returned no review threads (pagination exhausted), so none were resolved, left open, or skipped.
|
Thank you for this — it closes a gap that has bitten us for real: recordings the addon deliberately refused to keep kept surfacing as And thank you for holding it to the standard the rest of the streaming code is held to — acknowledgements that only fire on durable outcomes, the Merging now, with the extension pin bumped to the merge commit on |
Summary
The addon enforces its save limits (such as the minimum mission time) by not saving a recording. A streamed recording already lives on the server, though, and was saved as
partialonce its producer went away. So missions the addon chose not to keep showed up anyway.The server side of
discard_mission(streaming protocol version 3):discard_missionends a mission in place ofend_mission. The server removes the recording'sstreamingrow and files, plus any partial or failed segments that earlier attempts of the same mission id left behind. It releases the session slot and acknowledges likeend_mission({"type":"ack","for":"discard_mission","seq":N}), then closes the connection.end_mission, it is acknowledged only once the removal succeeded: progress acks stop short of its number until then. If the removal fails, the connection is closed without an ack and the session's row (removed last) is markedfailed, so the producer repeats the discard.resume_completed.partial.discard_missionwhose payload names a mission id removes every recording of that mission not saved throughend_mission: thepartialsaved when the resume grace expired or the producer's next mission superseded it, or the row a restart markedfailed. Extension recovery uses it to finish an unconfirmed discard afterresume_unknown. With no session of the mission registered, it reserves the mission id while it removes the recordings, so a start of it cannot slip in. A session of the mission still waiting for its producer (sameproducerId) is dropped in its place under the session's lock, so a concurrent start or resume finds it done. One being saved is waited for. A payload that is present but does not name a mission is not acknowledged, so the producer keeps its spool; no payload at all is acknowledged, as forend_missionwithout a session. The producer id is checked against a waiting session only: recording rows do not store their producer, and mission ids are random and never exposed by the API. One a connection still feeds, or another producer's, is left alone and the discard is not acknowledged. Recordings marked as saved throughend_mission(stream_ended) are never removed:endStreamnow marks it before the session leaves the registry, closing the window where a just-saved recording looked unended. A save whose mark cannot be written does not count:end_missionis not acknowledged and the mission is not remembered as finished, so the producer resends the mission (whose recording replaces this one) or discards it.start_missionmarked"recovered": true(a mission the extension resends in full from its spool) no longer supersedes the producer's other missions, and the producer's next start does not supersede it. The producer may be recording its next mission meanwhile, and either side used to be saved aspartialand resent. Aresumemarked"recovered": trueexempts the session it continues the same way. The flag is set under the session's lock as the resume takes the session over, and a supersede that chose the session before the resume rechecks it there, so it never closes a recovery that just resumed.start_mission, or ended as interrupted with nothing in it (recovery after a crash right after the start), is removed instead of saved as an emptypartial. That covers a start the producer gave up waiting for, and a mission registered just before the game moved on.start_missionis now acknowledged withprotocolVersion: 3, and every resume reply carries it. This tells the extension it may senddiscard_mission, with or without a session. Older servers never receive it.docs/streaming-architecture.mdgains a "Discarding a mission" section.Bumps
github.com/OCAP2/extension/v5to the latest commit of OCAP2/extension#217 for the protocol types. Before merging, bump it to that PR's merge commit on extensionmain.Companion addon PR: OCAP2/addon#132.
How to test
go test ./...New tests in
internal/server/handler_stream_discard_test.go:resume_completedend_missionTestStreamResume_RecoveredStartLeavesLiveMission)discardStreamafter the supersede), and the session-less repeat removes the partial; a session-less discard finds a session being saved (hook, no sleeps); a session-less discard during an end save keeps the saved recording; a start of the mission waits for a session-less discard's reservationTestStreamResume_AbandonedEmptyMissionIsDropped)resume_unknown, and a session-less discard then removes it (TestStreamDiscard_EndNotAcknowledgedWithoutEndedMark)TestStreamResume_RecoveredResumeAfterSupersedeChoseIt)TestStreamResume_SlotsReleasedOnEveryEnding)End-to-end: I ran a binary built from this branch with streaming enabled, then the extension's
OCAP_TEST_WEB_URLintegration tests against it (TestWebIngestionIntegration,TestWebResumeIntegration,TestWebCrashRecoveryIntegration,TestWebDiscardIntegration). All pass. The log showsstream: mission discarded, and the data directory holds only the kept mission.Checklist
go test ./...passescd ui && npm testpasses (if frontend changed): no frontend changediscard_mission;protocolVersion: 3includes everything in version 2