Second hotfix attempt for the "ball of spikes" terrain corruption.
The previous _datLock fix was insufficient because dat reads happen
from many render-thread code paths I didn't enumerate (animation
tick, OnLiveMotionUpdated, OnLivePositionUpdated, the live spawn
hydration, ApplyLoadedTerrain) and locking each is invasive and
fragile.
DatReaderWriter's DatCollection is fundamentally not thread-safe:
DatBinReader's internal buffer position is shared per-database, so
two concurrent .Get<T> calls corrupt each other's read state. The
ArgumentOutOfRangeException at DatBinReader.ReadBytesInternal in
the failure log is the smoking gun — one read started reading a
LandBlock, another moved the reader's position, the first one
asked for the wrong number of bytes.
Until Phase A.3 introduces a thread-safe dat wrapper (or until we
preload all dats into pure in-memory dictionaries), the streamer
runs synchronously: EnqueueLoad invokes the load delegate inline
on the calling thread and writes the result to the outbox in a
single call. The render-thread DrainCompletions loop picks it up
on the same frame.
API surface unchanged — Channel-based outbox, EnqueueLoad/Unload,
DrainCompletions, Start (now no-op), Dispose all preserved. Move
back to async loading is a single-class change once dat thread
safety lands.
Cost: visible frame hitch when crossing landblock boundaries
(rendering the new landblock is now on the render thread). For
default 5×5 the hitch is one landblock per cardinal step, ~50ms
worst case. Acceptable for the MVP — correctness over hitches.
Updated the off-thread test to assert the new synchronous contract
(loader runs on the calling thread). The other 4 tests still pass
unchanged because their spin-drain pattern works with synchronous
delivery too.
The previous _datLock from commit c991fb2 stays in place as
defensive belt-and-suspenders — it's free in synchronous mode and
keeps the contract documented at every dat-reading entry point.
212 tests green.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code review follow-up to commit 0904372. Five Important fixes plus
three Minor polish items found by the reviewer before StreamingController
depends on this class under churn.
I1: Dispose is now thread-safe via Interlocked.Exchange on an int
guard. Two concurrent Dispose calls no longer double-dispose the
CancellationTokenSource.
I2: EnqueueLoad/EnqueueUnload now throw ObjectDisposedException when
called after Dispose instead of silently dropping the job. Jobs
vanishing into a completed channel was a debugging hazard.
I3: Start throws ObjectDisposedException when called after Dispose
instead of silently doing nothing (the old guard only checked
whether the thread was non-null, not whether the streamer was
still usable).
I4: New test Load_ExecutesLoaderOnBackgroundThread captures the
loader delegate's ManagedThreadId and asserts it differs from
the test thread's id, proving the whole reason this class
exists (off-thread execution) is actually happening.
I5: New LandblockStreamResult.WorkerCrashed record type for the
outer catch in WorkerLoop. Previously the crash path wrote
Failed(0, ex.ToString()) which collided with landblock (0, 0)
in the north ocean, making "worker crashed" indistinguishable
from "landblock 0 failed to load".
Minor polish:
- M1: Test spin constants (SpinTimeoutMs, SpinStepMs,
SpinMaxIterations) extracted so the 200 x 10ms pattern has one
source of truth.
- M2: DefaultDrainBatchSize public const on LandblockStreamer so
the batch cap has a name and a comment explaining why 4.
- M3: Safety-argument comment on the sync-over-async
WaitToReadAsync call explaining why it cannot deadlock (dedicated
thread, no SyncContext).
- M6: XML remarks on the class and on DrainCompletions documenting
threading contract (Enqueue = any thread, Drain = single consumer
thread).
112 Core + 96 Core.Net tests green.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Background thread pulls load/unload jobs from an inbox channel, invokes
a caller-supplied Func<uint, LoadedLandblock?> (production wraps
LandblockLoader.Load, tests inject a fake), and posts results to an
outbox channel the render thread drains. Graceful shutdown via
CancellationToken; failed loads reported rather than retried.
4 new tests, all green.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
