From a7529a975a6c8bae5a4dd4138a8a7fab62754030 Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 28 Jul 2026 18:46:43 +0200 Subject: [PATCH] test(app): serialize the classes sharing camera/render process globals (#252) A full Release App run failed once at Issue181WallPressEquilibriumTests .Diagnostic_WallPressedCamera_EyeWanderAndViewerCellStability. It passed in isolation and did not recur across five further whole-suite runs, and the diff under test touched only the world texture-creation stack -- nothing in camera, visibility or physics. A cross-class parallelism race was the only plausible mechanism, not a regression. Ten App test classes share three process-global mutable statics, and xUnit runs distinct test classes in parallel by default: - CameraDiagnostics: AlignToSlope, CollideCamera, TranslationStiffness, RotationStiffness, UseRetailChaseCamera. These are not merely written, they are written AWAY from their defaults -- RetailChaseCameraTests sets AlignToSlope and CollideCamera to false, and three classes set UseRetailChaseCamera to false -- while RetailChaseCamera.Update, CameraController.Active, CameraFrameController, WorldRenderFrameBuilder and MouseLookController read them. - RenderingDiagnostics.ProbeFlapEnabled, written by CornerFloodReplayTests and Issue181WallPressEquilibriumTests. - System.Console.Out, redirected by those same two classes to capture probe output. Every one of these classes already saved and restored in try/finally. That is correct within a class and remains necessary, but it was never sufficient. A finally bounds a mutation in TIME along its own thread; it cannot stop another class from reading the static inside that window. Worse, two overlapping save/restore pairs can interleave so the second restore writes back the FIRST one's temporary value, leaving the global permanently wrong for the rest of the run. The Console.Out case is the sharpest instance: an interleaved restore can install a DISPOSED StringWriter as the process-wide Console.Out, which then throws in unrelated tests. Serializing the sharers is what makes each class's existing finally sufficient. The fix is a marker CollectionDefinition applied to the ten sharing classes, following the WorldEnvironmentControllerCollection precedent. No collection fixture: several members are [Theory] cases that need different knob values per case, so a fixture cannot own the save/restore without rewriting every member's internals, and it would not help the read side at all. Because every member references the same compile-time const for the collection name, the grouping cannot silently drift via a typo. Membership is deliberately narrow. It covers the eight writers plus two classes that drive production code which READS a knob another member moves off its default (HouseExitWalkReplayTests and CameraFrameControllerTests both run RetailChaseCamera.Update and assert on the resulting eye). Classes that merely construct a CameraController without a retail chase camera are NOT members -- their reads fall through the null branch and are insensitive. No production code changed; no assertion was weakened, and no retry, sleep or tolerance was added. Verification. Base commit f6275f45 measured empirically at 3,763 passed / 3 skipped. Post-fix: 136 whole-suite Release runs. Every failure observed was in the pre-existing zero-allocation family tracked as #250 (an Expected 0 / Actual N bytes assertion), and none was in any collection member. A matched 55-run baseline at f6275f45 reproduced that same family, confirming it predates this change. Serialization cost is inside run-to-run noise: the suite is ~3 s of a ~4.5 s wall-clock dotnet test, and the ten serialized classes are a small fraction of it. Co-Authored-By: Claude Fable 5 --- docs/ISSUES.md | 77 +++++++++++++++++++ .../Input/MouseLookControllerTests.cs | 1 + .../Rendering/CameraControllerTests.cs | 1 + .../Rendering/CameraDiagnosticsCollection.cs | 66 ++++++++++++++++ .../Rendering/CameraFrameControllerTests.cs | 1 + .../Rendering/CornerFloodReplayTests.cs | 1 + .../Rendering/HouseExitWalkReplayTests.cs | 1 + .../Issue177StairDescentCameraFloodTests.cs | 1 + .../Issue181CameraParkStabilityTests.cs | 1 + .../Issue181WallPressEquilibriumTests.cs | 1 + .../Rendering/RetailChaseCameraTests.cs | 1 + .../Rendering/WorldRenderFrameBuilderTests.cs | 1 + 12 files changed, 153 insertions(+) create mode 100644 tests/AcDream.App.Tests/Rendering/CameraDiagnosticsCollection.cs diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 6ab2b246..5aa6b8f5 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -97,6 +97,83 @@ Copy this block when adding a new issue: --- +## #252 — App test classes raced on process-global camera/render statics + +**Status:** DONE — 2026-07-28; serialized via `CameraDiagnosticsCollection` +**Severity:** LOW (test-infrastructure only; no production defect) +**Filed:** 2026-07-28 +**Component:** tests / xUnit parallelism + +**Description:** A full Release `AcDream.App.Tests` run failed once at +`Issue181WallPressEquilibriumTests.Diagnostic_WallPressedCamera_EyeWanderAndViewerCellStability`. +The test passed in isolation and did not recur across five further whole-suite +runs. The diff under test touched only the world texture-creation stack — +nothing in camera, visibility or physics — so a regression was never a +plausible mechanism. + +**Root cause:** Ten App test classes share three *process-global* mutable +statics. xUnit runs distinct test classes in parallel by default, so one +class's mutation window is observable by another class mid-test: + +- `AcDream.Core.Rendering.CameraDiagnostics` — `AlignToSlope`, + `CollideCamera`, `TranslationStiffness`, `RotationStiffness`, + `UseRetailChaseCamera`. `RetailChaseCameraTests` sets `AlignToSlope` and + `CollideCamera` to `false`, and three classes set `UseRetailChaseCamera` to + `false` — all *away from* their defaults. Read by `RetailChaseCamera.Update`, + `CameraController.Active`, `CameraFrameController`, `WorldRenderFrameBuilder` + and `MouseLookController`. +- `AcDream.Core.Rendering.RenderingDiagnostics.ProbeFlapEnabled` — written by + `CornerFloodReplayTests` and `Issue181WallPressEquilibriumTests`. +- `System.Console.Out` — redirected via `Console.SetOut` by those same two + classes to capture probe output. + +Every class saved and restored in `try`/`finally`, which is correct *within* a +class but not sufficient across classes. A `finally` bounds the mutation in +time along its own thread; it cannot stop a concurrent class from reading the +static inside that window. Two overlapping save/restore pairs can also +interleave so the second restore writes back the *first* one's temporary +value, leaving the global wrong for the remainder of the run. The +`Console.Out` case is the sharpest: an interleaved restore can install a +**disposed** `StringWriter` as the process-wide `Console.Out`, which then +throws in unrelated tests. + +**Fix:** `tests/AcDream.App.Tests/Rendering/CameraDiagnosticsCollection.cs` +adds a marker `[CollectionDefinition]` (no fixture — each member still needs +its own per-test values, several as `[Theory]` cases, so a fixture cannot own +the save/restore) applied to the ten sharing classes. Follows the existing +`WorldEnvironmentControllerCollection` precedent. No production code changed +and no assertion was weakened. Membership is deliberately narrow: classes that +merely construct a `CameraController` without a retail chase camera are not +members, because their reads are insensitive. + +**Verification:** Base commit `f6275f45` measured empirically at 3,763 passed / +3 skipped. Post-fix, 136 whole-suite Release runs; every failure observed was +in the pre-existing zero-allocation family (see #250) and none was in any +collection member. A matched 55-run baseline at `f6275f45` reproduced the same +zero-allocation family. Serialization cost is within run-to-run noise — the +suite is ~3 s of a ~4.5 s wall-clock `dotnet test`. + +**Files:** `tests/AcDream.App.Tests/Rendering/CameraDiagnosticsCollection.cs` +(new); `[Collection]` applied to `Issue181WallPressEquilibriumTests`, +`Issue181CameraParkStabilityTests`, `Issue177StairDescentCameraFloodTests`, +`RetailChaseCameraTests`, `CameraControllerTests`, +`WorldRenderFrameBuilderTests`, `CornerFloodReplayTests`, +`HouseExitWalkReplayTests`, `CameraFrameControllerTests`, +`MouseLookControllerTests`. + +**Follow-up (not fixed here):** `CameraDiagnostics` and `RenderingDiagnostics` +are `static` by design (the `PhysicsDiagnostics` runtime-toggle pattern). The +test-side collection is the correct fix for the observed race; making the +camera knobs an injectable instance carried by the camera would remove the +class of defect entirely, but that is production surgery and out of scope. +`AcDream.Core.Tests` has the same shape in a separate assembly (hence a +separate process, so it does not race with App): `CameraDiagnosticsTests` is +its only `CameraDiagnostics` writer, but four classes there call +`Console.SetOut` — `CellarLipWedgeTests`, `CameraCornerSealReplayTests`, +`Issue137CorridorSeamReplayTests`, `RenderingDiagnosticsVisibilityTests`. + +--- + ## #251 — glClientWaitSync returned 0 and crashed the render loop, once in nine connected runs **Status:** OPEN diff --git a/tests/AcDream.App.Tests/Input/MouseLookControllerTests.cs b/tests/AcDream.App.Tests/Input/MouseLookControllerTests.cs index 4d1702c7..6a8ad131 100644 --- a/tests/AcDream.App.Tests/Input/MouseLookControllerTests.cs +++ b/tests/AcDream.App.Tests/Input/MouseLookControllerTests.cs @@ -10,6 +10,7 @@ using Silk.NET.Input; namespace AcDream.App.Tests.Input; +[Collection(AcDream.App.Tests.Rendering.CameraDiagnosticsCollection.Name)] public sealed class MouseLookControllerTests { [Fact] diff --git a/tests/AcDream.App.Tests/Rendering/CameraControllerTests.cs b/tests/AcDream.App.Tests/Rendering/CameraControllerTests.cs index e84b20be..b8810c66 100644 --- a/tests/AcDream.App.Tests/Rendering/CameraControllerTests.cs +++ b/tests/AcDream.App.Tests/Rendering/CameraControllerTests.cs @@ -4,6 +4,7 @@ using Xunit; namespace AcDream.App.Tests.Rendering; +[Collection(CameraDiagnosticsCollection.Name)] public class CameraControllerTests { private static (CameraController ctl, ChaseCamera legacy, RetailChaseCamera retail) MakeChaseFixture() diff --git a/tests/AcDream.App.Tests/Rendering/CameraDiagnosticsCollection.cs b/tests/AcDream.App.Tests/Rendering/CameraDiagnosticsCollection.cs new file mode 100644 index 00000000..9f73f3cf --- /dev/null +++ b/tests/AcDream.App.Tests/Rendering/CameraDiagnosticsCollection.cs @@ -0,0 +1,66 @@ +using Xunit; + +namespace AcDream.App.Tests.Rendering; + +/// +/// Serializes every test class that shares the camera/render family of +/// process-global mutable statics. xUnit runs distinct test classes in +/// PARALLEL by default, and these statics live on the process, not on the test +/// — so two classes running concurrently observe each other's writes. +/// +/// +/// The per-class try/finally save/restore blocks are correct +/// within a class and remain necessary — but they are not sufficient. +/// A finally only bounds the mutation in TIME along its own thread; it +/// cannot stop another class from reading the static inside that window. Worse, +/// two overlapping save/restore pairs can interleave so the second restore +/// writes back the FIRST one's temporary value, leaving the global permanently +/// wrong for the rest of the run. Serializing the sharers is what actually makes +/// each class's finally sufficient. +/// +/// +/// Three distinct globals are covered, and every one of them is a shared +/// edge between at least two classes in this collection: +/// +/// AcDream.Core.Rendering.CameraDiagnostics — +/// AlignToSlope, CollideCamera, TranslationStiffness, +/// RotationStiffness, UseRetailChaseCamera. Read by +/// RetailChaseCamera.Update, CameraController.Active, +/// CameraFrameController, WorldRenderFrameBuilder and +/// MouseLookController. +/// AcDream.Core.Rendering.RenderingDiagnostics.ProbeFlapEnabled +/// — written by CornerFloodReplayTests and +/// Issue181WallPressEquilibriumTests. +/// System.Console.Out — redirected via +/// Console.SetOut by those same two classes to capture probe output. +/// Interleaved redirection can restore a DISPOSED StringWriter as the +/// process-wide Console.Out, which then throws in unrelated +/// tests. +/// +/// +/// +/// Membership is deliberately narrow: only classes that write one of these +/// globals, or that drive production code which READS a knob another member +/// mutates away from its default (RetailChaseCameraTests sets +/// AlignToSlope and CollideCamera to ; +/// the UseRetailChaseCamera writers set it to ). +/// Classes that merely construct a CameraController without a retail +/// chase camera are NOT members — their reads are insensitive. +/// +/// +/// +/// Marker only, no collection fixture: each member still needs its own +/// per-test values (several are [Theory] cases that set different knobs +/// per case), so a fixture cannot own the save/restore without rewriting every +/// member's internals. The existing per-test finally blocks already +/// return the defaults; this attribute supplies the mutual exclusion they were +/// missing. Follows the WorldEnvironmentControllerCollection precedent. +/// +/// +/// Evidence + history: docs/ISSUES.md #251. +/// +[CollectionDefinition(Name)] +public sealed class CameraDiagnosticsCollection +{ + public const string Name = "Camera diagnostics globals"; +} diff --git a/tests/AcDream.App.Tests/Rendering/CameraFrameControllerTests.cs b/tests/AcDream.App.Tests/Rendering/CameraFrameControllerTests.cs index 732906b3..3cc6580b 100644 --- a/tests/AcDream.App.Tests/Rendering/CameraFrameControllerTests.cs +++ b/tests/AcDream.App.Tests/Rendering/CameraFrameControllerTests.cs @@ -7,6 +7,7 @@ using AcDream.Core.Physics; namespace AcDream.App.Tests.Rendering; +[Collection(CameraDiagnosticsCollection.Name)] public sealed class CameraFrameControllerTests { [Fact] diff --git a/tests/AcDream.App.Tests/Rendering/CornerFloodReplayTests.cs b/tests/AcDream.App.Tests/Rendering/CornerFloodReplayTests.cs index 3d5a8213..351b356b 100644 --- a/tests/AcDream.App.Tests/Rendering/CornerFloodReplayTests.cs +++ b/tests/AcDream.App.Tests/Rendering/CornerFloodReplayTests.cs @@ -30,6 +30,7 @@ namespace AcDream.App.Tests.Rendering; /// corner press: (159.94, 7.70, 94.00) in 0172; captured eyes hover near (157.4..157.5, /// 7.91, 96.25) — inside 0171, 0.35 m past the doorway plane, near the ceiling. /// +[Collection(CameraDiagnosticsCollection.Name)] public class CornerFloodReplayTests { private readonly ITestOutputHelper _out; diff --git a/tests/AcDream.App.Tests/Rendering/HouseExitWalkReplayTests.cs b/tests/AcDream.App.Tests/Rendering/HouseExitWalkReplayTests.cs index e0c93dd8..ab1f1604 100644 --- a/tests/AcDream.App.Tests/Rendering/HouseExitWalkReplayTests.cs +++ b/tests/AcDream.App.Tests/Rendering/HouseExitWalkReplayTests.cs @@ -48,6 +48,7 @@ namespace AcDream.App.Tests.Rendering; /// exactly as GameWindow builds it (full-screen /// OutsideView ⇒ outdoor dynamics trivially cone-pass). /// +[Collection(CameraDiagnosticsCollection.Name)] public class HouseExitWalkReplayTests { private readonly ITestOutputHelper _out; diff --git a/tests/AcDream.App.Tests/Rendering/Issue177StairDescentCameraFloodTests.cs b/tests/AcDream.App.Tests/Rendering/Issue177StairDescentCameraFloodTests.cs index 2652301b..5cf4e472 100644 --- a/tests/AcDream.App.Tests/Rendering/Issue177StairDescentCameraFloodTests.cs +++ b/tests/AcDream.App.Tests/Rendering/Issue177StairDescentCameraFloodTests.cs @@ -31,6 +31,7 @@ namespace AcDream.App.Tests.Rendering; /// and the printed per-portal side-test D names the failing gate. If they stay /// admitted, the flood is exonerated and the artifact lives downstream. /// +[Collection(CameraDiagnosticsCollection.Name)] public class Issue177StairDescentCameraFloodTests { private const uint FacilityHub = 0x8A020000u; diff --git a/tests/AcDream.App.Tests/Rendering/Issue181CameraParkStabilityTests.cs b/tests/AcDream.App.Tests/Rendering/Issue181CameraParkStabilityTests.cs index 3e2950db..14172fdb 100644 --- a/tests/AcDream.App.Tests/Rendering/Issue181CameraParkStabilityTests.cs +++ b/tests/AcDream.App.Tests/Rendering/Issue181CameraParkStabilityTests.cs @@ -19,6 +19,7 @@ namespace AcDream.App.Tests.Rendering; /// from its INPUTS (player position/yaw jitter out of GameWindow); if it /// wobbles here, the camera loop itself fails to reach the fixed point. /// +[Collection(CameraDiagnosticsCollection.Name)] public class Issue181CameraParkStabilityTests { private readonly ITestOutputHelper _out; diff --git a/tests/AcDream.App.Tests/Rendering/Issue181WallPressEquilibriumTests.cs b/tests/AcDream.App.Tests/Rendering/Issue181WallPressEquilibriumTests.cs index bd57825b..0234a96c 100644 --- a/tests/AcDream.App.Tests/Rendering/Issue181WallPressEquilibriumTests.cs +++ b/tests/AcDream.App.Tests/Rendering/Issue181WallPressEquilibriumTests.cs @@ -31,6 +31,7 @@ namespace AcDream.App.Tests.Rendering; /// Diagnostic (reporting) first; the equilibrium fix turns the wander/flap /// numbers into hard pins. /// +[Collection(CameraDiagnosticsCollection.Name)] public class Issue181WallPressEquilibriumTests { private const uint FacilityHubLandblock = 0x8A020000u; diff --git a/tests/AcDream.App.Tests/Rendering/RetailChaseCameraTests.cs b/tests/AcDream.App.Tests/Rendering/RetailChaseCameraTests.cs index 32675ee3..653a5182 100644 --- a/tests/AcDream.App.Tests/Rendering/RetailChaseCameraTests.cs +++ b/tests/AcDream.App.Tests/Rendering/RetailChaseCameraTests.cs @@ -6,6 +6,7 @@ using Xunit; namespace AcDream.App.Tests.Rendering; +[Collection(CameraDiagnosticsCollection.Name)] public class RetailChaseCameraTests { [Fact] diff --git a/tests/AcDream.App.Tests/Rendering/WorldRenderFrameBuilderTests.cs b/tests/AcDream.App.Tests/Rendering/WorldRenderFrameBuilderTests.cs index 6bf373bf..a2812357 100644 --- a/tests/AcDream.App.Tests/Rendering/WorldRenderFrameBuilderTests.cs +++ b/tests/AcDream.App.Tests/Rendering/WorldRenderFrameBuilderTests.cs @@ -11,6 +11,7 @@ using AcDream.Core.World.Cells; namespace AcDream.App.Tests.Rendering; +[Collection(CameraDiagnosticsCollection.Name)] public sealed class WorldRenderFrameBuilderTests { [Fact]