feat(websocket): discard missions the addon does not save by smitt14ua · Pull Request #217 · OCAP2/extension

smitt14ua · 2026-10-26T19:48:37Z

Summary

The addon enforces minMissionTime only by not sending :MISSION:SAVE:. With the websocket backend that is not enough: the web server already holds the streamed mission and saves it as partial once the producer goes away, and spool recovery ends it as interrupted. Short missions the addon chose not to keep ended up on the web server anyway.

This adds a way to end a recording without saving it.

`:MISSION:DISCARD:`

New lifecycle command, async like :MISSION:SAVE:. It shares the save's in-flight guard, since both end the mission. It answers with a :MISSION:DISCARDED: callback: ["ok"], ["kept"] (the storage keeps what it recorded) or ["error", message].
Backends opt in through the new optional storage.Discardable interface. memory drops its data without exporting. websocket tells the server to drop the recording. postgres and sqlite (not selectable as storage yet) would keep what they wrote (kept). A backend that ends the mission but cannot drop it returns storage.ErrMissionKept, which is also reported as kept.

`:MISSION:START:` answers failures

:MISSION:START: now answers :MISSION:FAILED: with the error when the mission does not start, besides :MISSION:OK: on success. The addon waits for one of the two before it records.

Protocol version 3: `discard_mission`

start_mission now advertises protocolVersion: 3. discard_mission is sent only to servers that acknowledge version 3 or later.
discard_mission is a numbered envelope acknowledged like end_mission. Its payload names the mission (missionId, producerId). It survives reconnects like any other envelope. A resume_completed stands in for a lost discard ack, as it already does for end_mission.
With an older server (protocol 2 or legacy), DiscardMission returns storage.ErrMissionKept, deletes the spool and closes the connection. That server then saves what it got as partial, which is the behavior before this change. The spool is deleted so recovery does not resend the mission and get it saved a second time.

Recovery

An unconfirmed discard keeps the spool, which now ends with discard_mission. Recovery sees that last record and resends only the discard, never the mission data:
- resume_ok: send discard_mission on the resumed session, wait for the ack.
- resume_unknown from a protocol 3 server (resume replies now carry the version): send discard_mission without a session. The server drops what it kept of the mission, such as the partial it saved when the resume grace expired, or the row a restart marked failed.
- resume_completed, or resume_unknown from an older server: delete the spool.
- A server that does not answer resume: keep the spool and pause recovery, as for any other spool.
A start_mission resent in full from the spool (resume_unknown) now carries "recovered": true (new StartMissionPayload.Recovered), and so does recovery's resume (new ResumePayload.Recovered), so a session it continues is not superseded either. A protocol 3 server does not take it for the producer's next mission, so it no longer saves the producer's live mission as partial. The producer may be this process, or another process sharing spoolDir. This was already possible on main for an unconfirmed save.

Storage lives as long as the process (pre-existing bug)

The addon sends :SYS:INIT: and :STORAGE:INIT: on every mission load, and every :STORAGE:INIT: built a new storage backend without closing the old one. A mission the game moved on from stayed open in the leaked backend: with websocket, its socket stayed up, the web server showed it as streaming until the game server exited, and its spool was never recoverable meanwhile. :STORAGE:INIT: now initializes storage once and answers later calls with :STORAGE:OK: again. A later call means the next mission loaded, so it also ends a mission still open (new optional storage.Abandonable, below), before it answers, so the new mission's start cannot be taken for the abandoned one. A mission a save or discard dispatched before is still ending is left to it, so a slow server does not hold up the next mission. A :MISSION:START: dispatched before that load (one that waited behind a save of the previous mission) is answered with :MISSION:FAILED: instead of opening a mission nothing records; the check uses the monotonic clock. :MISSION:START:'s queue no longer blocks: since a start can wait behind a save, a full queue now refuses the next start (the addon reports it) instead of freezing the game's thread. It does not wait for the next recording to start, which the addon now defers until auto-start conditions are met.

Abandoned missions (side fix)

When the game moves to the next mission without saving or discarding (for example #missions), StartMission replaced the spool without closing it. The old spool kept its file lock until the process exited, and the next mission's start_mission went out on the socket that still fed the old one. The web server rejects that ("mission already active"), so the extension had to reconnect. Now the new AbandonMission runs when the next mission loads (and StartMission runs it too, as a backstop). It ends the abandoned mission as interrupted on its own connection, waiting up to 30 s. The server saves it as partial, as its supersede did before, and the spool is removed. The next mission then dials a fresh connection. Only an unconfirmed end leaves the spool for recovery. A legacy server (no resume) ignores interrupted and would save the mission as complete, so with one the connection is closed instead, and it saves the mission as partial on the disconnect, as before.

An abandoned mission that recorded nothing after its start (for example one the addon registered just before the mission unloaded) is discarded instead, when the server supports it, so no empty partial is published.

Abandoned recordings below the addon's save limits

Admins often load a mission ahead of a game, slot in, and switch with #missions or #restart. On #restart the next mission loads before the addon's deferred discard of a short recording can run, so each such run was saved as partial. :MISSION:START: now takes an optional fourth argument, discard: until :MISSION:KEEP: arrives, AbandonMission discards the recording instead of ending it as interrupted (new storage.Abandonable.SetDiscardAbandoned, reset at each start). OCAP2/addon#132 sends discard with each registration and :MISSION:KEEP: once the recording reaches minMissionTime. :MISSION:KEEP: is synchronous and takes only the backend's short lock, never the mission lock a save may hold. A server below protocol 3 still gets the interrupted end, as before.

Smaller fixes

:MISSION:START: takes an optional registration token as its third argument and echoes it in :MISSION:OK: / :MISSION:FAILED:. The addon uses it to tell a late answer to an earlier mission's start from its own.
Whether the server can discard is taken from what it last announced: the start ack, a resume reply, or the ack of a restarted start. If a reconnect's resume is answered by a server at protocol 2, a discard_mission still waiting for its ack is refused at once (kept) instead of waiting out the save timeout. (The writer may already have resent the logged envelope to that server, which ignores it as an unknown type.) (A server without resume support never answers the resume, so there the discard waits out the timeout and recovery keeps the spool, as for any unanswered resume.) Recovery also gives up a discard when the resume reply comes from a server below version 3.
With a server below version 3, which ignores recovered, the running process resends its own missions in full only while it records none.
A start whose ack never came also closes its connection, so a server that started it late sees its producer leave.
:MISSION:START: waits until a save or discard already dispatched has ended the mission in the backend (not for the upload after a save). Each runs on its own goroutine, so a discard dispatched just before the next start could otherwise reach the backend after it and drop the new mission.
A discard whose discard_mission never made it into the log does not keep the spool, which would have been resent in full. Nor does one whose last records fail to reach the disk when the spool closes (for example a full disk): a spool kept to finish a discard is flushed and synced, and on failure removed while its lock is held, so recovery cannot resend the mission without its discard.
Pre-existing race: spool.remove() released the spool's lock before deleting its files, so recovery could take a spool being removed (on Windows the delete then failed) and resend a mission that was saved, discarded or never started. A spool being removed is now marked dropped and its segments deleted while the lock is held, and the marker is deleted last. Recovery deletes a dropped spool it finds, and drops the spools it finishes the same way before releasing them.
:STORAGE:INIT: now answers :STORAGE:ERROR: when storage cannot start. The addon on main already logs it; with feat(recorder): honor save limits with streaming storage addon#132 it also stops waiting for storage to record.
With a legacy server, an abandoned mission is disconnected and its spool deleted (that server saves it as partial on the disconnect). On main the spool stayed and could be recovered after a server upgrade, as a second partial.
A start whose ack never came forgets the mission and removes its spool, so the next start does not end it as abandoned.
The save and discard workers clear the in-flight guard before the callback, so the addon may save or discard again as soon as it hears the previous one ended.
:MISSION:START: answers :MISSION:FAILED: when it panics too.

Companion PRs

Web (server side of discard_mission): feat(stream): drop missions the producer discards web#592
Addon (sends :MISSION:DISCARD: and defers :MISSION:START: to recording start): feat(recorder): honor save limits with streaming storage addon#132

Merge this one first; the web PR pins this branch's commit and will be bumped to the merge commit.

Compatibility

Additive protocol change. Servers keep receiving only what they understand.
pkg/streaming gains ProtocolVersionDiscard, TypeDiscardMission, DiscardMissionPayload and StartMissionPayload.Recovered. Older servers ignore recovered and keep superseding, as before. TypeDiscardMission is not in AllMessageTypes, because saved missions never carry it.
Without the addon change, nothing sends :MISSION:DISCARD:, discard or :MISSION:KEEP:, and behavior is unchanged. The current addon ignores the new :MISSION:FAILED: callback.

How to test

go test -race ./...

New tests in internal/storage/websocket/discard_test.go:

discard drops the recording, names the mission and deletes the spool; the next mission records normally
no-op without an active mission
lost discard ack resolved by resume_completed
server with protocol 2: no discard_mission sent, ErrMissionKept, spool deleted, connection closed; legacy server: no discard_mission sent, ErrMissionKept
server downgraded mid-mission: no discard_mission sent after the resume; recovery of a discard on such a server gives up without sending it
abandoned mission with nothing recorded is discarded (ended as interrupted on a server without discard); an abandoned mission whose end is not confirmed keeps its spool
abandoned mission below the addon's save limits is discarded; past them (SetDiscardAbandoned(false)) and in the next mission it is ended as interrupted; on a server without discard it is ended as interrupted; :MISSION:START: with discard and :MISSION:KEEP: reach the backend
an unconfirmed discard whose final spool flush fails removes the spool, and recovery never resends the mission
a discard waiting while the server is replaced by an older one is refused without waiting for the save timeout
a dropped spool is deleted by recovery, never resent
:STORAGE:INIT: reuses the backend, answers each call and abandons a mission left open; a failure answers :STORAGE:ERROR:; the registration token is echoed; a start waits for a dispatched discard
AbandonMission ends a mission as interrupted, and disconnects from a legacy server instead; with a server below protocol 3, this process' own spooled mission is not resent while it records another
recovery of an unconfirmed discard: only the discard is resent (resume_ok); a partial the server saved after the grace is dropped by a session-less discard; the same after a web restart; an older server is sent nothing and the spool is deleted; a resume timeout keeps the spool and pauses recovery
spooledDiscard across rotated segments
an abandoned mission is ended as interrupted before the next one starts, leaving no spool; a mission recovered in full while this producer records another is marked recovered and leaves the live one alone; a start without an ack leaves no mission behind. The fake server in resume_test.go now behaves like the web server here: it saves a producer's unfinished missions as partial when another starts (unless recovered), and rejects a second start on a connection that feeds a mission.

Also covered: memory.DiscardMission (including a no-op after a save), the :MISSION:DISCARD: worker (ok, kept, kept via ErrMissionKept, error, panic, rejected during a save), and :MISSION:FAILED: for a start that fails.

End-to-end against a locally built web server from the companion PR (OCAP_TEST_WEB_URL=... go test -run TestWeb ./internal/storage/websocket/): TestWebIngestionIntegration, TestWebResumeIntegration, TestWebCrashRecoveryIntegration and the new TestWebDiscardIntegration pass. The last one checks through /api/v1/operations that the discarded mission leaves no operation and that the next mission is saved; I also checked the data directory by hand. (TestWebCrashRecoveryIntegration assumes the server's default resume grace: with a grace shorter than its ~3 s recovery delay, its chunk count check fails, on main too.)

The addon only enforces its save limits (minimum mission time) by not sending :MISSION:SAVE:. With the websocket backend that is not enough: the web server already holds the streamed mission and saves it as partial once the producer goes away, and spool recovery ends it as interrupted. Short missions the addon chose not to keep ended up on the web server anyway. - :MISSION:DISCARD: ends the recording without saving it and answers with a :MISSION:DISCARDED: callback (ok, kept, error). Backends opt in through storage.Discardable; memory drops its data, postgres and sqlite keep what they wrote. - Protocol version 3 adds discard_mission. The producer advertises it in start_mission and only sends it to servers that acknowledge version 3. Older servers keep the previous behavior: the mission is abandoned and the server saves it as partial. - A discard that is not confirmed keeps the spool, which now ends with discard_mission. Recovery then only repeats the discard instead of resending the mission; a server that no longer holds the mission, or cannot resume, has nothing left to discard. - resume_completed stands in for a lost discard_mission ack as it does for end_mission. - A mission abandoned by the next StartMission closes its spool, so recovery can end it while the process runs instead of after it exits.

…e missions safe from recovery Review fixes: - discard_mission carries the mission and producer ids. When recovery of an unconfirmed discard gets resume_unknown from a protocol 3 server, it sends the discard without a session, so the server drops a partial recording it saved after the producer went away. Older servers are not sent it. - A spool whose resume handshake timed out is kept and recovery paused, instead of deleting a discard the server never heard. - Recovery does not resend this process' own mission in full while it records another: the server would take that start for the producer's next mission and save the live one as partial. It waits until the mission ends. - A server that cannot discard reports "kept" (storage.ErrMissionKept) instead of "error". - :MISSION:START: answers :MISSION:FAILED: when the mission does not start, so the addon stops waiting for it. - memory DiscardMission is a no-op without a mission; the discard worker flushes OTel like the save worker; README recovery notes updated.

…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.

…rk resent starts recovered Second review round: - An abandoned mission (the game moved on without save or discard) is now ended as interrupted before the next mission starts, on its own connection, and the next mission dials a fresh one. Closing its spool for recovery let another process sharing spoolDir resend it in full, and the server took that start for the producer's next mission and saved the live one as partial. The spool stays for recovery only if the end is not confirmed within 30 s. The own-process deferral is gone. - A start resent in full from the spool carries "recovered": true, so a protocol 3 server does not supersede the producer's live mission with it. This covers unconfirmed saves too, and siblings sharing spoolDir. - A start whose ack never came forgets the mission and removes its spool, so the next start does not end it as abandoned. - The save and discard workers clear the in-flight guard before the callback, so the addon may act on it at once. - :MISSION:START: also answers :MISSION:FAILED: when it panics. - Doc fixes: discard numbering, resume_completed, README recovery notes. - The fake server rejects a second start on a connection that feeds a mission, as the web server does, and honors recovered starts.

…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.

…ools under their lock Third review round: - Whether the server can discard comes from what it last announced: the start ack, a resume reply or a restarted start's ack. A server replaced by an older one mid-mission is never sent discard_mission, and recovery gives up a discard when resume comes from a server below version 3. - An abandoned mission that recorded nothing after its start is discarded instead of saved as an empty partial. - spool.remove() deletes the segments before releasing the lock: recovery could take a spool being removed and resend a mission that was saved, discarded or never started (on Windows the delete then failed). - :STORAGE:INIT: answers :STORAGE:ERROR: when storage cannot start. - abandonTimeout is a var for tests; tests for the downgrade, an empty or unconfirmed abandoned mission, and :MISSION:FAILED: on a panic. The fake server ignores discard_mission when it predates it, like the web server.

…he capture quiet Third review round: - The wait for the extension's storage has no timeout: it ends when the mission context is built, when :STORAGE:ERROR: arrives (now flagged by initSession, and sent by OCAP2/extension#217) or when the mission ends. Players are notified after 60 s. A slow storage used to lose the whole recording after 60 s. - A mission that ends while waiting for storage does not register at all. - In the registration waiter a mission end is checked before a failed start, so players are not told the start failed. - newMission gives up when the extension rejects :MISSION:START:. - stopRecording before the capture began only holds it back: no "stopped" notification or time state. - Init's auto-start does not start while a registration is pending, so it cannot undo an admin's pause. - The in-game test refuses to run with final functions; the tests README says to allow recompiling. Comments and docs updated.

…wngrade, echo a registration token Fourth review round: - :STORAGE:INIT:, which the addon sends on every mission load, built a new backend each time and leaked the old one; a mission the game moved on from stayed open in it. Storage is now initialized once and later calls are answered with :STORAGE:OK: again, so the next StartMission ends the abandoned mission. - :MISSION:START: echoes an optional registration token (third argument) in :MISSION:OK: and :MISSION:FAILED:. - A reconnect that finds the server replaced by one below protocol 3 refuses a discard still waiting for its ack instead of waiting out the save timeout. - A spool being removed is marked dropped and its segments deleted under its lock; recovery deletes a dropped spool and marks the spools it finishes before releasing them. - Against a server below protocol 3, own missions are resent in full only while no other records. A failed start closes its connection. The start ack's protocol is stored before reconnects can record a newer one. - Tests for each; the fake server exempts recovered missions from supersede, like the web server.

…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.

…ff a paused registration Fourth review round: - Each registration sends a token as :MISSION:START:'s third argument (echoed by OCAP2/extension#217); :MISSION:OK:/:MISSION:FAILED: with another token, or with nothing pending, are ignored. A late OK from the previous mission made the next one record into the old registration. - Init's auto-start does not start while a registered mission is held paused, which undid an admin's pause. - A recording gives up when no extension is running (no :SYS:VERSION: after 60 s); a start request during registration notifies players. - The in-game test runs the real stopRecording. Setting text, tests README and init comment corrected.

…t after a dispatched discard Fifth review round: - A later :STORAGE:INIT: (the next mission loaded) ends a mission still open through the new optional storage.Abandonable. Waiting for the next recording to start, which the addon now defers, left it open. - An abandoned mission on a legacy server is disconnected instead of ended: such a server ignores "interrupted" and would save it as complete. - :MISSION:START: waits for a save or discard already dispatched, so a discard cannot reach the backend after the next start and drop it. - A discard whose envelope never made it into the log does not keep the spool. Removing a spool deletes the dropped marker last, and recovery drops the spools it finishes under their lock too. - Tests for these and for the own-mission deferral on old servers; cmd tests no longer write Logger, which the package init reads.

…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.

… the backend Sixth review round: - A repeated :STORAGE:INIT: ends the abandoned mission before it answers :STORAGE:OK:, so the new mission's start cannot reach the backend first and be abandoned in its place. - :MISSION:START: waits until a dispatched save or discard has ended the mission in the backend, not for the upload after a save. - The downgrade test waits for the discard to be logged instead of sleeping; Abandonable's doc mentions the empty-mission discard.

…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.

…being ended at load Seventh review round: - :STORAGE:INIT: records when the mission loaded; a :MISSION:START: dispatched before it, which waited behind the previous mission's save, is answered with :MISSION:FAILED: instead of opening a mission nothing records. - AbandonMission leaves a mission a save or discard is still ending to it, instead of holding up the next mission's :STORAGE:OK: until that server answers. - Recovery's resume carries "recovered" (new ResumePayload.Recovered), and the own-mission check against old servers is repeated right before a full resend's start. - The save worker reads the export before the next start can reset it. - Comments and README: the abandon happens at mission load; an unconfirmed discard needs a healthy spool.

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.

…s monotonically Eighth review round: - :MISSION:START: is no longer a blocking queue. A start can wait behind the previous mission's save, and with the addon's re-registration a third start filled the queue and froze the game's thread; a full queue now refuses the start, which the addon reports. - The unloaded-mission check compares event times with Before, on the monotonic clock, so a wall clock stepping back cannot refuse a valid start. - Storage init skips the abandon only while a dispatched save or discard is ending the mission, and otherwise waits for the mission lock; a slow start could make the old time-bounded wait give up and leave a mission open. - Tests: :STORAGE:OK: follows the abandon; a dispatched save is left to end the mission. ErrMissionKept's doc.

…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.

fank · 2026-10-27T01:12:16Z

Ratatoskr reviewed this pull request.

Changes requested on 9cac5c36. See the review.

Finished 2026-10-27 01:20 UTC.

fank

REQUEST_CHANGES — 1 confirmed correctness finding.

Review details

Reviewed head 9cac5c36cc3d56464c60298761552d82dc948937.

[P2] A failed final spool flush resurrects a discarded mission

internal/storage/websocket/websocket.go:437–440 checks sp.healthy() before closing the spool, then ignores sp.close()'s error. The discard can still be buffered at that point. If its final flush fails (for example, disk full), the retained spool ends with ordinary mission data rather than discard_mission. Recovery consequently resends the mission and saves it as interrupted—the opposite of the requested discard.

Confirmed with a review-only Go test overlay, leaving the checkout unchanged: flush the start and three states, inject an I/O failure on the spool's next flush, suppress discard delivery, and let DiscardMission time out. The retained spool's discard sequence was 0. After simulated server state loss and recovery, the fake server reported 2 starts, completed=true, discarded=false; the assertion that recovery discards the mission failed.

Required change: confirm the discard record is successfully flushed before making its spool recoverable. If persistence fails, ensure the remaining mission data cannot be replayed (drop/mark it while still holding the spool lock, or preserve discard intent separately). Add a regression test for this final-flush failure, not just append failures or a spool already marked unhealthy.

Verification

Using Go 1.26.6 from /home/fkinder/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.26.6.linux-amd64/bin on PATH:

go test ./internal/storage/websocket ./internal/storage/memory ./internal/dispatcher ./internal/parser ./pkg/streaming — passed.
go test -overlay /tmp/pr217-overlay.json -run '^TestReviewDiscardFinalFlushFailure$' -v ./internal/storage/websocket — reproduced the defect above.
go test ./... — non-CGo packages passed; recorder/interface packages could not build with CGo disabled.
CGO_ENABLED=1 go test -race ./... — unavailable: gcc is not installed. This is an environment limitation, not a separate PR finding.
Real-web-server integration tests were not exercised; no companion server was configured.

Earlier discussions

GraphQL enumeration returned no review threads (no further pages), so there were no discussions to reply to or resolve.

…r keep a spool whose discard missed the disk :MISSION:START: takes an optional fourth argument, "discard": if the game moves on from the mission (#missions, #restart) before :MISSION:KEEP:, the next mission load discards it instead of ending it as interrupted, so the web keeps no partial of it. The addon sends :MISSION:KEEP: once the recording reaches its minimum mission time. A spool kept to finish an unconfirmed discard is synced on close; if its last records do not reach the disk, it is removed under its lock instead, so recovery cannot resend the mission without its discard.

…sion#217

smitt14ua · 2026-10-27T01:52:57Z

Fixed the P2 in 6e18784 (final flush failure resurrects a discarded mission).

A spool kept to finish an unconfirmed discard is now closed through spool.keepIntact: it flushes and syncs the last records, and if that fails, or spooling had already failed, it marks the spool dropped, deletes its segments and removes it while still holding the spool lock. Recovery can no longer pick up a spool that ends in mission data, so it never resends the mission without its discard_mission.
Regression test TestUnconfirmedDiscardWithFailedFinalFlushRemovesSpool: start plus three states flushed, an I/O failure injected into the final flush, the discard never delivered, and DiscardMission times out. The spool is gone, and after several recovery passes the server has seen one start and no completion. Without the fix it fails: the spool is kept.
go test -race ./... passes.

The same commit also adds :MISSION:KEEP: and an optional discard fourth argument to :MISSION:START:. A recording the game moves on from (#missions, #restart) before it reaches the addon's minimum time is discarded instead of being kept as partial. See the updated description, "Abandoned recordings below the addon's save limits". Checked end to end with a real web server: 20 s of recording then #restart leaves nothing; 80 s past the 1 min minimum is kept.

fank · 2026-10-27T01:53:23Z

Ratatoskr reviewed this pull request.

Approved on 6e187844. See the review.

Finished 2026-10-27 02:00 UTC.

github-actions · 2026-10-27T01:54:05Z

Merging this branch changes the coverage (1 decrease, 1 increase)

Impacted Packages	Coverage Δ	🤖
github.com/OCAP2/extension/v5/internal/storage/memory	99.26% (+0.03%)	👍
github.com/OCAP2/extension/v5/internal/storage/websocket	90.54% (-0.99%)	👎

Coverage details

_{Compared to commit 0ebd39e (run #39317860680)}

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/OCAP2/extension/v5/cmd/ocap_recorder/main.go	0.00% (ø)	0	0	0
github.com/OCAP2/extension/v5/cmd/ocap_recorder/mission_save.go	0.00% (ø)	0	0	0
github.com/OCAP2/extension/v5/cmd/ocap_recorder/storage.go	0.00% (ø)	0	0	0
github.com/OCAP2/extension/v5/internal/storage/memory/memory.go	100.00% (ø)	239 (+9)	239 (+9)	0
github.com/OCAP2/extension/v5/internal/storage/storage.go	0.00% (ø)	0	0	0
github.com/OCAP2/extension/v5/internal/storage/websocket/connection.go	92.23% (+0.27%)	296 (+10)	273 (+10)	23	👍
github.com/OCAP2/extension/v5/internal/storage/websocket/recover.go	85.90% (-1.68%)	234 (+65)	201 (+53)	33 (+12)	👎
github.com/OCAP2/extension/v5/internal/storage/websocket/spool.go	81.28% (-5.02%)	203 (+57)	165 (+39)	38 (+18)	👎
github.com/OCAP2/extension/v5/internal/storage/websocket/websocket.go	96.40% (+2.25%)	250 (+79)	241 (+80)	9 (-1)	👍
github.com/OCAP2/extension/v5/pkg/streaming/messages.go	0.00% (ø)	0	0	0

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:

github.com/OCAP2/extension/v5/cmd/ocap_recorder/main_test.go
github.com/OCAP2/extension/v5/cmd/ocap_recorder/mission_save_test.go
github.com/OCAP2/extension/v5/cmd/ocap_recorder/storage_test.go
github.com/OCAP2/extension/v5/internal/storage/memory/memory_test.go
github.com/OCAP2/extension/v5/internal/storage/websocket/discard_test.go
github.com/OCAP2/extension/v5/internal/storage/websocket/resume_test.go
github.com/OCAP2/extension/v5/internal/storage/websocket/web_integration_test.go
github.com/OCAP2/extension/v5/internal/storage/websocket/websocket_test.go

fank

APPROVE — 0 blocking findings.

Review details

Reviewed head 6e18784428921bf20ebcc5bc22d6f899997c28c4 against base 0ebd39eb0267569fa691c13f0a52db284eedf28a, including lifecycle dispatch ordering, backend reuse, discard/abandon behavior, protocol downgrade handling, and spool removal/recovery. No concrete defect requiring correction found.

Verification (Go 1.26.0 from /tmp/ocap216-review/go/bin/go):

CGO_ENABLED=0 /tmp/ocap216-review/go/bin/go test ./internal/storage/websocket ./internal/storage/memory/... ./pkg/streaming — passed.
CGO_ENABLED=0 /tmp/ocap216-review/go/bin/go test -count=3 ./internal/storage/websocket — passed all three runs.
CGO_ENABLED=1 /tmp/ocap216-review/go/bin/go test -race ./... — could not build: gcc is unavailable. Docker is also unavailable to this user (daemon socket permission denied).
CGO_ENABLED=0 /tmp/ocap216-review/go/bin/go test ./... — all other listed packages passed, but the command failed because the recorder and a3interface tests require CGO definitions. This is a local verification limitation, not evidence of a PR defect.
GitHub test, build, and coverage checks passed for the reviewed head.
Companion-web E2E tests were not exercised against a local web server; the focused suite's conditional integration tests do not establish E2E verification.

Earlier discussions: GraphQL enumeration returned no review threads (pagination complete); none required replies or resolution.

fank · 2026-10-27T09:39:40Z

Thank you for the port across all three layers — protocol, spool and recovery. The downgrade handling (ackRefused → spool kept, the old server does exactly what it always did) and keepIntact's flush-and-fsync are the kind of details that make this safe to deploy incrementally against mixed server versions.

Merging this first so the web pin can move to the merge commit.

* feat(stream): drop missions the producer discards 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. * fix(stream): acknowledge discards only once removed, finish them after 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. * fix(stream): lock out starts during a session-less discard, leave live 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. * fix(stream): keep the discarded row until last, exempt recovered sessions 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. * chore(deps): bump extension to the latest commit of OCAP2/extension#217 * fix(stream): drop abandoned missions that recorded nothing, make the 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. * fix(stream): judge empty sessions by what they received, refuse unreadable 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. * style: gofmt the test gate * test(stream): pin the protocol version on resume replies, cover a recovered 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. * fix(stream): exempt a session recovery resumed from supersede 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. * fix(stream): drop an interrupted end of an empty mission, mark repeated 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. * fix(stream): acknowledge an end only once it is marked ended, keep a 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. * chore(deps): bump OCAP2/extension to the latest commit of OCAP2/extension#217 * chore(deps): pin extension to the merge commit of OCAP2/extension#217 --------- Co-authored-by: Florian Kinder <florian.kinder@fankserver.com>

* feat(recorder): honor save limits with streaming storage minPlayerCount and minMissionTime only held because nothing reached storage until :MISSION:SAVE:. The websocket storage streams to the web server as it records, so missions that never met the limits were published anyway: missions that stayed below minPlayerCount showed up as empty partial recordings, short ones as partial recordings. - :MISSION:START: is no longer sent at mission load. The mission registers with the extension when a recording starts, so a mission that never meets the auto-start conditions leaves nothing in storage. A second start while the registration is pending no longer registers the mission twice. - When the mission ends (MPEnded/Ended with saveMissionEnded) before the recording reached minMissionTime, the recording is discarded with the new :MISSION:DISCARD: command instead of being left to the storage. The discard waits a frame, so a mission script exporting the short recording through OCAP_exportData on the same frame still saves it. - The automatic end-of-mission save passed its message as the winning side; it is now passed as the message. The PFH removal and state reset of exportData are shared with the discard as removeCaptureHandlers and resetRecording. * fix(recorder): start the capture only once the mission is registered, never orphan a registration Review fixes: - "Recording started.", "Mission has started!", the start notification, diary record and time state follow the capture (new OCAP_recorder_fnc_announceRecording). With the deferred :MISSION:START: they were sent before the capture ran, and the events were dropped. - The capture waits for :MISSION:OK: or the extension's new :MISSION:FAILED: however long it takes, and further start requests are ignored meanwhile without notifying players. A timeout let a late OK leave the extension with a mission nothing recorded, then saved as partial. A failed start, or storage that never initializes, sets OCAP_recorder_recording back to false. - A mission that ends while it registers discards the registration instead of starting a capture after the end. - A discard the extension rejects is logged as a warning. - Setting and doc texts: the discard happens only with saveMissionEnded; the minimum extension; registration log in seconds. Tests README section moved after the servicing suite's notes. * fix(recorder): honor pauses during registration, announce a start once Second review round: - Init's auto-start waiter no longer calls startRecording again for a recording that already captures, which announced a manual start twice. - A pause while the mission registers holds the capture back until recording resumes; a start request while it registers undoes the pause. A start that began the capture in the frame before the waiter runs is not repeated. - A mission registered with the extension whose capture never began is discarded at mission end without telling players, whatever saveMissionEnded says (discardRecording handles it). - Players are notified when the extension has not confirmed the mission after 30 s; newMission clears the pending flag when it cannot register. - Docs: OCAP_recorder_startTime signals the capture for mission scripts; the one-frame discard delay is part of the manual check. * fix(recorder): wait for storage until it fails, keep a pause before the capture quiet Third review round: - The wait for the extension's storage has no timeout: it ends when the mission context is built, when :STORAGE:ERROR: arrives (now flagged by initSession, and sent by OCAP2/extension#217) or when the mission ends. Players are notified after 60 s. A slow storage used to lose the whole recording after 60 s. - A mission that ends while waiting for storage does not register at all. - In the registration waiter a mission end is checked before a failed start, so players are not told the start failed. - newMission gives up when the extension rejects :MISSION:START:. - stopRecording before the capture began only holds it back: no "stopped" notification or time state. - Init's auto-start does not start while a registration is pending, so it cannot undo an admin's pause. - The in-game test refuses to run with final functions; the tests README says to allow recompiling. Comments and docs updated. * fix(recorder): match registration answers by token, keep auto-start off a paused registration Fourth review round: - Each registration sends a token as :MISSION:START:'s third argument (echoed by OCAP2/extension#217); :MISSION:OK:/:MISSION:FAILED: with another token, or with nothing pending, are ignored. A late OK from the previous mission made the next one record into the old registration. - Init's auto-start does not start while a registered mission is held paused, which undid an admin's pause. - A recording gives up when no extension is running (no :SYS:VERSION: after 60 s); a start request during registration notifies players. - The in-game test runs the real stopRecording. Setting text, tests README and init comment corrected. * feat(recorder): OCAP_recordingStarted event, start requests idempotent once capturing Fifth review round: - Raise the server-local CBA event OCAP_recordingStarted once the capture runs (and on each resume): OCAP_record now starts a fresh recording asynchronously, so mission scripts can wait for it. Documented on OCAP_record and in the lifecycle docs. - A start request while the capture runs only reports "already recording"; whether to start the capture is decided by startTime instead of the frame number, which restarted it within its first 10 frames. - A pause while storage initializes does not register the mission until recording resumes; the 30 s warning stays quiet while paused. - The registration token is two random numbers. The in-game test confirms registrations atomically and checks a repeated start. Headers of the new functions name their authors; docs no longer cite postgres/sqlite. * fix(recorder): hold a pause before the capture against auto-start; document the test build Sixth review round: - A pause before the capture began is held (OCAP_recorder_heldByPause): auto-start leaves it alone and "Mission has started!" keeps waiting; the next start request lifts it. A pause during storage init otherwise looked like "never started", and auto-start undid it. - The 60 s storage notice stays quiet while paused. - tests/README.md: the suites need a test build with DISABLE_COMPILE_CACHE, since CBA compiles PREPed functions final whatever allowFunctionsRecompile says. The save limits test checks for it, rejects a recording already started, and checks the held pause. I ran it on a local dedicated server with such a build and the branch's extension: all assertions pass. - Docs: an export must come in the frame of the mission end; wording. * test(recorder): reset the intercepted commands before the last save limits check * fix(recorder): re-register after an unanswered minute, say when a late export finds a discard Seventh review round: - A start request while a registration has gone unanswered for a minute (an older extension, a lost callback) registers again with a new token instead of waiting forever. - An OCAP_exportData that arrives after the mission-end discard tells players the recording was discarded, not that it never started; the event's doc says to export before or in the frame of the mission end. - The empty-server auto-restart leaves a held pause alone, as auto-start does; OCAP_pause documents it. - No "failed to start" notice for a paused start, nor the 30 s notice after the mission ended. Docs updated. * fix(recorder): resend only a :MISSION:START: that was sent, document the suites' CBA stubs Eighth review round: - The one-minute re-registration counts from when newMission sent the start, so a start request during a long storage init no longer adds a second registration of the same mission. - tests/README.md: recorder_regressions.sqf stubs CBA's own functions, which stay final, so its servicing and client sampler parts fail on main as well (seen on a local server with two clients). - Docs and comments: an export before the capture is refused; an older web server saves an abandoned empty mission as partial. * feat(recorder): drop a recording left by #missions or #restart before the minimum mission time A mission registers with "discard": until the recording reaches OCAP_settings_minMissionTime, the extension drops it if the game moves on without a save or discard. The capture loop sends :MISSION:KEEP: once it is long enough, after which such a recording is kept as partial, as before. * docs(recorder): a recording left by #missions or #restart, and how to verify it * fix(recorder): drop a given-up registration's token at once, so its late answers change nothing A start that registers again after an unanswered minute cleared only sessionPending; the old token stayed until the deferred newMission ran, so a late :MISSION:OK: in between began the capture against the old session, and a late :MISSION:FAILED: ended the new attempt. The token is now cleared with the retry, and the waiters of a superseded attempt leave it to the new one. The ExtensionCallback handler moves to OCAP_extension_fnc_handleCallback, so the regression suite delivers real callbacks in that window. * docs(recorder): what #missions, #restart, #shutdown and saveMissionEnded off leave on the web, as seen on a dedicated server

This was referenced Oct 26, 2026

feat(stream): drop missions the producer discards OCAP2/web#592

Merged

feat(recorder): honor save limits with streaming storage OCAP2/addon#132

Merged

smitt14ua added a commit to OCAP2/web that referenced this pull request Oct 26, 2026

chore(deps): bump extension to the latest commit of OCAP2/extension#217

36b7449

smitt14ua marked this pull request as ready for review October 27, 2026 01:11

smitt14ua requested a review from fank October 27, 2026 01:11

fank requested changes Oct 27, 2026

View reviewed changes

smitt14ua marked this pull request as draft October 27, 2026 01:24

smitt14ua added a commit to OCAP2/web that referenced this pull request Oct 27, 2026

chore(deps): bump OCAP2/extension to the latest commit of OCAP2/exten…

0270035

…sion#217

smitt14ua marked this pull request as ready for review October 27, 2026 01:52

smitt14ua requested a review from fank October 27, 2026 01:53

fank approved these changes Oct 27, 2026

View reviewed changes

fank merged commit 0a7ce6a into main Oct 27, 2026
6 checks passed

fank deleted the feat/websocket-discard-mission branch October 27, 2026 09:39

fank added a commit to OCAP2/web that referenced this pull request Oct 27, 2026

chore(deps): pin extension to the merge commit of OCAP2/extension#217

2a42bf9

Navigation Menu

Repository navigation

feat(websocket): discard missions the addon does not save - #217

feat(websocket): discard missions the addon does not save#217
fank merged 10 commits into
mainfrom
feat/websocket-discard-mission

smitt14ua commented Oct 26, 2026 •

edited

Loading

Uh oh!

fank commented Oct 27, 2026 •

edited

Loading

Uh oh!

fank left a comment

Uh oh!

smitt14ua commented Oct 27, 2026

Uh oh!

fank commented Oct 27, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 27, 2026

Uh oh!

fank left a comment

Uh oh!

fank commented Oct 27, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

smitt14ua commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

:MISSION:DISCARD:

:MISSION:START: answers failures

Protocol version 3: discard_mission

Recovery

Storage lives as long as the process (pre-existing bug)

Abandoned missions (side fix)

Abandoned recordings below the addon's save limits

Smaller fixes

Companion PRs

Compatibility

How to test

Uh oh!

fank commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fank left a comment

Choose a reason for hiding this comment

[P2] A failed final spool flush resurrects a discarded mission

Verification

Earlier discussions

Uh oh!

smitt14ua commented Oct 27, 2026

Uh oh!

fank commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 27, 2026

Merging this branch changes the coverage (1 decrease, 1 increase)

Uh oh!

fank left a comment

Choose a reason for hiding this comment

Uh oh!

fank commented Oct 27, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

smitt14ua commented Oct 26, 2026 •

edited

Loading

`:MISSION:DISCARD:`

`:MISSION:START:` answers failures

Protocol version 3: `discard_mission`

fank commented Oct 27, 2026 •

edited

Loading

fank commented Oct 27, 2026 •

edited

Loading