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 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-07-28 18:46:43 +02:00
parent 3b62b9bfa3
commit a7529a975a
12 changed files with 153 additions and 0 deletions

View file

@ -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