Skip to content

feat(recorder): improve automatic recording lifecycle - #126

Merged
fank merged 4 commits into
OCAP2:mainfrom
YetheSamartaka:main
Oct 17, 2026
Merged

fank merged 4 commits into
OCAP2:mainfrom
YetheSamartaka:main

Conversation

@YetheSamartaka

Copy link
Copy Markdown
Contributor

Added:

  • Auto-restart and connection event buffering
  • New CBA settings to configure everything these changes are touching

Changed:

  • Player counting and Steam ID handling

YetheSamartaka and others added 2 commits August 22, 2026 15:30
Added:
- Auto-restart and connection event buffering
- New CBA settings to configure everything these changes are touching

Changed:
- Player counting and Steam ID handling
@YetheSamartaka

Copy link
Copy Markdown
Contributor Author

Linked with OCAP2/extension#202

@thegamecracks thegamecracks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small review from me since I noticed old code removed by #125 came back up, despite not triggering a merge conflict :)

Comment thread addons/recorder/fnc_eh_connected.sqf Outdated
Co-authored-by: thegamecracks <61257169+thegamecracks@users.noreply.github.com>

@thegamecracks thegamecracks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the edit!

@fank
fank self-requested a review October 2, 2026 21:53
@fank

fank commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

Ratatoskr reviewed this pull request.

Changes requested on fdb88445. See the review.

Finished 2026-10-02 21:58 UTC.

@fank fank left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes: 2 findings to fix before merge (auto-restart ignores autoStart; stale buffered events replayed at frame 0), plus 3 minor notes.

Review details

I reviewed head fdb88445f1e96c60d2f6eab7cdf369ce06aa4877 against main, reading the new functions together with fnc_startRecording, fnc_exportData, fnc_captureLoop, fnc_init, the extension callback in extension/fnc_initSession.sqf, and the v1 export in the linked OCAP2/extension#202.

The core flow holds up. Events are buffered while startTime is nil or sessionReady is false. They are flushed after sessionReady and before captureLoop, and exportData resets startTime and sessionReady, so the restart re-registers through newMission. Pausing keeps startTime set, so events during a pause still go out immediately, as they did before. Headless-client exclusion via allPlayers - entities "HeadlessClient_F" is correct, and it now matches the CBA_fnc_players check that fnc_eh_disconnected.sqf already uses for the empty-server save.

Must fix

1. Auto-restart ignores OCAP_settings_autoStart and is on by default. (addons/recorder/fnc_autoRestartMonitor.sqf:19-29, addons/recorder/XEH_preInit.sqf:91-113)

The monitor checks autoRestartAfterEmpty, the pending flag, the client state and minPlayerCount. It never checks GVAR(autoStart). Here is a server that turned off auto-start so admins decide what gets recorded:

  1. An admin starts a recording by hand. Everyone leaves, and saveOnEmpty exports it.
  2. autoRestartAfterEmptyPending is now true.
  3. Later, enough players join for something unrelated. The monitor starts a new recording without anyone asking for it.
  4. When they leave and minMissionTime is met, that recording is saved and uploaded to the web.

Before this PR, nothing restarted after an empty-server save. This changes behaviour for every existing server and creates recordings that the operator explicitly opted out of. Please gate the restart on GVAR(autoStart) (the snapshot taken in fnc_init.sqf:80), or default autoRestartAfterEmpty to false. The first option seems more natural, since the setting is described as re-arming auto-start.

2. The buffer is unbounded, and replays stale events at frame 0 of whichever recording starts next. (addons/recorder/fnc_recordPlayerConnectionEvent.sqf:26-30, addons/recorder/fnc_flushPlayerConnectionEvents.sqf:19-38)

After an export, every connect and disconnect is appended to connectedPlayerNamesBuffer until a recording starts. Nothing bounds the buffer or clears it. On a persistent server that window can last hours or days, for example:

  • autoRestartAfterEmpty or autoStart is off;
  • or the player count stays below minPlayerCount (default 15) while people drift in and out.

When the next recording starts, all of those events are emitted at once, at the same frame, with the real timing lost. The recording then claims that players who left hours ago, during a different session, connected and disconnected at its start.

Before the first recording this matches existing behaviour, because events went out with captureFrameNo == 0. After an export, though, this is new data that never belonged to the recording. A simple fix: at flush time, emit connected only for players who are currently connected (from allPlayers, minus HCs), and drop the buffer. A cap or a clear-on-export alone would still replay irrelevant churn.

Minor / non-blocking

  • Frame 0 is the "forever" sentinel, not the first recorded frame. fnc_captureLoop.sqf:58 reserves frame 0 for "not yet recording". OCAP2/extension#202 maps internal frame 0 to v1 frame -1 (frameToV1). So "replay them at frame 0" actually lands before the recording's first frame (v1 0). Pre-start connection events already behaved this way, so this may be intended. If the goal is for them to show at the start of the timeline, frame 1 may be what you want. I haven't checked how the web player renders -1 events, so I'm unsure whether this is visible.
  • The restart can start a recording on an empty server. With excludeHeadlessClientsFromAutoStart = false, the empty-server save still fires, because CBA_fnc_players excludes HCs. But the monitor then counts the HCs. If minPlayerCount is no higher than the HC count, a new recording starts within about 10 seconds with no humans present. saveOnEmpty then won't fire again until a human joins and leaves. This is an edge case with a non-default setting, but a note in the setting description, or always excluding HCs in the restart path, would avoid it.
  • Naming and duplication. connectedPlayerNamesBuffer holds both connect and disconnect events, with UIDs, so the name is misleading. The event payload code is duplicated between fnc_recordPlayerConnectionEvent.sqf:33-45 and fnc_flushPlayerConnectionEvents.sqf:22-35. Having recordPlayerConnectionEvent take an optional frame and calling it from the flush would keep the Steam ID handling in one place.

Tests / checks

  • The repository has no SQF test harness. CI is only hemtt check / hemtt build, and gh pr checks reports no checks on this PR. HEMTT isn't available in my review environment, so I did not build or lint it, and nothing here is pinned by tests. A manual test on a dedicated server is worth recording in the PR, covering:
    1. empty-server save → rejoin → auto-restart;
    2. the same flow with autoStart = false;
    3. connect/disconnect churn between recordings, then inspecting the exported events.

Earlier review discussions

  • thegamecracks' thread on fnc_eh_connected.sqf (re-introduced adminUIcontrol call): already resolved by its author. I checked it against the current head: fnc_eh_connected.sqf no longer calls adminUIcontrol. It isn't my thread, so I took no action on it. No other unresolved threads exist.

@YetheSamartaka
YetheSamartaka requested a review from fank October 14, 2026 05:20
@YetheSamartaka

Copy link
Copy Markdown
Contributor Author

Hi @fank . I've looked into your suggestions and applied fixes. Please review them again.

@fank

fank commented Oct 14, 2026 •

Copy link
Copy Markdown
Member

Ratatoskr reviewed this pull request.

Approved on 23693b4e. See the review.

Finished 2026-10-14 05:24 UTC.

@fank fank left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE — 0 blocking findings.

Review details

Reviewed head 23693b4eda5c95fc48b7f2ba1de473aad038bf15 against main, including the surrounding recording, export, extension-session and mission-event code. No actionable correctness or safety defects found. The restart path is armed by empty-server export, respects the initialization-time auto-start setting, and avoids active/paused sessions. Startup connection snapshots discard between-recording history and preserve the existing playerUid JSON payload format.

Earlier discussions: GraphQL enumeration of review threads and comments completed without further pages; there are no unresolved threads. The already-resolved JIP admin-controls finding is fixed in the current code: fnc_eh_connected.sqf no longer calls adminUIcontrol. No threads were replied to or resolved by this review (authenticated reviewer: fank).

Checks and limitations: git diff --check main...HEAD passed. hemtt check and hemtt build could not run (hemtt: command not found). No Arma 3 server or SQF VM was available, so the dedicated-server scenarios in docs/recording-lifecycle.md were not executed; those scenarios are manual verification guidance, not automated regression tests. Runtime restart timing and exported event contents remain unverified. GitHub reported no status checks at final verification. Approval is the code-review verdict, not a bypass of merge gates.

@fank
fank merged commit 0b6c444 into OCAP2:main Oct 17, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants