diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 5f2dd459..8c35471c 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -257,7 +257,19 @@ mid-flight-teleport-only. Next-session queue: #344, #343, then S4b/S6. ## #343 — Shutdown after a wounded render loop: Silk Reset called inside the render loop, exit 82 -**Status:** OPEN. LOW-MEDIUM — only reachable after a render-frame exception +**Status:** FIXED 2026-08-08. Root cause pinned by IL-decompiling Silk: +`ViewImplementationBase._inRenderLoop` is cleared only on a frame callback's +NORMAL return, so a throwing callback leaves it armed forever and the +disposal's `Reset()` throws. The fix mirrors that bracket exactly +(`GameWindow._renderLoopArmed`, deliberately not cleared in a finally), +and `ReleaseNativeWindow` defers when armed: best-effort `Close()`, no +`Dispose()`, new terminal status `CompleteWithDeferredNativeRelease` with +`Error` kept null so the ORIGINAL wounding exception remains the primary +report. Sabotage-verified; healthy paths byte-unchanged. Clean-room suite +11,262 passed / 6 skipped / 1 failed = #340's documented flake, which +passed standalone (its second recorded firing). + +**Original status:** OPEN. LOW-MEDIUM — only reachable after a render-frame exception (#339's crash was the trigger), but it turns a diagnosable failure into an `AbandonedIncomplete` shutdown. `GameWindowLifetime.ReleaseNativeWindow:294` calls `ViewImplementationBase.Dispose` → `Reset` while the render loop is diff --git a/src/AcDream.App/Rendering/GameWindow.cs b/src/AcDream.App/Rendering/GameWindow.cs index b3b472b9..5192d72e 100644 --- a/src/AcDream.App/Rendering/GameWindow.cs +++ b/src/AcDream.App/Rendering/GameWindow.cs @@ -42,6 +42,18 @@ public sealed class GameWindow : private readonly WorldEvents _worldEvents; private readonly HostQuiescenceGate _hostQuiescence = new(); private IWindow? _window; + // #343: mirrors Silk's own ViewImplementationBase._inRenderLoop guard, + // which we cannot read directly (it is a private field on Silk's + // internal type). Set true before OnUpdate/OnRender run and cleared + // only on their normal (non-throwing) return — exactly like Silk's own + // bracket around DoUpdate/DoRender — so a frame callback that throws + // leaves this stuck true, same as Silk's real guard. GameWindowLifetime + // reads it before disposing the native window, so a wounded loop defers + // the release instead of calling Reset() while Silk still thinks it is + // mid-frame (which throws "You cannot call `Reset` inside of the render + // loop!" and would otherwise bury whatever exception actually wounded + // the loop). See docs/ISSUES.md #343. + private bool _renderLoopArmed; private SilkWindowCallbackBinding? _windowCallbacks; private GameWindowGraphics? _graphics; // Campaign V slice V6h: borrowed, not owned — _graphics owns the context and @@ -722,7 +734,11 @@ public sealed class GameWindow : _startupQuality = startup.Quality; _window = Window.Create(options); - _lifetime.PublishNativeWindow(_window); + IWindow window = _window; + _lifetime.PublishNativeWindow( + window, + isRenderLoopArmed: () => _renderLoopArmed, + requestClose: window.Close); _displayFramePacing.BindSurface( new SilkDisplayFramePacingSurface(_window)); // The fixed binding preserves main Render before post-render pacing, @@ -1505,15 +1521,24 @@ public sealed class GameWindow : private void OnUpdate(double dt) { + // #343: armed before the callback body runs, cleared only on normal + // return. Deliberately NOT a try/finally — if Tick throws, the flag + // must stay true (mirroring Silk's own stuck _inRenderLoop guard), + // not get cleared on the way out. + _renderLoopArmed = true; using var _updStage = _frameProfiler.BeginStage( AcDream.App.Diagnostics.FrameStage.Update); _frameGraphs.Tick(new AcDream.App.Update.UpdateFrameInput(dt)); + _renderLoopArmed = false; } // Performance overlay state — updated every ~0.5s and written to the // window title so there's zero rendering cost (no font/overlay needed). private void OnRender(double deltaSeconds) { + // #343: see OnUpdate above — armed on entry, cleared on every normal + // exit path below, left stuck true if anything here throws. + _renderLoopArmed = true; Vector2D size = _window!.Size; // Campaign V slice V6h: swapchain currency is the one piece of // presentation the RHI contract deliberately leaves to the host (plan @@ -1521,7 +1546,10 @@ public sealed class GameWindow : // out-of-date one is recreated at a frame boundary, the only safe point. // The handoff below stays exactly one call on both backends. if (_vulkanGraphics is { } vulkan && !vulkan.PrepareFrame()) + { + _renderLoopArmed = false; return; + } try { _frameGraphs.Render( @@ -1535,10 +1563,12 @@ public sealed class GameWindow : when (_vulkanGraphics is not null) { _vulkanGraphics.RequestRecreate(); + _renderLoopArmed = false; return; } _vulkanGraphics?.NoteFrameClosed(); + _renderLoopArmed = false; } // IsEntityCurrentlyMoving REMOVED (2026-07-09): it powered a cache-bypass diff --git a/src/AcDream.App/Rendering/GameWindowLifetime.cs b/src/AcDream.App/Rendering/GameWindowLifetime.cs index a21a3670..7d0dba7f 100644 --- a/src/AcDream.App/Rendering/GameWindowLifetime.cs +++ b/src/AcDream.App/Rendering/GameWindowLifetime.cs @@ -34,6 +34,14 @@ internal enum GameWindowLifetimeStatus RetryableIncomplete, Complete, CompleteWithCleanupFailures, + // #343: the resource-shutdown transaction converged, but the native + // window's release was deferred because Silk's per-frame render-loop + // guard was still armed (a render/update callback threw without + // reaching its own reset line). Disposing here would trade whatever + // wounded the loop for Silk's unrelated "Reset inside of the render + // loop" exception. Distinct from AbandonedIncomplete: nothing failed, + // the native release just did not happen yet. + CompleteWithDeferredNativeRelease, AbandonedIncomplete, } @@ -46,6 +54,7 @@ internal sealed record GameWindowLifetimeReport( public bool IsTerminal => Status is GameWindowLifetimeStatus.Complete or GameWindowLifetimeStatus.CompleteWithCleanupFailures + or GameWindowLifetimeStatus.CompleteWithDeferredNativeRelease or GameWindowLifetimeStatus.AbandonedIncomplete; } @@ -145,6 +154,12 @@ internal sealed class GameWindowLifetime private GameWindowShutdownRoots? _roots; private ResourceShutdownTransaction? _transaction; private IDisposable? _nativeWindow; + // #343: caller-supplied signal for "is Silk's render loop still armed" + // and a best-effort close request, both optional so every existing + // caller/test that publishes a bare IDisposable keeps disposing exactly + // as before. Null means "assume not armed" (unchanged prior behavior). + private Func? _isNativeRenderLoopArmed; + private Action? _requestNativeClose; private bool _shutdownRootsPublished; private bool _nativeReleaseAttempted; private bool _completing; @@ -169,12 +184,17 @@ internal sealed class GameWindowLifetime _shutdownRootsPublished || _injectedTransactionFactory is not null; internal bool RetainsShutdownGraph => _roots is not null || _transaction is not null; - public void PublishNativeWindow(IDisposable nativeWindow) + public void PublishNativeWindow( + IDisposable nativeWindow, + Func? isRenderLoopArmed = null, + Action? requestClose = null) { ArgumentNullException.ThrowIfNull(nativeWindow); if (_nativeReleaseAttempted || _nativeWindow is not null) throw new InvalidOperationException("A native window is already lifetime-owned."); _nativeWindow = nativeWindow; + _isNativeRenderLoopArmed = isRenderLoopArmed; + _requestNativeClose = requestClose; } public void PublishShutdownRoots(GameWindowShutdownRoots roots) @@ -237,7 +257,12 @@ internal sealed class GameWindowLifetime if (_report.Status == GameWindowLifetimeStatus.RetryableIncomplete) { AbandonRetainedRootsAfterTerminalFailure(); - Exception? nativeFailure = ReleaseNativeWindow(); + (_, Exception? nativeFailure) = ReleaseNativeWindow(); + // A deferred native release (#343) is not a failure — it is a + // decision not to dispose while Silk's render-loop guard is + // still armed. Never synthesize a native failure that didn't + // happen; the transaction's own error stays the sole reported + // cause, exactly as if the native step had been skipped. Exception terminalError = nativeFailure is null ? _report.Error ?? new InvalidOperationException( "Shutdown did not converge before native fallback.") @@ -256,8 +281,16 @@ internal sealed class GameWindowLifetime if (_report.IsTerminal && !_nativeReleaseAttempted) { - Exception? nativeFailure = ReleaseNativeWindow(); - if (nativeFailure is not null) + (NativeReleaseOutcome outcome, Exception? nativeFailure) = ReleaseNativeWindow(); + if (outcome == NativeReleaseOutcome.Deferred) + { + _report = new GameWindowLifetimeReport( + GameWindowLifetimeStatus.CompleteWithDeferredNativeRelease, + "native window", + _report.CleanupFailures, + null); + } + else if (nativeFailure is not null) { _report = new GameWindowLifetimeReport( GameWindowLifetimeStatus.AbandonedIncomplete, @@ -284,20 +317,54 @@ internal sealed class GameWindowLifetime "Shutdown roots must publish before completion starts.")); } - private Exception? ReleaseNativeWindow() + private enum NativeReleaseOutcome + { + Released, + Deferred, + Failed, + } + + private (NativeReleaseOutcome Outcome, Exception? Error) ReleaseNativeWindow() { if (_nativeReleaseAttempted) - return null; + return (NativeReleaseOutcome.Released, null); + + if (_isNativeRenderLoopArmed?.Invoke() == true) + { + // #343: Silk's ViewImplementationBase.Reset() (called from its + // Dispose()) throws "You cannot call `Reset` inside of the + // render loop!" when its internal render-loop guard is still + // set — which happens whenever a render/update frame callback + // threw without reaching its own reset line. Calling Dispose() + // here would trade the exception that actually wounded the loop + // for that unrelated Silk guard exception. Request a close as a + // courtesy and defer the physical release instead of disposing + // into that throw; this is a one-shot decision for this native + // window, matching every other release outcome below. + try + { + _requestNativeClose?.Invoke(); + } + catch + { + // Best-effort only — the deferred outcome does not depend on + // this succeeding, and the native window's own exception + // must never become the reported failure here. + } + _nativeReleaseAttempted = true; + return (NativeReleaseOutcome.Deferred, null); + } + _nativeReleaseAttempted = true; try { _nativeWindow?.Dispose(); _nativeWindow = null; - return null; + return (NativeReleaseOutcome.Released, null); } catch (Exception error) { - return error; + return (NativeReleaseOutcome.Failed, error); } } diff --git a/tests/AcDream.App.Tests/Rendering/GameWindowLifetimeTests.cs b/tests/AcDream.App.Tests/Rendering/GameWindowLifetimeTests.cs index a6e04231..46f35e1f 100644 --- a/tests/AcDream.App.Tests/Rendering/GameWindowLifetimeTests.cs +++ b/tests/AcDream.App.Tests/Rendering/GameWindowLifetimeTests.cs @@ -32,6 +32,81 @@ public sealed class GameWindowLifetimeTests Assert.False(lifetime.RetainsShutdownGraph); } + [Fact] + public void ArmedRenderLoopDefersNativeReleaseInsteadOfDisposing() + { + // #343: a render/update frame callback that threw leaves Silk's + // internal render-loop guard stuck armed. Disposing the native + // window into that state throws "You cannot call `Reset` inside of + // the render loop!" and buries whatever exception actually wounded + // the loop. GameWindowLifetime must detect the still-armed signal, + // request a close instead of disposing, and record a distinct + // deferred status rather than AbandonedIncomplete-with-throw. + var calls = new List(); + var transaction = new ResourceShutdownTransaction( + new ResourceShutdownStage("owners", + [ + new("owner", () => calls.Add("owner")), + ])); + var lifetime = new GameWindowLifetime(() => transaction); + var native = new RecordingDisposable("native", calls); + bool closeRequested = false; + lifetime.PublishNativeWindow( + native, + isRenderLoopArmed: () => true, + requestClose: () => closeRequested = true); + + GameWindowLifetimeReport report = lifetime.CompleteAndReleaseNativeWindow(); + + Assert.Equal(GameWindowLifetimeStatus.CompleteWithDeferredNativeRelease, report.Status); + Assert.Equal("native window", report.BlockedStage); + Assert.Null(report.Error); + Assert.Empty(report.CleanupFailures); + Assert.Equal(["owner"], calls); + Assert.Equal(0, native.DisposeCalls); + Assert.True(closeRequested); + Assert.True(lifetime.RetainsShutdownGraph); + Assert.True(report.IsTerminal); + + // Idempotent: the armed predicate never flips back false in this + // test double, so a repeated call must keep returning the same + // deferred report without a second Dispose attempt. + GameWindowLifetimeReport repeated = lifetime.CompleteAndReleaseNativeWindow(); + Assert.Same(report, repeated); + Assert.Equal(0, native.DisposeCalls); + } + + [Fact] + public void UnarmedRenderLoopStillDisposesNativeWindowNormally() + { + // The healthy post-loop path (Run()'s tail, GameWindow.cs) always + // completes with the loop unarmed. This pins that the new optional + // isRenderLoopArmed/requestClose parameters do not change that path: + // Dispose still runs exactly once, in the same order as before. + var calls = new List(); + var transaction = new ResourceShutdownTransaction( + new ResourceShutdownStage("owners", + [ + new("owner", () => calls.Add("owner")), + ])); + var lifetime = new GameWindowLifetime(() => transaction); + var native = new RecordingDisposable("native", calls); + bool closeRequested = false; + lifetime.PublishNativeWindow( + native, + isRenderLoopArmed: () => false, + requestClose: () => closeRequested = true); + + GameWindowLifetimeReport report = lifetime.CompleteAndReleaseNativeWindow(); + + Assert.Equal(GameWindowLifetimeStatus.Complete, report.Status); + Assert.Null(report.BlockedStage); + Assert.Equal(["owner", "native"], calls); + Assert.Equal(1, native.DisposeCalls); + Assert.False(closeRequested); + Assert.False(lifetime.RetainsShutdownGraph); + } + [Fact] public void HardBarrierFailureRetriesWithoutReplayThenCompletes() {