fix(app): #343 — a wounded render loop defers the native release instead of throwing over the real failure
Some checks are pending
Headless portability / portable-headless (ubuntu-latest) (push) Waiting to run
Headless portability / portable-headless (windows-latest) (push) Waiting to run
Headless portability / linux-graphical (push) Waiting to run
Headless portability / linux-vulkan (push) Waiting to run
Some checks are pending
Headless portability / portable-headless (ubuntu-latest) (push) Waiting to run
Headless portability / portable-headless (windows-latest) (push) Waiting to run
Headless portability / linux-graphical (push) Waiting to run
Headless portability / linux-vulkan (push) Waiting to run
Root cause pinned by IL-decompiling Silk.NET.Windowing.Common: ViewImplementationBase._inRenderLoop is set at DoRender/DoUpdate entry and cleared ONLY on normal return, so a throwing frame callback leaves it armed forever and any later Dispose -> Reset throws "You cannot call Reset inside of the render loop", exit 82, replacing the original wounding exception in the report. The fix mirrors Silk's own bracket exactly: GameWindow._renderLoopArmed set at OnUpdate/OnRender entry, cleared only on their normal return — deliberately NOT in a finally, so it tracks the wound the same way Silk's private field does. ReleaseNativeWindow checks it before disposing: armed -> best-effort Close() (swallowed so it can never become the reported failure), no Dispose, and a new terminal status CompleteWithDeferredNativeRelease with Error kept null — the original exception stays the primary report. Healthy paths (OnClosing's in-loop completion, Run()'s tail release) are byte-unchanged, and the new PublishNativeWindow parameters default to null so every existing caller and test behaves identically. Sabotage: disabling the armed-check flipped the deferral test to Expected CompleteWithDeferredNativeRelease / Actual Complete — the guard is what the test exercises. Clean-room suite 11,262 / 6 / 1, the 1 being #340's documented load flake (passed standalone; second recorded firing noted in its entry). Queue: #344 done, #343 done; next #345's instrumented mechanism session, then #341's boundary hunt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
52bdf4df71
commit
6c6664a685
4 changed files with 194 additions and 10 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<int> 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
|
||||
|
|
|
|||
|
|
@ -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<bool>? _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<bool>? 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);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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<string>();
|
||||
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<string>();
|
||||
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()
|
||||
{
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue