Skip to content

Commit d309911

Browse files
committed
fix(unity): honest WebGL saves, SSE transport hygiene, router cleanup, destroy honesty + benchmark Stop button
Audit wave 2026-07-11, findings 5/6/12/13 + owner request: - WebGL IndexedDB flush failures logged; 'Saved N objects' honest on failure - FetchSseOpenAiTransport: swallowed catches log with call id, concurrent ReadAsync fails loudly, stream state entries always removed on completion - AiGameCommandRouter.Dispose clears the static CommandReceived event - world_command destroy returns failure for missing targets - Benchmark window: 'Stop (save partial)' button next to Run while active
1 parent a31a49c commit d309911

10 files changed

Lines changed: 276 additions & 19 deletions

File tree

Assets/CoreAiUnity/CHANGELOG.md

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,26 @@ Unity host: **CoreAI.Source** build, EditMode / PlayMode tests, Editor menus, do
44

55
## [Unreleased]
66

7+
### Fixed (2026-07-11 audit wave)
8+
9+
- **WebGL persistence: IndexedDB flush failures are no longer silent.** `CoreAiWebGlPersistence` and
10+
`WorldStateManager` log a warning when `CoreAi_PersistFsSync` throws, and the "Saved N objects" log is
11+
honest about a failed flush (the save may not survive a reload).
12+
- **WebGL SSE transport hygiene** (`FetchSseOpenAiTransport`): all previously-empty catches log with the
13+
call id (including a failed browser-fetch abort), a second concurrent `ReadAsync` fails loudly instead of
14+
silently overwriting the pending slot, and terminal callbacks/open-failures always remove the stream's
15+
entry from the static state table (no more permanent leaks from abandoned streams).
16+
- **`AiGameCommandRouter.Dispose()` clears the static `CommandReceived` event**, so commands after a scene
17+
reload no longer reach dead subscribers from previous scenes (duplicate world mutations).
18+
- **`world_command destroy` reports failure for missing targets** (was: unconditional success, so the model
19+
could never self-correct a typo'd object name).
20+
21+
### Added
22+
23+
- **Benchmark window: Stop button.** "Stop (save partial)" appears next to "Run benchmark" while a run is
24+
active (same cooperative stop as the `CoreAI/Benchmarks/Stop Running Benchmark` menu item: finishes the
25+
current scenario, saves a partial report); shows "Stopping…" once requested.
26+
727
## 5.6.1 - Build-time policy registration + code-style pass (2026-07-11)
828

929
- **`AgentBuilder.Build()` auto-registers with the global policy** when one exists (immediate routability);

Assets/CoreAiUnity/Runtime/Source/Features/Llm/Infrastructure/FetchSseOpenAiTransport.cs

Lines changed: 82 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,19 @@ public async Task<OpenAiHttpSseOpenResult> OpenSseResponseStreamAsync(OpenAiHttp
8282
// to be captured so the continuation reliably runs on the browser main thread.
8383
// ConfigureAwait(false) routes the continuation to ThreadPool, which doesn't
8484
// exist in WebGL, and the await never resumes.
85-
StreamState.OpenInfo openInfo = await state.WaitForOpenAsync();
85+
StreamState.OpenInfo openInfo;
86+
try
87+
{
88+
openInfo = await state.WaitForOpenAsync();
89+
}
90+
catch
91+
{
92+
// WHY: WaitForOpenAsync can throw (typed jslib Timeout, cancellation). On that path the
93+
// stream is never handed to the caller, so nothing else would ever dispose the state or
94+
// remove its entry from the static States dictionary — clean up here before rethrowing.
95+
state.Dispose();
96+
throw;
97+
}
8698

8799
OpenAiHttpSseOpenResult result = new OpenAiHttpSseOpenResult
88100
{
@@ -256,9 +268,12 @@ public void AttachCancellation(CancellationToken cancellationToken)
256268
{
257269
CoreAi_FetchSseAbort(CallId);
258270
}
259-
catch
271+
catch (Exception ex)
260272
{
261-
// Browser abort must not throw back into the Unity cancellation path.
273+
// Browser abort must not throw back into the Unity cancellation path, but a
274+
// failed abort leaks the browser-side fetch — log it instead of swallowing.
275+
UnityEngine.Debug.LogWarning(
276+
$"[FetchSSE] CoreAi_FetchSseAbort failed on cancellation (callId={CallId}): {ex.Message}");
262277
}
263278

264279
SignalCancelled(cancellationToken);
@@ -286,8 +301,19 @@ public void SignalDone()
286301
_stream.PumpPendingRead();
287302
// Defensive: if SignalDone fires before SignalOpen (shouldn't happen, but
288303
// caller surfaces a transport error instead of awaiting forever.
304+
// TODO: In the jslib flow onOpen always precedes onDone; the only way to get here
305+
// without an open result is when the onOpen dyncall itself threw and was swallowed
306+
// by CoreAiSseFetch.jslib's callOpen catch. In that case the fetch may actually have
307+
// succeeded (its chunks are already buffered in _queue), but the real HTTP status is
308+
// lost, so this surfaces a fake "HTTP 0 / no headers" error. Recovering the real
309+
// result would require the jslib to re-deliver the open info alongside done.
289310
_openTcs.TrySetResult(new OpenInfo(0, "fetch completed without headers",
290311
new Dictionary<string, IEnumerable<string>>()));
312+
313+
// onDone is terminal (the jslib finish() guard drops any later callback for this id),
314+
// so drop the States entry now: a consumer that abandons the stream without disposing
315+
// it would otherwise leak the entry in the static dictionary permanently.
316+
States.TryRemove(CallId, out _);
291317
}
292318

293319
public void SignalError(Exception ex)
@@ -321,6 +347,10 @@ public void SignalError(Exception ex)
321347
_openTcs.TrySetResult(new OpenInfo(0, ex?.Message ?? "fetch error",
322348
new Dictionary<string, IEnumerable<string>>()));
323349
}
350+
351+
// onError is terminal (the jslib finish() guard drops any later callback for this id),
352+
// so drop the States entry now — same abandoned-stream leak rationale as SignalDone.
353+
States.TryRemove(CallId, out _);
324354
}
325355

326356
public void SignalCancelled(CancellationToken cancellationToken)
@@ -344,9 +374,27 @@ public void Dispose()
344374
// that is never removed (consumer drops the stream without reading to completion) is a
345375
// permanent leak; removing on Dispose bounds the dictionary to live streams only.
346376
States.TryRemove(CallId, out _);
347-
try { _cancelRegistration.Dispose(); } catch { }
348-
try { _signal.Set(); } catch { }
349-
try { _signal.Dispose(); } catch { }
377+
try
378+
{
379+
_cancelRegistration.Dispose();
380+
}
381+
catch (Exception ex)
382+
{
383+
UnityEngine.Debug.LogWarning(
384+
$"[FetchSSE] cancellation registration dispose failed (callId={CallId}): {ex.Message}");
385+
}
386+
387+
try
388+
{
389+
_signal.Set();
390+
_signal.Dispose();
391+
}
392+
catch (Exception ex)
393+
{
394+
// Double-dispose race: the signal may already be disposed; still worth surfacing.
395+
UnityEngine.Debug.LogWarning(
396+
$"[FetchSSE] signal release failed on dispose (callId={CallId}): {ex.Message}");
397+
}
350398
}
351399

352400
public FetchSseStream Stream => _stream;
@@ -404,6 +452,14 @@ public override Task<int> ReadAsync(byte[] buffer, int offset, int count, Cancel
404452
if (_error != null) return Task.FromException<int>(_error);
405453
if (cancellationToken.IsCancellationRequested) return Task.FromCanceled<int>(cancellationToken);
406454

455+
// WHY: there is a single pending-read slot; a second concurrent ReadAsync would silently
456+
// overwrite _pendingTcs and leave the first awaiter hanging until Dispose. Fail loudly.
457+
if (_pendingTcs != null)
458+
{
459+
return Task.FromException<int>(new InvalidOperationException(
460+
"FetchSseStream supports only one pending ReadAsync at a time; the previous read has not completed."));
461+
}
462+
407463
int n = TryReadNonBlocking(buffer, offset, count);
408464
if (n > 0) return Task.FromResult(n);
409465
if (_isDone) return Task.FromResult(0);
@@ -507,11 +563,29 @@ protected override void Dispose(bool disposing)
507563
// (with EOF) here, otherwise its TCS and per-read cancellation registration
508564
// stay alive until the whole request token ends.
509565
PumpPendingRead();
510-
try { CoreAi_FetchSseAbort(_owner.CallId); } catch { }
566+
try
567+
{
568+
CoreAi_FetchSseAbort(_owner.CallId);
569+
}
570+
catch (Exception ex)
571+
{
572+
// A failed abort leaks the browser-side fetch/ReadableStream for this call.
573+
UnityEngine.Debug.LogWarning(
574+
$"[FetchSSE] CoreAi_FetchSseAbort failed on stream dispose (callId={_owner.CallId}): {ex.Message}");
575+
}
576+
511577
// Owner disposal also removes the States entry and drops the request-token
512578
// cancellation registration, which would otherwise pin this stream (queue,
513579
// buffered chunks and all) to the caller's token lifetime on the success path.
514-
try { _owner.Dispose(); } catch { }
580+
try
581+
{
582+
_owner.Dispose();
583+
}
584+
catch (Exception ex)
585+
{
586+
UnityEngine.Debug.LogWarning(
587+
$"[FetchSSE] stream state dispose failed (callId={_owner.CallId}): {ex.Message}");
588+
}
515589
}
516590

517591
base.Dispose(disposing);

Assets/CoreAiUnity/Runtime/Source/Features/Messaging/Infrastructure/AiGameCommandRouter.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,14 @@ public void Dispose()
7575
{
7676
_disposed = true;
7777
_subscription?.Dispose();
78+
79+
// WHY: CommandReceived is process-global (static) while its subscribers are typically
80+
// scene-scoped (e.g. AiDashboardPresenter). A subscriber that misses its own unsubscribe
81+
// would survive a scene reload and receive commands routed by the next scene's router —
82+
// duplicate world mutations against destroyed objects. The router is registered once per
83+
// CoreAILifetimeScope, so clearing on scope teardown is safe: live subscribers in the new
84+
// scene re-attach via their own OnEnable/Initialize.
85+
CommandReceived = null;
7886
}
7987
}
8088
}

Assets/CoreAiUnity/Runtime/Source/Features/World/Infrastructure/CoreAiWorldCommandExecutor.cs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -749,11 +749,16 @@ private bool TryParentSpawned(GameObject child, string parentName, bool worldPos
749749

750750
private bool TryDestroy(CoreAiWorldCommandEnvelope env)
751751
{
752-
if (ResolveObject(env.targetName, out GameObject go))
752+
if (!ResolveObject(env.targetName, out GameObject go))
753753
{
754-
UnityEngine.Object.Destroy(go);
754+
// WHY: Returning true here reported "ok" for a typo'd/missing target, so the model
755+
// could never self-correct — every other verb fails on an unresolved target.
756+
_logger.LogWarning(GameLogFeature.MessagePipe,
757+
$"[World] destroy: object not found (name='{env.targetName}')");
758+
return false;
755759
}
756760

761+
UnityEngine.Object.Destroy(go);
757762
return true;
758763
}
759764

Assets/CoreAiUnity/Runtime/Source/Features/World/WorldStateManager.cs

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,26 @@ public sealed class WorldStateManager : IWorldStateManager, IDisposable
2626
private static extern void CoreAi_PersistFsSync();
2727
#endif
2828

29-
/// <summary>On WebGL pushes the in-memory IDBFS tree into IndexedDB after a save so it survives a reload/tab close.</summary>
30-
private static void PersistFsForWebGl()
29+
/// <summary>
30+
/// On WebGL pushes the in-memory IDBFS tree into IndexedDB after a save so it survives a
31+
/// reload/tab close. Returns false when the flush threw (the save reached IDBFS memory only),
32+
/// so the caller can report the save honestly instead of claiming durable success.
33+
/// </summary>
34+
private bool PersistFsForWebGl()
3135
{
3236
#if UNITY_WEBGL && !UNITY_EDITOR
33-
try { CoreAi_PersistFsSync(); } catch { /* best-effort flush */ }
37+
try
38+
{
39+
CoreAi_PersistFsSync();
40+
}
41+
catch (Exception ex)
42+
{
43+
_logger.LogWarning(GameLogFeature.Core,
44+
$"[WorldState] IndexedDB flush failed after save: {ex.Message}");
45+
return false;
46+
}
3447
#endif
48+
return true;
3549
}
3650

3751
private static readonly Color NoColor = new(-1f, -1f, -1f, -1f);
@@ -262,10 +276,19 @@ public void Save()
262276
File.Move(tmpPath, _saveFilePath);
263277
}
264278

265-
PersistFsForWebGl();
266-
267-
_logger.LogInfo(GameLogFeature.Core,
268-
$"[WorldState] Saved {objects.Count} objects.");
279+
if (PersistFsForWebGl())
280+
{
281+
_logger.LogInfo(GameLogFeature.Core,
282+
$"[WorldState] Saved {objects.Count} objects.");
283+
}
284+
else
285+
{
286+
// WHY: On WebGL the file only reached the in-memory IDBFS tree; the IndexedDB
287+
// flush threw, so a reload/tab close can still lose this save. Don't claim
288+
// durable success — the flush failure was already logged by PersistFsForWebGl.
289+
_logger.LogWarning(GameLogFeature.Core,
290+
$"[WorldState] Saved {objects.Count} objects to in-memory FS, but the IndexedDB flush failed; the save may not survive a reload.");
291+
}
269292
}
270293
catch (Exception ex)
271294
{

Assets/CoreAiUnity/Runtime/Source/Infrastructure/CoreAiWebGlPersistence.cs

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,17 @@ public static class CoreAiWebGlPersistence
2222
public static void Sync()
2323
{
2424
#if UNITY_WEBGL && !UNITY_EDITOR
25-
try { CoreAi_PersistFsSync(); } catch { /* best-effort flush */ }
25+
try
26+
{
27+
CoreAi_PersistFsSync();
28+
}
29+
catch (System.Exception ex)
30+
{
31+
// WHY: A failed flush means the preceding write may not survive a reload/tab close —
32+
// that must be visible in the console instead of silently losing the data.
33+
UnityEngine.Debug.LogWarning(
34+
$"[CoreAiWebGlPersistence] IndexedDB flush failed; last write may not survive a reload: {ex.Message}");
35+
}
2636
#endif
2737
}
2838
}
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
using CoreAI.Infrastructure.Logging;
2+
using CoreAI.Infrastructure.World;
3+
using CoreAI.Messaging;
4+
using NUnit.Framework;
5+
using UnityEngine;
6+
7+
namespace CoreAI.Tests.EditMode
8+
{
9+
/// <summary>
10+
/// Regression coverage for audit finding 13: <c>destroy</c> must fail (not silently report
11+
/// success) when the target object does not exist, so the model sees the failure on a typo'd
12+
/// name and can self-correct — matching every other verb's unresolved-target contract.
13+
/// </summary>
14+
public sealed class CoreAiWorldCommandExecutorDestroyEditModeTests
15+
{
16+
[Test]
17+
public void TryExecute_Destroy_MissingTarget_ReturnsFalse()
18+
{
19+
CoreAiWorldCommandExecutor executor = new(GameLoggerUnscopedFallback.Instance);
20+
21+
string json = JsonUtility.ToJson(CoreAiWorldCommandEnvelope.Destroy(
22+
"NoSuchObject_CoreAiAudit2026DestroyTest"));
23+
bool executed = executor.TryExecute(new ApplyAiGameCommand
24+
{
25+
CommandTypeId = AiGameCommandTypeIds.WorldCommand,
26+
JsonPayload = json,
27+
SourceTaskHint = "editmode_destroy_missing"
28+
});
29+
30+
Assert.IsFalse(executed,
31+
"destroy must fail when the target object cannot be resolved in the scene.");
32+
}
33+
}
34+
}

Assets/CoreAiUnity/Tests/EditMode/CoreAiWorldCommandExecutorDestroyEditModeTests.cs.meta

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Assets/CoreAiUnity/Tests/EditMode/GameCreationBenchmarkWindowUitk.cs

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ public sealed class GameCreationBenchmarkWindowUitk : EditorWindow
4141
private ToolbarToggle _compareTab;
4242
private ToolbarToggle _modelsTab;
4343
private Label _statusLabel;
44+
private Button _stopButton;
4445
private VisualElement _progressFill;
4546
private Label _progressText;
4647
private VisualElement _progressLines;
@@ -491,10 +492,21 @@ private VisualElement BuildConnectionSection()
491492
private VisualElement BuildRunButtonSection()
492493
{
493494
VisualElement section = Section(null);
495+
VisualElement row = Row();
494496
Button run = new(RunBenchmark) { text = "Run benchmark" };
495497
run.style.height = 34;
496498
run.style.unityFontStyleAndWeight = FontStyle.Bold;
497-
section.Add(run);
499+
run.style.flexGrow = 1;
500+
row.Add(run);
501+
_stopButton = new Button(GameCreationBenchmarkLauncher.StopRunningBenchmark)
502+
{
503+
text = "Stop (save partial)"
504+
};
505+
_stopButton.style.height = 34;
506+
_stopButton.style.marginLeft = 6;
507+
_stopButton.style.display = DisplayStyle.None;
508+
row.Add(_stopButton);
509+
section.Add(row);
498510
return section;
499511
}
500512

@@ -664,6 +676,13 @@ private void UpdateLiveStatus()
664676
}
665677

666678
bool active = BenchmarkProgress.IsRunning;
679+
if (_stopButton != null)
680+
{
681+
_stopButton.style.display = active ? DisplayStyle.Flex : DisplayStyle.None;
682+
_stopButton.SetEnabled(active && !BenchmarkProgress.StopRequested);
683+
_stopButton.text = BenchmarkProgress.StopRequested ? "Stopping…" : "Stop (save partial)";
684+
}
685+
667686
// A solo scenario (Total <= 1, e.g. running G6's free build alone) has a count-based Fraction
668687
// that sits at 0% for its whole multi-minute run and only jumps to 100% at the very end - not
669688
// useful to a human watching. Fall back to the scenario's own elapsed/remaining wall-clock

0 commit comments

Comments
 (0)