feat(physics): C4 route 4b-2 — remote far snap through the canonical placement
Flips the SetPositionSimple classification (contact, PlayerDistance >= 96 m) for remotes onto 4b-1's drive controller and deletes both legacy far blocks, both duplicated 96f/4f constant pairs, and both `?? Vector3.Zero` fabrications. The 4 m constant now exists exactly once. Teleport and cell-less stay legacy for 4b-3. Retail: MoveOrTeleport @0x00516330's far branch runs StopInterpolating @0x005163CB before SetPositionSimple @0x005163D9 and returns 1 @0x005163E8 regardless — the SetPositionError is discarded — so HandleReceivedPosition arms ConstrainTo @0x00454272 post-move on commit AND on failure. The x87 parity decode at @0x00516393-@0x0051639E puts exactly 96.0 on the far branch. SetPositionSimple @0x005162B0 builds flags 0x1012 at @0x005162C4. Non-commit outcomes still advance the body, because retail's SetPositionInternal @0x00515BD0 commits the destination via store_position @0x00515CE2 when no cell resolves. The partition is by STAGE, not heuristic, enforced by an exhaustive switch: Refused/Contention/NotApplicable/RejectedPreparation store (the placement never executed); Committed/Deferred/RejectedByPlacement do not (the engine ran and refused, matching retail's non-storing returns @0x00515CB2 and @0x00515CD5). Without this a refused far snap froze the remote with an emptied queue. Also fixes a shipped defect this route made live: ParkDeferred's quiescence parks withdrew the entity (InWorld=false, clock suspended, residency removed) and were never restorable, while Forget(restoreCancelledPark: true) runs for every accepted Position on every entity. The restorable decision now lives inside ParkDeferred AFTER SnapToCell, reading body.CellPosition.ObjCellId — the value RestoreParkWithdrawal actually restores at — against every live quiescence rather than one minimum-OperationId token. The three pre-snap fields are hoisted into locals because SnapToCell ends with InWorld = true. ParkCollisionResidents passes restorableOnCancel: false explicitly; the plain unplaceable park is provably unchanged. RestoreParkWithdrawal re-tests the prefix at restore time so a retained route-2 park cannot re-admit into a prefix that began quiescing during the park. CanAttemptDestination is retained as an OPTIMISATION only, with the two Core predicates it cannot reproduce written down at the pre-flight, plus the two properties that depend on it staying there. Four fix rounds and eight Opus reviews. The slice was fully green at 10,990, 10,997 and 11,004 while containing real defects — a frozen remote pinned as correct by its own test, a fallback that over-wrote on the exact retail paths that decline to store, and a park guard incomplete on two independent axes. Register: AP-137 (leftover classifications take AP-87's catch-up; states the cell-less enqueue-vs-place delta deferred to 4b-3, that RejectedData is applied anyway, and the headless divergence), AP-138 (the refusable far placement), AP-136 narrowed to match the relocation. #309's acceptance steps rewritten — step 5 previously asserted a recovery the code does not perform — and gated on a new ACDREAM_PROBE_PARK=1 signal so the check cannot pass while broken. Suite 11,009 passed / 4 skipped / 0 failed against a measured 10,968 baseline. The 10,973 figure recorded earlier was wrong and is corrected here. Connected gate outstanding: the two-client far-snap walk and #309. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
1b631f127d
commit
7f1c1f5aa6
24 changed files with 5234 additions and 375 deletions
|
|
@ -84,9 +84,28 @@ route 4b-1 contract's "no connected gate" line does not apply to the park fix.
|
|||
core behind EVERY production placement, and the merge-time
|
||||
`restoreCancelledPark: true` is on the accepted-Position path every remote and
|
||||
the local player traverse. So shipped behaviour changes for any entity whose
|
||||
placement parks.
|
||||
placement parks — including the local player, whose route-2 ForcePosition
|
||||
corrections now roll a cancelled quiescence park back instead of leaving the
|
||||
character withdrawn for the rest of the session.
|
||||
|
||||
Proposed connected check (two clients, local ACE). Scope widened 2026-08-04 at
|
||||
C4 route 4b-2 round 3: the slice made `SubmitPreparedPlacementCore`'s two
|
||||
collision-prefix QUIESCENCE parks restorable as well, which the original steps
|
||||
(the plain unplaceable-destination park only) never exercised, and made the
|
||||
LOCAL PLAYER traverse the same restore rather than only remotes.
|
||||
|
||||
**Run the whole gate with `ACDREAM_PROBE_PARK=1`** (added 2026-08-04, round 4).
|
||||
Steps 4 and 5 both need a ForcePosition to land INSIDE a transient
|
||||
collision-prefix quiescence window, which the tester cannot synchronise with a
|
||||
teleport or portal arrival — so without a signal a clean teleport and a
|
||||
correctly-parked one look identical and those steps pass while broken. The flag
|
||||
emits one `[park]` line per park (guid, cause, the caller's pre-snap
|
||||
`resultCell`, the POST-snap `restoreCell` the rollback would use, `eligible` =
|
||||
the caller's half, `captured` = the final decision) and one `[park-restore]`
|
||||
line per rollback (`residency` = whether canonical residency was re-taken).
|
||||
Nothing is emitted for an ordinary committing placement, so an empty log means
|
||||
the window was never entered — retry the step; do not record a pass.
|
||||
|
||||
Proposed connected check (two clients, local ACE):
|
||||
1. Walk the observed character to a landblock boundary so a remote sits in a
|
||||
landblock the observer has not streamed, forcing a `DeferredCell` park.
|
||||
2. Confirm the remote no longer vanishes permanently — the pre-fix symptom was
|
||||
|
|
@ -94,9 +113,45 @@ Proposed connected check (two clients, local ACE):
|
|||
3. Confirm it appears at the SERVER-authoritative destination pose, not at a
|
||||
stale pre-park pose, and that it becomes collidable once the landblock
|
||||
publishes.
|
||||
4. Confirm the local player's own ForcePosition corrections (route 2) still
|
||||
land unchanged — that path shares the same cancel.
|
||||
Steps 1-3 are the user-visible acceptance for AP-136's residual.
|
||||
4. **Quiescing swept NEIGHBOUR (new).** Stand/run within about a metre of a
|
||||
landblock seam while the neighbouring landblock across that seam is being
|
||||
retired or republished by streaming (recentre by travelling, then provoke a
|
||||
server ForcePosition — a `/teleport`-class correction or a portal arrival —
|
||||
at the seam). The sweep footprint reaches the quiescing neighbour, so the
|
||||
placement parks even though neither the source nor the destination is
|
||||
quiescing. Confirm the LOCAL PLAYER is not left frozen/invisible after the
|
||||
next server Position: it must stay in the world, keep simulating, and stay
|
||||
collidable. **Performed only when the log shows BOTH** `[park] …
|
||||
cause=quiescence:0x<neighbour-prefix> … eligible=True captured=True` **and a
|
||||
later** `[park-restore] … residency=True` for the same guid. This is the
|
||||
shape round 3 measured as reachable; the "quiescing source landblock" shape
|
||||
is NOT reachable through either accepted-Position caller on a first submit,
|
||||
because both commit the accepted wire cell to `record.FullCellId` before
|
||||
submitting.
|
||||
5. **Quiescing DESTINATION (new).** Provoke a ForcePosition into a landblock
|
||||
that is mid-retirement. The park is deliberately NOT restored here (AP-136's
|
||||
reason applies exactly): the character is left withdrawn — out of world,
|
||||
object clock suspended, not a spatial root.
|
||||
**Corrected 2026-08-04 (round 4).** The earlier text asked the tester to
|
||||
confirm the player "recovers on the next server Position rather than staying
|
||||
withdrawn indefinitely". It does not recover on that packet, and
|
||||
`QuiescingDestinationPrefix_ForcePositionParkIsNotRestored` pins the
|
||||
opposite: route 2 dispatches only for `ForcePosition`, so the ordinary
|
||||
`Apply` that follows merges behind the drive and cancels the park WITHOUT
|
||||
restoring it. Recovery needs a later packet that actually runs a placement
|
||||
and commits — another accepted ForcePosition (correction, teleport, or
|
||||
portal arrival) once that landblock's quiescence has released. Confirm
|
||||
exactly that, and confirm streaming's retirement of that landblock still
|
||||
COMPLETES rather than stalling; the retirement is the thing the
|
||||
non-restorable park exists to protect. **Performed only when the log shows**
|
||||
`[park] … cause=quiescence:0x<destination-prefix> … eligible=True
|
||||
captured=False` (eligible-but-declined is the decision under test) **and no**
|
||||
`[park-restore]` **line for that guid until the recovering ForcePosition.**
|
||||
6. Confirm the local player's own ordinary ForcePosition corrections (route 2)
|
||||
still land unchanged — that path shares the same cancel — including the
|
||||
case where the correction lands in a landblock that is NOT quiescing, which
|
||||
must be indistinguishable from pre-slice behaviour.
|
||||
Steps 1-5 are the user-visible acceptance for AP-136's residual.
|
||||
|
||||
## #310 — Retained preparation retry stalls landblock retirement with no bound
|
||||
|
||||
|
|
|
|||
File diff suppressed because one or more lines are too long
232
docs/research/2026-08-04-c4-route-4b-2-delta-review-findings.md
Normal file
232
docs/research/2026-08-04-c4-route-4b-2-delta-review-findings.md
Normal file
|
|
@ -0,0 +1,232 @@
|
|||
# C4 route 4b-2 — delta review FAIL (round 2 correction), 2026-08-04
|
||||
|
||||
Both delta Opus reviews returned **FAIL** on the fix round. Original slice is WIP
|
||||
`dfd27896`; the fix round is uncommitted on top of it.
|
||||
|
||||
The fix round did real work and most of it is verified correct — see
|
||||
"Do not churn" at the end. Two defects block, and one of them is worse than the
|
||||
defect the round was fixing.
|
||||
|
||||
## MAJOR A — the `store_position` fallback fires on the two retail paths that decline to store
|
||||
|
||||
The round's load-bearing claim (`RuntimeRemotePlacementDriveController.cs:549-553`
|
||||
and AP-138 part 1) is that retail's non-storing returns "are reached only after a
|
||||
cell resolved and the transition ran — states this controller's non-commit
|
||||
outcomes never represent, because they all mean the placement never executed."
|
||||
|
||||
**The second clause is false for `Rejected`.**
|
||||
|
||||
Retail, `CPhysicsObj::SetPositionInternal` @0x00515BD0:
|
||||
- `CheckPositionInternal == 0` @0x00515C85 → `handle_all_collisions` @0x00515CC2
|
||||
→ `return ((eax_14 - eax_14) & 2) + 2` @0x00515CD5 → **2 or 4**. No store.
|
||||
- `sphere_path.curr_cell == 0` @0x00515C8F → `return 3` @0x00515CB2. No store.
|
||||
|
||||
acdream ports that enum literally — `PhysicsSetPosition.cs:12-20`
|
||||
(`NoValidPosition = 2, NoCell = 3, Collided = 4`, doc-commented against
|
||||
`acclient.h` enum 491). The producing chain:
|
||||
`PhysicsEngine.cs:1548-1575` / `:1580-1584` → `RuntimeSetPositionState.cs:3034-3039`
|
||||
(`if (!result.IsSuccessful) return Outcome(Rejected, …)`), which is **after**
|
||||
`_physics.Engine.SetPosition(...)` at `:2983` → `RuntimeRemotePlacementDriveController.cs:775-780`
|
||||
`default:` → `Rejected` → `:578-579` stores.
|
||||
|
||||
So a far snap whose destination the engine's own sweep refuses — a remote
|
||||
server-snapped into geometry, or into a cell the sphere path cannot resolve —
|
||||
now teleports the canonical body into that refused destination. Retail leaves the
|
||||
object where it was. Reachable in the ordinary case: `CanAttemptDestination`
|
||||
passing means collision *is* published, which is exactly when the engine runs.
|
||||
|
||||
**Worse sub-case:** `RuntimeSetPositionState.cs:3057-3081` returns `Cancelled`
|
||||
**after `CommitCanonical` succeeded** ("retail lets that physical commit land
|
||||
regardless"). `Cancelled` also falls into `default:` → `Rejected` → the fallback
|
||||
overwrites the just-settled body (contact plane, step-down) with the raw
|
||||
unresolved destination. `RemoteMotion.Body` **is** `record.PhysicsBody` in
|
||||
production (`RuntimePhysicsState.cs:1039`, sole construction site).
|
||||
|
||||
**Zero of the five new non-commit tests cover `Rejected`.** The 10-row
|
||||
discrimination table has no row for the one outcome where the fallback is wrong.
|
||||
|
||||
### The required partition
|
||||
|
||||
- **Store** (placement genuinely never executed → retail @0x00515C1D
|
||||
`store_position`): `Refused`, `Contention`, `NotApplicable`, **and** `Rejected`
|
||||
arising from a *preparation* failure (`InvalidData`/`RejectedAuthority`,
|
||||
`:770-773`) which never reached the engine.
|
||||
- **Do not store** (engine ran and refused → retail @0x00515CB2 / @0x00515CD5):
|
||||
`Rejected` arising from `outcome.Status`, and `Cancelled`-after-commit, which
|
||||
must not overwrite a settled pose.
|
||||
|
||||
**The status enum is too coarse to express this.** Widen it or thread the
|
||||
originating outcome; do not approximate it with a heuristic.
|
||||
|
||||
## MAJOR B — the quiescence pre-flight is incomplete on BOTH sides
|
||||
|
||||
Both reviewers found this independently by different routes. `CanAttemptDestination`
|
||||
(`RuntimeRemotePlacementDriveController.cs:911-915`) tests one prefix, and
|
||||
`IsCollisionPrefixQuiescing` (`RuntimeSetPositionState.cs:827-829`) masks to that
|
||||
one prefix, called with the **destination** only.
|
||||
|
||||
Core's predicates are broader in two independent ways:
|
||||
|
||||
1. **Source landblock** — `PlacementTouchesPrefix` (`:3836-3841`) also matches
|
||||
`request.CurrentCellId`, populated from `operation.Record.FullCellId` whenever
|
||||
`body.InWorld` (`:2884-2888`). A far snap *out of* a quiescing landblock passes
|
||||
the pre-flight and parks at `:2946`/`:2974`. **This is the likelier shape** —
|
||||
streaming retires and republishes continuously, and a remote at >=96 m sits
|
||||
near the window edge where retirement happens.
|
||||
2. **Swept neighbour** — `ResultTouchesPrefix` (`:3843-3858`) scans **every**
|
||||
`QueriedCellIds` entry. That footprint provably spans neighbour landblocks:
|
||||
`PhysicsEngine.cs:1347` binds it to the transition's candidate array,
|
||||
materialized at `:1384`; `CellArray.Add` mirrors every id
|
||||
(`CellArray.cs:43-52`); `CellTransit.FindCellSet` (`TransitionTypes.cs:3521-3527`)
|
||||
reaches `AddAllOutsideCells` (`CellTransit.cs:918`, `:985`); and
|
||||
`AddOutsideCell` (`CellTransit.cs:375-384`) states explicitly there is **no
|
||||
same-block filter** — neighbour cells come out with the neighbour's prefix,
|
||||
added whenever the sphere is within its radius of a boundary
|
||||
(`AddAllOutsideCells:337-350`).
|
||||
|
||||
Either route reaches `ParkDeferred` with `restorableOnCancel` defaulting **false**
|
||||
(`:4276`), so `CancelToken`'s `restoreCancelledPark: true` finds
|
||||
`ParkWithdrawal.Captured` false (`:3399-3410`, `:4285`) and `RestoreParkWithdrawal`
|
||||
never runs. The remote is left `InWorld = false`, `Active` cleared, clock
|
||||
suspended, `FullCellId = 0`, spatial projection withdrawn (`:4294-4311`), with the
|
||||
only operation that could wake it destroyed. The fallback pose write does not
|
||||
help — it writes a pose to a withdrawn body, which merely makes the stranding
|
||||
invisible to a position assertion.
|
||||
|
||||
**A pre-flight cannot close this.** The sweep footprint does not exist until the
|
||||
sweep has run. This is the same structural conclusion 4b-1's review reached, and
|
||||
`SubmitAndResolve`'s own comment at `:768-770` cites `TryGetBlockingQuiescence` by
|
||||
name as the residual — recording the hole while the code claims to close it.
|
||||
|
||||
### MAJOR C — even a would-COMMIT placement is converted to a non-restorable park
|
||||
|
||||
`:3012`'s check is `result.IsSuccessful && TryGetBlockingQuiescence(...)`, and
|
||||
`IsSuccessful` is `Error == Ok` (`PhysicsSetPosition.cs:151`) — true for a fully
|
||||
committed result. It sits **ahead** of the restorable `result.IsDeferred` park at
|
||||
`:3050-3054`. So a healthy, resident, about-to-commit far snap near a seam is
|
||||
rewritten to `DeferredCell` at `:3017-3020` and parked non-restorably. The claim
|
||||
that "the restorable engine-result park stays reachable" holds only when no
|
||||
quiescing prefix is touched; the quiescence branch pre-empts it.
|
||||
|
||||
### The pinned fix — at the source, as the campaign already decided
|
||||
|
||||
AP-136's blanket "every quiescence/retirement park is non-restorable" is
|
||||
**over-broad for the two `SubmitPreparedPlacementCore` parks**. Its stated reason
|
||||
— re-admitting a spatial root into a retiring prefix would block the retirement —
|
||||
is about `ParkCollisionResidents` (`:3683`), where the entity's **own** cell is
|
||||
retiring and `IsAffectedCollisionResident` (`:3722-3738`) /
|
||||
`HasOldPrefixPlacementDebt` (`:3809-3834`) would pin the prefix forever.
|
||||
|
||||
It does **not** hold at `:2974`/`:3025`: `RestoreParkWithdrawal` restores residency
|
||||
at `body.CellPosition.ObjCellId` (`:3475`) — the destination cell — while the
|
||||
blocking prefix is the merely-swept neighbour or the departed source. Restoring
|
||||
there re-admits nothing into the retiring prefix.
|
||||
|
||||
**Make `:2974`/`:3025` restorable when the restore cell's prefix differs from the
|
||||
blocking quiescence prefix.** Keep `CanAttemptDestination` as an optimisation —
|
||||
it uses Core's own read-only predicate, is exact within its subset, adds no
|
||||
timer/retry/flag, and strictly shrinks the reachable set. It must simply stop
|
||||
being the correctness mechanism. Update AP-136 and AP-138(2) to match.
|
||||
|
||||
## MAJOR D — `StoreAcceptedDestinationPose` writes through a possibly-superseded incarnation
|
||||
|
||||
`CancelToken` publishes its receipt **synchronously**
|
||||
(`RuntimeRemotePlacementDriveController.cs:925-930`), and this same diff's own doc
|
||||
(`LiveEntityNetworkUpdateController.cs:1005-1018`) states a caller "MUST
|
||||
re-validate position ownership … on EVERY placement status, before writing
|
||||
anything else for the packet". `:576-580` calls `StoreAcceptedDestinationPose`
|
||||
immediately after that dispatch with **no currency check**. `RestoreParkWithdrawal`
|
||||
does guard (`RuntimeSetPositionState.cs:3468-3469`). The App-side re-validation
|
||||
happens only after the seam returns — too late.
|
||||
|
||||
This is the exact rule the R5 fix in this same diff introduced. One
|
||||
`IsCurrent(record)` test closes it.
|
||||
|
||||
## MINOR
|
||||
|
||||
- **N1 — `Advance()`'s window-drop path is asymmetric.** `:692-696` calls
|
||||
`CancelToken` without `StoreAcceptedDestinationPose`, so a retained retry whose
|
||||
destination leaves the window reproduces a smaller version of the freeze this
|
||||
round exists to fix. Its comment at `:685` still claims the entry point "keeps
|
||||
its last committed pose" — no longer true.
|
||||
- **N2 — post-sweep quiescence park overwrites a settled pose.** At `:3017` the
|
||||
parked `result` is the committed one, whose `Position` is `spherePath.CurPos`
|
||||
(`PhysicsEngine.cs:1605`) — collision-settled. `ParkDeferred:4290` snaps there;
|
||||
`StoreAcceptedDestinationPose:627-630` then writes the raw destination over it.
|
||||
Latent while MAJOR B stands; live the moment the park becomes restorable.
|
||||
- **N3 — `Advance()` reopens the `CurrentCellId` condition.** `:690-702` re-checks
|
||||
only `CanAttemptDestination(destination)`. Non-Position rebucket paths
|
||||
(`RemoteTeleportController.cs:532`, `DatLiveEntityProjectionMaterializer.cs:777`,
|
||||
`EquippedChildRenderController.cs:408`) can move `record.FullCellId` to a third,
|
||||
quiescing landblock in between.
|
||||
- **N4 — five stale comments, inside the round meant to end the disease.**
|
||||
`:777-778` ("Rejected/Cancelled — … so the body never moved") is the one whose
|
||||
correctness would have exposed MAJOR A. Also `:685` (N1), `:91-93`
|
||||
(`Rejected`'s doc omits the engine-refusal producer), `:2144-2151` (doesn't
|
||||
mention the guard now in front of it), `:2183-2187` (provenance claim fine,
|
||||
value claim false). And `LiveEntityNetworkUpdateController.cs:1078-1079` cites
|
||||
`:1886`/`:2130` where the actual early returns are `:1977`/`:2241` — wrong
|
||||
citations *in the fix for the stale-comment finding*.
|
||||
- **N5 — `NotApplicable` via `record.PhysicsBody is null`** (`:436`) still runs the
|
||||
fallback, writing a `RemoteMotion` body that in that state is not the canonical
|
||||
one (`RemoteMotion.cs:271`).
|
||||
- **N6 — `teleport_hook` enumeration incomplete.** @0x00514ED0 also calls
|
||||
`report_collision_end(this, 1)` @0x00514F31, omitted from
|
||||
`RuntimeRemoteFarSnapPosition.cs:454-457` and AP-137.
|
||||
- **N7 — a test assertion is order-dependent.**
|
||||
`RemotePlacementLedger_ConvergesAcrossGuidReuse_WithoutTeardown:708-711` —
|
||||
`Assert.Equal(0, drive.PendingCount)` is zero only because the preceding
|
||||
`CaptureOwnership()` invoked `CountLivePending`, which **mutates** `_pending`.
|
||||
Swap the asserts and it fails. Also omits `AssertConverged`.
|
||||
|
||||
## Required tests
|
||||
|
||||
- `Rejected` from an engine refusal: assert the body does **not** move.
|
||||
- `Cancelled`-after-commit: assert the settled pose survives.
|
||||
- `Rejected` from a preparation failure: assert the body **does** advance.
|
||||
- Quiescing **source** landblock: far-snap out of it, assert
|
||||
`body.InWorld && record.ObjectClock.IsActive && record.FullCellId != 0`.
|
||||
- Quiescing **neighbour**: far-snap to a point one sphere radius inside a
|
||||
landblock seam with the neighbour quiescing, same assertions.
|
||||
- `Advance()`'s window-drop path: assert the pose advances.
|
||||
|
||||
## Do not churn — independently verified correct
|
||||
|
||||
- The retail decode of `SetPositionInternal`'s storing branch (@0x00515CDA →
|
||||
@0x00515CE2 → @0x00515CF2 → @0x00515CF7 → `return 0` @0x00515D07). Exact.
|
||||
- **R2's deferral to 4b-3 is the right call**, and both reviewers agree.
|
||||
`teleport_hook` @0x00514ED0 runs *before* the placement @0x00516420, so a
|
||||
pose-only half-port would strand a live moveto, stick, and leash.
|
||||
`RuntimeTeleportHookPhase.BeforePositionOperation` exists
|
||||
(`RuntimeAuthoritativePositionRouteClassifier.cs:410`) and is recorded-and-dropped
|
||||
for the remote arm — a representable delta with the data model already present.
|
||||
- **The carried `SEND_POSITION_EVENT_SPF = 0x1000` question is SETTLED: the flag
|
||||
is inert.** @0x00515330 takes `(CPhysicsObj*, CTransition const*)` — no struct;
|
||||
@0x00515BD0 reads only bit 5; @0x00516040 tests only bits 8/9; a tree-wide
|
||||
search finds no bit-12 test on a `SetPositionStruct`.
|
||||
- **Pose parity and the decoy.** `TryGetWorldFrameOffset` is the source of
|
||||
`ShadowWorldOffsetX/Y`; `StoreAcceptedDestinationPose:627-635` composes
|
||||
identically to `RuntimeSetPositionMoverPreparer.TryBuild`
|
||||
(`RuntimeSetPositionMoverPreparation.cs:155-168`). `worldPos` is **not** an
|
||||
input — the decoy retains its discriminating power.
|
||||
- The status is genuinely no longer discarded; both arms thread
|
||||
`RemoteContactRouting(Arm, Placement)`.
|
||||
- Register bookkeeping: `grep -c "^| AP-"` = 96, header updated, no duplicates,
|
||||
nothing retired kept a row.
|
||||
- R5's ordering is identical in both arms, and its "not constructible" claim
|
||||
holds — though it is the standard signal the guard belongs behind a testable
|
||||
Runtime seam rather than duplicated at two App call sites. Name that for 4b-3.
|
||||
- The `StopInterpolating` "unobservable" claim is **true**:
|
||||
`TryExecuteAcceptedRemotePosition` never receives `remote`, and no placement or
|
||||
projection path reads or writes the queue. Stating it beats a false pin.
|
||||
- R7's `Assert.True(remote.Interp.IsActive)` genuinely closes its hole. R8's two
|
||||
park tests provoke their states honestly.
|
||||
- AP-136's scoping was read correctly and was not retrofitted (untouched in the
|
||||
diff).
|
||||
|
||||
## Gate
|
||||
|
||||
Complete Release suite. **Baseline 10,968 / 4 / 0** at `1b631f12` (App 4088,
|
||||
Runtime 1073), measured. The fix round measured 10,997 / 4 / 0 while containing
|
||||
every defect above. Known flakes: **#302** (`PortalProjectionTests…`, App.Tests)
|
||||
and **#308** (`NakEmissionTests.LossSoak_…`, Core.Net.Tests). Do not conflate.
|
||||
206
docs/research/2026-08-04-c4-route-4b-2-review-findings.md
Normal file
206
docs/research/2026-08-04-c4-route-4b-2-review-findings.md
Normal file
|
|
@ -0,0 +1,206 @@
|
|||
# C4 route 4b-2 — dual review FAIL + the pinned correction (2026-08-04)
|
||||
|
||||
Both mandated Opus reviews returned **FAIL**. Nothing is committed as final; the
|
||||
work sits at WIP `dfd27896`.
|
||||
|
||||
Review A (retail faithfulness + deletions) confirmed **every retail citation**
|
||||
independently, including the x87 parity decode proving exactly 96.0 takes the
|
||||
FAR branch. The deletions are complete and exact, the "Do NOT touch" list was
|
||||
respected, AP-87 was neither carried onto the far branch nor deleted from the
|
||||
near branch, and the two-site `ConstrainTo` partition is exhaustive and disjoint.
|
||||
Review B confirmed the `worldPos` decoy genuinely discriminates, both test
|
||||
removals are real replacements rather than drops, and `OwnsFarSnap` is exact
|
||||
against every classifier emission site. **Do not churn any of that.**
|
||||
|
||||
## The one root defect — everything else is downstream
|
||||
|
||||
**A refused / contended / rejected far snap leaves the remote frozen with an
|
||||
emptied interpolation queue.**
|
||||
|
||||
`ApplyRemoteContactRouting` discards the status
|
||||
(`LiveEntityNetworkUpdateController.cs:1002`, `_ = placementDrive.…`), while
|
||||
`Interp.Clear()` has already run unconditionally on the route flag
|
||||
(`RuntimeRemotePlacementDriveController.cs:485-487`). Three of five outcomes
|
||||
move nothing: `Refused` (`:405`, `:624`), `Contention` (`:414`, `:588`),
|
||||
`Rejected` (`:592`, `:630`). The body keeps its stale pose, the queue is empty,
|
||||
and the next 5-10 Hz packet reproduces the state.
|
||||
|
||||
**Reachable, not theoretical.** The graphical service window is
|
||||
`GpuWorldState.IsNearTier` — collision-published *right now*
|
||||
(`GraphicalRemotePlacementServiceWindow.cs:89-90`) — with default near radius 4
|
||||
(`GameWindow.cs:145`). A remote that is rendered but still streaming is refused
|
||||
while genuinely beyond 96 m. `RetrySetupUnavailable` is reachable for any remote
|
||||
whose prepared Setup collision has not resolved.
|
||||
|
||||
**Retail never does this.** The far branch always calls `SetPositionSimple`
|
||||
@0x005163D9, and even when `SetPositionInternal` finds no cell it still commits
|
||||
the destination pose — `store_position` @0x00515CE2, then
|
||||
`GotoLostCell` @0x00515CF2 and `reenter_visibility` on cell arrival. That is the
|
||||
finding AP-136 is built on. The deleted legacy block also always tracked. **The
|
||||
shipped state is strictly further from retail than either.**
|
||||
|
||||
### Both reviewers' second MAJOR collapses into this one
|
||||
|
||||
Review B rated the `TryAdoptWireCellAfterRouting` arm-asymmetry MAJOR, arguing a
|
||||
refused player far snap leaves `record.FullCellId` a landblock away from the
|
||||
body and that `rmState.CellId` seeds the per-tick sweep — the #184
|
||||
invisible-but-solid producer.
|
||||
|
||||
Review A found the same asymmetry and rated it MINOR with the mechanism:
|
||||
`RebucketLiveEntity(update.Guid, p.LandblockId)` at `:1592` has **already**
|
||||
committed the wire full cell to canonical
|
||||
(`LiveEntityRuntime.cs:895-909`), and `RemoteMotion.CellId` reads through to
|
||||
`record.FullCellId` for every bound remote (`RemoteMotion.cs:169-176` +
|
||||
`RuntimePhysicsState.cs:965-984`). **Review A is right on the mechanism.** The
|
||||
suppressed write is a no-op on refusal.
|
||||
|
||||
**Resolution: the cell/body divergence is real but is caused by the frozen body,
|
||||
not by the suppression.** Fix the freeze and it disappears. Do not restructure
|
||||
the suppression to chase it.
|
||||
|
||||
## The correction — pinned, not open for redesign
|
||||
|
||||
**On every non-commit outcome the far arm must still advance the body to the
|
||||
accepted destination pose.** That is retail's `store_position`, and it restores
|
||||
the tracking the deleted legacy block had.
|
||||
|
||||
Constraints on the fix:
|
||||
|
||||
- The status must stop being discarded at `:1002`.
|
||||
- The service window stays an **optimisation**, not a correctness mechanism —
|
||||
4b-1's review already ordered that framing corrected and it shipped
|
||||
uncorrected. Do not delete it; do not let it be the reason a remote stops
|
||||
tracking.
|
||||
- Do **not** add a timer, retry, settle window, or suppression flag. The
|
||||
fallback is a pose write because retail writes the pose, not because it makes
|
||||
a symptom go away.
|
||||
- `LiveEntityNetworkRemoteFarSnapIntegrationTests.cs:120-147` currently
|
||||
**pins the freeze as correct** (`Assert.Equal(before, body.Position)`). It
|
||||
must be inverted, not deleted quietly.
|
||||
|
||||
## Also required
|
||||
|
||||
**R1 — AP-137's justification is factually wrong (review A M2).** The row claims
|
||||
the deleted `_playerController?.Position ?? Vector3.Zero` distance had "no
|
||||
relationship to `player_distance`". It was streaming-origin-relative
|
||||
(`LiveEntityNetworkUpdateController.cs:1360-1366`) and the streaming origin
|
||||
recentres on the player's landblock — a biased but genuinely correlated proxy,
|
||||
error bounded by roughly one landblock. The policy may still be right; the
|
||||
stated reason is not true. Rewrite it to say what the deleted test actually
|
||||
computed.
|
||||
|
||||
**R2 — AP-137 must state the cell-less delta (review A M3).** Retail routes a
|
||||
cell-less body through @0x00516386 → `SetPosition` @0x00516420, an
|
||||
**unconditional placement** sitting *before* the contact test. 4b-2 routes that
|
||||
classification into `ApplyInterpolate`, which **enqueues** whenever
|
||||
`!firstUp && willBeDrTicked && bodyToTarget <= 4 m`. For a cell-less remote
|
||||
already tracking, acdream now queues where retail places — at *any* distance,
|
||||
not only >=96 m. State it. If it should instead place, say so and change it.
|
||||
|
||||
**R3 — AP-137 must state that `RejectedData` is applied anyway.** It is the one
|
||||
classification meaning "this payload failed validation"
|
||||
(`classifier.cs:535-547`), and `UnroutedCatchUp` hands the same payload to
|
||||
`ApplyInterpolate`. Not a regression — the legacy block did the same — but the
|
||||
slice's stated purpose was an explicit handler, and the row omits it.
|
||||
|
||||
**R4 — a register row for the refusable far placement.** `grep` for
|
||||
"Refused|service window" in the register returns zero. 4b-1 was dormant and owed
|
||||
nothing; **4b-2 is the commit that makes it live and therefore owes the row**
|
||||
(CLAUDE.md register rule 1). Whatever residual divergence survives the fix above
|
||||
gets measured against retail's `store_position`/`GotoLostCell` behaviour.
|
||||
|
||||
**R5 — the two arms' guard/arm ordering must be identical (review A M4).**
|
||||
|
||||
| arm | routing | re-entrancy guard | post-op arm |
|
||||
|---|---|---|---|
|
||||
| player | `:2020` | `:2047-2054` (`return`) | `:2037` — **before** the guard |
|
||||
| NPC | `:2202` | `:2214-2221` (`return`) | `:2235` — **after** the guard |
|
||||
|
||||
The NPC arm therefore arms `ConstrainTo` **zero** times on a superseded
|
||||
incarnation, where retail arms unconditionally on the nonzero return
|
||||
@0x00454254/@0x00454272. The player arm arms the leash on a possibly-superseded
|
||||
`rmState.Host`. This also contradicts the diff's own new remarks at `:958-966`
|
||||
("MUST re-validate … before writing anything else for the packet") — the player
|
||||
arm writes the leash first.
|
||||
|
||||
**R6 — six comments the change falsifies** (review A m6; review B found the same
|
||||
class independently as N6). This is the **fifth** consecutive slice shipping
|
||||
stale comments asserting behaviour the code no longer has:
|
||||
`LiveEntityNetworkUpdateController.cs:835`, `:868`, `:1589-1591`, `:2076-2081`;
|
||||
`RuntimeEntityObjectLifetime.cs:600-605`; and
|
||||
`RuntimeRemotePlacementDriveController.cs:106-143`, which still headlines "The
|
||||
central decision — refuse, do not park" and asserts a cancelled park leaves the
|
||||
entity invisible and intangible for the session — falsified at HEAD by the park
|
||||
restore **this same class's** `CancelToken` uses (`:698-703`). 4b-1's review
|
||||
already ordered that one corrected.
|
||||
|
||||
**R7 — three tests assert less than their names claim (review B N2/N3).**
|
||||
- `NoClassificationAtAll_NearAndTicked_EnqueuesInsteadOfSnapping`
|
||||
(`:191-213`) never inspects `remote.Interp`. A regression to "snap when
|
||||
`bodyToTarget > 4 m`, else do nothing" passes it *and* its sibling. One
|
||||
`Assert.True(remote.Interp.IsActive)` closes it.
|
||||
- The `StopInterpolating` ordering test (`RuntimeRemotePlacementDriveControllerTests.cs:830-871`)
|
||||
proves the clear is unconditional, not that it precedes the placement. Its doc
|
||||
comment claims proof it does not have. Either pin the order or state plainly
|
||||
that it is unobservable in acdream.
|
||||
|
||||
**R8 — the `DeferredCell` park branch has zero coverage (review B N5).** The
|
||||
opt-in chain is correct by reading (`:623` → `:698-703` →
|
||||
`RuntimeSetPositionState.CancelCore` → `RestoreParkWithdrawal`), but no test in
|
||||
4b-1 or 4b-2 exercises it, and the far arm is the first production path that can
|
||||
provoke a park. Add the test: commit the destination collision generation, open
|
||||
a `CollisionPrefixQuiescence` on the destination prefix, far-snap, then assert
|
||||
`body.InWorld && record.ObjectClock.IsActive && record.FullCellId != 0`.
|
||||
|
||||
**R9 — currency across GUID reuse / incarnation / generation is undelivered
|
||||
(review B N4).** Contract item 7 named five dimensions; two shipped
|
||||
(interleaving, teardown). `_pending` and `_awaitingAcknowledgement` are keyed by
|
||||
`RuntimeEntityKey` (guid+incarnation), so a reused GUID makes a *new* key and the
|
||||
old entry self-heals only on a ledger read (`:670-688`) or in `Advance()`
|
||||
(`:513-525`), which early-returns unless something is pending. Dead-incarnation
|
||||
entries accumulate until `DetachRoute`. Converges at teardown; does **not**
|
||||
converge in-session. Add the test: far-snap `G` at incarnation 1, retire it,
|
||||
re-create `G` at incarnation 2, assert `RemotePlacementDrivePendingCount == 0`
|
||||
**without** a teardown.
|
||||
|
||||
**R10 — `AirborneNoOperation` falls into `default:`** (review A m7). Unreachable
|
||||
today — both callers early-return (`:1876-1881`, `:2130-2136`) — but the
|
||||
`default:` comment asserts unreachability where an explicit case enforces it.
|
||||
|
||||
## Not blocking, recorded
|
||||
|
||||
- **n8**: `OwnsFarSnap` also matches the classifier's FORCE_POSITION shape
|
||||
(`classifier.cs:329-345`), unreachable for remotes via `ValidAcceptedAuthority`
|
||||
(`:522-525`) and `PhysicsTimestampGate` (`:190`). Defense-in-depth; note only.
|
||||
- **NOT VERIFIED, carried**: what `SetPositionInternal` @0x00515330 does with the
|
||||
0x1000 (`SendPositionEvent`) bit, which reaches
|
||||
`TryPrepareAndSubmitAuthoredPlacement` for the far snap while the controller
|
||||
doc argues acdream has "no ack" for remotes. Those are two different things
|
||||
(the `SetPositionStruct` flag vs `CommandInterpreter::SendPositionEvent`
|
||||
@0x00454091); 4b-1's review cleared only the latter. Settle with a read of
|
||||
@0x00515330-@0x00515543.
|
||||
- **Process**: `ParkCollisionResidents_StaysUnreachable_AfterRefusalAndAfterCommit`
|
||||
(`RuntimeRemotePlacementDriveControllerTests.cs:782`) still calls the method
|
||||
directly at `:824` — the shape 4b-1's finding B2 declared void. It shipped at
|
||||
`2e8e09ac`, so it is outside this diff, but it is in the tree being counted as
|
||||
evidence.
|
||||
- **Headless (contract item 6) is satisfied vacuously.** Nothing in headless
|
||||
constructs the drive controller and `RuntimeLiveEntitySessionController.OnPositionUpdated`
|
||||
returns early for every non-local GUID (`:212-217`). The contract allowed this
|
||||
only if stated plainly; AP-137 does not mention headless, and
|
||||
`IRuntimeRemotePlacementServiceWindow`'s doc still describes the headless
|
||||
implementation as a live consumer. State it.
|
||||
|
||||
## Gate
|
||||
|
||||
Complete Release suite, not a subset. **Corrected baseline: 10,968 / 4 / 0** at
|
||||
`1b631f12`, measured independently in a throwaway worktree (App 4088, Runtime
|
||||
1073). The 10,973 figure in the 4b-2 contract was wrong and propagated from an
|
||||
earlier mis-record — an inflated baseline is exactly what would let a future
|
||||
slice delete tests and still look green. The WIP state measured 10,990 / 4 / 0
|
||||
while containing the freeze defect above.
|
||||
|
||||
Two known flakes, do not chase and do NOT conflate: **#302**
|
||||
(`PortalProjectionTests…`, GC-allocation assertion, App.Tests) and **#308**
|
||||
(`NakEmissionTests.LossSoak_…`, wall-clock deadline, Core.Net.Tests, full-suite
|
||||
load only).
|
||||
211
docs/research/2026-08-04-c4-route-4b-2-round3-correction.md
Normal file
211
docs/research/2026-08-04-c4-route-4b-2-round3-correction.md
Normal file
|
|
@ -0,0 +1,211 @@
|
|||
# C4 route 4b-2 — round 3 correction (2026-08-04)
|
||||
|
||||
Both round-2 reviews returned **FAIL**. The findings are now small and local;
|
||||
this is a closing-out round, not a structural change.
|
||||
|
||||
**Do NOT revert the shared-core park change.** Both reviewers verified it removes
|
||||
a session-permanent strand from routes 1 and 2:
|
||||
`RuntimeEntityObjectLifetime.cs:1767-1769` calls
|
||||
`Forget(canonical, restoreCancelledPark: true)` on **every** accepted Position for
|
||||
**every** entity, so before this round a local-player ForcePosition or a
|
||||
first-entry placement that hit either quiescence park was left `InWorld = false`,
|
||||
clock suspended, `FullCellId = 0`, with its operation destroyed by the next
|
||||
packet. The change is directionally right and fixes shipped behaviour.
|
||||
|
||||
## A1 — one relocation closes both shared-core MAJORs
|
||||
|
||||
`IsRestorableQuiescencePark` (`RuntimeSetPositionState.cs:3877-3880`) has two
|
||||
independent defects, and moving the decision fixes both:
|
||||
|
||||
1. **It compares against one quiescence, not the set.** It tests
|
||||
`quiescence.Token.LandblockPrefix` — the token `TryGetBlockingQuiescence`
|
||||
returned — but both overloads (`:3906-3925`, `:3927-3949`) return the
|
||||
**minimum-`OperationId`** match, not the only one. `_collisionPrefixQuiescence`
|
||||
is a per-prefix dictionary (`:589`, `:885`) and streaming opens one per
|
||||
landblock mutation (activation `RuntimePhysicsState.cs:1820-1824`, retirement
|
||||
`:2025-2030`), so a recenter has several live. Source `S` quiescing before
|
||||
destination `D`: the predicate tests `D != S`, returns true, and the restore
|
||||
re-admits the spatial root into `D`, which *is* quiescing — the AP-136 pin. At
|
||||
5-10 Hz the park→cancel→restore→re-park cycle can starve `D`'s mutation
|
||||
permission for as long as `S` stays quiescing (`:927`).
|
||||
2. **Its input is not the value the restore uses.** It is fed `deferred.CellId` /
|
||||
`held.CellId` (`:2982`, `:3036`), but `RestoreParkWithdrawal` reads
|
||||
`body.CellPosition.ObjCellId` (`:3481`), set by `ParkDeferred`'s
|
||||
`body.SnapToCell` (`:4346-4349`) → `PhysicsBody.StageDormantCellFrame`
|
||||
(`PhysicsBody.cs:212-223`), which for any outdoor cell runs
|
||||
`LandDefs.AdjustToOutside`. That function's own doc (`LandDefs.cs:115-122`)
|
||||
says the resulting cell id **may belong to a neighbour landblock**.
|
||||
Reachability NOT VERIFIED, and the relocation removes the question.
|
||||
|
||||
**Fix: move the restorable decision inside `ParkDeferred`, after `SnapToCell`,
|
||||
and express it as `!IsCollisionPrefixQuiescing(body.CellPosition.ObjCellId)`.**
|
||||
That method (`:827-829`) already exists, is `internal`, masks its own argument,
|
||||
and is the pre-flight's own predicate; it subsumes the current test exactly.
|
||||
Make it non-static as needed; the `CollisionPrefixQuiescence` parameter then goes
|
||||
away.
|
||||
|
||||
**Blast radius note that must not be lost:** the remote far-snap arm is *masked*
|
||||
from defect 1 by `CanAttemptDestination`'s destination pre-flight
|
||||
(`RuntimeRemotePlacementDriveController.cs:599`, `:1155`). The exposed callers are
|
||||
**routes 1 and 2**, which have no such pre-flight
|
||||
(`RuntimeAcceptedPositionDriveController.TryExecuteAcceptedLocalPosition:330-392`
|
||||
goes straight from `ClassifyForcePosition` to `TryBeginExclusiveAuthoredPlacement`).
|
||||
All three new quiescence tests drive the remote arm and explicitly assert the
|
||||
**safe** configuration (`:1193`, `:1268`), so none of them can see it.
|
||||
|
||||
## A2 — two tests the round is missing
|
||||
|
||||
- **Concurrent quiescences**, source `OperationId` < destination `OperationId`,
|
||||
far snap `S → D`: assert the record is not re-admitted as a spatial root in `D`,
|
||||
or that `D`'s mutation permission still converges.
|
||||
- **A route-2 local-player ForcePosition** parking at `:2948` under a quiescing
|
||||
source: assert `body.InWorld && record.ObjectClock.IsActive &&
|
||||
record.FullCellId != 0` after the next packet's merge-time `Forget`.
|
||||
|
||||
## A3 — say "blocking prefix", or make it true
|
||||
|
||||
`IsRestorableQuiescencePark`'s doc (`:3866-3868`) and AP-136's narrowed row both
|
||||
claim "the rollback re-admits nothing into the retiring prefix." It re-admits
|
||||
nothing into **the blocking** prefix, which is a different statement. A1's fix
|
||||
makes the stronger claim true; if any wording survives it, scope it.
|
||||
|
||||
## A4 — #309's acceptance steps are now stale
|
||||
|
||||
AP-136 says this needs the two-client connected check in #309, but
|
||||
`docs/ISSUES.md` is **unmodified in this diff**. #309's steps (`docs/ISSUES.md:82-95`)
|
||||
describe only "a landblock the observer has not streamed" — the plain
|
||||
unplaceable park. They do not exercise a quiescing **source** or a quiescing
|
||||
**swept neighbour**, which are exactly the shapes this round newly makes
|
||||
restorable, and step 4 asks only that route 2's corrections "still land
|
||||
unchanged" without naming the local-player-parks-into-a-quiescing-landblock case
|
||||
that is now behaviourally different. **Deferring the run is fine; leaving its
|
||||
scope stale is not.** Update the steps in this commit. Also AP-136's Risk column
|
||||
still says "a remote" — the local player now traverses the same restore.
|
||||
|
||||
## A5 — record the dependency at `CanAttemptDestination`
|
||||
|
||||
N3's "a retained retry can no longer strand" property and MAJOR 1's masking
|
||||
**both** depend on the pre-flight staying in place. The class doc calls it "an
|
||||
optimisation" with no mention of either. If it is ever deleted as redundant, both
|
||||
re-open silently. Write that down at the pre-flight.
|
||||
|
||||
(The round's stated reason for declining N3's `CurrentCellId` re-check — that it
|
||||
would "re-derive a private Core predicate outside Core" — is false;
|
||||
`CanAttemptDestination` already calls Core's own `internal`
|
||||
`IsCollisionPrefixQuiescing`. The conclusion still holds, for the reason above.
|
||||
Correct the reason.)
|
||||
|
||||
## B1 — a 36-line doc block was detached from its method
|
||||
|
||||
`RuntimeRemotePlacementDriveController.cs:1009-1085`. `CountLivePending()` was
|
||||
inserted **between** `CountLiveAwaitingAcknowledgement`'s existing doc and its
|
||||
method. Now `:1009-1044` (which describes pruning `_awaitingAcknowledgement`, the
|
||||
**second** ownership registration, and `_awaitingAcknowledgementScratch`)
|
||||
documents `CountLivePending()` at `:1066` — which prunes `_pending`, backs the
|
||||
**first** registration, and uses `_pendingScratch`. Every specific is wrong for
|
||||
the member it now sits on. `:1045-1065` is a second `<summary>` on the same
|
||||
member, and `CountLiveAwaitingAcknowledgement()` at `:1085` has **no** doc.
|
||||
|
||||
## B2 — the stage rule is true by coincidence and stated as a falsehood
|
||||
|
||||
`:172-188` and `:993-1005` assert that `RejectedByPlacement`'s producers are
|
||||
"returned AFTER `_physics.Engine.SetPosition` ran". `RuntimeSetPositionStatus.Rejected`
|
||||
has **three** producers and only one is post-engine:
|
||||
|
||||
| site | what | stage |
|
||||
|---|---|---|
|
||||
| `:2854` | entry validation (`!ownsToken`, stage mismatch, null body, stale `IsPreparationAuthorityCurrent`, velocity-version mismatch) | **pre-engine** |
|
||||
| `:2945` | `!IsStructurallyValid(canonicalRequest)` | **pre-engine** |
|
||||
| `:3044` | `!result.IsSuccessful` after the engine ran at `:2997` | post-engine |
|
||||
|
||||
Both pre-engine sites reach `SubmitAndResolve`'s `default:` and are filed
|
||||
`RejectedByPlacement` → **no store** — contradicting the round's own rule
|
||||
(`RejectedPreparation`'s doc `:128-142`) that a producer which never reached the
|
||||
engine stores.
|
||||
|
||||
The reviewer traced both **unreachable today**, but only by duplicated guards:
|
||||
`PrepareMover` (`:1563-1577`) re-checks everything `:2830-2855` re-checks with
|
||||
nothing reentrant between; `IsPreparationAuthorityCurrent` (`:5290-5306`) catches
|
||||
all seven version dimensions one stage earlier as `RejectedAuthority` →
|
||||
`RejectedPreparation`; the `AwaitingCell` divergence needs `DormantLocalActivation`,
|
||||
which `:1380` forbids for a record with a body; and `:2945` is shadowed by
|
||||
`PrepareMover:1636`'s identical check.
|
||||
|
||||
**Either state in the `default:` comment that `:3044` is the only reachable
|
||||
producer and why, or thread the stage out of Core so the split is enforced.** As
|
||||
written the comment says something false about Core — the same shape as round 1's
|
||||
MAJOR A, which was also a false clause in that same comment.
|
||||
|
||||
## B3 — the deleted phrase came back
|
||||
|
||||
`RuntimeRemotePlacementDriveControllerTests.cs:1191`, an **added** line:
|
||||
`// The complete pre-flight passes: it reads only the destination.`
|
||||
`CanAttemptDestination`'s doc in the same diff (`:1111-1113`) says it is
|
||||
"correcting this comment's earlier 'the complete pre-flight' framing", and the
|
||||
whole of MAJOR B is that no pre-flight can be complete. The sentence also
|
||||
contradicts itself in eight words.
|
||||
|
||||
## Minor
|
||||
|
||||
- **m1** — `TryAdoptWireCellAfterRouting`'s new scope paragraph
|
||||
(`LiveEntityNetworkUpdateController.cs:1128-1143`) enumerates commit and
|
||||
`RejectedByPlacement` and "every outcome that resolved NO cell". `Deferred` is
|
||||
in neither: `ParkDeferred:4345-4349` snaps to the parked result cell,
|
||||
`WithdrawCanonical` zeroes `FullCellId`, and `RestoreParkWithdrawal:3485-3491`
|
||||
re-commits from `body.CellPosition.ObjCellId` — the **swept** cell for the
|
||||
post-sweep park, not necessarily the wire cell. Behaviour is unaffected (the
|
||||
suppression keys on the arm); the claim is not.
|
||||
- **m2** — `Contention`'s doc (`:118-127`) names two producers; there are at
|
||||
least four. `TryBeginExclusiveAuthoredPlacement:1349-1354` also refuses on
|
||||
`HasRetainedCompletion(key)` — ordinary right after a `Committed` far snap
|
||||
whose projection sink declined — and `BeginAcceptedPlacementCore:1404-1418` on
|
||||
`!_entities.IsCurrent(record)`. Both pre-engine, so the store side is right.
|
||||
- **m3** — the MAJOR D test (`:1516-1552`) reaches the superseded state via
|
||||
`lifetime.Entities.RemoveActive(record)` and picks `Refused`, which never calls
|
||||
`CancelToken`. It pins that the guard exists, not that it sits **after** the
|
||||
synchronous cancellation receipt — the defect's actual mechanism.
|
||||
- **m4** — `NotApplicable` via `record.Key is null` (body non-null) stores
|
||||
(`:576-581` → `:800`); the enum doc `:97-101` justifies only the no-body
|
||||
sub-case. Unreachable in production
|
||||
(`RuntimeEntityObjectLifetime.cs:600-612`).
|
||||
- **m5** — `ParkCollisionResidents` (`:3689-3696`) relies on `restorableOnCancel`
|
||||
defaulting false. The old blanket doc was itself the guard against a forgetful
|
||||
caller; now that it is gone, pass `false` explicitly.
|
||||
- **n1** — `:644` cites classifier `:467` for `StopInterpolating: !nearby`; it is
|
||||
`:468`. Pre-existing.
|
||||
- **n2** — `ParkDeferred` carries two adjacent `<summary>` blocks
|
||||
(`:4292-4301`, `:4302-4325`); only the second is emitted.
|
||||
- **n3** — the Runtime test class doc (`:30-33`) asserts every test was
|
||||
revert-verified, with no per-test mapping in the tree. Either map them or drop
|
||||
the claim to what the file can show.
|
||||
|
||||
## Do not churn — verified across both reviews
|
||||
|
||||
Retail decode of `SetPositionInternal` @0x00515BD0 (both storing and non-storing
|
||||
branches) is exact and matches the enum docs line-for-line. The 7-value switch is
|
||||
exhaustive with `_ => throw`, and no consumer outside the controller reads the
|
||||
value for a decision, so the `Rejected` split cannot silently miss a case.
|
||||
`Deferred`-not-storing is right, with a stronger unreachability proof than the
|
||||
comment gives (`ParkCollisionResidents:3563-3568` **throws** rather than parking
|
||||
an entity that already holds an operation). MAJOR D's guard is the correct
|
||||
predicate (`RuntimeEntityDirectory.cs:75-77`), sufficient, and correctly placed
|
||||
after the receipt at both sites. **N5 is genuinely unconstructible** —
|
||||
`:917-923`/`:935-940` throw on binding a component whose body differs from
|
||||
canonical. Pose parity survives the `remote.Body` → `record.PhysicsBody` change
|
||||
byte-for-byte. The neighbour test's construction is arithmetically verified
|
||||
(`191.95 mod 24 = 23.95 > 24 - 0.1` → `lx+1` → `0xB3`), it provably hits the
|
||||
post-sweep park, and its Z arm discriminates (terrain 7, authored 6). N7's
|
||||
reframing is accurate. All six required tests discriminate; the
|
||||
cancelled-after-commit construction is genuinely clever. Route 2's re-issue
|
||||
funnel does **not** interact with the restore (it decides on
|
||||
`PositionAuthorityVersion`/`_newestForce`, none of which the restore moves).
|
||||
`RestoreParkWithdrawal` writes exactly five things and no sweep footprint,
|
||||
cross-cell registration, or BSP residency among them. **No added line in the diff
|
||||
carries a source line-number citation** — that failure mode is fixed.
|
||||
|
||||
## Gate
|
||||
|
||||
Complete Release suite. Baseline **10,968 / 4 / 0** at `1b631f12`. Round 2
|
||||
measured **11,004 / 4 / 0** while containing everything above. Known flakes, do
|
||||
not chase or conflate: **#302** (`PortalProjectionTests…`, App.Tests), **#308**
|
||||
(`NakEmissionTests.LossSoak_…`, Core.Net.Tests).
|
||||
174
docs/research/2026-08-04-c4-route-4b-2-round4-correction.md
Normal file
174
docs/research/2026-08-04-c4-route-4b-2-round4-correction.md
Normal file
|
|
@ -0,0 +1,174 @@
|
|||
# C4 route 4b-2 — round 4 correction (final), 2026-08-04
|
||||
|
||||
Both round-3 reviews returned **FAIL**, and both bound the failure to the same
|
||||
place. **No shared-core edit is warranted.** Review A verified the relocation
|
||||
line by line and states plainly: the code is correct.
|
||||
|
||||
Round 4 is one test, one deleted sentence, two `docs/ISSUES.md` fixes, and a
|
||||
handful of doc corrections.
|
||||
|
||||
## The MAJOR — both reviewers, independently
|
||||
|
||||
**Nothing in the tree discriminates round 3's predicate from round 2's**, and a
|
||||
test doc cites a sibling test that does not exist.
|
||||
|
||||
`RuntimeRemotePlacementDriveControllerTests.cs:1343` names
|
||||
`ConcurrentQuiescences_ParkIsNotRestoredIntoTheQuiescingDestination`. That
|
||||
identifier occurs exactly twice in the tree, both inside that one comment.
|
||||
|
||||
Review A evaluated round 2's form
|
||||
(`blockingToken.LandblockPrefix != (result.CellId & 0xFFFF0000)`) against all six
|
||||
quiescence tests. **Every row agrees with round 3.** Revert the relocation to the
|
||||
single-token comparison and the suite is still green at 11,007. So A1's two
|
||||
stated defects — comparing one token instead of the live set, and reading the
|
||||
pre-snap cell instead of the post-snap one — have no automated floor under them
|
||||
at all, while the tree asserts otherwise.
|
||||
|
||||
**Write the test.** Both reviewers identified the same and only discriminating
|
||||
construction, in `RuntimeAcceptedPositionDriveControllerTests` (route 2 has no
|
||||
pre-flight, which is what makes it the caller that can reach the shape):
|
||||
|
||||
- two live quiescences, source `OperationId` < destination `OperationId`
|
||||
- park at `:2948` under the source
|
||||
- assert `!IsSpatialRoot(record)` after the next packet's merge-time `Forget`
|
||||
|
||||
It fails under the round-2 form (`Source != Dest` → restorable → re-admitted into
|
||||
the retiring destination) and passes under round 3. Then delete or correct the
|
||||
`:1343` cross-reference.
|
||||
|
||||
## #309 — the gate currently passes while broken
|
||||
|
||||
**S1.** Step 5 (`docs/ISSUES.md:115-119`) says the player "recovers on the next
|
||||
server Position rather than staying withdrawn indefinitely".
|
||||
`QuiescingDestinationPrefix_ForcePositionParkIsNotRestored` (`:906-912`) asserts
|
||||
the opposite for that packet — `InWorld == false`, clock inactive,
|
||||
`!IsSpatialRoot`. Route 2's drive dispatches **only** for `ForcePosition`; an
|
||||
ordinary `Apply` merges behind it (`RuntimeAcceptedPositionDriveController.cs:337`).
|
||||
So the packet the step names is precisely the one that does not recover it. Name
|
||||
what actually does — a later ForcePosition, or the quiescence releasing plus a
|
||||
placement.
|
||||
|
||||
**S2.** Steps 4 and 5 both need a ForcePosition to land *inside* a transient
|
||||
collision-prefix quiescence window, which the user cannot synchronise with a
|
||||
teleport or portal arrival, and neither step names a signal confirming the park
|
||||
fired. A tester sees a clean teleport and records a pass. **Give each step a
|
||||
stated confirmation signal** — a probe line, or a documented
|
||||
`CaptureOwnership().DeferredCellCount` check. If a deterministic way to hold a
|
||||
prefix quiescing for the duration is cheap, prefer it.
|
||||
|
||||
Step 5's retirement-still-completes clause is good; steps 1-3 and 6 are
|
||||
performable as written.
|
||||
|
||||
## Doc corrections
|
||||
|
||||
- **D1** — AP-136 says restore covers the plain unplaceable park
|
||||
"**unconditionally**". It does not: `ParkDeferred` now gates *every* park,
|
||||
including `:3063`, on `!IsCollisionPrefixQuiescing(body.CellPosition.ObjCellId)`
|
||||
(`:4358-4360`). The same row states it correctly two sentences later — the row
|
||||
contradicts itself.
|
||||
- **D2** — AP-136's trailing parenthetical still summarises #309 as the original
|
||||
three steps plus "route 2's corrections are unchanged". This commit rewrote
|
||||
#309. The row's body says the scope widened; its own summary does not.
|
||||
- **D3** — `RuntimeRemotePlacementDriveController.cs:1064-1066` attributes the
|
||||
null-body re-test to `PrepareMover`'s guard clause
|
||||
(`RuntimeSetPositionState.cs:1563-1577`), which has **no** body test. The real
|
||||
dependency is `RuntimeRemotePlacementDriveController.cs:625`
|
||||
(`record.PhysicsBody is null` → `NotApplicable`). Conclusion holds; the
|
||||
attribution points a maintainer at the wrong guard.
|
||||
- **D4 — B2's chosen value was not delivered.** Choosing "state it" over "enforce
|
||||
it" is sound and both reviewers concur with the decision — but **none** of the
|
||||
four dependencies is annotated at its own site: `PrepareMover`'s guard
|
||||
(`:1563-1577`), `IsPreparationAuthorityCurrent` (`:5304-5320`),
|
||||
`PrepareDormantLocalActivationOwnership`'s `record.PhysicsBody is not null`
|
||||
(`:1380`), `TryBeginExclusiveAuthoredPlacement`'s existing-operation refusal
|
||||
(`:1349-1351`). Deleting any one is invisible from the site, which is the whole
|
||||
thing "state it" was supposed to buy. Four one-line back-references close it.
|
||||
- **D5 — scope the `CurrentCellId` claim to the first submit.** `ParkDeferred`'s
|
||||
doc (`:4300-4307`, `:4320-4325`), AP-136 and AP-138(2) all say the pre-engine
|
||||
arm "names the destination" in production. True on the first submit, **false
|
||||
for a retained retry**: `Advance()` re-reads `record.FullCellId` at submit with
|
||||
no fresh merge (`RuntimeRemotePlacementDriveController.cs:941-954`,
|
||||
`RuntimeAcceptedPositionDriveController.cs:663-667`), and
|
||||
`RemoteTeleportController.cs:532` is a shipped writer that moves it to the
|
||||
pre-teleport landblock. `CanAttemptDestination`'s own doc (`:1234-1241`) already
|
||||
says this correctly — the three others contradict it. Otherwise the next reader
|
||||
concludes the `CurrentCellId` arm is dead code.
|
||||
Same family: `:1252-1253` ("every shape this predicate cannot see is a
|
||||
merely-swept NEIGHBOUR") is contradicted by the third-landblock shape named ten
|
||||
lines above it.
|
||||
|
||||
## D6 — make the `Captured` claim exact rather than caveated
|
||||
|
||||
`Captured` is a **park-time** snapshot. For the far snap the window is
|
||||
nanoseconds (`SubmitAndResolve`'s `DeferredCell` arm cancels synchronously,
|
||||
`:1038`). For **route 2** the park is *retained*
|
||||
(`RuntimeAcceptedPositionDriveController.cs:745-753`, `AwaitingCommitWake: true`)
|
||||
and the restore lands on the next packet's merge-time `Forget`, ~150 ms later.
|
||||
Streaming opens a quiescence per landblock mutation, so a prefix clean at park
|
||||
time can be quiescing at restore time, and `RestoreParkWithdrawal` re-admits into
|
||||
it unconditionally.
|
||||
|
||||
It self-heals and is no worse than the baseline, so it is a residual rather than
|
||||
a regression — but **add the re-test**: one `IsCollisionPrefixQuiescing(residentCellId)`
|
||||
condition at `RuntimeSetPositionState.cs:3488`, in the method that already reads
|
||||
that variable. That makes the docs' claim true at the moment it matters instead
|
||||
of requiring a "when the park was taken" caveat everywhere it appears. Prefer the
|
||||
exact claim over the caveat.
|
||||
|
||||
## NIT
|
||||
|
||||
- **N1** — `IsCollisionPrefixQuiescing(0)` re-opens the C3c-F3 sentinel
|
||||
conflation. `AdjustToOutside` zeroes the cell id on map-edge failure
|
||||
(`LandDefs.cs:139`), and prefix `0x00000000` is the *legitimate* corner
|
||||
landblock (0,0) per `BeginCollisionPrefixQuiescence:840-846`, not "no
|
||||
landblock". Review A could not construct a reachable path, but this file
|
||||
already carries a shipped-crash comment about exactly this conflation. Add the
|
||||
`!= 0u` guard.
|
||||
- **N2** — `FarSnap_ConcurrentQuiescences_RefusesBeforeOpeningAPark` is
|
||||
behaviourally redundant with the destination-prefix test (both fail identically
|
||||
if the pre-flight's `IsCollisionPrefixQuiescing` half is removed), and its
|
||||
`sourceQuiescence.OperationId < destinationQuiescence.OperationId` assert never
|
||||
influences an observed outcome. Keep it, but its doc should not imply stronger
|
||||
A5 evidence than it carries.
|
||||
- **N3** — `RuntimeAcceptedPositionExecutionStatus.DeferredCell`'s doc ("the
|
||||
destination landblock's collision generation was not ready") is the status two
|
||||
new route-2 tests assert for a *quiescence* park. Pre-existing, in a file this
|
||||
diff does not touch — the next stale comment in line. Fix it or file it.
|
||||
|
||||
## Do not churn — verified across both reviews
|
||||
|
||||
The A1 relocation is mechanically correct: `IsCollisionPrefixQuiescing` consults
|
||||
the full map (`:827-829`; one live entry per prefix via Remove-then-Add
|
||||
`:857`/`:885`); the post-snap cell is exactly what `RestoreParkWithdrawal:3484`
|
||||
restores at, with no writer in between; `SnapToCell` really does set
|
||||
`InWorld = true` (`PhysicsBody.cs:201-205`) so the three-local hoist is required,
|
||||
not stylistic; `RetryDeferred` re-parks without a second `SnapToCell`, keeping
|
||||
the decision bound to the cell it was taken against; the plain park at `:3063` is
|
||||
**provably** unchanged (the `:3017` quiescence check already returned false, and
|
||||
`AdjustToOutside` is idempotent on an adjusted pair); `ParkCollisionResidents`
|
||||
passes `false` explicitly; no prefix pin or streaming stall is possible
|
||||
(`HasOldPrefixPlacementDebt:3829` skips `WakeableLostCell`, and the operation is
|
||||
retired before the restore). All four `ParkDeferred` call sites accounted for.
|
||||
|
||||
**The pushback is a legitimate correction**, traced independently through
|
||||
`TryApplyPosition:1771-1774` → `RefreshDerivedState:230-237` → `SetFullCell`, and
|
||||
the remote leg through `RebucketLiveEntity` → `CommitRebucket:1816`. Route 1 and
|
||||
remote creates cannot reach the source shape at all (`CurrentCellId` requires
|
||||
`body.InWorld`, false for a first-entry body).
|
||||
|
||||
Both route-2 tests drive production (`MergeAccepted` → real `TryApplyPosition`,
|
||||
real `PhysicsTimestampGate`, then `TryExecuteAcceptedLocalPosition`); nothing is
|
||||
hand-built. Keeping `FarSnap_QuiescingSourceLandblock_` is right — the state is
|
||||
constructible in the fixture, only production cannot reach it — and its caveat is
|
||||
accurate. A5 is satisfied at the pre-flight itself. B1, B3, m1-m5, n2, n3 all
|
||||
fixed and accurate. **n1 in the round-3 correction was MINE and wrong** —
|
||||
classifier `:467` is correct, `:468` is `ConstrainPhase`, and the member-name
|
||||
replacement is exact. No added line carries a source line-number citation —
|
||||
verified by scanning every `+` line.
|
||||
|
||||
## Gate
|
||||
|
||||
Complete Release suite. Baseline **10,968 / 4 / 0** at `1b631f12`; round 3
|
||||
measured **11,007 / 4 / 0**. Known flakes, do not chase or conflate: **#302**
|
||||
(`PortalProjectionTests…`, App.Tests), **#308** (`NakEmissionTests.LossSoak_…`,
|
||||
Core.Net.Tests).
|
||||
Loading…
Add table
Add a link
Reference in a new issue