From cf39934a1311b9d2fcdcfdd8b4fd67f9bad27775 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 14:01:08 +1000 Subject: [PATCH 01/19] feat(audience-session): session lifecycle (SDK-145) Unity session lifecycle with engagement-aware duration, crash-safe heartbeats, and thread-safe integration with ImmutableAudience. - Session class: fires session_start on Init, session_heartbeat every 60s while foreground, session_end on Shutdown or Resume after >30s pause. duration excludes pause time. - ImmutableAudience: _session field, serialised under _initLock across Init / SetConsent / Shutdown / ResetState. OnPause / OnResume route to Session on the Unity lifecycle bridge. - SetConsent upgrade from None starts a new Session under the lock. - SetConsent downgrade to None disposes the Session; session_end is gated out by CanTrack so no post-revocation event leaks. - Shutdown drains the heartbeat timer before emitting session_end so wire ordering holds. - Log.Emit swallows Writer exceptions so SafeTrack stays safe on the Timer thread. - EnqueueChecked gate closes the Track-races-SetConsent(None) purge window. Tests cover heartbeat quiescence while paused, extended-pause rollover, clock rewind clamps, exception containment on _track and performance-snapshot, double-Start timer drain, concurrent SetConsent upgrade, concurrent Init+SetConsent consistency. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/Core/Session.cs | 448 +++++++++ .../Audience/Runtime/Core/Session.cs.meta | 11 + .../Audience/Runtime/ImmutableAudience.cs | 339 +++++-- src/Packages/Audience/Runtime/Utility/Log.cs | 21 +- .../Tests/Runtime/Core/SessionTests.cs | 914 ++++++++++++++++++ .../Tests/Runtime/Core/SessionTests.cs.meta | 11 + .../Tests/Runtime/ImmutableAudienceTests.cs | 159 ++- 7 files changed, 1796 insertions(+), 107 deletions(-) create mode 100644 src/Packages/Audience/Runtime/Core/Session.cs create mode 100644 src/Packages/Audience/Runtime/Core/Session.cs.meta create mode 100644 src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs create mode 100644 src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs.meta diff --git a/src/Packages/Audience/Runtime/Core/Session.cs b/src/Packages/Audience/Runtime/Core/Session.cs new file mode 100644 index 000000000..5c6c0b6b8 --- /dev/null +++ b/src/Packages/Audience/Runtime/Core/Session.cs @@ -0,0 +1,448 @@ +#nullable enable + +using System; +using System.Collections.Generic; +using System.Threading; + +namespace Immutable.Audience +{ + // Session lifecycle. session_start on Init, session_heartbeat every 60s + // while foregrounded (quiesced during Pause), session_end on Shutdown or + // pause > 30s evaluated on the next Resume. Shares the Web SDK wire + // schema (sessionId plus integer "duration" in seconds) but not the + // duration semantic — see End() for detail. Unity-specific extensions: + // engagement-aware duration (excludes pause time) and heartbeat-carried + // performance metrics. Games don't have browser visibility APIs, and + // long alt-tabs would inflate session time if we counted wall-clock. + // + // Thread safety: heartbeat fires on a thread-pool thread; Start / End / + // Pause / Resume / Dispose run on the caller's thread. A private lock + // guards state. The _track callback fires outside the lock so re-entrant + // track implementations can safely take their own locks. + internal sealed class Session : IDisposable + { + internal const int HeartbeatIntervalMs = 60_000; + + // 30 seconds. Intentionally short: a player who alt-tabs out of the + // game for half a minute has stopped engaging and the next resume + // should roll the session. Not a port of Web SDK SESSION_MAX_AGE + // (30 minutes idle on the cookie). Browsers have no foreground + // pause concept, so the two values measure different things. + internal const int PauseTimeoutMs = 30_000; + + private readonly Action> _track; + private readonly Func>? _performanceSnapshot; + private readonly Func _getUtcNow; + private readonly int _heartbeatIntervalMs; + private readonly object _lock = new object(); + + private Timer? _heartbeatTimer; + private string? _sessionId; + private DateTime _sessionStart; + private DateTime? _pausedAt; + // Total pause time accumulated across every Pause/Resume cycle + // since Start. Subtracted from wall-clock in session_end so + // "duration" reflects engagement rather than real time. Post-resume + // session_heartbeat callers read this too, so a resumed session's + // first tick does not overcount the interval that spanned the pause; + // heartbeats that fire while _pausedAt is set are quiesced at the + // top of OnHeartbeat and never touch the accumulator. + private TimeSpan _accumulatedPause; + private bool _disposed; + + // Current session ID. Null before Start() is called and after End()/Dispose(). + internal string? SessionId + { + get { lock (_lock) return _sessionId; } + } + + // track fires session_start / _heartbeat / _end via + // ImmutableAudience.Track (passed as a delegate so tests can drive + // Session with a mock and without touching the static SDK surface). + // performanceSnapshot merges fps/memory into each heartbeat; null + // on non-Unity runtimes. getUtcNow and heartbeatIntervalMs are test + // seams that production callers leave at their defaults (system + // clock, 60_000 ms). + internal Session( + Action> track, + Func>? performanceSnapshot = null, + Func? getUtcNow = null, + int heartbeatIntervalMs = HeartbeatIntervalMs) + { + _track = track ?? throw new ArgumentNullException(nameof(track)); + _performanceSnapshot = performanceSnapshot; + _getUtcNow = getUtcNow ?? (() => DateTime.UtcNow); + _heartbeatIntervalMs = heartbeatIntervalMs; + } + + // Start a new session. Fires session_start and begins the heartbeat timer. + internal void Start() + { + // Phase 1 — stop the old heartbeat timer (if any) and drain its + // in-flight callback OUTSIDE the lock before we mutate session + // state. The callback itself acquires _lock, so waiting on the + // drain under _lock would deadlock. Leaving _sessionId / + // _sessionStart / _accumulatedPause at their old values across + // the drain means a trailing callback emits a heartbeat for the + // OLD session — wire-ordered before the new session_start — + // instead of reading the new state with zero-duration noise. + // + // Double-Start is a misuse path (supported pattern is End/Dispose + // then Start); on the normal End→Start rollover _heartbeatTimer + // is already null from End's drain, so this drain is a no-op. + Timer? oldTimer; + lock (_lock) + { + if (_disposed) return; + oldTimer = _heartbeatTimer; + if (oldTimer != null) + { + oldTimer.Change(Timeout.Infinite, Timeout.Infinite); + _heartbeatTimer = null; + } + } + + if (oldTimer != null) + { + using var waited = new ManualResetEvent(false); + try + { + // 500 ms budget (vs End's 1 s) because double-Start is + // unexpected; we do not want a mistake path to block the + // caller for a full second. Dispose(wh) returns false on + // an already-disposed timer — skip the wait in that case. + if (oldTimer.Dispose(waited)) + waited.WaitOne(TimeSpan.FromMilliseconds(500)); + } + catch (ObjectDisposedException) + { + } + } + + // Phase 2 — atomically populate new session state and schedule + // the new timer. _disposed may have flipped during the drain; + // re-check. + string sessionId; + lock (_lock) + { + if (_disposed) return; + + _sessionId = Guid.NewGuid().ToString(); + _sessionStart = _getUtcNow(); + _pausedAt = null; + _accumulatedPause = TimeSpan.Zero; + + sessionId = _sessionId; + _heartbeatTimer = new Timer(_ => OnHeartbeat(), null, _heartbeatIntervalMs, _heartbeatIntervalMs); + } + + SafeTrack("session_start", new Dictionary + { + ["sessionId"] = sessionId + }); + } + + // Called when the game loses focus (e.g. alt-tab, minimize). Records + // the pause start; quiesces session_heartbeat so no engagement signal + // is emitted while the app is backgrounded. The 30s pause threshold + // is evaluated on the next Resume call — no timer fires on Pause, so + // a game that never resumes (force-killed while backgrounded) sees + // session_end only when Shutdown runs. Expected to be called on the + // Unity main thread by the lifecycle bridge. + internal void Pause() + { + lock (_lock) + { + if (_disposed || _sessionId == null) return; + // Already paused — keep the original _pausedAt so the live + // pause window stays anchored to the first Pause. Without + // this guard a double-Pause would advance _pausedAt forward + // and ComputeEngagedSecondsLocked's (now - _pausedAt) live + // pause would undercount the window by the delta, over- + // crediting engagement time. + if (_pausedAt.HasValue) return; + _pausedAt = _getUtcNow(); + } + } + + // Called when the game regains focus. If paused up to 30s the + // session continues; any longer and the old session ends and a new + // one starts. Expected to be called on the Unity main thread by the + // lifecycle bridge. + internal void Resume() + { + bool extended; + lock (_lock) + { + if (_disposed || _sessionId == null || _pausedAt == null) return; + + var pauseDuration = _getUtcNow() - _pausedAt.Value; + _pausedAt = null; + + // Clamp to zero for wall-clock rewinds (NTP correction, manual + // clock change). A negative pauseDuration would reduce + // _accumulatedPause and hand the next session an artificial + // engagement credit. End already clamps the outgoing duration, + // but the accumulator needs its own guard. + if (pauseDuration < TimeSpan.Zero) pauseDuration = TimeSpan.Zero; + + extended = pauseDuration.TotalMilliseconds > PauseTimeoutMs; + + // Credit the pause in both paths. Short pauses: End will never + // fire for this session, the credit prevents heartbeats from + // overcounting. Extended pauses: End fires next and subtracts + // the credit so session_end reports the pre-pause engaged time, + // then resets _accumulatedPause to zero before Start runs. No + // risk of double-count — the reset in End (and again in Start) + // is what makes the extended-pause path safe to credit here. + _accumulatedPause += pauseDuration; + } + + if (extended) + { + // Extended pause — end old session, start new one. Both fire + // their track events outside our lock so reentrant track + // implementations can safely take their own locks. + // + // Invariant between End() and Start(): _sessionId is null + // (End reset it), _heartbeatTimer is null (drained), and + // _disposed may flip if Dispose races. Every other public + // method (Pause, Resume, End, OnHeartbeat, Dispose) guards + // on one of those fields and early-returns, so a concurrent + // call landing in this window is a no-op rather than a + // corruption vector. + End(); + Start(); + } + } + + // End the current session. Fires session_end and stops the heartbeat. + // + // Drains any in-flight heartbeat callback before emitting session_end + // so the usual wire ordering (heartbeats -> session_end) holds. On + // drain timeout (1 s, see DrainHeartbeatTimer), ordering is + // best-effort: a stuck heartbeat callback whose _track call is still + // running when the drain gives up can land after session_end on the + // wire. The drain logs a warning in that case so the anomaly is + // observable. + internal void End() + { + // Phase 1 — stop the timer and wait for any in-flight callback + // OUTSIDE the lock. OnHeartbeat itself takes _lock; waiting under + // the lock would deadlock. See DrainHeartbeatTimer for the wait + // budget and the logging on timeout. + DrainHeartbeatTimer(); + + // Phase 2 — atomically capture the outgoing session fields and + // reset state so subsequent Start (on extended-pause rollover) or + // Dispose (on shutdown) sees a clean slate. + string sessionId; + long duration; + lock (_lock) + { + if (_sessionId == null) return; + sessionId = _sessionId!; + + // ComputeEngagedSecondsLocked folds the in-flight pause in + // itself, so End on a still-paused session reports engaged + // time that excludes the final pause window. + duration = ComputeEngagedSecondsLocked(); + ResetSessionStateLocked(); + } + + // duration is engagement-aware: wall-clock since Start minus + // every credited pause. Web SDK's session_end emits wall-clock + // seconds with no pause concept (browsers have no foreground + // pause). Same wire field, different semantic. Unity always + // emits duration (minimum 0); Web SDK guards it behind + // sessionStartTime, which in practice tracks sessionId and + // is set/cleared together with it. Backend dashboards + // comparing surfaces should not assume Unity and Web session + // lengths are directly comparable. + SafeTrack("session_end", new Dictionary + { + ["sessionId"] = sessionId, + ["duration"] = duration + }); + } + + public void Dispose() + { + lock (_lock) + { + if (_disposed) return; + _disposed = true; + } + + // End handles the timer drain, the atomic state capture, and the + // session_end emit. Dispose adds nothing beyond the _disposed + // latch (which blocks subsequent Start/Pause/Resume calls). + End(); + } + + // ----------------------------------------------------------------- + // Private + // ----------------------------------------------------------------- + + // Fires a single heartbeat. Called by the internal timer every 60s + // and exposed as internal so tests can exercise the heartbeat path + // without waiting the full interval. Skips emission while the + // session is paused so "foregrounded-only" is what actually ships: + // a backgrounded game must not dribble stable-duration heartbeats + // into the pipeline for the entire alt-tab. + internal void OnHeartbeat() + { + string sessionId; + long duration; + lock (_lock) + { + if (_disposed || _sessionId == null) return; + // Quiesce while paused — session_heartbeat is an engagement + // signal, and a paused session is by definition disengaged. + // Resume resumes ticking; the timer keeps firing in the + // background on thread-pool cadence but this method is the + // gate that controls what reaches _track. + if (_pausedAt.HasValue) return; + // Non-null at this point — the early-return above ensures it. + sessionId = _sessionId!; + + duration = ComputeEngagedSecondsLocked(); + } + + // Build and emit the heartbeat outside the lock so the performance + // snapshot delegate and track callback cannot cause contention or + // reentrant-lock surprises. + var properties = new Dictionary + { + ["sessionId"] = sessionId, + ["duration"] = duration + }; + + var perf = SafePerformanceSnapshot(); + if (perf != null) + { + foreach (var kv in perf) + { + // Core heartbeat fields are owned by Session; a provider + // that returns a "sessionId" or "duration" key (buggy or + // malicious) must not be able to rewrite them on the wire. + // Drop colliding keys rather than overwrite. + if (properties.ContainsKey(kv.Key)) continue; + properties[kv.Key] = kv.Value; + } + } + + SafeTrack("session_heartbeat", properties); + } + + // Invokes _track with a catch-all so a throwing callback cannot + // escape. OnHeartbeat runs on a thread-pool timer callback where an + // unhandled exception is swallowed by the runtime on .NET Framework + // / Mono but can terminate the process on .NET 5+. Start and End run + // on the caller's thread (typically Unity main) where an exception + // from _track would bubble into Init / Shutdown / SetConsent. Same + // guard covers both paths. + private void SafeTrack(string eventName, Dictionary properties) + { + try + { + _track(eventName, properties); + } + catch (Exception ex) + { + Log.Warn($"Session: {eventName} track callback threw {ex.GetType().Name}: {ex.Message}. Event dropped."); + } + } + + // Invokes the performance snapshot provider with a catch-all. The + // provider is studio-supplied (PerformanceSnapshotProvider) and + // crosses an API boundary; without this guard a throwing provider + // propagates into the heartbeat timer callback. + private Dictionary? SafePerformanceSnapshot() + { + if (_performanceSnapshot == null) return null; + try + { + return _performanceSnapshot(); + } + catch (Exception ex) + { + Log.Warn($"Session: performance snapshot threw {ex.GetType().Name}: {ex.Message}. Heartbeat ships without performance fields."); + return null; + } + } + + // Stops the heartbeat timer and waits for any in-flight callback to + // complete. Runs OUTSIDE _lock because OnHeartbeat takes _lock itself + // and waiting under the lock would deadlock. Idempotent: safe to call + // repeatedly. The wait budget is 1 second because Dispose can run on + // Application.quitting (Unity main thread) and a frozen quit is worse + // than a dropped trailing heartbeat. Logs a warning on timeout so + // stuck-heartbeat anomalies surface when Debug logging is on. + private void DrainHeartbeatTimer() + { + Timer? timer; + lock (_lock) + { + timer = _heartbeatTimer; + _heartbeatTimer = null; + } + if (timer == null) return; + + using var waited = new ManualResetEvent(false); + try + { + // Timer.Dispose(WaitHandle) returns false when the timer was + // already disposed — in that case the WaitHandle is NOT + // signaled, so WaitOne would time out and fire a spurious + // "drain timeout" warning. Skip the wait (and the warning) + // when the timer reports it is already gone. + if (!timer.Dispose(waited)) + return; + + if (!waited.WaitOne(TimeSpan.FromSeconds(1))) + { + Log.Warn("Session: heartbeat callback did not complete within 1s on timer stop. " + + "A trailing session_heartbeat may race with the next session lifecycle event."); + } + } + catch (ObjectDisposedException) + { + // Timer already disposed — nothing to wait for. + } + } + + // Caller must hold _lock. Engagement time in seconds: wall-clock + // since Start, minus every credited pause (both the committed + // _accumulatedPause and any in-flight window since _pausedAt), + // rounded to a whole second to match Web SDK's Math.round(... / + // 1000). Folding the live pause in here rather than at each call + // site keeps callers from racing the pause state against their + // own local copy — End and OnHeartbeat both run under _lock, so + // the read is consistent with ResetSessionStateLocked / Pause / + // Resume. Clamped to zero so a wall-clock rewind (NTP correction, + // manual change) can never produce a negative duration on the wire. + private long ComputeEngagedSecondsLocked() + { + var now = _getUtcNow(); + var livePause = _pausedAt.HasValue ? now - _pausedAt.Value : TimeSpan.Zero; + var engagedSeconds = ((now - _sessionStart) - _accumulatedPause - livePause).TotalSeconds; + if (engagedSeconds < 0) return 0; + return (long)Math.Round(engagedSeconds, MidpointRounding.AwayFromZero); + } + + // Caller must hold _lock. Clears per-session state after End so + // the Session falls back to an unstarted shape (SessionId == null, + // no pause accumulator). End is the only caller; Start inlines + // the same three assignments because it simultaneously writes + // live values (a new Guid, _getUtcNow()) that cannot be expressed + // by a "reset" helper. Adding a new state field requires updating + // both Start and this helper. + private void ResetSessionStateLocked() + { + _sessionId = null; + _pausedAt = null; + _accumulatedPause = TimeSpan.Zero; + } + } +} diff --git a/src/Packages/Audience/Runtime/Core/Session.cs.meta b/src/Packages/Audience/Runtime/Core/Session.cs.meta new file mode 100644 index 000000000..a7ef5b1da --- /dev/null +++ b/src/Packages/Audience/Runtime/Core/Session.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 159b16ab94bb14a51b0d3642318a3d6e +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/src/Packages/Audience/Runtime/ImmutableAudience.cs b/src/Packages/Audience/Runtime/ImmutableAudience.cs index e90ef7b46..b52155f7d 100644 --- a/src/Packages/Audience/Runtime/ImmutableAudience.cs +++ b/src/Packages/Audience/Runtime/ImmutableAudience.cs @@ -13,7 +13,9 @@ namespace Immutable.Audience public static class ImmutableAudience { // Reference fields are written inside _initLock; readers fence off the volatile _initialized load. - // _consent and _userId are mutated outside the lock and need volatile themselves. + // _consent and _session are written only inside _initLock but read outside (Track's CanTrack, + // OnPause/OnResume) so they stay volatile for release/acquire visibility. + // _userId is written outside the lock (Identify, Reset) and needs volatile for the same reason. private static AudienceConfig? _config; private static DiskStore? _store; private static EventQueue? _queue; @@ -43,6 +45,19 @@ public static class ImmutableAudience // Unity context without the core referencing UnityEngine. internal static Func>? LaunchContextProvider; + // Session is created when consent allows — either at Init or on + // upgrade from None — and is disposed on Shutdown or SetConsent(None). + // Holds the current sessionId, fires session_start / session_heartbeat + // / session_end via the Track callback, and handles pause/resume + // rollover. + // + // Volatile + nullable because the field is read outside _initLock on + // the Unity main thread (OnPause, OnResume) and written outside the + // lock by SetConsent and Shutdown. volatile gives a release/acquire + // fence so a freshly-assigned Session from SetConsent(None → *) is + // visible to a subsequent OnPause on any thread. + private static volatile Session? _session; + // Starts the SDK. Call once at launch. public static void Init(AudienceConfig config) { @@ -56,6 +71,7 @@ public static void Init(AudienceConfig config) throw new ArgumentException("PersistentDataPath is required", nameof(config)); ConsentLevel consentAtInit; + Session? sessionToStart; lock (_initLock) { if (_initialized) @@ -87,11 +103,58 @@ public static void Init(AudienceConfig config) // Snapshot under the lock so a racing SetConsent(None) can't drop the launch event. consentAtInit = _consent; + + // Create the session object under the lock so ResetState / + // Shutdown have a consistent view. Start() is deferred + // until outside the lock because Track() (which + // session_start calls) acquires its own locks. + if (consentAtInit.CanTrack()) + _session = new Session(Track); + + // Captured under the lock so the Start() call below + // operates on the Session this Init created, not a + // replacement from a SetConsent that slips in after we + // release _initLock. SetConsent itself takes _initLock so + // it cannot land in the middle of this block, but between + // the lock release and Start() a SetConsent(None) can + // Dispose the captured Session (Start then early-returns + // on _disposed, suppressing session_start) or a + // SetConsent(None)+SetConsent(Anonymous) pair can swap + // _session for a new one (we still Start the captured + // original, which is disposed and no-ops; the upgrade path + // Starts the replacement itself). Either way, no duplicate + // session_start on the wire and no "events after consent + // dropped" leak. + sessionToStart = _session; } + // session_start fires before game_launch so the wire stream + // shows the new sessionId ahead of the launch event. + sessionToStart?.Start(); + FireGameLaunch(config, consentAtInit); } + // Notifies the session that the game was paused (alt-tab, + // minimize, OS pause). If not resumed within 30 s the session + // ends and a new one starts on resume. Called only from the + // Unity lifecycle bridge, so it stays internal; the Unity + // assembly reaches it via InternalsVisibleTo in AssemblyInfo.cs. + internal static void OnPause() + { + if (!_initialized) return; + _session?.Pause(); + } + + // Notifies the session that the game resumed. Called only from + // the Unity lifecycle bridge; see OnPause for the visibility + // rationale. + internal static void OnResume() + { + if (!_initialized) return; + _session?.Resume(); + } + // ----------------------------------------------------------------- // Track // ----------------------------------------------------------------- @@ -340,47 +403,87 @@ public static void SetConsent(ConsentLevel level) { if (!_initialized) return; - var config = _config; - var queue = _queue; - if (config == null) return; + // Serialize the whole transition under _initLock: + // - Two concurrent SetConsent calls that both see previous=None + // would otherwise both take the upgrade branch and each build + // a fresh Session, stranding one timer. The lock forces them + // to observe each other. + // - A SetConsent landing in the narrow window between Init's + // _initialized = true and its _session = new Session(...) + // assignment would otherwise see _session = null, skip the + // Dispose path, and let Init finish creating a Session whose + // timer never gets disposed. Under the lock, SetConsent can + // only enter after Init has fully released it, so _session + // reflects Init's final assignment. + Session? sessionToStart = null; + lock (_initLock) + { + if (!_initialized) return; - var previous = _consent; - if (level == previous) return; + var config = _config; + var queue = _queue; + if (config == null) return; - // Snapshot the anonymousId BEFORE Identity.Reset (on downgrade to - // None) wipes the file. The PUT audit trail needs it to record - // whose consent changed. - var anonymousIdForPut = previous == ConsentLevel.None - ? Identity.GetOrCreate(config.PersistentDataPath!, level) - : Identity.Get(config.PersistentDataPath!); + var previous = _consent; + if (level == previous) return; - _consent = level; + // Snapshot the anonymousId BEFORE Identity.Reset (on downgrade to + // None) wipes the file. The PUT audit trail needs it to record + // whose consent changed. + var anonymousIdForPut = previous == ConsentLevel.None + ? Identity.GetOrCreate(config.PersistentDataPath!, level) + : Identity.Get(config.PersistentDataPath!); - try - { - // PersistentDataPath is validated non-null in Init; compiler can't propagate that. - ConsentStore.Save(config.PersistentDataPath!, level); - } - catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException) - { - Log.Warn($"SetConsent — failed to persist consent level: {ex.GetType().Name}: {ex.Message}. " + - "In-memory level is updated but will revert on next launch."); - NotifyErrorCallback(config.OnError, AudienceErrorCode.ConsentPersistFailed, - $"Consent persist failed: {ex.Message}"); - } + _consent = level; - if (level == ConsentLevel.None) - { - queue?.PurgeAll(); - Identity.Reset(config.PersistentDataPath!); - } - else if (previous == ConsentLevel.Full && level == ConsentLevel.Anonymous) - { - _userId = null; - queue?.ApplyAnonymousDowngrade(); + try + { + // PersistentDataPath is validated non-null in Init; compiler can't propagate that. + ConsentStore.Save(config.PersistentDataPath!, level); + } + catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException) + { + Log.Warn($"SetConsent — failed to persist consent level: {ex.GetType().Name}: {ex.Message}. " + + "In-memory level is updated but will revert on next launch."); + NotifyErrorCallback(config.OnError, AudienceErrorCode.ConsentPersistFailed, + $"Consent persist failed: {ex.Message}"); + } + + if (level == ConsentLevel.None) + { + // Dispose the session for heartbeat-timer cleanup. + // session_end is intentionally NOT emitted: Session.Dispose + // calls back into Track("session_end", ...) which sees + // _consent == None (set above) and drops the event via + // CanTrack. That matches revocation semantics — no events + // should leave the device after consent is None — and the + // queue purge below clears anything on disk anyway. + _session?.Dispose(); + _session = null; + + queue?.PurgeAll(); + Identity.Reset(config.PersistentDataPath!); + } + else if (previous == ConsentLevel.Full && level == ConsentLevel.Anonymous) + { + _userId = null; + queue?.ApplyAnonymousDowngrade(); + } + else if (previous == ConsentLevel.None && _session == null) + { + // Upgrade from None: the previous session was disposed on + // the prior downgrade. Start a fresh one so + // session_heartbeat and session_end can resume. Start() + // is deferred until outside the lock because Track() + // (which session_start calls) acquires its own locks. + _session = new Session(Track); + sessionToStart = _session; + } + + SyncConsentToBackend(config, level, anonymousIdForPut); } - SyncConsentToBackend(config, level, anonymousIdForPut); + sessionToStart?.Start(); } // Fire-and-forget PUT /v1/audience/tracking-consent. Failures do not @@ -454,71 +557,87 @@ await transport.SendBatchAsync().ConfigureAwait(false)) // Flush and stop the SDK. public static void Shutdown() { - if (!_initialized) return; - - // Drain in-flight timer callbacks before disposing dependents. - // Parameterless Timer.Dispose returns immediately and would race SendBatch. - var timer = _sendTimer; - if (timer != null) + // Serialize with Init and SetConsent under the same lock. Without + // this, Shutdown racing a concurrent Init could observe + // _initialized = true (volatile set by Init) and proceed to tear + // down fields Init is still in the middle of assigning, or Init + // could finish assigning fields that Shutdown already disposed. + // The lock also pins _controlClient / _shutdownCancellationSource + // against a SetConsent whose SyncConsentToBackend Task closure + // captured them just before Shutdown disposed them. + lock (_initLock) { - using var disposed = new ManualResetEvent(false); - if (timer.Dispose(disposed)) + if (!_initialized) return; + + // End the session first so session_end hits the queue before + // the final flush. + _session?.Dispose(); + _session = null; + + // Drain in-flight timer callbacks before disposing dependents. + // Parameterless Timer.Dispose returns immediately and would race SendBatch. + var timer = _sendTimer; + if (timer != null) { - disposed.WaitOne(TimeSpan.FromSeconds(2)); + using var disposed = new ManualResetEvent(false); + if (timer.Dispose(disposed)) + { + disposed.WaitOne(TimeSpan.FromSeconds(2)); + } + _sendTimer = null; } - _sendTimer = null; - } - // Clear the in-flight guard in case the WaitOne above timed out - // with a SendBatch callback still running: without this, a later - // Init would leave _sendInFlight stranded at 1 and suppress every - // tick of the new timer. - Interlocked.Exchange(ref _sendInFlight, 0); + // Clear the in-flight guard in case the WaitOne above timed out + // with a SendBatch callback still running: without this, a later + // Init would leave _sendInFlight stranded at 1 and suppress every + // tick of the new timer. + Interlocked.Exchange(ref _sendInFlight, 0); - _queue?.Shutdown(); + _queue?.Shutdown(); - // Best-effort final send, capped so a slow network can't hang quit. - if (_transport != null) - { - var timeoutMs = _config?.ShutdownFlushTimeoutMs ?? 2_000; - try + // Best-effort final send, capped so a slow network can't hang quit. + if (_transport != null) { - var send = _transport.SendBatchAsync(); - if (!send.Wait(timeoutMs)) + var timeoutMs = _config?.ShutdownFlushTimeoutMs ?? 2_000; + try { - Log.Warn($"Shutdown flush exceeded {timeoutMs}ms — abandoning. " + - "Queued events remain on disk and will retry on next startup."); + var send = _transport.SendBatchAsync(); + if (!send.Wait(timeoutMs)) + { + Log.Warn($"Shutdown flush exceeded {timeoutMs}ms — abandoning. " + + "Queued events remain on disk and will retry on next startup."); + } + } + catch (Exception ex) + { + Log.Warn($"Shutdown flush threw: {ex.GetType().Name}: {ex.Message}"); } } - catch (Exception ex) - { - Log.Warn($"Shutdown flush threw: {ex.GetType().Name}: {ex.Message}"); - } - } - // Cancel in-flight control-plane HTTP requests (DeleteData / SyncConsentToBackend) - // before disposing the client so awaiting callers observe OperationCanceledException - // rather than ObjectDisposedException. - _shutdownCancellationSource?.Cancel(); - - _transport?.Dispose(); - _queue?.Dispose(); - _controlClient?.Dispose(); - _shutdownCancellationSource?.Dispose(); - _shutdownCancellationSource = null; - - // Drop Identity's in-memory cache so a subsequent Init with a - // different persistentDataPath reads the file from the new path - // instead of returning the previous session's id. - Identity.ClearCache(); - - _initialized = false; - _config = null; - _store = null; - _queue = null; - _transport = null; - _controlClient = null; - _userId = null; + // Cancel in-flight control-plane HTTP requests (DeleteData / SyncConsentToBackend) + // before disposing the client so awaiting callers observe OperationCanceledException + // rather than ObjectDisposedException. + _shutdownCancellationSource?.Cancel(); + + _transport?.Dispose(); + _queue?.Dispose(); + _controlClient?.Dispose(); + _shutdownCancellationSource?.Dispose(); + _shutdownCancellationSource = null; + + // Drop Identity's in-memory cache so a subsequent Init with a + // different persistentDataPath reads the file from the new path + // instead of returning the previous session's id. + Identity.ClearCache(); + + _initialized = false; + _config = null; + _store = null; + _queue = null; + _transport = null; + _controlClient = null; + _userId = null; + } } // ----------------------------------------------------------------- @@ -532,14 +651,25 @@ public static void Shutdown() // re-assigns it on the same SubsystemRegistration call. internal static void ResetState() { - if (_initialized) - Shutdown(); - - _consent = ConsentLevel.None; - // Drop Identity's static cache so a subsequent Init with a different - // persistentDataPath (tests, domain reload with changed config) reads - // the file from the new path, not the previous session's cached id. - Identity.ClearCache(); + // Same lock as Shutdown/Init so a concurrent Init cannot repopulate + // fields we are in the middle of clearing. Monitor is recursive, + // so the inner Shutdown() re-enters cleanly on the same thread. + lock (_initLock) + { + if (_initialized) + Shutdown(); + + _consent = ConsentLevel.None; + // Shutdown already nulls _session. Repeat here as a defensive + // belt-and-braces step so a future Shutdown refactor that bails + // before the null (early return on a new guard, reordering, + // etc.) cannot leak a stale Session into the next Init cycle. + _session = null; + // Drop Identity's static cache so a subsequent Init with a different + // persistentDataPath (tests, domain reload with changed config) reads + // the file from the new path, not the previous session's cached id. + Identity.ClearCache(); + } } internal static ConsentLevel CurrentConsentForTesting => _consent; @@ -550,6 +680,13 @@ internal static void ResetState() // guard can be exercised without a real timer. internal static void SendBatchForTesting() => SendBatch(); + // Drives a single heartbeat through the active Session so lifecycle + // tests can assert that OnPause / OnResume actually route to + // Session.Pause / Session.Resume. The real heartbeat cadence is + // 60 s (Session.HeartbeatIntervalMs) so a timer-driven pass would + // either take 60 s or need a bespoke interval override per test. + internal static void InvokeSessionHeartbeatForTesting() => _session?.OnHeartbeat(); + // ----------------------------------------------------------------- // Private // ----------------------------------------------------------------- @@ -662,6 +799,14 @@ private static void FireGameLaunch(AudienceConfig config, ConsentLevel consentAt if (config.DistributionPlatform != null) properties["distributionPlatform"] = config.DistributionPlatform; + // No sessionId on game_launch. Event Reference (v1) schema for + // game_launch lists platform, version, buildGuid, + // distributionPlatform, unityVersion — not sessionId. + // Correlation with the session happens at the pipeline layer via + // eventTimestamp ordering; session_start fires immediately + // before game_launch (see Init ordering) and carries the id + // explicitly. + Track("game_launch", properties.Count > 0 ? properties : null); } } diff --git a/src/Packages/Audience/Runtime/Utility/Log.cs b/src/Packages/Audience/Runtime/Utility/Log.cs index af7ee940d..02a69ffba 100644 --- a/src/Packages/Audience/Runtime/Utility/Log.cs +++ b/src/Packages/Audience/Runtime/Utility/Log.cs @@ -22,12 +22,25 @@ internal static void Warn(string message) => private static void Emit(string line) { - if (Writer != null) + // Swallow anything the Writer (or Console.WriteLine) throws so + // callers can treat Log.Warn / Log.Debug as never-throwing. The + // SDK's safety wrappers (Session.SafeTrack, SafePerformanceSnapshot, + // Shutdown's flush-timeout path) log from inside their own catch + // blocks; a throwing Writer would otherwise escape the wrapper and + // propagate to the Timer thread (process kill on .NET 5+) or to + // Application.quitting (blocking shutdown). + try + { + if (Writer != null) + { + Writer(line); + return; + } + Console.WriteLine(line); + } + catch { - Writer(line); - return; } - Console.WriteLine(line); } } } diff --git a/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs b/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs new file mode 100644 index 000000000..23039f82b --- /dev/null +++ b/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs @@ -0,0 +1,914 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Threading; +using NUnit.Framework; + +namespace Immutable.Audience.Tests +{ + [TestFixture] + internal class SessionTests + { + private List<(string name, Dictionary props)> _events; + + [SetUp] + public void SetUp() + { + _events = new List<(string, Dictionary)>(); + } + + private void MockTrack(string name, Dictionary props) + { + _events.Add((name, props)); + } + + // ----------------------------------------------------------------- + // Start / End + // ----------------------------------------------------------------- + + [Test] + public void Start_FiresSessionStart_WithSessionId() + { + using var session = new Session(MockTrack); + session.Start(); + + Assert.AreEqual(1, _events.Count); + Assert.AreEqual("session_start", _events[0].name); + Assert.IsTrue(_events[0].props.ContainsKey("sessionId")); + Assert.IsNotEmpty((string)_events[0].props["sessionId"]); + } + + [Test] + public void Start_GeneratesUniqueSessionId() + { + using var session = new Session(MockTrack); + session.Start(); + var id1 = session.SessionId; + + session.End(); + session.Start(); + var id2 = session.SessionId; + + Assert.AreNotEqual(id1, id2); + } + + [Test] + public void End_FiresSessionEnd_WithDuration() + { + var now = new DateTime(2026, 4, 20, 12, 0, 0, DateTimeKind.Utc); + using var session = new Session(MockTrack, performanceSnapshot: null, getUtcNow: () => now); + session.Start(); + now = now.AddSeconds(2); + session.End(); + + var endEvent = _events.FirstOrDefault(e => e.name == "session_end"); + Assert.IsNotNull(endEvent.props); + Assert.IsTrue(endEvent.props.ContainsKey("sessionId")); + Assert.IsTrue(endEvent.props.ContainsKey("duration")); + Assert.AreEqual(2L, (long)endEvent.props["duration"]); + } + + [Test] + public void Dispose_FiresSessionEnd() + { + var session = new Session(MockTrack); + session.Start(); + session.Dispose(); + + Assert.IsTrue(_events.Any(e => e.name == "session_end")); + } + + [Test] + public void Dispose_CalledTwice_DoesNotFireTwice() + { + var session = new Session(MockTrack); + session.Start(); + session.Dispose(); + var count = _events.Count(e => e.name == "session_end"); + session.Dispose(); + Assert.AreEqual(count, _events.Count(e => e.name == "session_end")); + } + + // ----------------------------------------------------------------- + // Heartbeat + // ----------------------------------------------------------------- + + [Test] + public void Heartbeat_FiresAfterInterval() + { + // Timer-driven heartbeat path. Uses the heartbeatIntervalMs + // constructor override so we can observe the timer without waiting + // the production 60-second cadence, and a ManualResetEvent to + // rendezvous on the thread-pool callback. + using var heartbeatFired = new ManualResetEvent(false); + var events = new List<(string name, Dictionary props)>(); + var gate = new object(); + + void Track(string name, Dictionary props) + { + lock (gate) events.Add((name, props)); + if (name == "session_heartbeat") heartbeatFired.Set(); + } + + using var session = new Session(Track, heartbeatIntervalMs: 50); + session.Start(); + + Assert.IsTrue(heartbeatFired.WaitOne(TimeSpan.FromSeconds(5)), + "timer-driven heartbeat should fire within 5s of a 50ms interval"); + + lock (gate) + { + var beat = events.FirstOrDefault(e => e.name == "session_heartbeat"); + Assert.IsNotNull(beat.props, "heartbeat event should carry a properties dictionary"); + Assert.IsTrue(beat.props.ContainsKey("sessionId")); + Assert.IsTrue(beat.props.ContainsKey("duration")); + } + } + + [Test] + public void Heartbeat_WithoutPerformanceSnapshot_OnlyCarriesCoreProperties() + { + using var session = new Session(MockTrack); + session.Start(); + + session.OnHeartbeat(); + + var beat = _events.Last(e => e.name == "session_heartbeat"); + CollectionAssert.AreEquivalent( + new[] { "sessionId", "duration" }, + beat.props.Keys); + } + + [Test] + public void Heartbeat_MergesPerformanceSnapshotProperties() + { + Func> snapshot = () => new Dictionary + { + ["fpsAvg"] = 58.4, + ["fpsMin"] = 42.1, + ["memoryUsedMb"] = 512L, + ["memoryReservedMb"] = 768L, + }; + using var session = new Session(MockTrack, snapshot); + session.Start(); + + session.OnHeartbeat(); + + var beat = _events.Last(e => e.name == "session_heartbeat"); + Assert.AreEqual(58.4, beat.props["fpsAvg"]); + Assert.AreEqual(42.1, beat.props["fpsMin"]); + Assert.AreEqual(512L, beat.props["memoryUsedMb"]); + Assert.AreEqual(768L, beat.props["memoryReservedMb"]); + Assert.IsTrue(beat.props.ContainsKey("sessionId")); + Assert.IsTrue(beat.props.ContainsKey("duration")); + } + + [Test] + public void Heartbeat_SnapshotCannotOverwriteCoreFields() + { + // Core fields (sessionId, duration) are owned by Session. A + // provider that returns a dictionary containing either key must + // not be able to clobber the wire values — otherwise a buggy + // studio-side snapshot could silently rewrite session identity + // or engagement arithmetic on every heartbeat. Sabotage: removing + // the ContainsKey guard lets "spoofed-id" and 99999L land on the + // wire and both assertions below fail. + Func> snapshot = () => new Dictionary + { + ["sessionId"] = "spoofed-id", + ["duration"] = 99999L, + ["fpsAvg"] = 60.0, + }; + using var session = new Session(MockTrack, snapshot); + session.Start(); + + session.OnHeartbeat(); + + var beat = _events.Last(e => e.name == "session_heartbeat"); + Assert.AreNotEqual("spoofed-id", (string)beat.props["sessionId"], + "snapshot must not overwrite Session-owned sessionId"); + Assert.AreNotEqual(99999L, (long)beat.props["duration"], + "snapshot must not overwrite Session-owned duration"); + Assert.AreEqual(60.0, beat.props["fpsAvg"], + "non-colliding snapshot fields should still merge"); + } + + [Test] + public void Heartbeat_SnapshotReturningNull_DoesNotThrowAndOmitsFields() + { + Func> snapshot = () => null; + using var session = new Session(MockTrack, snapshot); + session.Start(); + + session.OnHeartbeat(); + + var beat = _events.Last(e => e.name == "session_heartbeat"); + Assert.IsFalse(beat.props.ContainsKey("fpsAvg")); + } + + // ----------------------------------------------------------------- + // Pause / Resume + // ----------------------------------------------------------------- + + [Test] + public void Pause_ThenResume_ShortPause_ContinuesSession() + { + var now = new DateTime(2026, 4, 20, 12, 0, 0, DateTimeKind.Utc); + using var session = new Session(MockTrack, performanceSnapshot: null, getUtcNow: () => now); + session.Start(); + var originalId = session.SessionId; + + session.Pause(); + now = now.AddSeconds(5); // well under the 30 s threshold + session.Resume(); + + Assert.AreEqual(originalId, session.SessionId, "short pause should not change session"); + Assert.IsFalse(_events.Any(e => e.name == "session_end"), + "short pause should not fire session_end"); + } + + [Test] + public void Pause_ThenResume_LongPause_StartsNewSession() + { + // Uses the injected clock to jump past the 30-second threshold + // without sleeping. + var now = new DateTime(2026, 4, 20, 12, 0, 0, DateTimeKind.Utc); + using var session = new Session(MockTrack, performanceSnapshot: null, getUtcNow: () => now); + session.Start(); + var id1 = session.SessionId; + + session.Pause(); + now = now.AddSeconds(31); // > 30 second PauseTimeoutMs + session.Resume(); + + Assert.AreNotEqual(id1, session.SessionId, + "pause longer than PauseTimeoutMs should end the old session and start a new one"); + Assert.IsTrue(_events.Any(e => e.name == "session_end"), + "old session should have fired session_end"); + Assert.AreEqual(2, _events.Count(e => e.name == "session_start"), + "a new session_start should fire after the long pause"); + } + + [Test] + public void Pause_CalledTwice_SecondCallIsNoOp() + { + // Double-Pause without an intervening Resume must not advance + // _pausedAt. The live-pause window stays anchored to the first + // Pause so engagement arithmetic covers the full backgrounded + // interval. Sabotage: removing the already-paused guard makes + // _pausedAt jump to the second Pause and this test reports a + // larger duration (over-crediting engagement by the double-pause + // gap). + var now = new DateTime(2026, 4, 20, 12, 0, 0, DateTimeKind.Utc); + DateTime Clock() => now; + + using var session = new Session(MockTrack, performanceSnapshot: null, getUtcNow: Clock); + session.Start(); + + now = now.AddSeconds(5); + session.Pause(); // first Pause anchors _pausedAt at T=5 + + now = now.AddSeconds(3); + session.Pause(); // second Pause at T=8 — should be ignored + + now = now.AddSeconds(2); // paused for another 2s + session.Resume(); // resume at T=10, pause window spans T=5 to T=10 + + now = now.AddSeconds(3); // 3s more engagement + session.End(); + + var sessionEnd = _events.Last(e => e.name == "session_end"); + var duration = (long)sessionEnd.props["duration"]; + // Wall-clock Start→End = 13s, paused from T=5 to T=10 = 5s, engaged = 8s. + Assert.AreEqual(8L, duration, + "double Pause must preserve the first Pause timestamp so engagement arithmetic covers the full pause window"); + } + + [Test] + public void Resume_WithoutPause_IsNoOp() + { + using var session = new Session(MockTrack); + session.Start(); + var eventsBefore = _events.Count; + + session.Resume(); + + Assert.AreEqual(eventsBefore, _events.Count, "resume without pause should not fire events"); + } + + // ----------------------------------------------------------------- + // Pause-adjusted duration + // ----------------------------------------------------------------- + + [Test] + public void Resume_NegativePauseDuration_ClampsAccumulatorToZero() + { + // Wall-clock can rewind during a pause: NTP correction, manual + // clock change, or a debugger that froze the process. Without + // the clamp at Resume, a negative pauseDuration would shrink + // _accumulatedPause and hand the next session_end an + // artificial engagement credit. Sabotage: removing the clamp + // would let this test report a duration that exceeds the + // wall-clock window, which the assertion below pins. + var now = new DateTime(2026, 4, 20, 12, 0, 0, DateTimeKind.Utc); + DateTime Clock() => now; + + using var session = new Session(MockTrack, performanceSnapshot: null, getUtcNow: Clock); + session.Start(); + + now = now.AddSeconds(10); // 10 s engaged + session.Pause(); + + now = now.AddSeconds(-5); // clock rewinds 5 s during the pause + session.Resume(); + + now = now.AddSeconds(2); // 2 s further engagement after resume + session.End(); + + var sessionEnd = _events.Last(e => e.name == "session_end"); + var duration = (long)sessionEnd.props["duration"]; + // Wall-clock from Start to End is 10 + (-5) + 2 = 7 s. The + // pause duration was clamped to 0, so engaged seconds = 7 - 0 = 7. + // Without the clamp, _accumulatedPause would be -5, the + // subtraction would over-credit, and engaged seconds would + // be 12 — well outside the wall-clock window. + Assert.AreEqual(7L, duration, + "negative pauseDuration must clamp to zero so the accumulator does not over-credit engagement"); + } + + [Test] + public void End_ClockRewindsSinceStart_ClampsDurationToZero() + { + // Wall-clock can rewind between Start and End with no pause + // in between (server-side NTP correction, headless build with + // a manual clock set). Without the clamp in + // ComputeEngagedSecondsLocked, End would ship a negative + // duration. The Resume-side clamp does not cover this path + // because it only fires when _pausedAt was set. Sabotage: + // removing `if (engagedSeconds < 0) return 0;` would let this + // test report -3 instead of 0. + var now = new DateTime(2026, 4, 20, 12, 0, 0, DateTimeKind.Utc); + DateTime Clock() => now; + + using var session = new Session(MockTrack, performanceSnapshot: null, getUtcNow: Clock); + session.Start(); + + now = now.AddSeconds(-3); // clock rewinds after Start, no pause + session.End(); + + var sessionEnd = _events.Last(e => e.name == "session_end"); + var duration = (long)sessionEnd.props["duration"]; + Assert.AreEqual(0L, duration, + "negative engaged time from a wall-clock rewind must clamp to zero"); + } + + [Test] + public void End_AfterShortPause_ReportsDurationMinusPause() + { + // 10 seconds session, 3 seconds paused inside it → 7 seconds engaged. + var now = new DateTime(2026, 4, 20, 12, 0, 0, DateTimeKind.Utc); + DateTime Clock() => now; + + using var session = new Session(MockTrack, performanceSnapshot: null, getUtcNow: Clock); + session.Start(); + + now = now.AddSeconds(4); + session.Pause(); + + now = now.AddSeconds(3); // 3 s paused + session.Resume(); + + now = now.AddSeconds(3); + session.End(); + + var sessionEnd = _events.Last(e => e.name == "session_end"); + var duration = (long)sessionEnd.props["duration"]; + Assert.AreEqual(7L, duration, + "session_end duration should exclude the 3s paused interval"); + } + + [Test] + public void End_WhilePaused_ExcludesInFlightPauseFromDuration() + { + // Session running 5s, then paused for 2s and ended without resuming. + var now = new DateTime(2026, 4, 20, 12, 0, 0, DateTimeKind.Utc); + DateTime Clock() => now; + + using var session = new Session(MockTrack, performanceSnapshot: null, getUtcNow: Clock); + session.Start(); + + now = now.AddSeconds(5); + session.Pause(); + + now = now.AddSeconds(2); + session.End(); // ends while paused + + var sessionEnd = _events.Last(e => e.name == "session_end"); + var duration = (long)sessionEnd.props["duration"]; + Assert.AreEqual(5L, duration, + "session_end fired while paused should count only pre-pause engaged time"); + } + + [Test] + public void End_AfterExtendedPauseRollover_ReportsPrePauseDuration() + { + // Extended pause (>30 s) on Resume runs End → Start. The + // session_end event for the old session must report only the + // engaged time before the pause, not wall-clock from start to + // resume (which would include the pause). Regression guard + // for the extended-pause rollover path: a naive duration that + // forgot to credit the in-flight pause before End fires would + // ship wall-clock seconds and break engagement dashboards. + var now = new DateTime(2026, 4, 20, 12, 0, 0, DateTimeKind.Utc); + DateTime Clock() => now; + + using var session = new Session(MockTrack, performanceSnapshot: null, getUtcNow: Clock); + session.Start(); + + now = now.AddSeconds(10); // 10 s engaged before pause + session.Pause(); + + now = now.AddSeconds(40); // 40 s paused — extended (>30 s threshold) + session.Resume(); + + var sessionEnd = _events.First(e => e.name == "session_end"); + var duration = (long)sessionEnd.props["duration"]; + Assert.AreEqual(10L, duration, + "session_end on extended-pause rollover should report pre-pause engaged time, not wall-clock"); + } + + [Test] + public void Heartbeat_AfterShortPause_ReportsPauseAdjustedDuration() + { + // Engaged 6s, paused 2s, resumed, then heartbeat → 6 s engaged. + var now = new DateTime(2026, 4, 20, 12, 0, 0, DateTimeKind.Utc); + DateTime Clock() => now; + + using var session = new Session(MockTrack, performanceSnapshot: null, getUtcNow: Clock); + session.Start(); + + now = now.AddSeconds(4); + session.Pause(); + + now = now.AddSeconds(2); + session.Resume(); + + now = now.AddSeconds(2); + session.OnHeartbeat(); + + var heartbeat = _events.Last(e => e.name == "session_heartbeat"); + var duration = (long)heartbeat.props["duration"]; + Assert.AreEqual(6L, duration, + "heartbeat duration should exclude the 2s paused interval"); + } + + // ----------------------------------------------------------------- + // Double-Start safety + // ----------------------------------------------------------------- + + [Test] + public void Start_WithoutPriorEnd_DoesNotStrandTheOldTimer() + { + // Call Start twice in a row. The implementation should stop the + // first timer cleanly; the second heartbeat scheduling should + // succeed and the second session id takes over. + using var session = new Session(MockTrack); + session.Start(); + var firstId = session.SessionId; + + session.Start(); + var secondId = session.SessionId; + + Assert.AreNotEqual(firstId, secondId, + "second Start should generate a fresh sessionId"); + + Assert.AreEqual(2, _events.Count(e => e.name == "session_start"), + "both session_start events should fire"); + } + + // ----------------------------------------------------------------- + // Heartbeat-during-pause quiescence + // ----------------------------------------------------------------- + + [Test] + public void Heartbeat_WhilePaused_DoesNotFire() + { + // The class contract is session_heartbeat every 60s while + // foregrounded. A paused session is backgrounded by definition, + // so OnHeartbeat must not emit until Resume clears the pause. + // Without this guard a backgrounded alt-tab would dribble + // stable-duration heartbeats for the entire pause window. + using var session = new Session(MockTrack); + session.Start(); + session.Pause(); + + session.OnHeartbeat(); + + Assert.IsFalse(_events.Any(e => e.name == "session_heartbeat"), + "OnHeartbeat should not emit while the session is paused"); + } + + [Test] + public void End_HeartbeatExceedsDrainBudget_LogsWarningAndContinues() + { + // DrainHeartbeatTimer waits up to 1 second for an in-flight + // heartbeat callback before End emits session_end. If the + // callback exceeds the budget the implementation logs a + // warning and continues, accepting the risk of a trailing + // heartbeat racing the next lifecycle event. Sabotage paths + // that would skip the warning: raising the budget past the + // 1.5 s callback sleep (WaitOne returns true before timeout); + // removing the `if (!waited.WaitOne(...))` guard. WaitOne + // (TimeSpan.Zero) on an unsignaled handle returns false, so + // lowering the budget does not skip the warning. This test + // pins the budget-exceeded path so future drain-budget edits + // cannot silently drop the warning. + var warnings = new List(); + var prevWriter = Log.Writer; + Log.Writer = line => { lock (warnings) warnings.Add(line); }; + try + { + using var beatStarted = new ManualResetEvent(false); + void Track(string name, Dictionary props) + { + if (name == "session_heartbeat") + { + beatStarted.Set(); + // Block past the 1 s drain budget so DrainHeartbeatTimer + // times out. Self-releases after 1.5 s so the callback + // does eventually finish. + Thread.Sleep(1500); + } + } + + using var session = new Session(Track, heartbeatIntervalMs: 50); + session.Start(); + Assert.IsTrue(beatStarted.WaitOne(TimeSpan.FromSeconds(2)), + "heartbeat callback must enter before End is invoked"); + + session.End(); + + lock (warnings) + { + Assert.IsTrue(warnings.Any(w => w.Contains("heartbeat callback did not complete")), + "End must log the drain-timeout warning when an in-flight heartbeat exceeds 1 s"); + } + } + finally + { + Log.Writer = prevWriter; + } + } + + [Test] + public void Heartbeat_AfterResume_Fires() + { + // Pair for Heartbeat_WhilePaused_DoesNotFire: once Resume + // clears _pausedAt, heartbeats must flow again. + using var session = new Session(MockTrack); + session.Start(); + session.Pause(); + session.Resume(); + + session.OnHeartbeat(); + + Assert.IsTrue(_events.Any(e => e.name == "session_heartbeat"), + "OnHeartbeat should emit again once the session is resumed"); + } + + // ----------------------------------------------------------------- + // Exception containment + // ----------------------------------------------------------------- + + [Test] + public void OnHeartbeat_TrackCallbackThrows_DoesNotEscape() + { + // OnHeartbeat runs on a thread-pool timer callback; an unhandled + // exception there can terminate the process on .NET 5+. The + // SafeTrack wrapper catches and logs instead. Sabotage: removing + // the try/catch in SafeTrack would let the exception propagate + // up to the caller (which here is the test thread, so this + // assertion would fail). + var warnings = new List(); + var prevWriter = Log.Writer; + Log.Writer = line => { lock (warnings) warnings.Add(line); }; + try + { + void ThrowingTrack(string name, Dictionary props) + { + if (name == "session_heartbeat") + throw new InvalidOperationException("track explode"); + } + + using var session = new Session(ThrowingTrack); + session.Start(); + + Assert.DoesNotThrow(() => session.OnHeartbeat(), + "a throwing track callback on the heartbeat path must not escape Session"); + + lock (warnings) + { + Assert.IsTrue(warnings.Any(w => w.Contains("session_heartbeat track callback threw")), + "SafeTrack must log a warning when the callback throws"); + } + } + finally { Log.Writer = prevWriter; } + } + + [Test] + public void OnHeartbeat_PerformanceSnapshotThrows_ShipsHeartbeatWithoutPerfFields() + { + // PerformanceSnapshotProvider is studio-supplied and crosses an + // API boundary. A throwing provider must not prevent the + // heartbeat from shipping — the SafePerformanceSnapshot wrapper + // returns null on exception so the heartbeat ships with the + // core fields only. + var warnings = new List(); + var prevWriter = Log.Writer; + Log.Writer = line => { lock (warnings) warnings.Add(line); }; + try + { + Func> snapshot = () => + throw new InvalidOperationException("perf explode"); + + using var session = new Session(MockTrack, snapshot); + session.Start(); + + Assert.DoesNotThrow(() => session.OnHeartbeat(), + "a throwing performance snapshot must not escape Session"); + + var beat = _events.Last(e => e.name == "session_heartbeat"); + CollectionAssert.AreEquivalent( + new[] { "sessionId", "duration" }, + beat.props.Keys, + "heartbeat should carry only the core fields when the snapshot throws"); + + lock (warnings) + { + Assert.IsTrue(warnings.Any(w => w.Contains("performance snapshot threw")), + "SafePerformanceSnapshot must log a warning when the provider throws"); + } + } + finally { Log.Writer = prevWriter; } + } + + [Test] + public void Start_TrackCallbackThrows_DoesNotEscape() + { + // Start fires session_start via _track. A throwing implementation + // would otherwise propagate into Init / SetConsent on the caller's + // thread. SafeTrack swallows and logs. Sabotage: removing + // SafeTrack's try/catch fails Assert.DoesNotThrow; a regression + // that swallows silently without logging fails the warning check. + var warnings = new List(); + var prevWriter = Log.Writer; + Log.Writer = line => { lock (warnings) warnings.Add(line); }; + try + { + void ThrowingTrack(string name, Dictionary props) + { + if (name == "session_start") + throw new InvalidOperationException("track explode"); + } + + using var session = new Session(ThrowingTrack); + + Assert.DoesNotThrow(() => session.Start(), + "a throwing track callback on session_start must not escape Start"); + + lock (warnings) + { + Assert.IsTrue(warnings.Any(w => w.Contains("session_start track callback threw")), + "SafeTrack must log a warning when the Start callback throws"); + } + } + finally { Log.Writer = prevWriter; } + } + + [Test] + public void End_TrackCallbackThrows_DoesNotEscape() + { + // End fires session_end via _track. Dispose wraps End, so a + // throwing implementation would otherwise propagate into + // Shutdown / SetConsent on the caller's thread. Sabotage: + // removing SafeTrack's try/catch fails Assert.DoesNotThrow; a + // regression that swallows silently without logging fails the + // warning check. + var warnings = new List(); + var prevWriter = Log.Writer; + Log.Writer = line => { lock (warnings) warnings.Add(line); }; + try + { + void ThrowingTrack(string name, Dictionary props) + { + if (name == "session_end") + throw new InvalidOperationException("track explode"); + } + + using var session = new Session(ThrowingTrack); + session.Start(); + + Assert.DoesNotThrow(() => session.End(), + "a throwing track callback on session_end must not escape End"); + + lock (warnings) + { + Assert.IsTrue(warnings.Any(w => w.Contains("session_end track callback threw")), + "SafeTrack must log a warning when the End callback throws"); + } + } + finally { Log.Writer = prevWriter; } + } + + [Test] + public void SafeTrack_LogWriterThrows_DoesNotEscape() + { + // SafeTrack logs to Log.Warn when the _track delegate throws. If + // the Log.Writer itself throws, the log call would escape + // SafeTrack's catch and propagate up to the Timer thread (process + // kill on .NET 5+) or the caller thread (Init / Shutdown / + // SetConsent). Log.Emit's internal try/catch must swallow the + // Writer failure. Sabotage: removing Log.Emit's try/catch would + // fail Assert.DoesNotThrow below. + var prevWriter = Log.Writer; + Log.Writer = _ => throw new InvalidOperationException("log explode"); + try + { + void ThrowingTrack(string name, Dictionary props) + { + if (name == "session_heartbeat") + throw new InvalidOperationException("track explode"); + } + + using var session = new Session(ThrowingTrack); + session.Start(); + + Assert.DoesNotThrow(() => session.OnHeartbeat(), + "Log.Writer throwing inside SafeTrack's catch must not escape Session"); + } + finally { Log.Writer = prevWriter; } + } + } + + // ----------------------------------------------------------------- + // Session integration through ImmutableAudience + // ----------------------------------------------------------------- + + [TestFixture] + internal class SessionIntegrationTests + { + private string _testDir; + + [SetUp] + public void SetUp() + { + _testDir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); + Directory.CreateDirectory(_testDir); + } + + [TearDown] + public void TearDown() + { + ImmutableAudience.ResetState(); + Identity.Reset(_testDir); + if (Directory.Exists(_testDir)) + Directory.Delete(_testDir, recursive: true); + } + + private AudienceConfig MakeConfig(ConsentLevel consent = ConsentLevel.Anonymous) + { + return new AudienceConfig + { + PublishableKey = "pk_imapik-test-key1", + Consent = consent, + PersistentDataPath = _testDir, + FlushIntervalSeconds = 600, + FlushSize = 1000, + HttpHandler = new KeepOnDiskHandler() + }; + } + + // Reads every queued event file. Returns an empty array when the + // queue directory has not been created yet (SetConsent(None) purges + // it, Init with ConsentLevel.None never writes one) so tests can + // assert "no such event" without a DirectoryNotFoundException crash + // masking the real signal. + private string[] ReadQueueFiles() + { + var queueDir = Path.Combine(_testDir, "imtbl_audience", "queue"); + if (!Directory.Exists(queueDir)) return Array.Empty(); + return Directory.GetFiles(queueDir, "*.json").Select(File.ReadAllText).ToArray(); + } + + [Test] + public void Init_FiresSessionStart() + { + ImmutableAudience.Init(MakeConfig()); + ImmutableAudience.Shutdown(); + + Assert.IsTrue(ReadQueueFiles().Any(c => c.Contains("\"session_start\""))); + } + + [Test] + public void Shutdown_FiresSessionEnd() + { + ImmutableAudience.Init(MakeConfig()); + ImmutableAudience.Shutdown(); + + Assert.IsTrue(ReadQueueFiles().Any(c => c.Contains("\"session_end\""))); + } + + [Test] + public void Init_ConsentNone_DoesNotStartSession() + { + ImmutableAudience.Init(MakeConfig(ConsentLevel.None)); + ImmutableAudience.Shutdown(); + + Assert.IsFalse(ReadQueueFiles().Any(c => c.Contains("\"session_start\""))); + } + + [Test] + public void SetConsent_NoneToAnonymous_StartsSession() + { + ImmutableAudience.Init(MakeConfig(ConsentLevel.None)); + ImmutableAudience.SetConsent(ConsentLevel.Anonymous); + ImmutableAudience.Shutdown(); + + Assert.IsTrue(ReadQueueFiles().Any(c => c.Contains("\"session_start\"")), + "upgrading from None should start a session"); + } + + [Test] + public void OnPauseAndOnResume_RouteThroughActiveSession() + { + // Pin that OnPause / OnResume actually reach the live Session, + // not just that they compile. A heartbeat fired while the + // session is paused must not emit; after OnResume the next + // heartbeat must emit. Sabotage: emptying either wrapper body + // fails one of the two assertions below. + // + // Signature-pin (rename / parameter change) also still holds + // via the direct ImmutableAudience.OnPause() / OnResume() call + // sites — a change there breaks compilation here. + ImmutableAudience.Init(MakeConfig()); + + ImmutableAudience.OnPause(); + ImmutableAudience.InvokeSessionHeartbeatForTesting(); + ImmutableAudience.FlushQueueToDiskForTesting(); + + Assert.IsFalse( + ReadQueueFiles().Any(c => c.Contains("\"session_heartbeat\"")), + "OnPause must route to Session.Pause — paused sessions quiesce heartbeats"); + + ImmutableAudience.OnResume(); + ImmutableAudience.InvokeSessionHeartbeatForTesting(); + ImmutableAudience.FlushQueueToDiskForTesting(); + + Assert.IsTrue( + ReadQueueFiles().Any(c => c.Contains("\"session_heartbeat\"")), + "OnResume must route to Session.Resume — resumed sessions emit heartbeats again"); + + ImmutableAudience.Shutdown(); + } + + [Test] + public void OnPauseAndOnResume_BeforeInit_AreNoOps() + { + // Both wrappers gate on _initialized and return early. A + // lifecycle bridge that fires before Init (rare, but possible + // on subsystem-registration order quirks) must not throw. + Assert.DoesNotThrow(() => ImmutableAudience.OnPause()); + Assert.DoesNotThrow(() => ImmutableAudience.OnResume()); + } + + [Test] + public void SetConsent_AnonymousToNone_DoesNotEmitSessionEnd() + { + // Consent revocation purges the queue and disposes the session. + // Session.Dispose fires End → Track("session_end"), but by the + // time End runs the consent level has already been flipped to + // None, so CanTrack gates the track call. No session_end event + // should appear on disk. Regression guard: a future reorder of + // "flip consent" vs "dispose session" would silently leak a + // session_end that the consent-revocation promise forbids. + ImmutableAudience.Init(MakeConfig(ConsentLevel.Anonymous)); + ImmutableAudience.SetConsent(ConsentLevel.None); + ImmutableAudience.Shutdown(); + + Assert.IsFalse( + ReadQueueFiles().Any(c => c.Contains("\"session_end\"")), + "SetConsent(None) must not leak a session_end event past the queue purge"); + } + + private class KeepOnDiskHandler : System.Net.Http.HttpMessageHandler + { + protected override System.Threading.Tasks.Task SendAsync( + System.Net.Http.HttpRequestMessage request, System.Threading.CancellationToken ct) + { + return System.Threading.Tasks.Task.FromResult( + new System.Net.Http.HttpResponseMessage(System.Net.HttpStatusCode.ServiceUnavailable)); + } + } + } +} \ No newline at end of file diff --git a/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs.meta b/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs.meta new file mode 100644 index 000000000..0bf7e4c8f --- /dev/null +++ b/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 0a6df3c78a22047d28b52002bac442b5 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs b/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs index c5283f0d4..d3111915e 100644 --- a/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs +++ b/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs @@ -136,16 +136,42 @@ public void Track_NullOrEmptyEventName_DoesNotEnqueue() { ImmutableAudience.Init(MakeConfig()); - var beforeQueue = AudiencePaths.QueueDir(_testDir); - var beforeCount = Directory.Exists(beforeQueue) ? Directory.GetFiles(beforeQueue, "*.json").Length : 0; - Assert.DoesNotThrow(() => ImmutableAudience.Track((string)null)); Assert.DoesNotThrow(() => ImmutableAudience.Track("")); ImmutableAudience.Shutdown(); - var afterCount = Directory.GetFiles(beforeQueue, "*.json").Length; - // Only game_launch should have been enqueued; null/empty Track calls dropped. - Assert.AreEqual(beforeCount + 1, afterCount, "null/empty event names must be dropped, not enqueued"); + + // Assert the invariant directly: no enqueued message carries a + // null or empty eventName. Earlier versions counted files + // before/after the Track calls, which raced with the async + // disk drain — Init enqueues session_start + game_launch, and + // Shutdown adds session_end, so the file count after Shutdown + // is deterministic but the before-count is not. Counting is + // the wrong axis: what the test actually wants to pin is + // "no empty-name event ever hit the queue", regardless of + // what else was enqueued alongside it. + // + // Deserialize each message and inspect the eventName field + // directly. A raw substring scan would false-positive on an + // event whose property value happened to be the literal + // string "eventName":"" (unlikely but possible) and would + // silently break on any future JSON encoding change (whitespace, + // key ordering, escape style). + var queueDir = AudiencePaths.QueueDir(_testDir); + if (!Directory.Exists(queueDir)) return; + foreach (var file in Directory.GetFiles(queueDir, "*.json")) + { + var msg = JsonReader.DeserializeObject(File.ReadAllText(file)); + if ((string)msg["type"] != "track") continue; + + if (!msg.TryGetValue("eventName", out var eventNameObj)) + Assert.Fail($"track message {Path.GetFileName(file)} missing eventName field"); + + Assert.IsNotNull(eventNameObj, + $"queue file {Path.GetFileName(file)} has a null eventName"); + Assert.IsNotEmpty((string)eventNameObj, + $"queue file {Path.GetFileName(file)} has an empty eventName"); + } } [Test] @@ -601,6 +627,127 @@ public void SetConsent_DowngradeToNone_StressTest_NoLeak() } } + [Test] + public void Init_ConcurrentWithSetConsent_LeavesConsistentState() + { + // Pre-fix (before 1784ae3f), SetConsent mutated _consent and + // _session outside any lock. A SetConsent landing between Init's + // _initialized = true and its _session = new Session(...) + // observed _session = null, skipped the dispose path, and let + // Init finish creating a Session whose timer was never disposed. + // + // Limitation: the race window is narrow and not deterministically + // reproducible without a test hook inside Init. This is a + // probabilistic guard — many iterations of concurrent Init / + // SetConsent(None) from two threads, asserting only that the + // final state is consistent (consent is whichever the last lock + // holder set, no exceptions escape, Init did not silently ignore + // the race). Regressions that fully remove SetConsent's lock + // would still show up here via ConsentLevel mismatches or + // exceptions on a majority of iterations. + const int iterations = 50; + for (int iter = 0; iter < iterations; iter++) + { + Exception? initEx = null; + Exception? setConsentEx = null; + + var setConsentTask = Task.Run(() => + { + try + { + Thread.Yield(); + ImmutableAudience.SetConsent(ConsentLevel.None); + } + catch (Exception ex) { setConsentEx = ex; } + }); + + var initTask = Task.Run(() => + { + try { ImmutableAudience.Init(MakeConfig(ConsentLevel.Anonymous)); } + catch (Exception ex) { initEx = ex; } + }); + + Assert.IsTrue(Task.WaitAll(new[] { initTask, setConsentTask }, TimeSpan.FromSeconds(5)), + $"iteration {iter}: Init / SetConsent must complete within 5s"); + Assert.IsNull(initEx, $"iteration {iter}: Init threw {initEx}"); + Assert.IsNull(setConsentEx, $"iteration {iter}: SetConsent threw {setConsentEx}"); + + // Either order is valid: + // - SetConsent runs first: _initialized is false, SetConsent + // early-returns, Init then initialises with Anonymous. + // - Init runs first: Init sets Anonymous, SetConsent flips + // to None under the lock, consent ends at None. + var finalConsent = ImmutableAudience.CurrentConsentForTesting; + Assert.That(finalConsent, + Is.EqualTo(ConsentLevel.None).Or.EqualTo(ConsentLevel.Anonymous), + $"iteration {iter}: unexpected final consent {finalConsent}"); + + ImmutableAudience.ResetState(); + if (Directory.Exists(AudiencePaths.AudienceDir(_testDir))) + Directory.Delete(AudiencePaths.AudienceDir(_testDir), recursive: true); + } + } + + [Test] + public void SetConsent_ConcurrentUpgradeFromNone_StartsOneSession_StressTest() + { + // Starting from ConsentLevel.None, N threads race to + // SetConsent(Anonymous). Without the _initLock in SetConsent, + // multiple threads observe previous == None, each take the + // upgrade branch, each build a fresh Session, each Start() it. + // The last _session = new Session(...) wins; the earlier + // instances keep their heartbeat timers running on the + // thread pool forever (heartbeats land as dropped-by-CanTrack + // no-ops but the Timer allocations leak unbounded). + // + // Wire-visible symptom: multiple session_start events hit the + // queue per iteration. With the lock, exactly one thread + // flips _consent, the rest observe previous == Anonymous and + // return without touching _session. + // + // Sabotage: removing the lock (or widening the else-if to skip + // the previous-consent check) fails this test reliably within + // a handful of iterations. + const int iterations = 100; + const int callersPerIteration = 4; + + for (int iter = 0; iter < iterations; iter++) + { + ImmutableAudience.Init(MakeConfig(ConsentLevel.None)); + + var barrier = new Barrier(callersPerIteration); + var callers = new Task[callersPerIteration]; + for (int c = 0; c < callersPerIteration; c++) + { + callers[c] = Task.Run(() => + { + barrier.SignalAndWait(); + ImmutableAudience.SetConsent(ConsentLevel.Anonymous); + }); + } + + Task.WaitAll(callers, TimeSpan.FromSeconds(5)); + ImmutableAudience.FlushQueueToDiskForTesting(); + + var queueDir = AudiencePaths.QueueDir(_testDir); + var sessionStarts = Directory.Exists(queueDir) + ? Directory.GetFiles(queueDir, "*.json") + .Select(File.ReadAllText) + .Count(c => c.Contains("\"session_start\"")) + : 0; + + if (sessionStarts != 1) + { + Assert.Fail( + $"iteration {iter}: expected exactly one session_start from concurrent SetConsent upgrade, got {sessionStarts}"); + } + + ImmutableAudience.ResetState(); + if (Directory.Exists(AudiencePaths.AudienceDir(_testDir))) + Directory.Delete(AudiencePaths.AudienceDir(_testDir), recursive: true); + } + } + [Test] public void ResetState_ClearsIdentityCache_AcrossInitWithDifferentPath() { From 582b05ea844001777c70aa46055f3c6cd7a2cadc Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 14:11:47 +1000 Subject: [PATCH 02/19] refactor(audience): tighten comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace narrative comment blocks with terse fact-headers. - Session.cs: class header 13→3 lines; method headers condensed; inline rationale kept only where non-obvious. - ImmutableAudience.cs: Init/Shutdown/SetConsent/ResetState narratives condensed; redundant blocks removed. - Log.cs: Emit swallow rationale condensed. No semantic changes. All 218 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/Core/Session.cs | 229 +++----------- .../Audience/Runtime/ImmutableAudience.cs | 283 +++++------------- src/Packages/Audience/Runtime/Utility/Log.cs | 10 +- 3 files changed, 131 insertions(+), 391 deletions(-) diff --git a/src/Packages/Audience/Runtime/Core/Session.cs b/src/Packages/Audience/Runtime/Core/Session.cs index 5c6c0b6b8..8f5b78513 100644 --- a/src/Packages/Audience/Runtime/Core/Session.cs +++ b/src/Packages/Audience/Runtime/Core/Session.cs @@ -6,28 +6,14 @@ namespace Immutable.Audience { - // Session lifecycle. session_start on Init, session_heartbeat every 60s - // while foregrounded (quiesced during Pause), session_end on Shutdown or - // pause > 30s evaluated on the next Resume. Shares the Web SDK wire - // schema (sessionId plus integer "duration" in seconds) but not the - // duration semantic — see End() for detail. Unity-specific extensions: - // engagement-aware duration (excludes pause time) and heartbeat-carried - // performance metrics. Games don't have browser visibility APIs, and - // long alt-tabs would inflate session time if we counted wall-clock. - // - // Thread safety: heartbeat fires on a thread-pool thread; Start / End / - // Pause / Resume / Dispose run on the caller's thread. A private lock - // guards state. The _track callback fires outside the lock so re-entrant - // track implementations can safely take their own locks. + // Unity session lifecycle. Emits session_start / session_heartbeat / session_end. + // duration is engagement time (excludes pause). Heartbeat fires off-thread; + // public methods run on the caller's thread. _track fires outside _lock. internal sealed class Session : IDisposable { internal const int HeartbeatIntervalMs = 60_000; - // 30 seconds. Intentionally short: a player who alt-tabs out of the - // game for half a minute has stopped engaging and the next resume - // should roll the session. Not a port of Web SDK SESSION_MAX_AGE - // (30 minutes idle on the cookie). Browsers have no foreground - // pause concept, so the two values measure different things. + // 30s: alt-tab beyond this rolls the session on Resume. internal const int PauseTimeoutMs = 30_000; private readonly Action> _track; @@ -40,13 +26,7 @@ internal sealed class Session : IDisposable private string? _sessionId; private DateTime _sessionStart; private DateTime? _pausedAt; - // Total pause time accumulated across every Pause/Resume cycle - // since Start. Subtracted from wall-clock in session_end so - // "duration" reflects engagement rather than real time. Post-resume - // session_heartbeat callers read this too, so a resumed session's - // first tick does not overcount the interval that spanned the pause; - // heartbeats that fire while _pausedAt is set are quiesced at the - // top of OnHeartbeat and never touch the accumulator. + // Subtracted from wall-clock so duration reflects engagement. private TimeSpan _accumulatedPause; private bool _disposed; @@ -56,13 +36,8 @@ internal string? SessionId get { lock (_lock) return _sessionId; } } - // track fires session_start / _heartbeat / _end via - // ImmutableAudience.Track (passed as a delegate so tests can drive - // Session with a mock and without touching the static SDK surface). - // performanceSnapshot merges fps/memory into each heartbeat; null - // on non-Unity runtimes. getUtcNow and heartbeatIntervalMs are test - // seams that production callers leave at their defaults (system - // clock, 60_000 ms). + // track: fires session events. performanceSnapshot: merges fps/memory + // into heartbeats (null on non-Unity). getUtcNow/heartbeatIntervalMs: test seams. internal Session( Action> track, Func>? performanceSnapshot = null, @@ -75,21 +50,12 @@ internal Session( _heartbeatIntervalMs = heartbeatIntervalMs; } - // Start a new session. Fires session_start and begins the heartbeat timer. + // Starts a session. Fires session_start and arms the heartbeat timer. internal void Start() { - // Phase 1 — stop the old heartbeat timer (if any) and drain its - // in-flight callback OUTSIDE the lock before we mutate session - // state. The callback itself acquires _lock, so waiting on the - // drain under _lock would deadlock. Leaving _sessionId / - // _sessionStart / _accumulatedPause at their old values across - // the drain means a trailing callback emits a heartbeat for the - // OLD session — wire-ordered before the new session_start — - // instead of reading the new state with zero-duration noise. - // - // Double-Start is a misuse path (supported pattern is End/Dispose - // then Start); on the normal End→Start rollover _heartbeatTimer - // is already null from End's drain, so this drain is a no-op. + // Phase 1: drain old timer outside _lock (callback re-enters _lock). + // Old state left intact so a trailing callback emits for the old session + // — wire-ordered before the new session_start. Timer? oldTimer; lock (_lock) { @@ -107,10 +73,7 @@ internal void Start() using var waited = new ManualResetEvent(false); try { - // 500 ms budget (vs End's 1 s) because double-Start is - // unexpected; we do not want a mistake path to block the - // caller for a full second. Dispose(wh) returns false on - // an already-disposed timer — skip the wait in that case. + // 500ms budget (double-Start is a misuse path). if (oldTimer.Dispose(waited)) waited.WaitOne(TimeSpan.FromMilliseconds(500)); } @@ -119,9 +82,7 @@ internal void Start() } } - // Phase 2 — atomically populate new session state and schedule - // the new timer. _disposed may have flipped during the drain; - // re-check. + // Phase 2: populate new state. Re-check _disposed (may have flipped during drain). string sessionId; lock (_lock) { @@ -142,33 +103,20 @@ internal void Start() }); } - // Called when the game loses focus (e.g. alt-tab, minimize). Records - // the pause start; quiesces session_heartbeat so no engagement signal - // is emitted while the app is backgrounded. The 30s pause threshold - // is evaluated on the next Resume call — no timer fires on Pause, so - // a game that never resumes (force-killed while backgrounded) sees - // session_end only when Shutdown runs. Expected to be called on the - // Unity main thread by the lifecycle bridge. + // Pause on focus-loss. Quiesces heartbeat; 30s threshold evaluated on next Resume. internal void Pause() { lock (_lock) { if (_disposed || _sessionId == null) return; - // Already paused — keep the original _pausedAt so the live - // pause window stays anchored to the first Pause. Without - // this guard a double-Pause would advance _pausedAt forward - // and ComputeEngagedSecondsLocked's (now - _pausedAt) live - // pause would undercount the window by the delta, over- - // crediting engagement time. + // Keep the original anchor; double-Pause would shift it forward + // and under-count the pause window. if (_pausedAt.HasValue) return; _pausedAt = _getUtcNow(); } } - // Called when the game regains focus. If paused up to 30s the - // session continues; any longer and the old session ends and a new - // one starts. Expected to be called on the Unity main thread by the - // lifecycle bridge. + // Resume on focus-gain. Pause >30s rolls the session (End + Start). internal void Resume() { bool extended; @@ -179,63 +127,33 @@ internal void Resume() var pauseDuration = _getUtcNow() - _pausedAt.Value; _pausedAt = null; - // Clamp to zero for wall-clock rewinds (NTP correction, manual - // clock change). A negative pauseDuration would reduce - // _accumulatedPause and hand the next session an artificial - // engagement credit. End already clamps the outgoing duration, - // but the accumulator needs its own guard. + // Clamp: wall-clock rewind (NTP) would otherwise over-credit engagement. if (pauseDuration < TimeSpan.Zero) pauseDuration = TimeSpan.Zero; extended = pauseDuration.TotalMilliseconds > PauseTimeoutMs; - // Credit the pause in both paths. Short pauses: End will never - // fire for this session, the credit prevents heartbeats from - // overcounting. Extended pauses: End fires next and subtracts - // the credit so session_end reports the pre-pause engaged time, - // then resets _accumulatedPause to zero before Start runs. No - // risk of double-count — the reset in End (and again in Start) - // is what makes the extended-pause path safe to credit here. + // Credit in both paths. End (and then Start) reset the accumulator + // on the extended-pause rollover so there is no double-count. _accumulatedPause += pauseDuration; } if (extended) { - // Extended pause — end old session, start new one. Both fire - // their track events outside our lock so reentrant track - // implementations can safely take their own locks. - // - // Invariant between End() and Start(): _sessionId is null - // (End reset it), _heartbeatTimer is null (drained), and - // _disposed may flip if Dispose races. Every other public - // method (Pause, Resume, End, OnHeartbeat, Dispose) guards - // on one of those fields and early-returns, so a concurrent - // call landing in this window is a no-op rather than a - // corruption vector. + // Extended pause: roll the session. End/Start fire _track outside _lock. + // Between End and Start other public methods early-return on _sessionId=null. End(); Start(); } } - // End the current session. Fires session_end and stops the heartbeat. - // - // Drains any in-flight heartbeat callback before emitting session_end - // so the usual wire ordering (heartbeats -> session_end) holds. On - // drain timeout (1 s, see DrainHeartbeatTimer), ordering is - // best-effort: a stuck heartbeat callback whose _track call is still - // running when the drain gives up can land after session_end on the - // wire. The drain logs a warning in that case so the anomaly is - // observable. + // Ends the session. Drains heartbeat before emitting session_end so wire + // order holds (drain timeout is best-effort; logs a warning on timeout). internal void End() { - // Phase 1 — stop the timer and wait for any in-flight callback - // OUTSIDE the lock. OnHeartbeat itself takes _lock; waiting under - // the lock would deadlock. See DrainHeartbeatTimer for the wait - // budget and the logging on timeout. + // Phase 1: drain outside _lock (OnHeartbeat re-enters _lock). DrainHeartbeatTimer(); - // Phase 2 — atomically capture the outgoing session fields and - // reset state so subsequent Start (on extended-pause rollover) or - // Dispose (on shutdown) sees a clean slate. + // Phase 2: capture fields and reset so subsequent Start/Dispose sees clean state. string sessionId; long duration; lock (_lock) @@ -243,22 +161,13 @@ internal void End() if (_sessionId == null) return; sessionId = _sessionId!; - // ComputeEngagedSecondsLocked folds the in-flight pause in - // itself, so End on a still-paused session reports engaged - // time that excludes the final pause window. + // ComputeEngagedSecondsLocked folds in the live pause. duration = ComputeEngagedSecondsLocked(); ResetSessionStateLocked(); } - // duration is engagement-aware: wall-clock since Start minus - // every credited pause. Web SDK's session_end emits wall-clock - // seconds with no pause concept (browsers have no foreground - // pause). Same wire field, different semantic. Unity always - // emits duration (minimum 0); Web SDK guards it behind - // sessionStartTime, which in practice tracks sessionId and - // is set/cleared together with it. Backend dashboards - // comparing surfaces should not assume Unity and Web session - // lengths are directly comparable. + // duration is engagement-aware (excludes pause). Web SDK emits + // wall-clock; dashboards should not assume parity. SafeTrack("session_end", new Dictionary { ["sessionId"] = sessionId, @@ -274,9 +183,8 @@ public void Dispose() _disposed = true; } - // End handles the timer drain, the atomic state capture, and the - // session_end emit. Dispose adds nothing beyond the _disposed - // latch (which blocks subsequent Start/Pause/Resume calls). + // End does the drain + emit. Dispose adds the _disposed latch + // which blocks subsequent Start/Pause/Resume. End(); } @@ -284,12 +192,8 @@ public void Dispose() // Private // ----------------------------------------------------------------- - // Fires a single heartbeat. Called by the internal timer every 60s - // and exposed as internal so tests can exercise the heartbeat path - // without waiting the full interval. Skips emission while the - // session is paused so "foregrounded-only" is what actually ships: - // a backgrounded game must not dribble stable-duration heartbeats - // into the pipeline for the entire alt-tab. + // Fires a heartbeat. Internal so tests can drive without waiting 60s. + // Skips while paused so backgrounded games don't dribble heartbeats. internal void OnHeartbeat() { string sessionId; @@ -297,21 +201,14 @@ internal void OnHeartbeat() lock (_lock) { if (_disposed || _sessionId == null) return; - // Quiesce while paused — session_heartbeat is an engagement - // signal, and a paused session is by definition disengaged. - // Resume resumes ticking; the timer keeps firing in the - // background on thread-pool cadence but this method is the - // gate that controls what reaches _track. + // Paused sessions don't ship heartbeats (timer still fires; this gates _track). if (_pausedAt.HasValue) return; - // Non-null at this point — the early-return above ensures it. sessionId = _sessionId!; duration = ComputeEngagedSecondsLocked(); } - // Build and emit the heartbeat outside the lock so the performance - // snapshot delegate and track callback cannot cause contention or - // reentrant-lock surprises. + // Build outside _lock so snapshot + track don't re-enter. var properties = new Dictionary { ["sessionId"] = sessionId, @@ -323,10 +220,7 @@ internal void OnHeartbeat() { foreach (var kv in perf) { - // Core heartbeat fields are owned by Session; a provider - // that returns a "sessionId" or "duration" key (buggy or - // malicious) must not be able to rewrite them on the wire. - // Drop colliding keys rather than overwrite. + // Don't let the provider clobber core fields. if (properties.ContainsKey(kv.Key)) continue; properties[kv.Key] = kv.Value; } @@ -335,13 +229,8 @@ internal void OnHeartbeat() SafeTrack("session_heartbeat", properties); } - // Invokes _track with a catch-all so a throwing callback cannot - // escape. OnHeartbeat runs on a thread-pool timer callback where an - // unhandled exception is swallowed by the runtime on .NET Framework - // / Mono but can terminate the process on .NET 5+. Start and End run - // on the caller's thread (typically Unity main) where an exception - // from _track would bubble into Init / Shutdown / SetConsent. Same - // guard covers both paths. + // Guards _track from escaping. Heartbeat is a Timer callback (process + // kill on .NET 5+); Start/End are caller-thread (would bubble to Init/Shutdown). private void SafeTrack(string eventName, Dictionary properties) { try @@ -354,10 +243,7 @@ private void SafeTrack(string eventName, Dictionary properties) } } - // Invokes the performance snapshot provider with a catch-all. The - // provider is studio-supplied (PerformanceSnapshotProvider) and - // crosses an API boundary; without this guard a throwing provider - // propagates into the heartbeat timer callback. + // Guards the studio-supplied snapshot from escaping to the timer thread. private Dictionary? SafePerformanceSnapshot() { if (_performanceSnapshot == null) return null; @@ -372,13 +258,8 @@ private void SafeTrack(string eventName, Dictionary properties) } } - // Stops the heartbeat timer and waits for any in-flight callback to - // complete. Runs OUTSIDE _lock because OnHeartbeat takes _lock itself - // and waiting under the lock would deadlock. Idempotent: safe to call - // repeatedly. The wait budget is 1 second because Dispose can run on - // Application.quitting (Unity main thread) and a frozen quit is worse - // than a dropped trailing heartbeat. Logs a warning on timeout so - // stuck-heartbeat anomalies surface when Debug logging is on. + // Stops the timer and waits for the in-flight callback. Runs outside + // _lock (OnHeartbeat re-enters). 1s budget (quits must not hang). Warns on timeout. private void DrainHeartbeatTimer() { Timer? timer; @@ -392,11 +273,7 @@ private void DrainHeartbeatTimer() using var waited = new ManualResetEvent(false); try { - // Timer.Dispose(WaitHandle) returns false when the timer was - // already disposed — in that case the WaitHandle is NOT - // signaled, so WaitOne would time out and fire a spurious - // "drain timeout" warning. Skip the wait (and the warning) - // when the timer reports it is already gone. + // Dispose(wh)=false: already disposed, wh never signals — skip the wait. if (!timer.Dispose(waited)) return; @@ -408,20 +285,11 @@ private void DrainHeartbeatTimer() } catch (ObjectDisposedException) { - // Timer already disposed — nothing to wait for. } } - // Caller must hold _lock. Engagement time in seconds: wall-clock - // since Start, minus every credited pause (both the committed - // _accumulatedPause and any in-flight window since _pausedAt), - // rounded to a whole second to match Web SDK's Math.round(... / - // 1000). Folding the live pause in here rather than at each call - // site keeps callers from racing the pause state against their - // own local copy — End and OnHeartbeat both run under _lock, so - // the read is consistent with ResetSessionStateLocked / Pause / - // Resume. Clamped to zero so a wall-clock rewind (NTP correction, - // manual change) can never produce a negative duration on the wire. + // Caller must hold _lock. Engagement seconds = wall-clock − accumulated − live pause. + // Rounded to match Web SDK's Math.round. Clamped ≥0 for clock rewinds. private long ComputeEngagedSecondsLocked() { var now = _getUtcNow(); @@ -431,13 +299,8 @@ private long ComputeEngagedSecondsLocked() return (long)Math.Round(engagedSeconds, MidpointRounding.AwayFromZero); } - // Caller must hold _lock. Clears per-session state after End so - // the Session falls back to an unstarted shape (SessionId == null, - // no pause accumulator). End is the only caller; Start inlines - // the same three assignments because it simultaneously writes - // live values (a new Guid, _getUtcNow()) that cannot be expressed - // by a "reset" helper. Adding a new state field requires updating - // both Start and this helper. + // Caller must hold _lock. Clears per-session state after End. + // Start inlines equivalent assignments; new state fields must update both. private void ResetSessionStateLocked() { _sessionId = null; diff --git a/src/Packages/Audience/Runtime/ImmutableAudience.cs b/src/Packages/Audience/Runtime/ImmutableAudience.cs index b52155f7d..5f9dc72ed 100644 --- a/src/Packages/Audience/Runtime/ImmutableAudience.cs +++ b/src/Packages/Audience/Runtime/ImmutableAudience.cs @@ -13,9 +13,8 @@ namespace Immutable.Audience public static class ImmutableAudience { // Reference fields are written inside _initLock; readers fence off the volatile _initialized load. - // _consent and _session are written only inside _initLock but read outside (Track's CanTrack, - // OnPause/OnResume) so they stay volatile for release/acquire visibility. - // _userId is written outside the lock (Identify, Reset) and needs volatile for the same reason. + // _consent, _session are written in _initLock, read outside → volatile. + // _userId is written outside the lock (Identify, Reset) → volatile. private static AudienceConfig? _config; private static DiskStore? _store; private static EventQueue? _queue; @@ -28,34 +27,19 @@ public static class ImmutableAudience private static volatile bool _initialized; private static readonly object _initLock = new object(); - // Guard against overlapping timer ticks. System.Threading.Timer fires - // callbacks on independent ThreadPool threads and does not serialise - // them; without this gate, a slow SendBatchAsync (up to the HTTP - // timeout) would stack on every interval tick, each tick holding its - // own thread blocked on a pending request. + // Gate against overlapping timer ticks (Timer callbacks run on independent ThreadPool threads). private static int _sendInFlight; - // AudienceUnityHooks sets this at SubsystemRegistration so Unity studios - // can omit PersistentDataPath from AudienceConfig and Init will fill it - // from Application.persistentDataPath. Non-Unity callers must still set - // PersistentDataPath on the config. + // AudienceUnityHooks sets these at SubsystemRegistration. + // DefaultPersistentDataPathProvider fills PersistentDataPath from + // Application.persistentDataPath. LaunchContextProvider supplies + // Unity context for game_launch without Core referencing UnityEngine. internal static Func? DefaultPersistentDataPathProvider; - - // AudienceUnityHooks sets this so game_launch can auto-include - // Unity context without the core referencing UnityEngine. internal static Func>? LaunchContextProvider; - // Session is created when consent allows — either at Init or on - // upgrade from None — and is disposed on Shutdown or SetConsent(None). - // Holds the current sessionId, fires session_start / session_heartbeat - // / session_end via the Track callback, and handles pause/resume - // rollover. - // - // Volatile + nullable because the field is read outside _initLock on - // the Unity main thread (OnPause, OnResume) and written outside the - // lock by SetConsent and Shutdown. volatile gives a release/acquire - // fence so a freshly-assigned Session from SetConsent(None → *) is - // visible to a subsequent OnPause on any thread. + // Active session. Created at Init (or on upgrade from None) and disposed + // on Shutdown or SetConsent(None). Volatile so OnPause/OnResume see + // assignments from SetConsent without taking _initLock. private static volatile Session? _session; // Starts the SDK. Call once at launch. @@ -83,7 +67,7 @@ public static void Init(AudienceConfig config) _config = config; Log.Enabled = config.Debug; - // Persisted consent overrides the config default so a prior runtime downgrade survives restart. + // Persisted consent overrides the config default (prior downgrade survives restart). _consent = ConsentStore.Load(config.PersistentDataPath) ?? config.Consent; _store = new DiskStore(config.PersistentDataPath); @@ -101,30 +85,17 @@ public static void Init(AudienceConfig config) _initialized = true; - // Snapshot under the lock so a racing SetConsent(None) can't drop the launch event. + // Snapshot so a racing SetConsent(None) can't drop the launch event. consentAtInit = _consent; - // Create the session object under the lock so ResetState / - // Shutdown have a consistent view. Start() is deferred - // until outside the lock because Track() (which - // session_start calls) acquires its own locks. + // Session created under the lock; Start() deferred until after + // release because session_start → Track takes its own locks. if (consentAtInit.CanTrack()) _session = new Session(Track); - // Captured under the lock so the Start() call below - // operates on the Session this Init created, not a - // replacement from a SetConsent that slips in after we - // release _initLock. SetConsent itself takes _initLock so - // it cannot land in the middle of this block, but between - // the lock release and Start() a SetConsent(None) can - // Dispose the captured Session (Start then early-returns - // on _disposed, suppressing session_start) or a - // SetConsent(None)+SetConsent(Anonymous) pair can swap - // _session for a new one (we still Start the captured - // original, which is disposed and no-ops; the upgrade path - // Starts the replacement itself). Either way, no duplicate - // session_start on the wire and no "events after consent - // dropped" leak. + // Captured reference: a later SetConsent(None) may dispose this + // Session (Start then no-ops on _disposed). Either way no duplicate + // session_start and no post-revocation leak. sessionToStart = _session; } @@ -135,20 +106,14 @@ public static void Init(AudienceConfig config) FireGameLaunch(config, consentAtInit); } - // Notifies the session that the game was paused (alt-tab, - // minimize, OS pause). If not resumed within 30 s the session - // ends and a new one starts on resume. Called only from the - // Unity lifecycle bridge, so it stays internal; the Unity - // assembly reaches it via InternalsVisibleTo in AssemblyInfo.cs. + // Pause/Resume hooks for the Unity lifecycle bridge. + // Internal; reached via InternalsVisibleTo from the Unity assembly. internal static void OnPause() { if (!_initialized) return; _session?.Pause(); } - // Notifies the session that the game resumed. Called only from - // the Unity lifecycle bridge; see OnPause for the visibility - // rationale. internal static void OnResume() { if (!_initialized) return; @@ -159,12 +124,8 @@ internal static void OnResume() // Track // ----------------------------------------------------------------- - // Send a typed event. - // - // Prefer this overload for predefined event names (e.g. purchase) — the - // IEvent implementation enforces required fields and value types at - // compile time. The string overload accepts any property shape and - // cannot catch missing or mistyped fields. + // Sends a typed event. Prefer this over the string overload — + // IEvent implementations validate required fields at compile time. public static void Track(IEvent evt) { if (!CanTrack()) return; @@ -203,16 +164,9 @@ public static void Track(IEvent evt) Enqueue(msg); } - // Send a custom event. - // - // For predefined event names (e.g. purchase, progression, resource, - // milestone_reached), prefer the typed overload — - // Track(new Purchase { Currency = "USD", Value = 9.99m }) — which - // validates required fields at send time. This overload accepts any - // property shape and does not: Track("purchase", new Dictionary...) - // that omits currency or value still enqueues and ships, but breaks - // attribution and conversion reporting downstream because the - // payload is missing the fields CDP needs to reconstruct the event. + // Sends a custom event. For predefined names (purchase, progression, + // resource, milestone_reached), prefer the typed overload which + // validates required fields. public static void Track(string eventName, Dictionary? properties = null) { if (!CanTrack()) return; @@ -235,16 +189,12 @@ public static void Track(string eventName, Dictionary? propertie // Identity // ----------------------------------------------------------------- - // Attach a known user id to subsequent events. + // Attaches a known user id to subsequent events. public static void Identify(string userId, IdentityType identityType, Dictionary? traits = null) => Identify(userId, identityType.ToLowercaseString(), traits); - // Attach a known user id to subsequent events. String overload for - // providers not in IdentityType. - // - // identityType is required: data-deletion processing relies on it to - // match identify events to the correct identity namespace, so an - // event without one cannot be cleaned up. + // String overload for providers outside the IdentityType enum. + // identityType is required — data-deletion matches events by this namespace. public static void Identify(string userId, string identityType, Dictionary? traits = null) { if (!_initialized) return; @@ -272,15 +222,12 @@ public static void Identify(string userId, string identityType, Dictionary Alias(fromId, fromType.ToLowercaseString(), toId, toType.ToLowercaseString()); - // Link two user ids for the same player. String overload for - // providers not in IdentityType. - // - // fromType and toType are required: data-deletion processing uses - // them to match alias events to the correct identity namespaces. + // String overload for providers outside the IdentityType enum. + // from/toType are required — data-deletion matches by these namespaces. public static void Alias(string fromId, string fromType, string toId, string toType) { if (!_initialized) return; @@ -303,16 +250,8 @@ public static void Alias(string fromId, string fromType, string toId, string toT Enqueue(msg); } - // Log out the current player. Clears the user id, generates a fresh - // anonymous id, and discards queued events (in-memory and on-disk) - // so the next player on this device isn't attributed to the previous - // one. - // - // To send queued events before they're discarded, - // invoke await FlushAsync() first: - // - // await ImmutableAudience.FlushAsync(); - // ImmutableAudience.Reset(); + // Logs out the current player. Clears userId, mints a fresh anonymousId, + // discards queued events. Call FlushAsync() first to preserve queued events. public static void Reset() { if (!_initialized) return; @@ -325,9 +264,7 @@ public static void Reset() Identity.Reset(config.PersistentDataPath!); } - // Ask the backend to erase this player's data. Returns a task the - // caller can await to know when the request is acknowledged, or - // discard for fire-and-forget. + // Asks the backend to erase this player's data. Await for ack, or discard for fire-and-forget. public static Task DeleteData(string? userId = null) { if (!_initialized) return Task.CompletedTask; @@ -343,7 +280,7 @@ public static Task DeleteData(string? userId = null) } else { - // Get, not GetOrCreate — a brand-new install must not register an ID just to delete it. + // Get (not GetOrCreate): a fresh install must not register an id just to delete it. var anonymousId = Identity.Get(config.PersistentDataPath!); if (string.IsNullOrEmpty(anonymousId)) return Task.CompletedTask; @@ -371,7 +308,7 @@ public static Task DeleteData(string? userId = null) } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { - // Shutdown cancelled the request — no error fired; caller is tearing down. + // Shutdown cancelled — caller is tearing down, no error fired. } catch (Exception ex) { @@ -390,7 +327,7 @@ private static void NotifyErrorCallback(Action? onError, Audience } catch { - // Swallow: a buggy OnError must not crash the SDK surface. + // Swallow: a buggy OnError must not crash the SDK. } } @@ -398,23 +335,14 @@ private static void NotifyErrorCallback(Action? onError, Audience // Consent // ----------------------------------------------------------------- - // Change the player's consent level. + // Changes the player's consent level. public static void SetConsent(ConsentLevel level) { if (!_initialized) return; - // Serialize the whole transition under _initLock: - // - Two concurrent SetConsent calls that both see previous=None - // would otherwise both take the upgrade branch and each build - // a fresh Session, stranding one timer. The lock forces them - // to observe each other. - // - A SetConsent landing in the narrow window between Init's - // _initialized = true and its _session = new Session(...) - // assignment would otherwise see _session = null, skip the - // Dispose path, and let Init finish creating a Session whose - // timer never gets disposed. Under the lock, SetConsent can - // only enter after Init has fully released it, so _session - // reflects Init's final assignment. + // Serialised under _initLock: prevents concurrent upgrades from each + // building a fresh Session (stranding timers), and prevents racing + // Init's _session assignment. Session? sessionToStart = null; lock (_initLock) { @@ -427,9 +355,8 @@ public static void SetConsent(ConsentLevel level) var previous = _consent; if (level == previous) return; - // Snapshot the anonymousId BEFORE Identity.Reset (on downgrade to - // None) wipes the file. The PUT audit trail needs it to record - // whose consent changed. + // Snapshot anonymousId before Identity.Reset (on None) wipes it. + // The PUT audit trail needs to record whose consent changed. var anonymousIdForPut = previous == ConsentLevel.None ? Identity.GetOrCreate(config.PersistentDataPath!, level) : Identity.Get(config.PersistentDataPath!); @@ -438,7 +365,7 @@ public static void SetConsent(ConsentLevel level) try { - // PersistentDataPath is validated non-null in Init; compiler can't propagate that. + // PersistentDataPath validated non-null in Init; compiler can't propagate that. ConsentStore.Save(config.PersistentDataPath!, level); } catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException) @@ -451,13 +378,8 @@ public static void SetConsent(ConsentLevel level) if (level == ConsentLevel.None) { - // Dispose the session for heartbeat-timer cleanup. - // session_end is intentionally NOT emitted: Session.Dispose - // calls back into Track("session_end", ...) which sees - // _consent == None (set above) and drops the event via - // CanTrack. That matches revocation semantics — no events - // should leave the device after consent is None — and the - // queue purge below clears anything on disk anyway. + // Dispose for timer cleanup. session_end is gated out by + // CanTrack (post-flip), matching revocation semantics. _session?.Dispose(); _session = null; @@ -471,11 +393,8 @@ public static void SetConsent(ConsentLevel level) } else if (previous == ConsentLevel.None && _session == null) { - // Upgrade from None: the previous session was disposed on - // the prior downgrade. Start a fresh one so - // session_heartbeat and session_end can resume. Start() - // is deferred until outside the lock because Track() - // (which session_start calls) acquires its own locks. + // Upgrade from None: previous session was disposed. Start a + // fresh one. Start() deferred to outside the lock. _session = new Session(Track); sessionToStart = _session; } @@ -486,8 +405,7 @@ public static void SetConsent(ConsentLevel level) sessionToStart?.Start(); } - // Fire-and-forget PUT /v1/audience/tracking-consent. Failures do not - // block or surface; the local consent change has already applied. + // Fire-and-forget PUT /v1/audience/tracking-consent. private static void SyncConsentToBackend(AudienceConfig config, ConsentLevel level, string? anonymousId) { var client = _controlClient; @@ -502,7 +420,7 @@ private static void SyncConsentToBackend(AudienceConfig config, ConsentLevel lev { ["status"] = level.ToLowercaseString(), ["source"] = Constants.ConsentSource, - // Json.Serialize emits null → "anonymousId": null. Preserves the backend's ability to distinguish "unknown" from a missing field. + // Explicit null lets the backend distinguish "unknown" from a missing field. ["anonymousId"] = anonymousId!, }); @@ -523,7 +441,7 @@ private static void SyncConsentToBackend(AudienceConfig config, ConsentLevel lev } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { - // Shutdown cancelled the request — no error fired. + // Shutdown cancelled. } catch (Exception ex) { @@ -537,7 +455,7 @@ private static void SyncConsentToBackend(AudienceConfig config, ConsentLevel lev // Flush / Shutdown // ----------------------------------------------------------------- - // Send pending events now. + // Sends all pending events now. public static async Task FlushAsync() { if (!_initialized) return; @@ -554,28 +472,21 @@ await transport.SendBatchAsync().ConfigureAwait(false)) } } - // Flush and stop the SDK. + // Flushes and stops the SDK. public static void Shutdown() { - // Serialize with Init and SetConsent under the same lock. Without - // this, Shutdown racing a concurrent Init could observe - // _initialized = true (volatile set by Init) and proceed to tear - // down fields Init is still in the middle of assigning, or Init - // could finish assigning fields that Shutdown already disposed. - // The lock also pins _controlClient / _shutdownCancellationSource - // against a SetConsent whose SyncConsentToBackend Task closure - // captured them just before Shutdown disposed them. + // Serialised with Init / SetConsent under _initLock so teardown + // does not race field assignments or the SyncConsentToBackend closure. lock (_initLock) { if (!_initialized) return; - // End the session first so session_end hits the queue before - // the final flush. + // End session first so session_end hits the queue before the final flush. _session?.Dispose(); _session = null; // Drain in-flight timer callbacks before disposing dependents. - // Parameterless Timer.Dispose returns immediately and would race SendBatch. + // Parameterless Timer.Dispose would return immediately and race SendBatch. var timer = _sendTimer; if (timer != null) { @@ -587,10 +498,8 @@ public static void Shutdown() _sendTimer = null; } - // Clear the in-flight guard in case the WaitOne above timed out - // with a SendBatch callback still running: without this, a later - // Init would leave _sendInFlight stranded at 1 and suppress every - // tick of the new timer. + // Clear the gate in case WaitOne timed out with SendBatch still running + // — a later Init would otherwise be stranded at 1. Interlocked.Exchange(ref _sendInFlight, 0); _queue?.Shutdown(); @@ -614,9 +523,8 @@ public static void Shutdown() } } - // Cancel in-flight control-plane HTTP requests (DeleteData / SyncConsentToBackend) - // before disposing the client so awaiting callers observe OperationCanceledException - // rather than ObjectDisposedException. + // Cancel in-flight control-plane requests before disposing the client + // so awaiters see OperationCanceledException, not ObjectDisposedException. _shutdownCancellationSource?.Cancel(); _transport?.Dispose(); @@ -625,9 +533,8 @@ public static void Shutdown() _shutdownCancellationSource?.Dispose(); _shutdownCancellationSource = null; - // Drop Identity's in-memory cache so a subsequent Init with a - // different persistentDataPath reads the file from the new path - // instead of returning the previous session's id. + // Drop Identity's in-memory cache so a later Init with a different + // persistentDataPath reads the new file, not the stale cached id. Identity.ClearCache(); _initialized = false; @@ -644,30 +551,21 @@ public static void Shutdown() // Internal — shared with tests and AudienceUnityHooks // ----------------------------------------------------------------- - // Shuts down (if initialised) and clears per-session state so a - // fresh Init starts clean. Used on test teardown and by Unity - // SubsystemRegistration to survive "disable domain reload". - // LaunchContextProvider is not cleared: AudienceUnityHooks - // re-assigns it on the same SubsystemRegistration call. + // Shuts down (if initialised) and clears per-session state. Used on + // test teardown and Unity SubsystemRegistration to survive "disable + // domain reload". LaunchContextProvider is re-assigned by AudienceUnityHooks. internal static void ResetState() { - // Same lock as Shutdown/Init so a concurrent Init cannot repopulate - // fields we are in the middle of clearing. Monitor is recursive, - // so the inner Shutdown() re-enters cleanly on the same thread. + // Same lock as Shutdown/Init; Monitor is recursive so inner Shutdown re-enters. lock (_initLock) { if (_initialized) Shutdown(); _consent = ConsentLevel.None; - // Shutdown already nulls _session. Repeat here as a defensive - // belt-and-braces step so a future Shutdown refactor that bails - // before the null (early return on a new guard, reordering, - // etc.) cannot leak a stale Session into the next Init cycle. + // Defensive: Shutdown nulls _session too, but a future refactor + // that bails before that null must not leak a stale Session. _session = null; - // Drop Identity's static cache so a subsequent Init with a different - // persistentDataPath (tests, domain reload with changed config) reads - // the file from the new path, not the previous session's cached id. Identity.ClearCache(); } } @@ -676,15 +574,10 @@ internal static void ResetState() internal static void FlushQueueToDiskForTesting() => _queue?.FlushSync(); - // Invokes the timer callback body directly so the overlapping-tick - // guard can be exercised without a real timer. + // Drives SendBatch without a real timer so the overlapping-tick guard is testable. internal static void SendBatchForTesting() => SendBatch(); - // Drives a single heartbeat through the active Session so lifecycle - // tests can assert that OnPause / OnResume actually route to - // Session.Pause / Session.Resume. The real heartbeat cadence is - // 60 s (Session.HeartbeatIntervalMs) so a timer-driven pass would - // either take 60 s or need a bespoke interval override per test. + // Drives a single heartbeat so lifecycle tests don't wait the 60s cadence. internal static void InvokeSessionHeartbeatForTesting() => _session?.OnHeartbeat(); // ----------------------------------------------------------------- @@ -696,7 +589,7 @@ private static bool CanTrack() return _initialized && _consent.CanTrack(); } - // Shallow-copy the caller's dict so a post-call mutation cannot race the drain-thread serialiser. + // Shallow-copy so post-call mutation can't race the drain-thread serialiser. private static Dictionary? SnapshotCallerDict(Dictionary? src) => src != null ? new Dictionary(src) : null; @@ -705,16 +598,14 @@ private static void Enqueue(Dictionary? msg) var queue = _queue; if (queue == null) return; - // Re-check consent inside the drain lock so a SetConsent(None) racing - // the caller's CanTrack cannot leak this event past the purge. + // Re-check consent inside _drainLock so a racing SetConsent(None) can't leak past the purge. queue.EnqueueChecked(msg, () => _consent.CanTrack()); } private static void SendBatch() { - // CAS in the guard before doing any work; a previous tick still - // running means skip entirely, including the reschedule — the - // in-flight tick will reschedule on its own finally path. + // CAS the gate; a previous tick still running → skip (including reschedule, + // which the in-flight tick handles in its finally). if (Interlocked.CompareExchange(ref _sendInFlight, 1, 0) != 0) return; @@ -731,7 +622,7 @@ private static void SendBatch() } catch (Exception ex) { - // ThreadPool timer thread; no caller above to catch. + // Timer-thread callback; no caller above to catch. Log.Warn($"SendBatch unexpected exception: {ex.GetType().Name}: {ex.Message}"); } } @@ -744,7 +635,7 @@ private static void SendBatch() } } - // Realigns the timer to NextAttemptAt so we don't repoll through a long backoff window. + // Realigns the timer to NextAttemptAt so backoff windows aren't repolled. private static void RescheduleSendTimer(HttpTransport transport) { var timer = _sendTimer; @@ -764,18 +655,15 @@ private static void RescheduleSendTimer(HttpTransport transport) timer.Change(nextMs, sendIntervalMs); } - // consentAtInit snapshot is only used to skip the launch event under None; - // Track still consults live _consent via CanTrack, so a SetConsent(None) - // landing between Init returning and here still drops the event. + // consentAtInit only gates the launch; Track still checks live _consent via CanTrack. private static void FireGameLaunch(AudienceConfig config, ConsentLevel consentAtInit) { if (!consentAtInit.CanTrack()) return; var properties = new Dictionary(); - // Unity-side auto-detected context (platform, version, buildGuid, - // unityVersion) from AudienceUnityHooks. Core stays pure C#; the - // Unity layer fills these via LaunchContextProvider. + // Unity auto-detected context (platform, version, buildGuid, unityVersion). + // Core stays pure C#; Unity layer fills via LaunchContextProvider. var provider = LaunchContextProvider; if (provider != null) { @@ -794,19 +682,12 @@ private static void FireGameLaunch(AudienceConfig config, ConsentLevel consentAt } } - // Config-supplied distributionPlatform wins over any provider value; - // studios set it explicitly because Unity cannot auto-detect the store. + // Config-supplied distributionPlatform overrides the provider value. if (config.DistributionPlatform != null) properties["distributionPlatform"] = config.DistributionPlatform; - // No sessionId on game_launch. Event Reference (v1) schema for - // game_launch lists platform, version, buildGuid, - // distributionPlatform, unityVersion — not sessionId. - // Correlation with the session happens at the pipeline layer via - // eventTimestamp ordering; session_start fires immediately - // before game_launch (see Init ordering) and carries the id - // explicitly. - + // No sessionId on game_launch per Event Reference. Pipeline correlates + // via eventTimestamp with the session_start that fires just before. Track("game_launch", properties.Count > 0 ? properties : null); } } diff --git a/src/Packages/Audience/Runtime/Utility/Log.cs b/src/Packages/Audience/Runtime/Utility/Log.cs index 02a69ffba..f6a5442c7 100644 --- a/src/Packages/Audience/Runtime/Utility/Log.cs +++ b/src/Packages/Audience/Runtime/Utility/Log.cs @@ -22,13 +22,9 @@ internal static void Warn(string message) => private static void Emit(string line) { - // Swallow anything the Writer (or Console.WriteLine) throws so - // callers can treat Log.Warn / Log.Debug as never-throwing. The - // SDK's safety wrappers (Session.SafeTrack, SafePerformanceSnapshot, - // Shutdown's flush-timeout path) log from inside their own catch - // blocks; a throwing Writer would otherwise escape the wrapper and - // propagate to the Timer thread (process kill on .NET 5+) or to - // Application.quitting (blocking shutdown). + // Swallow Writer/Console throws so Log.Warn/Debug is never-throwing. + // SDK safety wrappers log from inside their own catches; a throwing + // Writer would otherwise reach the Timer thread (process kill on .NET 5+). try { if (Writer != null) From 878939996672cfed505fe4b91ba7fd69f48b7ff4 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 14:12:33 +1000 Subject: [PATCH 03/19] refactor(audience-session): trackDelegate named delegate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace Action> in the Session constructor with a named internal TrackDelegate. Self-documents the calling convention (parameter names visible in IDE) and makes the Session API easier to skim. Test mocks convert via method-group → compatible delegate; no test changes. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/Core/Session.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Packages/Audience/Runtime/Core/Session.cs b/src/Packages/Audience/Runtime/Core/Session.cs index 8f5b78513..5bc2355ba 100644 --- a/src/Packages/Audience/Runtime/Core/Session.cs +++ b/src/Packages/Audience/Runtime/Core/Session.cs @@ -6,6 +6,11 @@ namespace Immutable.Audience { + // Fires a session event (session_start / session_heartbeat / session_end) + // through ImmutableAudience.Track. Declared as a named delegate so Session + // can be driven by tests with a mock without touching the static SDK surface. + internal delegate void TrackDelegate(string eventName, Dictionary properties); + // Unity session lifecycle. Emits session_start / session_heartbeat / session_end. // duration is engagement time (excludes pause). Heartbeat fires off-thread; // public methods run on the caller's thread. _track fires outside _lock. @@ -16,7 +21,7 @@ internal sealed class Session : IDisposable // 30s: alt-tab beyond this rolls the session on Resume. internal const int PauseTimeoutMs = 30_000; - private readonly Action> _track; + private readonly TrackDelegate _track; private readonly Func>? _performanceSnapshot; private readonly Func _getUtcNow; private readonly int _heartbeatIntervalMs; @@ -39,7 +44,7 @@ internal string? SessionId // track: fires session events. performanceSnapshot: merges fps/memory // into heartbeats (null on non-Unity). getUtcNow/heartbeatIntervalMs: test seams. internal Session( - Action> track, + TrackDelegate track, Func>? performanceSnapshot = null, Func? getUtcNow = null, int heartbeatIntervalMs = HeartbeatIntervalMs) From 91ed73dd0bb46309f2265e00482886f16b2ceea7 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 14:22:19 +1000 Subject: [PATCH 04/19] =?UTF-8?q?fix(audience-session):=20rename=20wire=20?= =?UTF-8?q?field=20duration=20=E2=86=92=20durationSec?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Align with the Unity SDK Event Reference (v1), which specifies session_heartbeat and session_end properties as durationSec. The explicit unit suffix matches the fpsAvg / memoryUsedMb style on the same event and makes the units unambiguous on the wire. Web SDK still emits "duration"; dashboards comparing surfaces must join on the Unity-specific key. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/Core/Session.cs | 4 +-- .../Tests/Runtime/Core/SessionTests.cs | 30 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/Packages/Audience/Runtime/Core/Session.cs b/src/Packages/Audience/Runtime/Core/Session.cs index 5bc2355ba..90142d699 100644 --- a/src/Packages/Audience/Runtime/Core/Session.cs +++ b/src/Packages/Audience/Runtime/Core/Session.cs @@ -176,7 +176,7 @@ internal void End() SafeTrack("session_end", new Dictionary { ["sessionId"] = sessionId, - ["duration"] = duration + ["durationSec"] = duration }); } @@ -217,7 +217,7 @@ internal void OnHeartbeat() var properties = new Dictionary { ["sessionId"] = sessionId, - ["duration"] = duration + ["durationSec"] = duration }; var perf = SafePerformanceSnapshot(); diff --git a/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs b/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs index 23039f82b..184c9389a 100644 --- a/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs +++ b/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs @@ -65,8 +65,8 @@ public void End_FiresSessionEnd_WithDuration() var endEvent = _events.FirstOrDefault(e => e.name == "session_end"); Assert.IsNotNull(endEvent.props); Assert.IsTrue(endEvent.props.ContainsKey("sessionId")); - Assert.IsTrue(endEvent.props.ContainsKey("duration")); - Assert.AreEqual(2L, (long)endEvent.props["duration"]); + Assert.IsTrue(endEvent.props.ContainsKey("durationSec")); + Assert.AreEqual(2L, (long)endEvent.props["durationSec"]); } [Test] @@ -122,7 +122,7 @@ void Track(string name, Dictionary props) var beat = events.FirstOrDefault(e => e.name == "session_heartbeat"); Assert.IsNotNull(beat.props, "heartbeat event should carry a properties dictionary"); Assert.IsTrue(beat.props.ContainsKey("sessionId")); - Assert.IsTrue(beat.props.ContainsKey("duration")); + Assert.IsTrue(beat.props.ContainsKey("durationSec")); } } @@ -136,7 +136,7 @@ public void Heartbeat_WithoutPerformanceSnapshot_OnlyCarriesCoreProperties() var beat = _events.Last(e => e.name == "session_heartbeat"); CollectionAssert.AreEquivalent( - new[] { "sessionId", "duration" }, + new[] { "sessionId", "durationSec" }, beat.props.Keys); } @@ -161,7 +161,7 @@ public void Heartbeat_MergesPerformanceSnapshotProperties() Assert.AreEqual(512L, beat.props["memoryUsedMb"]); Assert.AreEqual(768L, beat.props["memoryReservedMb"]); Assert.IsTrue(beat.props.ContainsKey("sessionId")); - Assert.IsTrue(beat.props.ContainsKey("duration")); + Assert.IsTrue(beat.props.ContainsKey("durationSec")); } [Test] @@ -177,7 +177,7 @@ public void Heartbeat_SnapshotCannotOverwriteCoreFields() Func> snapshot = () => new Dictionary { ["sessionId"] = "spoofed-id", - ["duration"] = 99999L, + ["durationSec"] = 99999L, ["fpsAvg"] = 60.0, }; using var session = new Session(MockTrack, snapshot); @@ -188,7 +188,7 @@ public void Heartbeat_SnapshotCannotOverwriteCoreFields() var beat = _events.Last(e => e.name == "session_heartbeat"); Assert.AreNotEqual("spoofed-id", (string)beat.props["sessionId"], "snapshot must not overwrite Session-owned sessionId"); - Assert.AreNotEqual(99999L, (long)beat.props["duration"], + Assert.AreNotEqual(99999L, (long)beat.props["durationSec"], "snapshot must not overwrite Session-owned duration"); Assert.AreEqual(60.0, beat.props["fpsAvg"], "non-colliding snapshot fields should still merge"); @@ -279,7 +279,7 @@ public void Pause_CalledTwice_SecondCallIsNoOp() session.End(); var sessionEnd = _events.Last(e => e.name == "session_end"); - var duration = (long)sessionEnd.props["duration"]; + var duration = (long)sessionEnd.props["durationSec"]; // Wall-clock Start→End = 13s, paused from T=5 to T=10 = 5s, engaged = 8s. Assert.AreEqual(8L, duration, "double Pause must preserve the first Pause timestamp so engagement arithmetic covers the full pause window"); @@ -327,7 +327,7 @@ public void Resume_NegativePauseDuration_ClampsAccumulatorToZero() session.End(); var sessionEnd = _events.Last(e => e.name == "session_end"); - var duration = (long)sessionEnd.props["duration"]; + var duration = (long)sessionEnd.props["durationSec"]; // Wall-clock from Start to End is 10 + (-5) + 2 = 7 s. The // pause duration was clamped to 0, so engaged seconds = 7 - 0 = 7. // Without the clamp, _accumulatedPause would be -5, the @@ -358,7 +358,7 @@ public void End_ClockRewindsSinceStart_ClampsDurationToZero() session.End(); var sessionEnd = _events.Last(e => e.name == "session_end"); - var duration = (long)sessionEnd.props["duration"]; + var duration = (long)sessionEnd.props["durationSec"]; Assert.AreEqual(0L, duration, "negative engaged time from a wall-clock rewind must clamp to zero"); } @@ -383,7 +383,7 @@ public void End_AfterShortPause_ReportsDurationMinusPause() session.End(); var sessionEnd = _events.Last(e => e.name == "session_end"); - var duration = (long)sessionEnd.props["duration"]; + var duration = (long)sessionEnd.props["durationSec"]; Assert.AreEqual(7L, duration, "session_end duration should exclude the 3s paused interval"); } @@ -405,7 +405,7 @@ public void End_WhilePaused_ExcludesInFlightPauseFromDuration() session.End(); // ends while paused var sessionEnd = _events.Last(e => e.name == "session_end"); - var duration = (long)sessionEnd.props["duration"]; + var duration = (long)sessionEnd.props["durationSec"]; Assert.AreEqual(5L, duration, "session_end fired while paused should count only pre-pause engaged time"); } @@ -433,7 +433,7 @@ public void End_AfterExtendedPauseRollover_ReportsPrePauseDuration() session.Resume(); var sessionEnd = _events.First(e => e.name == "session_end"); - var duration = (long)sessionEnd.props["duration"]; + var duration = (long)sessionEnd.props["durationSec"]; Assert.AreEqual(10L, duration, "session_end on extended-pause rollover should report pre-pause engaged time, not wall-clock"); } @@ -458,7 +458,7 @@ public void Heartbeat_AfterShortPause_ReportsPauseAdjustedDuration() session.OnHeartbeat(); var heartbeat = _events.Last(e => e.name == "session_heartbeat"); - var duration = (long)heartbeat.props["duration"]; + var duration = (long)heartbeat.props["durationSec"]; Assert.AreEqual(6L, duration, "heartbeat duration should exclude the 2s paused interval"); } @@ -640,7 +640,7 @@ public void OnHeartbeat_PerformanceSnapshotThrows_ShipsHeartbeatWithoutPerfField var beat = _events.Last(e => e.name == "session_heartbeat"); CollectionAssert.AreEquivalent( - new[] { "sessionId", "duration" }, + new[] { "sessionId", "durationSec" }, beat.props.Keys, "heartbeat should carry only the core fields when the snapshot throws"); From e5c8d0a8873447a9d3bfc973fe7696b04229cc17 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 14:23:34 +1000 Subject: [PATCH 05/19] docs(audience-session): document Start serialisation contract Start / End / Dispose are not reentrant-safe from multiple threads. Caller (ImmutableAudience) serialises via _initLock. Pin the contract in the class-level comment so a future caller doesn't assume the public-surface thread-safety guarantee extends to Start. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/Core/Session.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Packages/Audience/Runtime/Core/Session.cs b/src/Packages/Audience/Runtime/Core/Session.cs index 90142d699..02a4d8bf4 100644 --- a/src/Packages/Audience/Runtime/Core/Session.cs +++ b/src/Packages/Audience/Runtime/Core/Session.cs @@ -14,6 +14,11 @@ namespace Immutable.Audience // Unity session lifecycle. Emits session_start / session_heartbeat / session_end. // duration is engagement time (excludes pause). Heartbeat fires off-thread; // public methods run on the caller's thread. _track fires outside _lock. + // + // Serialisation: Start / End / Dispose are NOT reentrant-safe from multiple + // threads. Callers serialise them (ImmutableAudience holds _initLock across + // Init / SetConsent / Shutdown / Reset, which are the only public entry points + // that touch a Session). Pause / Resume / OnHeartbeat are thread-safe. internal sealed class Session : IDisposable { internal const int HeartbeatIntervalMs = 60_000; From 2043d90823adcaf220638ae945a2f8aeaab4c0cd Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 14:23:55 +1000 Subject: [PATCH 06/19] docs(audience-session): correct Pause double-call rationale The prior comment named only the live-pause path (End fires while paused). The test that motivates the guard exercises the Resume path, where Resume's pauseDuration (not the live pause) shrinks. Mention both so a future reader can map either test to the rationale. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/Core/Session.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Packages/Audience/Runtime/Core/Session.cs b/src/Packages/Audience/Runtime/Core/Session.cs index 02a4d8bf4..d6a0b813d 100644 --- a/src/Packages/Audience/Runtime/Core/Session.cs +++ b/src/Packages/Audience/Runtime/Core/Session.cs @@ -119,8 +119,9 @@ internal void Pause() lock (_lock) { if (_disposed || _sessionId == null) return; - // Keep the original anchor; double-Pause would shift it forward - // and under-count the pause window. + // Keep the original anchor. Shifting forward shrinks Resume's + // pauseDuration (and ComputeEngagedSecondsLocked's live pause + // when End fires while paused), over-crediting engagement. if (_pausedAt.HasValue) return; _pausedAt = _getUtcNow(); } From 06f976e0972f173d3f13ee08d4575e1fd7b054b2 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 14:24:31 +1000 Subject: [PATCH 07/19] fix(audience-session): drop ex.Message from SafeTrack warnings Session.SafeTrack and SafePerformanceSnapshot log warnings when the studio-supplied _track or performance-snapshot delegate throws. The exception type is diagnostic; the message is consumer-supplied and may contain PII (e.g., a track impl that includes player-data in its exception text). Log only the exception type name. Studios can attach a debugger for the full message if needed. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/Core/Session.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Packages/Audience/Runtime/Core/Session.cs b/src/Packages/Audience/Runtime/Core/Session.cs index d6a0b813d..a8da12ba2 100644 --- a/src/Packages/Audience/Runtime/Core/Session.cs +++ b/src/Packages/Audience/Runtime/Core/Session.cs @@ -250,7 +250,7 @@ private void SafeTrack(string eventName, Dictionary properties) } catch (Exception ex) { - Log.Warn($"Session: {eventName} track callback threw {ex.GetType().Name}: {ex.Message}. Event dropped."); + Log.Warn($"Session: {eventName} track callback threw {ex.GetType().Name}. Event dropped."); } } @@ -264,7 +264,7 @@ private void SafeTrack(string eventName, Dictionary properties) } catch (Exception ex) { - Log.Warn($"Session: performance snapshot threw {ex.GetType().Name}: {ex.Message}. Heartbeat ships without performance fields."); + Log.Warn($"Session: performance snapshot threw {ex.GetType().Name}. Heartbeat ships without performance fields."); return null; } } From 1320a1b366361c3140fe60ff897f5594360525bd Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 14:25:49 +1000 Subject: [PATCH 08/19] fix(audience-session): log.Debug breadcrumb on double-Pause The double-Pause guard silently returned. A studio seeing unexpected engagement numbers had no way to confirm a spurious lifecycle-bridge Pause was the cause. Log.Debug (gated by config.Debug) surfaces the event during integration without spamming production logs. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/Core/Session.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Packages/Audience/Runtime/Core/Session.cs b/src/Packages/Audience/Runtime/Core/Session.cs index a8da12ba2..423b57ea1 100644 --- a/src/Packages/Audience/Runtime/Core/Session.cs +++ b/src/Packages/Audience/Runtime/Core/Session.cs @@ -122,7 +122,11 @@ internal void Pause() // Keep the original anchor. Shifting forward shrinks Resume's // pauseDuration (and ComputeEngagedSecondsLocked's live pause // when End fires while paused), over-crediting engagement. - if (_pausedAt.HasValue) return; + if (_pausedAt.HasValue) + { + Log.Debug("Session: Pause while already paused — ignoring."); + return; + } _pausedAt = _getUtcNow(); } } From c0f25273dd207e81163a654ea0c42793f454b83d Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 14:47:08 +1000 Subject: [PATCH 09/19] fix(audience-session): reset starts a new session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior Reset cleared userId and purged the queue but kept the old Session running. Subsequent Track events carried the old sessionId with a new anonymousId — a confusing "session straddles two users" wire state. Match Web SDK reset() behaviour: - Dispose old session (session_end is enqueued then wiped by PurgeAll, matching Web SDK's silent teardown). - Reset identity on disk (next GetOrCreate mints a fresh anonymousId). - Mint a new Session when consent allows tracking; fire session_start via Start() outside _initLock. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Audience/Runtime/ImmutableAudience.cs | 37 +++++++++++++++---- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/src/Packages/Audience/Runtime/ImmutableAudience.cs b/src/Packages/Audience/Runtime/ImmutableAudience.cs index 5f9dc72ed..1e76df46f 100644 --- a/src/Packages/Audience/Runtime/ImmutableAudience.cs +++ b/src/Packages/Audience/Runtime/ImmutableAudience.cs @@ -250,18 +250,39 @@ public static void Alias(string fromId, string fromType, string toId, string toT Enqueue(msg); } - // Logs out the current player. Clears userId, mints a fresh anonymousId, - // discards queued events. Call FlushAsync() first to preserve queued events. + // Logs out the current player. Clears userId, discards queued events, + // mints a fresh anonymousId, and starts a new session. Matches Web SDK + // reset(): no session_end is emitted for the old session (it is enqueued + // and then purged). Call FlushAsync() first to preserve queued events. public static void Reset() { - if (!_initialized) return; + Session? sessionToStart = null; + AudienceConfig? config; + lock (_initLock) + { + if (!_initialized) return; + config = _config; + if (config == null) return; - var config = _config; - if (config == null) return; + // Dispose old session. session_end lands in the queue and is + // wiped by PurgeAll below — matches Web SDK's silent-teardown. + _session?.Dispose(); + _session = null; - _userId = null; - _queue?.PurgeAll(); - Identity.Reset(config.PersistentDataPath!); + _queue?.PurgeAll(); + Identity.Reset(config.PersistentDataPath!); + _userId = null; + + // Mint a new session if consent allows tracking. + if (_consent.CanTrack()) + { + _session = new Session(Track); + sessionToStart = _session; + } + } + + // Start outside _initLock — session_start → Track takes its own locks. + sessionToStart?.Start(); } // Asks the backend to erase this player's data. Await for ack, or discard for fire-and-forget. From 768ab7605365ade015f7e23b6de29f2e5a58a92f Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 14:29:19 +1000 Subject: [PATCH 10/19] test(audience-session): reset starts a new session, does not emit session_end Pin the M2-A1 fix (commit dc286a0e): Reset must end the old session silently, mint a fresh anonymousId, and fire session_start for the new session. Matches Web SDK reset(). - Reset_StartsNewSession_DoesNotEmitSessionEnd: new session_start present, no session_end on disk, new anonymousId differs from the first. - Reset_ConsentNone_DoesNotStartSession: at None consent, Reset must not spin up a new session (no session_start emitted). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Tests/Runtime/Core/SessionTests.cs | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs b/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs index 184c9389a..d39372c2a 100644 --- a/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs +++ b/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs @@ -882,6 +882,57 @@ public void OnPauseAndOnResume_BeforeInit_AreNoOps() Assert.DoesNotThrow(() => ImmutableAudience.OnResume()); } + [Test] + public void Reset_StartsNewSession_DoesNotEmitSessionEnd() + { + // Reset must end the old session and start a new one so subsequent + // Track events carry a fresh sessionId alongside the fresh + // anonymousId — matches Web SDK reset() semantics. The old + // session's session_end is enqueued by Session.Dispose but wiped + // by the PurgeAll in Reset, so the wire sees only a session_start + // for the new session. + ImmutableAudience.Init(MakeConfig(ConsentLevel.Anonymous)); + + // Drain game_launch + session_start for the initial session so we + // only see post-Reset events. + ImmutableAudience.FlushQueueToDiskForTesting(); + var queueDir = Path.Combine(_testDir, "imtbl_audience", "queue"); + foreach (var f in Directory.GetFiles(queueDir, "*.json")) File.Delete(f); + + var firstAnonymousId = Identity.Get(_testDir); + Assert.IsNotNull(firstAnonymousId, "first session should have minted an anonymousId"); + + ImmutableAudience.Reset(); + ImmutableAudience.FlushQueueToDiskForTesting(); + + var files = ReadQueueFiles(); + Assert.IsTrue(files.Any(c => c.Contains("\"session_start\"")), + "Reset must fire session_start for the new session"); + Assert.IsFalse(files.Any(c => c.Contains("\"session_end\"")), + "Reset must not leak session_end for the old session (Web SDK parity)"); + + var secondAnonymousId = Identity.Get(_testDir); + Assert.IsNotNull(secondAnonymousId, "Reset should have minted a fresh anonymousId"); + Assert.AreNotEqual(firstAnonymousId, secondAnonymousId, + "Reset must mint a new anonymousId"); + + ImmutableAudience.Shutdown(); + } + + [Test] + public void Reset_ConsentNone_DoesNotStartSession() + { + // At consent=None there is no session running; Reset must not + // spin one up (Web SDK reset() guards on !isTrackingDisabled()). + ImmutableAudience.Init(MakeConfig(ConsentLevel.None)); + + ImmutableAudience.Reset(); + ImmutableAudience.Shutdown(); + + Assert.IsFalse(ReadQueueFiles().Any(c => c.Contains("\"session_start\"")), + "Reset at consent=None must not fire session_start"); + } + [Test] public void SetConsent_AnonymousToNone_DoesNotEmitSessionEnd() { From 30a2d2cbb157dd32c96b713daea31cc4267da6fe Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 15:16:39 +1000 Subject: [PATCH 11/19] docs(audience-session): plain-language comment pass Rewrites jargon-heavy comments (off-thread, serialise, wire-ordered, fence, CAS, Timer thread) into plain language that reads without a threading background. No code changes. All 220 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/Core/Session.cs | 36 +++++++++++-------- .../Audience/Runtime/ImmutableAudience.cs | 15 ++++---- src/Packages/Audience/Runtime/Utility/Log.cs | 7 ++-- 3 files changed, 35 insertions(+), 23 deletions(-) diff --git a/src/Packages/Audience/Runtime/Core/Session.cs b/src/Packages/Audience/Runtime/Core/Session.cs index 423b57ea1..7e96d04ef 100644 --- a/src/Packages/Audience/Runtime/Core/Session.cs +++ b/src/Packages/Audience/Runtime/Core/Session.cs @@ -12,13 +12,15 @@ namespace Immutable.Audience internal delegate void TrackDelegate(string eventName, Dictionary properties); // Unity session lifecycle. Emits session_start / session_heartbeat / session_end. - // duration is engagement time (excludes pause). Heartbeat fires off-thread; - // public methods run on the caller's thread. _track fires outside _lock. + // duration is engagement time (excludes pause). The heartbeat runs on a + // background thread; other methods run on the thread that called them. The + // track callback is invoked with the internal lock released. // - // Serialisation: Start / End / Dispose are NOT reentrant-safe from multiple - // threads. Callers serialise them (ImmutableAudience holds _initLock across - // Init / SetConsent / Shutdown / Reset, which are the only public entry points - // that touch a Session). Pause / Resume / OnHeartbeat are thread-safe. + // Start / End / Dispose are not safe to call from multiple threads at once. + // Callers run them one at a time (ImmutableAudience holds its init lock while + // calling Init / SetConsent / Shutdown / Reset — the only public entry points + // that touch a Session). Pause / Resume / OnHeartbeat are safe to call from + // any thread. internal sealed class Session : IDisposable { internal const int HeartbeatIntervalMs = 60_000; @@ -63,9 +65,10 @@ internal Session( // Starts a session. Fires session_start and arms the heartbeat timer. internal void Start() { - // Phase 1: drain old timer outside _lock (callback re-enters _lock). - // Old state left intact so a trailing callback emits for the old session - // — wire-ordered before the new session_start. + // Phase 1: shut down the old timer with the internal lock released + // (the callback takes that lock itself). Old state left intact so a + // trailing callback sends a heartbeat for the old session — the + // backend receives it before the new session_start. Timer? oldTimer; lock (_lock) { @@ -216,7 +219,8 @@ internal void OnHeartbeat() lock (_lock) { if (_disposed || _sessionId == null) return; - // Paused sessions don't ship heartbeats (timer still fires; this gates _track). + // A paused session doesn't send heartbeats. The timer keeps + // firing internally; this check stops the event from going out. if (_pausedAt.HasValue) return; sessionId = _sessionId!; @@ -244,8 +248,10 @@ internal void OnHeartbeat() SafeTrack("session_heartbeat", properties); } - // Guards _track from escaping. Heartbeat is a Timer callback (process - // kill on .NET 5+); Start/End are caller-thread (would bubble to Init/Shutdown). + // Stops exceptions from the track callback from reaching upstream. + // Heartbeat runs on a background timer — an uncaught exception there + // crashes the game on modern .NET. Start / End run on the caller's + // thread, where it would bubble into Init / Shutdown. private void SafeTrack(string eventName, Dictionary properties) { try @@ -258,7 +264,8 @@ private void SafeTrack(string eventName, Dictionary properties) } } - // Guards the studio-supplied snapshot from escaping to the timer thread. + // Stops exceptions from the studio-supplied snapshot callback from + // reaching the background timer. private Dictionary? SafePerformanceSnapshot() { if (_performanceSnapshot == null) return null; @@ -288,7 +295,8 @@ private void DrainHeartbeatTimer() using var waited = new ManualResetEvent(false); try { - // Dispose(wh)=false: already disposed, wh never signals — skip the wait. + // Timer was already disposed. The signal handle won't fire, so + // don't wait for it. if (!timer.Dispose(waited)) return; diff --git a/src/Packages/Audience/Runtime/ImmutableAudience.cs b/src/Packages/Audience/Runtime/ImmutableAudience.cs index 1e76df46f..dfd28b007 100644 --- a/src/Packages/Audience/Runtime/ImmutableAudience.cs +++ b/src/Packages/Audience/Runtime/ImmutableAudience.cs @@ -12,9 +12,11 @@ namespace Immutable.Audience // Entry point for the Immutable Audience SDK. public static class ImmutableAudience { - // Reference fields are written inside _initLock; readers fence off the volatile _initialized load. - // _consent, _session are written in _initLock, read outside → volatile. - // _userId is written outside the lock (Identify, Reset) → volatile. + // Reference fields are written inside _initLock; readers check the + // `volatile _initialized` flag first so they never see a half-initialised state. + // _consent and _session are written only inside _initLock but read outside, + // so they stay `volatile` to make writes visible across threads. + // _userId is written outside the lock (Identify, Reset) — `volatile` for the same reason. private static AudienceConfig? _config; private static DiskStore? _store; private static EventQueue? _queue; @@ -610,7 +612,8 @@ private static bool CanTrack() return _initialized && _consent.CanTrack(); } - // Shallow-copy so post-call mutation can't race the drain-thread serialiser. + // Copy the dictionary so the caller editing it later can't corrupt the + // message while the background thread is writing it to disk. private static Dictionary? SnapshotCallerDict(Dictionary? src) => src != null ? new Dictionary(src) : null; @@ -625,8 +628,8 @@ private static void Enqueue(Dictionary? msg) private static void SendBatch() { - // CAS the gate; a previous tick still running → skip (including reschedule, - // which the in-flight tick handles in its finally). + // If a previous send is still running, skip this one. That send + // will reschedule the next tick when it finishes. if (Interlocked.CompareExchange(ref _sendInFlight, 1, 0) != 0) return; diff --git a/src/Packages/Audience/Runtime/Utility/Log.cs b/src/Packages/Audience/Runtime/Utility/Log.cs index f6a5442c7..d22537ce7 100644 --- a/src/Packages/Audience/Runtime/Utility/Log.cs +++ b/src/Packages/Audience/Runtime/Utility/Log.cs @@ -22,9 +22,10 @@ internal static void Warn(string message) => private static void Emit(string line) { - // Swallow Writer/Console throws so Log.Warn/Debug is never-throwing. - // SDK safety wrappers log from inside their own catches; a throwing - // Writer would otherwise reach the Timer thread (process kill on .NET 5+). + // Swallow anything the Writer or Console throws so Log.Warn and + // Log.Debug never throw themselves. If they did, an exception from + // logging inside a catch block would reach the background timer + // and crash the game on modern .NET. try { if (Writer != null) From 859cdbfef2c099514090fda07976046823247e69 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 15:35:10 +1000 Subject: [PATCH 12/19] fix(audience-session): drop nullable annotations on concurrent-race locals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tests project does not enable nullable reference types, so `Exception? initEx = null` triggered CS8632 "The annotation for nullable reference types should only be used in code within a '#nullable' annotations context." Drop the `?` to match the file's and project's existing convention (other tests write e.g. `string capturedKey = null`). Behaviour is unchanged — the locals are still null until the catch writes to them. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs b/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs index d3111915e..b9816eb82 100644 --- a/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs +++ b/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs @@ -648,8 +648,8 @@ public void Init_ConcurrentWithSetConsent_LeavesConsistentState() const int iterations = 50; for (int iter = 0; iter < iterations; iter++) { - Exception? initEx = null; - Exception? setConsentEx = null; + Exception initEx = null; + Exception setConsentEx = null; var setConsentTask = Task.Run(() => { From c8f01736bf328eff5c9d84398abe9f5f296f98f7 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 16:29:16 +1000 Subject: [PATCH 13/19] refactor(audience-session): release _initLock before blocking teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Shutdown and Reset held _initLock across every blocking step — session heartbeat drain (1s), send-timer drain (2s), queue drain-thread join (5s), and the final send (2s by default). Worst-case ~10s. Any caller racing on the lock (SetConsent from a UI thread, a follow-up Init) waited for the full budget. Split both methods into two phases: - Phase 1 under _initLock: flip _initialized, capture disposables into locals, null out the statics. Nanoseconds. - Phase 2 outside _initLock: dispose session, drain timer, shut down queue, flush transport, cancel cts, dispose everything. Same order as before, same idempotency on re-entry — callers arriving after the flag flip see _initialized == false and return cleanly. ResetState used to wrap Shutdown in its own _initLock (relying on Monitor recursion). With Shutdown now releasing the lock before its teardown, the outer wrap would have re-stranded waiters for the full Phase 2 budget. Dropped to Shutdown() + a brief post-lock for the defensive _consent/_session reset. Addresses https://github.com/immutable/unity-immutable-sdk/pull/699#discussion_r3128665711 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Audience/Runtime/ImmutableAudience.cs | 168 ++++++++++-------- 1 file changed, 94 insertions(+), 74 deletions(-) diff --git a/src/Packages/Audience/Runtime/ImmutableAudience.cs b/src/Packages/Audience/Runtime/ImmutableAudience.cs index dfd28b007..c72073012 100644 --- a/src/Packages/Audience/Runtime/ImmutableAudience.cs +++ b/src/Packages/Audience/Runtime/ImmutableAudience.cs @@ -258,33 +258,37 @@ public static void Alias(string fromId, string fromType, string toId, string toT // and then purged). Call FlushAsync() first to preserve queued events. public static void Reset() { - Session? sessionToStart = null; - AudienceConfig? config; + // Phase 1 under _initLock: swap _session and reset identity. Blocking + // work (session drain, disk purge, new session_start) runs outside + // the lock so callers racing on _initLock don't wait on it. + Session? oldSession; + Session? newSession = null; + EventQueue? queueForPurge; + lock (_initLock) { if (!_initialized) return; - config = _config; + var config = _config; if (config == null) return; - // Dispose old session. session_end lands in the queue and is - // wiped by PurgeAll below — matches Web SDK's silent-teardown. - _session?.Dispose(); - _session = null; + oldSession = _session; + queueForPurge = _queue; - _queue?.PurgeAll(); Identity.Reset(config.PersistentDataPath!); _userId = null; - // Mint a new session if consent allows tracking. - if (_consent.CanTrack()) - { - _session = new Session(Track); - sessionToStart = _session; - } + // Swap under the lock so racing SetConsent/OnPause/OnResume see + // either the old, the new, or null — never a torn reference. + _session = _consent.CanTrack() ? new Session(Track) : null; + newSession = _session; } - // Start outside _initLock — session_start → Track takes its own locks. - sessionToStart?.Start(); + // Phase 2 outside _initLock: + // Dispose enqueues session_end → PurgeAll wipes it → Start emits the + // new session_start. Order matches the in-lock sequence this replaces. + oldSession?.Dispose(); + queueForPurge?.PurgeAll(); + newSession?.Start(); } // Asks the backend to erase this player's data. Await for ack, or discard for fire-and-forget. @@ -498,76 +502,92 @@ await transport.SendBatchAsync().ConfigureAwait(false)) // Flushes and stops the SDK. public static void Shutdown() { - // Serialised with Init / SetConsent under _initLock so teardown - // does not race field assignments or the SyncConsentToBackend closure. + // Phase 1 under _initLock: flip _initialized and capture references. + // Other callers racing on _initLock re-check _initialized once they + // acquire and early-return, so they don't wait on Phase 2's drain / + // flush / dispose budget (up to ~10s worst case). + Session? session; + Timer? timer; + EventQueue? queue; + HttpTransport? transport; + HttpClient? controlClient; + CancellationTokenSource? cts; + int timeoutMs; + lock (_initLock) { if (!_initialized) return; - // End session first so session_end hits the queue before the final flush. - _session?.Dispose(); - _session = null; + // Flip the gate first. Init / SetConsent / Reset acquiring after + // this see _initialized == false and return cleanly. + _initialized = false; + + session = _session; _session = null; + timer = _sendTimer; _sendTimer = null; + queue = _queue; _queue = null; + transport = _transport; _transport = null; + controlClient = _controlClient; _controlClient = null; + cts = _shutdownCancellationSource; _shutdownCancellationSource = null; + + timeoutMs = _config?.ShutdownFlushTimeoutMs ?? 2_000; - // Drain in-flight timer callbacks before disposing dependents. - // Parameterless Timer.Dispose would return immediately and race SendBatch. - var timer = _sendTimer; - if (timer != null) + // Drop Identity's in-memory cache so a later Init with a different + // persistentDataPath reads the new file, not the stale cached id. + Identity.ClearCache(); + + _config = null; + _store = null; + _userId = null; + } + + // Phase 2 outside _initLock: end session, drain timers, flush, dispose. + + // End session first so session_end hits the queue before the final flush. + session?.Dispose(); + + // Drain in-flight timer callbacks before disposing dependents. + // Parameterless Timer.Dispose would return immediately and race SendBatch. + if (timer != null) + { + using var disposed = new ManualResetEvent(false); + if (timer.Dispose(disposed)) { - using var disposed = new ManualResetEvent(false); - if (timer.Dispose(disposed)) - { - disposed.WaitOne(TimeSpan.FromSeconds(2)); - } - _sendTimer = null; + disposed.WaitOne(TimeSpan.FromSeconds(2)); } + } - // Clear the gate in case WaitOne timed out with SendBatch still running - // — a later Init would otherwise be stranded at 1. - Interlocked.Exchange(ref _sendInFlight, 0); + // Clear the gate in case WaitOne timed out with SendBatch still running + // — a later Init would otherwise be stranded at 1. + Interlocked.Exchange(ref _sendInFlight, 0); - _queue?.Shutdown(); + queue?.Shutdown(); - // Best-effort final send, capped so a slow network can't hang quit. - if (_transport != null) + // Best-effort final send, capped so a slow network can't hang quit. + if (transport != null) + { + try { - var timeoutMs = _config?.ShutdownFlushTimeoutMs ?? 2_000; - try + var send = transport.SendBatchAsync(); + if (!send.Wait(timeoutMs)) { - var send = _transport.SendBatchAsync(); - if (!send.Wait(timeoutMs)) - { - Log.Warn($"Shutdown flush exceeded {timeoutMs}ms — abandoning. " + - "Queued events remain on disk and will retry on next startup."); - } - } - catch (Exception ex) - { - Log.Warn($"Shutdown flush threw: {ex.GetType().Name}: {ex.Message}"); + Log.Warn($"Shutdown flush exceeded {timeoutMs}ms — abandoning. " + + "Queued events remain on disk and will retry on next startup."); } } + catch (Exception ex) + { + Log.Warn($"Shutdown flush threw: {ex.GetType().Name}: {ex.Message}"); + } + } - // Cancel in-flight control-plane requests before disposing the client - // so awaiters see OperationCanceledException, not ObjectDisposedException. - _shutdownCancellationSource?.Cancel(); - - _transport?.Dispose(); - _queue?.Dispose(); - _controlClient?.Dispose(); - _shutdownCancellationSource?.Dispose(); - _shutdownCancellationSource = null; - - // Drop Identity's in-memory cache so a later Init with a different - // persistentDataPath reads the new file, not the stale cached id. - Identity.ClearCache(); + // Cancel in-flight control-plane requests before disposing the client + // so awaiters see OperationCanceledException, not ObjectDisposedException. + cts?.Cancel(); - _initialized = false; - _config = null; - _store = null; - _queue = null; - _transport = null; - _controlClient = null; - _userId = null; - } + transport?.Dispose(); + queue?.Dispose(); + controlClient?.Dispose(); + cts?.Dispose(); } // ----------------------------------------------------------------- @@ -579,12 +599,12 @@ public static void Shutdown() // domain reload". LaunchContextProvider is re-assigned by AudienceUnityHooks. internal static void ResetState() { - // Same lock as Shutdown/Init; Monitor is recursive so inner Shutdown re-enters. + // Shutdown manages its own serialisation and releases _initLock before + // its Phase 2 teardown, so calling it here does not strand waiters. + Shutdown(); + lock (_initLock) { - if (_initialized) - Shutdown(); - _consent = ConsentLevel.None; // Defensive: Shutdown nulls _session too, but a future refactor // that bails before that null must not leak a stale Session. From ef58761ba13752645193f9c82bc3bf9984d8eece Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 16:40:45 +1000 Subject: [PATCH 14/19] refactor(audience-session): apply two-phase lock to SetConsent and Reset's Identity.Reset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends acc3bab4 to the remaining _initLock callers that still held blocking work: SetConsent held _initLock through Session.Dispose (heartbeat drain, up to 1s on Full→None), ConsentStore.Save (disk), PurgeAll (disk I/O under _drainLock), Identity.Reset (File.Delete), and ApplyAnonymousDowngrade (synchronous file rewrite). Same two-phase shape as Shutdown: flip _consent and swap _session / _userId under the lock; persist, dispose, purge, downgrade, sync, and start outside. Reset still held Identity.Reset inside the lock. Hoisted for symmetry with Shutdown / SetConsent. Order in Phase 2 is Dispose → PurgeAll → Identity.Reset → newSession.Start, matching the in-lock sequence it replaces (session_end is enqueued then wiped; new session_start runs against the fresh anonymousId). Updated the class-level memory-model comment with one paragraph naming the four lifecycle methods and why their hold time is now nanoseconds, not seconds. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Audience/Runtime/ImmutableAudience.cs | 117 +++++++++++------- 1 file changed, 73 insertions(+), 44 deletions(-) diff --git a/src/Packages/Audience/Runtime/ImmutableAudience.cs b/src/Packages/Audience/Runtime/ImmutableAudience.cs index c72073012..c16e7cba0 100644 --- a/src/Packages/Audience/Runtime/ImmutableAudience.cs +++ b/src/Packages/Audience/Runtime/ImmutableAudience.cs @@ -16,7 +16,13 @@ public static class ImmutableAudience // `volatile _initialized` flag first so they never see a half-initialised state. // _consent and _session are written only inside _initLock but read outside, // so they stay `volatile` to make writes visible across threads. - // _userId is written outside the lock (Identify, Reset) — `volatile` for the same reason. + // _userId is written outside the lock (Identify) — `volatile` for the same reason. + // + // Init / Shutdown / Reset / SetConsent hold _initLock only to flip state + // and capture references; they release the lock before running blocking + // teardown (Session.Dispose, timer drain, queue shutdown, transport + // flush, disposes). This keeps the hold time to nanoseconds so a caller + // arriving on a different thread is not stranded behind those budgets. private static AudienceConfig? _config; private static DiskStore? _store; private static EventQueue? _queue; @@ -258,9 +264,10 @@ public static void Alias(string fromId, string fromType, string toId, string toT // and then purged). Call FlushAsync() first to preserve queued events. public static void Reset() { - // Phase 1 under _initLock: swap _session and reset identity. Blocking - // work (session drain, disk purge, new session_start) runs outside - // the lock so callers racing on _initLock don't wait on it. + // Phase 1 under _initLock: swap _session and clear _userId. Blocking + // work (session drain, disk purge, identity wipe, new session_start) + // runs outside the lock so callers racing on _initLock don't wait. + AudienceConfig? config; Session? oldSession; Session? newSession = null; EventQueue? queueForPurge; @@ -268,13 +275,11 @@ public static void Reset() lock (_initLock) { if (!_initialized) return; - var config = _config; + config = _config; if (config == null) return; oldSession = _session; queueForPurge = _queue; - - Identity.Reset(config.PersistentDataPath!); _userId = null; // Swap under the lock so racing SetConsent/OnPause/OnResume see @@ -283,11 +288,13 @@ public static void Reset() newSession = _session; } - // Phase 2 outside _initLock: - // Dispose enqueues session_end → PurgeAll wipes it → Start emits the - // new session_start. Order matches the in-lock sequence this replaces. + // Phase 2 outside _initLock. Order: Dispose enqueues session_end → + // PurgeAll wipes it → Identity.Reset clears the anonymousId file → + // Start emits the new session_start against the fresh id. Matches + // the in-lock sequence this replaces. oldSession?.Dispose(); queueForPurge?.PurgeAll(); + Identity.Reset(config.PersistentDataPath!); newSession?.Start(); } @@ -367,69 +374,91 @@ public static void SetConsent(ConsentLevel level) { if (!_initialized) return; - // Serialised under _initLock: prevents concurrent upgrades from each - // building a fresh Session (stranding timers), and prevents racing - // Init's _session assignment. - Session? sessionToStart = null; + // Phase 1 under _initLock: flip _consent and swap _session / _userId. + // Phase 2 outside the lock runs the blocking side effects (persist, + // dispose, purge, downgrade, backend sync, new session_start) so a + // concurrent Shutdown / Init / Reset isn't held waiting on them. + ConsentLevel previous; + AudienceConfig? config; + EventQueue? queue; + Session? oldSession = null; + Session? newSession = null; + string? anonymousIdForPut; + bool downgradeFullToAnonymous = false; + lock (_initLock) { if (!_initialized) return; - var config = _config; - var queue = _queue; + config = _config; + queue = _queue; if (config == null) return; - var previous = _consent; + previous = _consent; if (level == previous) return; // Snapshot anonymousId before Identity.Reset (on None) wipes it. // The PUT audit trail needs to record whose consent changed. - var anonymousIdForPut = previous == ConsentLevel.None + anonymousIdForPut = previous == ConsentLevel.None ? Identity.GetOrCreate(config.PersistentDataPath!, level) : Identity.Get(config.PersistentDataPath!); _consent = level; - try - { - // PersistentDataPath validated non-null in Init; compiler can't propagate that. - ConsentStore.Save(config.PersistentDataPath!, level); - } - catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException) - { - Log.Warn($"SetConsent — failed to persist consent level: {ex.GetType().Name}: {ex.Message}. " + - "In-memory level is updated but will revert on next launch."); - NotifyErrorCallback(config.OnError, AudienceErrorCode.ConsentPersistFailed, - $"Consent persist failed: {ex.Message}"); - } - if (level == ConsentLevel.None) { - // Dispose for timer cleanup. session_end is gated out by - // CanTrack (post-flip), matching revocation semantics. - _session?.Dispose(); + // Swap the session reference under the lock; dispose outside. + // session_end is gated out by CanTrack (post-flip), matching + // revocation semantics. + oldSession = _session; _session = null; - - queue?.PurgeAll(); - Identity.Reset(config.PersistentDataPath!); } else if (previous == ConsentLevel.Full && level == ConsentLevel.Anonymous) { _userId = null; - queue?.ApplyAnonymousDowngrade(); + downgradeFullToAnonymous = true; } else if (previous == ConsentLevel.None && _session == null) { - // Upgrade from None: previous session was disposed. Start a - // fresh one. Start() deferred to outside the lock. - _session = new Session(Track); - sessionToStart = _session; + // Upgrade from None: allocate + publish the new Session under + // the lock so a concurrent SetConsent / Init sees the new + // reference and the double-allocation guard above fires. + newSession = new Session(Track); + _session = newSession; } + } - SyncConsentToBackend(config, level, anonymousIdForPut); + // Phase 2 outside _initLock. + try + { + // PersistentDataPath validated non-null in Init; compiler can't propagate that. + ConsentStore.Save(config.PersistentDataPath!, level); + } + catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException) + { + Log.Warn($"SetConsent — failed to persist consent level: {ex.GetType().Name}: {ex.Message}. " + + "In-memory level is updated but will revert on next launch."); + NotifyErrorCallback(config.OnError, AudienceErrorCode.ConsentPersistFailed, + $"Consent persist failed: {ex.Message}"); } - sessionToStart?.Start(); + if (level == ConsentLevel.None) + { + oldSession?.Dispose(); + queue?.PurgeAll(); + Identity.Reset(config.PersistentDataPath!); + } + else if (downgradeFullToAnonymous) + { + // Synchronous: EventQueue.ApplyAnonymousDowngrade holds _drainLock + // while it rewrites on-disk files, blocking the in-queue drain + // and shutting the race with HttpTransport. See the method's comment. + queue?.ApplyAnonymousDowngrade(); + } + + newSession?.Start(); + + SyncConsentToBackend(config, level, anonymousIdForPut); } // Fire-and-forget PUT /v1/audience/tracking-consent. From f8855b169d40760510623d1aa31c299cf2015531 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 16:40:54 +1000 Subject: [PATCH 15/19] test(audience-session): regression test proving _initLock is released before Phase 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Locks a Shutdown inside its Phase 2 final flush via a BlockingHandler that never returns from SendAsync, then calls Reset on the main thread with a stopwatch. Pre-refactor, Reset would wait on _initLock for the full ShutdownFlushTimeoutMs (up to 10s in this test's setup). Post-refactor, Reset acquires the lock immediately, sees !_initialized, and returns in microseconds. Asserts Reset completes in under 500ms — generous margin to avoid CI flakes while still catching a regression that re-introduces a seconds-scale hold. Addresses the reviewer's concern from PR #699 and guards against future changes that move blocking work back inside the lock. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Tests/Runtime/ImmutableAudienceTests.cs | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs b/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs index b9816eb82..8cd9f491d 100644 --- a/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs +++ b/src/Packages/Audience/Tests/Runtime/ImmutableAudienceTests.cs @@ -951,6 +951,44 @@ public void Track_AfterShutdown_IsIgnored() Assert.DoesNotThrow(() => ImmutableAudience.Track("should_not_crash")); } + [Test] + public void Shutdown_ReleasesInitLock_BeforeBlockingTeardown() + { + // Hanging handler: the final flush inside Shutdown's Phase 2 will + // block in transport.SendBatchAsync().Wait(timeoutMs). Pre-refactor, + // _initLock was held across that wait — SetConsent / Reset on another + // thread would be stranded for the full ShutdownFlushTimeoutMs. + var handler = new BlockingHandler(); + var config = MakeConfig(); + config.HttpHandler = handler; + config.ShutdownFlushTimeoutMs = 10_000; + + ImmutableAudience.Init(config); + ImmutableAudience.Track("ensure_nonempty_queue"); + ImmutableAudience.FlushQueueToDiskForTesting(); + + // Phase 1 flips _initialized and releases the lock; Phase 2 enters + // the hanging final flush. + var shutdown = Task.Run(() => ImmutableAudience.Shutdown()); + + Assert.IsTrue(handler.EnteredSendAsync.Wait(TimeSpan.FromSeconds(5)), + "Shutdown should reach the hanging final flush inside Phase 2"); + + // Reset unconditionally acquires _initLock. If Phase 2 held the lock + // this would block for ~10s; post-refactor the lock is free, Reset + // sees !_initialized and returns in microseconds. + var sw = System.Diagnostics.Stopwatch.StartNew(); + ImmutableAudience.Reset(); + sw.Stop(); + + Assert.Less(sw.ElapsedMilliseconds, 500, + $"Reset must not block on Shutdown's Phase 2; took {sw.ElapsedMilliseconds}ms"); + + handler.Release.Set(); + Assert.IsTrue(shutdown.Wait(TimeSpan.FromSeconds(15)), + "Shutdown should finish after handler release"); + } + // ----------------------------------------------------------------- // Full -> Anonymous consent downgrade // ----------------------------------------------------------------- From 0e30a242a15fdfa80da1dccb1b887cdd55d09eea Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 17:47:34 +1000 Subject: [PATCH 16/19] fix(audience-session): emit session_end before flipping _initialized in Shutdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two-phase refactor broke Shutdown_FiresSessionEnd. Phase 1 flipped _initialized to false, then Phase 2 called session.Dispose(). Session fires session_end through ImmutableAudience.Track, which guards on CanTrack() = _initialized && _consent.CanTrack(). With _initialized already false, Track dropped the event on the floor. Added Session.EmitEndAndSeal — an emit-only variant of End() that fires session_end and resets state but does not drain the heartbeat timer. Shutdown calls it under _initLock before the flag flip, so Track's gate still lets session_end through. Heartbeat timer drain continues to happen in Phase 2 via session.Dispose(); the second End() inside Dispose no-ops because EmitEndAndSeal already cleared _sessionId. Lock hold time is unchanged: EmitEndAndSeal takes Session._lock for microseconds to compute duration and reset state, then releases before calling SafeTrack → Track → Enqueue (which takes EventQueue's _drainLock, also microseconds). Nothing blocking runs under _initLock. Also drops the column-aligned capture block in Shutdown Phase 1 — dotnet-format rejects the extra whitespace, so one assignment per line. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/Core/Session.cs | 25 +++++++++++++++++ .../Audience/Runtime/ImmutableAudience.cs | 27 ++++++++++++++----- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/src/Packages/Audience/Runtime/Core/Session.cs b/src/Packages/Audience/Runtime/Core/Session.cs index 7e96d04ef..b077539fe 100644 --- a/src/Packages/Audience/Runtime/Core/Session.cs +++ b/src/Packages/Audience/Runtime/Core/Session.cs @@ -193,6 +193,31 @@ internal void End() }); } + // Emits session_end and seals the session without draining the heartbeat + // timer. Use when the caller needs to fire session_end inside a short + // gating lock (e.g. ImmutableAudience.Shutdown under _initLock while + // _initialized is still true) and will drain + dispose the timer after + // releasing the lock. Idempotent: a subsequent Dispose() → End() will + // find _sessionId null and no-op the re-emission. + internal void EmitEndAndSeal() + { + string sessionId; + long duration; + lock (_lock) + { + if (_disposed || _sessionId == null) return; + sessionId = _sessionId!; + duration = ComputeEngagedSecondsLocked(); + ResetSessionStateLocked(); + } + + SafeTrack("session_end", new Dictionary + { + ["sessionId"] = sessionId, + ["durationSec"] = duration + }); + } + public void Dispose() { lock (_lock) diff --git a/src/Packages/Audience/Runtime/ImmutableAudience.cs b/src/Packages/Audience/Runtime/ImmutableAudience.cs index c16e7cba0..cac1202c6 100644 --- a/src/Packages/Audience/Runtime/ImmutableAudience.cs +++ b/src/Packages/Audience/Runtime/ImmutableAudience.cs @@ -547,16 +547,29 @@ public static void Shutdown() { if (!_initialized) return; - // Flip the gate first. Init / SetConsent / Reset acquiring after + // Emit session_end inside the lock while _initialized is still + // true so Track's CanTrack gate lets it through. Heartbeat timer + // drain is deferred to Phase 2; the second emission from + // session.Dispose → End() no-ops because EmitEndAndSeal reset + // _sessionId. + _session?.EmitEndAndSeal(); + + // Flip the gate. Init / SetConsent / Reset acquiring after // this see _initialized == false and return cleanly. _initialized = false; - session = _session; _session = null; - timer = _sendTimer; _sendTimer = null; - queue = _queue; _queue = null; - transport = _transport; _transport = null; - controlClient = _controlClient; _controlClient = null; - cts = _shutdownCancellationSource; _shutdownCancellationSource = null; + session = _session; + _session = null; + timer = _sendTimer; + _sendTimer = null; + queue = _queue; + _queue = null; + transport = _transport; + _transport = null; + controlClient = _controlClient; + _controlClient = null; + cts = _shutdownCancellationSource; + _shutdownCancellationSource = null; timeoutMs = _config?.ShutdownFlushTimeoutMs ?? 2_000; From 1812e0ddc9d5ea405b0ccb92cfb90b7acff67766 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 18:24:21 +1000 Subject: [PATCH 17/19] refactor(audience-session): call EmitEndAndSeal before _initLock, not inside it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor Bugbot flagged that EmitEndAndSeal's Track call chain (Identity GetOrCreate → Enqueue → _drainLock) was running under _initLock in 20e5e869, contradicting the "nanoseconds" claim and re-introducing the exact concern nattb8 raised in the original review. Move _session?.EmitEndAndSeal() to before the lock acquire. Idempotent via Session._sessionId reset — safe to call without outer serialisation. _initLock now covers only the flag flip and reference captures. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Audience/Runtime/ImmutableAudience.cs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/Packages/Audience/Runtime/ImmutableAudience.cs b/src/Packages/Audience/Runtime/ImmutableAudience.cs index cac1202c6..937c5a3f5 100644 --- a/src/Packages/Audience/Runtime/ImmutableAudience.cs +++ b/src/Packages/Audience/Runtime/ImmutableAudience.cs @@ -531,6 +531,14 @@ await transport.SendBatchAsync().ConfigureAwait(false)) // Flushes and stops the SDK. public static void Shutdown() { + // Fire session_end before taking _initLock. _initialized is still + // true here so Track's CanTrack gate lets it through. Idempotent + // under concurrent Shutdown / SetConsent(None) via the _sessionId + // reset inside EmitEndAndSeal — a second call finds _sessionId + // null and no-ops. Heartbeat timer drain still runs in Phase 2 + // via session.Dispose(); its re-emission inside End() also no-ops. + _session?.EmitEndAndSeal(); + // Phase 1 under _initLock: flip _initialized and capture references. // Other callers racing on _initLock re-check _initialized once they // acquire and early-return, so they don't wait on Phase 2's drain / @@ -547,13 +555,6 @@ public static void Shutdown() { if (!_initialized) return; - // Emit session_end inside the lock while _initialized is still - // true so Track's CanTrack gate lets it through. Heartbeat timer - // drain is deferred to Phase 2; the second emission from - // session.Dispose → End() no-ops because EmitEndAndSeal reset - // _sessionId. - _session?.EmitEndAndSeal(); - // Flip the gate. Init / SetConsent / Reset acquiring after // this see _initialized == false and return cleanly. _initialized = false; From d4fd6e7b836992a8e9834f6ee7242c168ae00793 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 18:54:53 +1000 Subject: [PATCH 18/19] refactor(audience-session): hoist Identity out of _initLock; double-seal in Shutdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Cursor Bugbot findings on 63b13bb3: SetConsent held _initLock across Identity.GetOrCreate / Identity.Get. On the None → Anonymous/Full upgrade path, GetOrCreate creates directories and writes a UUID file — real disk I/O under the lock. Moved the capture outside _initLock. Identity's own _sync lock serialises the I/O; TOCTOU on previous is accepted — a racing SetConsent fires its own PUT and our slightly-stale anonymousId still identifies the user. Shutdown's outside-lock EmitEndAndSeal targeted whatever _session pointed to at call time. A Reset completing fully (including new session Start()) in the narrow window before Shutdown's lock acquire would leave the new session's session_start on the wire with no matching session_end — Shutdown disposes the new session after flipping _initialized=false, so Track drops the closing event. Added a second EmitEndAndSeal inside the lock as a race guard. Idempotent on the same instance (no-op via _sessionId null check); the slow path only runs when the race actually happened. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Audience/Runtime/ImmutableAudience.cs | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/Packages/Audience/Runtime/ImmutableAudience.cs b/src/Packages/Audience/Runtime/ImmutableAudience.cs index 937c5a3f5..53027a429 100644 --- a/src/Packages/Audience/Runtime/ImmutableAudience.cs +++ b/src/Packages/Audience/Runtime/ImmutableAudience.cs @@ -374,16 +374,32 @@ public static void SetConsent(ConsentLevel level) { if (!_initialized) return; + var config = _config; + if (config == null) return; + + // Snapshot check before any I/O: no-op if already at target consent. + var snapshotPrevious = _consent; + if (level == snapshotPrevious) return; + + // Capture anonymousId for the PUT audit trail outside _initLock. + // Identity methods hold their own _sync lock; disk I/O on a cold + // cache (None → Anonymous/Full upgrade creates the UUID file) does + // not block _initLock. A racing SetConsent may change _consent + // between this read and our lock acquire — acceptable, the racing + // call fires its own PUT and our slightly-stale ID still + // identifies the user. + var anonymousIdForPut = snapshotPrevious == ConsentLevel.None + ? Identity.GetOrCreate(config.PersistentDataPath!, level) + : Identity.Get(config.PersistentDataPath!); + // Phase 1 under _initLock: flip _consent and swap _session / _userId. // Phase 2 outside the lock runs the blocking side effects (persist, // dispose, purge, downgrade, backend sync, new session_start) so a // concurrent Shutdown / Init / Reset isn't held waiting on them. ConsentLevel previous; - AudienceConfig? config; EventQueue? queue; Session? oldSession = null; Session? newSession = null; - string? anonymousIdForPut; bool downgradeFullToAnonymous = false; lock (_initLock) @@ -397,12 +413,6 @@ public static void SetConsent(ConsentLevel level) previous = _consent; if (level == previous) return; - // Snapshot anonymousId before Identity.Reset (on None) wipes it. - // The PUT audit trail needs to record whose consent changed. - anonymousIdForPut = previous == ConsentLevel.None - ? Identity.GetOrCreate(config.PersistentDataPath!, level) - : Identity.Get(config.PersistentDataPath!); - _consent = level; if (level == ConsentLevel.None) @@ -555,6 +565,15 @@ public static void Shutdown() { if (!_initialized) return; + // Race guard: a concurrent Reset or SetConsent(upgrade-from-None) + // may have swapped _session to a new instance that has already + // fired session_start. Seal it too so its session_end lands + // before the flag flip. Idempotent on the same instance (no-op + // via _sessionId null check); the slow path only runs when + // Reset fully completed its Start() between the outside-lock + // call above and this point — a narrow window. + _session?.EmitEndAndSeal(); + // Flip the gate. Init / SetConsent / Reset acquiring after // this see _initialized == false and return cleanly. _initialized = false; From 95b42b09348857c542f1413b0973e45fe3414ea7 Mon Sep 17 00:00:00 2001 From: ImmutableJeffrey Date: Thu, 23 Apr 2026 19:10:56 +1000 Subject: [PATCH 19/19] fix(audience-session): clamp livePause in ComputeEngagedSecondsLocked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor Bugbot finding on c43044a6. Resume() clamps pauseDuration ≥ 0 for wall-clock rewinds, but the mirror calculation in ComputeEngagedSecondsLocked did not. If the clock rewinds while the session is still paused and End fires (e.g. Shutdown while backgrounded with NTP pulling time back), livePause goes negative. Being subtracted, it inflated engagedSeconds past the wall-clock window. The final engagedSeconds ≥ 0 clamp caught negatives but not over-credit. Mirror the Resume-side clamp: livePause < 0 → 0. Added End_ClockRewindsWhilePaused_DoesNotInflateDuration as a regression guard. 10s before pause, clock rewinds 5s while paused, End fires — duration must stay within the 5s wall-clock window. Without the clamp the test reports 15s. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Packages/Audience/Runtime/Core/Session.cs | 6 ++++ .../Tests/Runtime/Core/SessionTests.cs | 29 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/src/Packages/Audience/Runtime/Core/Session.cs b/src/Packages/Audience/Runtime/Core/Session.cs index b077539fe..a0f4414a9 100644 --- a/src/Packages/Audience/Runtime/Core/Session.cs +++ b/src/Packages/Audience/Runtime/Core/Session.cs @@ -342,6 +342,12 @@ private long ComputeEngagedSecondsLocked() { var now = _getUtcNow(); var livePause = _pausedAt.HasValue ? now - _pausedAt.Value : TimeSpan.Zero; + // Clamp: mirrors the Resume() guard. If the clock rewinds while the + // session is still paused and End / EmitEndAndSeal fires (e.g. + // Shutdown while backgrounded), livePause would be negative and, + // being subtracted, would inflate engagedSeconds past the wall-clock + // window. The final ≥0 clamp catches negatives but not inflation. + if (livePause < TimeSpan.Zero) livePause = TimeSpan.Zero; var engagedSeconds = ((now - _sessionStart) - _accumulatedPause - livePause).TotalSeconds; if (engagedSeconds < 0) return 0; return (long)Math.Round(engagedSeconds, MidpointRounding.AwayFromZero); diff --git a/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs b/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs index d39372c2a..0024d5ac0 100644 --- a/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs +++ b/src/Packages/Audience/Tests/Runtime/Core/SessionTests.cs @@ -363,6 +363,35 @@ public void End_ClockRewindsSinceStart_ClampsDurationToZero() "negative engaged time from a wall-clock rewind must clamp to zero"); } + [Test] + public void End_ClockRewindsWhilePaused_DoesNotInflateDuration() + { + // Wall-clock rewinds while the session is still paused (e.g. the + // app is backgrounded and NTP corrects backwards before Shutdown + // fires End). Without the livePause ≥ 0 clamp in + // ComputeEngagedSecondsLocked, livePause goes negative and, + // being subtracted, inflates duration past the wall-clock window + // — the final engagedSeconds ≥ 0 clamp only catches negatives, + // not over-credit. Sabotage: removing the livePause clamp lets + // this test report 15s instead of the ≤ 5s wall-clock window. + var now = new DateTime(2026, 4, 20, 12, 0, 0, DateTimeKind.Utc); + DateTime Clock() => now; + + using var session = new Session(MockTrack, performanceSnapshot: null, getUtcNow: Clock); + session.Start(); + + now = now.AddSeconds(10); + session.Pause(); + + now = now.AddSeconds(-5); // clock rewinds 5s while paused + session.End(); + + var sessionEnd = _events.Last(e => e.name == "session_end"); + var duration = (long)sessionEnd.props["durationSec"]; + Assert.LessOrEqual(duration, 5L, + "clock rewind while paused must not over-credit engagement past the wall-clock window"); + } + [Test] public void End_AfterShortPause_ReportsDurationMinusPause() {