Repository navigation
feat(recorder): honor save limits with streaming storage - #132
Conversation
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.
… 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.
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.
…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.
…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 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.
…cument 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.
…e 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.
…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.
|
Ratatoskr reviewed this pull request. Approved on Finished 2026-10-27 01:15 UTC. |
fank
left a comment
fank
left a comment
There was a problem hiding this comment.
APPROVE — 0 blocking findings.
Review details
Reviewed head 43d8e75f2adb098c4f81e48f0773b39db4f74528 against 5e855f37cba481f3e58f87212de5b16c5e2e798e, including surrounding capture, event, extension-call and export code. No concrete correctness or safety defect found. Traced pause/resume during initialization and registration, repeated starts, registration failure, mission-end cleanup, and same-frame export versus deferred discard. The extracted export reset and handler cleanup preserve the existing behavior.
Verification:
git diff --check 5e855f37cba481f3e58f87212de5b16c5e2e798e HEAD: passed.hemtt checkandhemtt build: both unavailable locally (hemtt: command not found). GitHub's Build/build check reports SUCCESS.- Read the new regression suite; did not execute it. Arma 3 and the extension runtime are unavailable here. In particular, actual callback token handling and execution of the deferred discard after a real MPEnded remain runtime-unverified locally; the suite simulates registration responses and mission-end calls.
- Companion extension/web integration was not executed; deployment requires the documented companion changes.
Discussions: GraphQL review-thread enumeration returned no threads (no further pages). None resolved, left open, or skipped. Checkout unchanged.
… 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.
|
New since your approval: 75a16a4 and 36fb09e. Admins load a mission ahead of a game, slot in and switch with
|
|
Ratatoskr reviewed this pull request. Changes requested on Finished 2026-10-27 01:58 UTC. |
fank
left a comment
fank
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — 1 blocking finding: registration retry can start capture against a superseded session.
Review details
Reviewed head 36fb09ec2a278562e46f1a3bdadbb5d63ed8f2b9.
[P2] Invalidate the registration token before scheduling a retry
addons/recorder/fnc_startRecording.sqf:54–57
After an unanswered registration reaches 60 seconds, another start clears sessionPending here, then sets it back to true at line 75. However, registrationToken remains the old token until newMission runs inside the deferred CBA_fnc_waitUntilAndExecute statement. CBA queues that statement even when the context is already available.
If the original :MISSION:OK: arrives after the retry request but before that deferred statement runs, the callback still matches the old token and sets sessionReady = true. The existing registration waiter can consequently begin capture against the original session. The queued retry still sends another :MISSION:START:; newMission does not clear sessionReady, and the capture's frame/entity state is not reset when the extension replaces the session. This can split or corrupt the recording, with capture proceeding before the replacement registration is confirmed. A late :MISSION:FAILED: in the same gap can likewise terminate the replacement attempt.
Invalidate the old token synchronously when abandoning the attempt, and ensure callbacks and deferred continuations cannot act on a superseded attempt. Add a regression that delivers the original token's OK/FAILED between the retry request and its deferred registration, asserting no capture or failure transition occurs until the replacement's own answer. The current suite substitutes readiness flags for callbacks and does not exercise this interleaving.
Verification
git diff 5e855f37cba481f3e58f87212de5b16c5e2e798e HEAD --check: passed.- GitHub
Build / build: successful at the reviewed head. hemtt checkandhemtt build: both unavailable (hemtt: command not found, exit 127).- In-game regression suites were not run; no configured Arma 3/CBA/recorder runtime was available in this review environment. The finding above is from source/control-flow inspection, including CBA's deferred waiter implementation and companion extension PR #217's registration handler.
- GraphQL review-thread enumeration returned no threads (
hasNextPage: false), so there were no earlier discussions to resolve, retain, or skip. - Reviewed checkout left unchanged; no commits or pushes.
…ate 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.
|
The P2 was real; fixed in b7ff07a.
|
|
Ratatoskr reviewed this pull request. Approved on Finished 2026-10-27 02:13 UTC. |
fank
left a comment
fank
left a comment
There was a problem hiding this comment.
APPROVE — 0 blocking findings; the earlier registration-retry defect is fixed.
Review details
Reviewed b7ff07ae21d45d9c5bd51764194e8b58ad484e2e against 5e855f37cba481f3e58f87212de5b16c5e2e798e, including surrounding initialization, capture, pause/resume, export and extension callback code. No remaining actionable correctness or safety defect found.
Rechecked the earlier blocking finding against this head: addons/recorder/fnc_startRecording.sqf:57 now invalidates the old token synchronously, before scheduling the replacement registration. The callback's token/pending checks reject the abandoned registration's responses, and the capture and warning continuations check their own attempt's token. The regression added in tests/save_limits_regressions.sqf:138–157 delivers callbacks through the actual callback handler, including an old OK in the previously vulnerable gap. This addresses the prior finding; this conclusion is from source inspection, not a local in-game test run.
Verification:
git diff --check 5e855f3 HEAD: passed.hemtt checkandhemtt build: unavailable locally, both exit 127 (hemtt: command not found).- GitHub
Build / build: SUCCESS at the reviewed head. - Read the regression suite; did not execute it. No configured Arma 3/CBA/recorder runtime is available here. Actual mission-unload timing and companion extension/web integration remain locally unverified; deployment requires the documented companion changes.
Discussions: GraphQL enumeration returned no review threads (hasNextPage: false); no thread replies or resolutions were performed. The previous finding was in the review body linked above, not an unresolved thread. No discussions left open or skipped. Reviewed checkout unchanged; no commits or pushes.
…ded off leave on the web, as seen on a dedicated server
|
Thank you — the save-limit wiring reads clean end to end: the discard at mission end, |
Summary
minPlayerCountandminMissionTimeonly held because nothing reached storage until:MISSION:SAVE:. Thewebsocketstorage streams to the web server as it records, so missions that never met the limits were published anyway:minPlayerCountshowed up on the web as an emptypartialrecording.:MISSION:START:was sent at mission load, so the web server opened a recording before any check.minMissionTimeshowed up aspartial. It was not saved, but the web server saved the stream once the game server moved on.Changes
:MISSION:START:is sent when a recording starts, not at mission load.initSessionstill builds the world/mission context when storage is ready.startRecordingwaits for that context, since storage init may still run, for example a websocket server that is slow to answer. It waits until the extension reports:STORAGE:ERROR:(new in feat(websocket): discard missions the addon does not save extension#217) and notifies players after 60 s. Onmainsuch a slow storage simply registered later, so a fixed timeout here would have lost the recording.:MISSION:OK:, however long that takes; after 30 s a warning is logged and players are notified. The extension may take a while, for example while it still saves the previous recording. A timeout would let a late OK leave the extension with a mission nothing records. The new:MISSION:FAILED:answer, or storage that never initializes, ends the attempt and setsOCAP_recorder_recordingback to false.OCAP_extension_sessionPending), start requests register nothing more (they tell players recording starts once the mission is registered), but they undo a pause. A minute after an unanswered:MISSION:START:(a lost callback), a start request sends it again with a new token; nothing is resent while storage is still initializing. The old token is dropped with that request, before the new start goes out, and the waiters of the given-up attempt leave it to the new one, so a late:MISSION:OK:or:MISSION:FAILED:of the old start changes nothing. Init's auto-start waiter, and the empty-server auto-restart, do not start then, nor while a pause before the capture is held (OCAP_recorder_heldByPause), so they cannot undo an admin's pause. A pause before the capture began only holds it back: no "stopped recording" notification for a recording players were never told about, and no time state for an unregistered mission. A mission that ends while it registers, or is registered but paused before its capture began, has its registration discarded, whateversaveMissionEndedsays. One whose registration is answered only after the mission unloaded is refused by the extension, or dropped when the next mission loads, since it recorded nothing.OCAP_exportDatamust come on the server before the mission ends or in the same frame to save a short recording: a later one finds it discarded, and says so. Documented on the event.newMissiongives up the registration when the extension rejects:MISSION:START:synchronously, and a recording gives up when no extension is running (no:SYS:VERSION:after 60 s).:MISSION:START:'s third argument; the extension echoes it in:MISSION:OK:/:MISSION:FAILED:, and an answer with another token, or with none pending, is ignored. A late OK from the previous mission therefore no longer makes the next mission record into the old registration. An older extension sends no token and is trusted as before.OCAP_recorder_fnc_announceRecording), and so does init's "Mission has started!". With the deferred registration they would run before it, and the events would be dropped because nothing records yet (onmainthis already happened to a recording started again after an export). Like onmain, they are stamped at frame 0. Init's auto-start waiter no longer callsstartRecordingagain for a recording that already captures, which announced a manual start twice.OCAP_recordnow starts a fresh recording asynchronously. Onmain, with the mission registered at load, it recorded on the same frame.OCAP_recorder_recordingis true from the start request on. The new server-local CBA eventOCAP_recordingStartedfires once the capture runs, and again on each resume, andOCAP_recorder_startTimeis set then. A script that records its own events or exports right afterOCAP_recordshould wait for the event. This is documented onOCAP_recordand indocs/recording-lifecycle.md.OCAP_recorder_startTime, not by the frame number.MPEnded/EndedwithsaveMissionEndedcall the newOCAP_recorder_fnc_handleMissionEnded. A recording that reachedminMissionTimeis saved as before. A shorter one goes through the newOCAP_recorder_fnc_discardRecording, which stops recording, sends:MISSION:DISCARD:, notifies players / writes a diary record and resets state. The discard waits one frame, so a mission script that exports the short recording viaOCAP_exportDataon the same frame still saves it.#missions/#restartare dropped. Both fireMPEnded. After#missionsthe deferred discard above runs. After#restartthe next mission loads first, so it never ran, and such runs were saved aspartial. The extension ends a recording still open when the next mission loads. Each registration now sendsdiscardas:MISSION:START:'s fourth argument: until the recording reachesminMissionTime, the extension discards it then instead of ending it as interrupted. From that frame the capture loop sends:MISSION:KEEP:(once per recording,OCAP_extension_recordingKept), and such a recording is saved aspartial, as before. Admins who load a mission ahead of a game, slot in and switch with#missionsno longer leave those runs on the web.:MISSION:DISCARDED:callback is logged (ok/kept/error). A discard the extension rejects (older extension, or a save still running) is logged as a warning.exportData's handler removal and state reset are moved toremoveCaptureHandlersandresetRecording, shared with the discard. No behavior change.ExtensionCallbackhandler's body moves frominitSessionto the newOCAP_extension_fnc_handleCallback, so the regression suite can deliver real callbacks. No behavior change."Mission ended automatically"as the winning side, so the end event carried a quoted string as side and an empty message. It is now passed as the message.What each storage does with a discard
memory: drops the data, no file written.websocket: the web server drops the recording (needs feat(stream): drop missions the producer discards web#592, protocol 3). An older web server still saves it aspartial, the previous behavior.postgres/sqlite: not selectable in the extension yet; they would keep what they wrote.Not covered
A recording still open when the game server process exits is still saved by the web server as
partial, whatever its length, once its resume grace expires. This covers a crash, and#shutdown, which fires noMPEnded(checked on a local dedicated server: a 20 s recording was saved aspartial).With
saveMissionEndeddisabled, a recording is neither saved nor discarded at mission end. When the next mission loads, the extension drops it belowminMissionTimeand keeps it aspartialpast it.Unchanged on purpose: when the last player leaves before
minMissionTime,saveOnEmptydoes not save, and the recording continues on the empty server. That holds as before for a recording whose registration completes after the last player left. A mission change before it reaches the minimum now drops it. One that runs on past the minimum is kept. The addon gets no reliable hook for those. Making the extension or web apply the minimum itself would close this gap; I left that for a follow-up.Companion PRs
:MISSION:DISCARD:, protocol 3, recoverydiscard_missionOrder: extension → web → addon. This PR needs the extension PR: an older extension rejects
:MISSION:DISCARD:and:MISSION:KEEP:as unknown commands and never answers a start that failed with:MISSION:FAILED:, so such a recording would keep waiting.How to test
hemtt checkandhemtt buildpass locally, with no new warnings.tests/save_limits_regressions.sqf(seetests/README.md). Its stubs, like the existing suites', need a test build withDISABLE_COMPILE_CACHE, because CBA compilesPREPed functions final. The README now explains that build, and the script refuses to run without it, so no real lifecycle command reaches the extension. I ran it on a local dedicated server (-autoInit, no players) with such a build, CBA and the extension DLL built from feat(websocket): discard missions the addon does not save extension#217 (memory storage): all 34 assertions pass, with no script errors in the RPT. It checks: a single registration at start, asking for a discard if abandoned; a registration given up after a minute, whose late OK and FAILED (delivered through the real callback handler before and after the new start) change nothing until the new registration's OK begins the capture; no capture or announcement before the extension confirms, even past 30 s; one announcement while capturing; a failed start ending the recording; a mission ended during registration discarded; a pause during registration holding the capture until resume; a start request undoing that pause; a registered mission never captured discarded at mission end; discard of a short mission; a same-frame export winning over the discard; a long mission still saved; nothing done withsaveMissionEndedoff; a repeated start not restarting the capture; a pause before the capture held until a start request lifts it.recorder_regressions.sqf(two players, one with Zeus) I ran on the same local server with two clients, on this branch and onmain: identical results. All 13 server-side submission checks pass on both; its servicing and client sampler parts fail on both, because the suite stubs CBA's own functions, which are always final.tests/README.mdnow says so.docs/recording-lifecycle.md(belowminPlayerCount; mission ended before/afterminMissionTime, websocket and memory; start events in the recording; a recording left by#missions/#restartbelow and pastminMissionTime).