A local-player ForcePosition had TWO independent writers for one accepted
packet: LocalForcePositionTransaction snapped the physics body
(PlayerMovementController.BlipPosition, a raw SnapToCell with no collision
resolve), while LiveEntityNetworkUpdateController's generic tail separately
wrote position/cell/rotation to the render WorldEntity from the raw wire and
rebucketed it. Two stores, one packet — the divergence class 670f307c fixed on
the remote path. The outbound AutonomousPosition ack also fired BEFORE any
canonical commit existed: we told ACE "got it, I'm here" before deciding where
"here" was, and the trailing isCurrent() could only suppress the continuation,
never recall the packet.
RuntimeAcceptedPositionDriveController is now the one Runtime-owned seam. Both
hosts call the identical TryExecuteAcceptedLocalPosition; App and headless
project the committed result through the existing placement projection sink
(LiveEntityRuntime.TryApplyRuntimePlacementPlace already performed the same
four writes, from committed state rather than a wire guess).
Retail: SmartBox::HandleReceivedPosition @0x00453FD0's FORCE_POSITION branch is
get_heading -> Frame::set_heading -> SmartBox::BlipPlayer @0x00453940 -> stamp
POSITION_TS -> SendPositionEvent @0x00454091 -> return @0x0045409D. BlipPlayer
is CPhysicsObj::SetPositionSimple @0x005162B0 with flags 0x1012
(Teleport|Slide|SendPositionEvent) — a real collision-resolving SetPosition,
not a snap. The pinned classifier already encoded this exactly.
Named behaviour changes:
* The ack is now an OUTPUT of the committed route, fired strictly after the
canonical commit and exactly once per accepted force packet.
* The ForcePosition route no longer re-arms the constraint leash. The force
branch returns at 0x0045409D, ahead of all three ConstrainTo sites
(0x00454272, 0x0045418A, 0x004541EC); the old re-arm cited retail's "Player,
normal" branch, which BlipPlayer is not on. The teleport, CommitPreparedPosition
and first-entry callers legitimately still constrain and are untouched.
* A force correction that terminates WITHOUT committing still sends its
position event and is not retried — retail's BlipPlayer discards
SetPositionSimple's SetPositionError return and acks unconditionally.
A single _pending funnel owns the in-flight placement, deciding on the token's
PositionAuthorityVersion against the record's: equal -> clear; advanced with the
newest accepted event still a force -> re-issue, re-classified; advanced to an
ordinary Apply -> clear, since newer server truth owns that pose. This closes a
double-apply/double-ack and a silently-dropped correction that two earlier
iterations of this slice each introduced.
AD-62 records the residual: a ForcePosition our async collision publication
cannot carry to a committed placement is not re-applied. Retail has no park —
its world is fully resident and its placement synchronous — so the state is
unreachable there. AP-131 is NOT retired; its legacy Position caller is route 4.
Deleted: LocalForcePositionTransaction, PlayerMovementController.BlipPosition,
HeadlessSessionWorldProjection.BlipLocalPlayer.
Gates: complete Release solution 10,858 passed / 4 skipped / 0 failed (baseline
10,844/4/0). Two independent Opus reviews (retail-conformance and
architecture/adversarial) PASS on the final diff after three FAIL rounds; every
intermediate state was fully green, so the suite caught none of the four real
defects. Connected acceptance is NOT run: nothing a user can do makes ACE emit
a ForcePosition without retail's @pklite, which acdream does not implement — see
docs/research/2026-08-03-c4-route-2-visual-gate.md.
Known gap, recorded not claimed: the plan's acceptance item 2 is unmet. The App
double-write check is a source pin, and "the committed projection moves the
render entity" is uncovered at any layer (#292). Filed alongside: #286-#291,
#293-#296.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
479 lines
26 KiB
Markdown
479 lines
26 KiB
Markdown
# C4 route 2 — dual review findings and required fixes (2026-08-03)
|
|
|
|
Both mandated reviews returned **FAIL** on the first implementation pass.
|
|
Nothing is committed. This is the consolidated fix list; it supersedes the
|
|
implementer's own closing report where they disagree.
|
|
|
|
Reviews: retail-conformance (Opus) and architecture/adversarial (Opus), run
|
|
independently against the same uncommitted diff.
|
|
|
|
## Verified correct — do not churn these
|
|
|
|
Both reviews independently confirmed, with addresses:
|
|
|
|
- `AuthoritativeTeleportFlags` = `Teleport|Slide|SendPositionEvent` = `0x1012`,
|
|
byte-exact against `CPhysicsObj::SetPositionSimple` @0x005162B0.
|
|
- Leash re-arm removal is retail-correct: the FORCE_POSITION branch returns at
|
|
0x0045409D, strictly before all three `ConstrainTo` sites (0x00454272,
|
|
0x0045418A, 0x004541EC). The teleport / `CommitPreparedPosition` /
|
|
`ArmConstraintLeashAtCommittedPlacement` callers correctly still constrain.
|
|
- Heading preservation happens exactly once, upstream in
|
|
`InboundPhysicsStateController.ApplyAcceptedPosition:788-798`. The seam does
|
|
not re-apply or drop it.
|
|
- Ack ordering is correct and fires exactly once on both the synchronous and
|
|
the deferred path; the `CanSendPositionEvent` gate matches retail's
|
|
`CommandInterpreter::SendPositionEvent` @0x006B4770, and correctly does NOT
|
|
apply `ShouldSendPositionEvent`'s rate limit (retail's force branch calls
|
|
SendPositionEvent directly).
|
|
- Route-request field derivation matches the continuation executor field for
|
|
field.
|
|
- Contract items 2, 4 (the Runtime command itself), 6, and 7 pass. AP-131
|
|
correctly not retired.
|
|
- The re-modelled `PlayerMovementControllerTests` are relocations, not
|
|
weakenings.
|
|
|
|
## Required fixes, in priority order
|
|
|
|
### R1 — HIGH — the DeferredCell park cannot survive in production
|
|
|
|
`RuntimeEntityObjectLifetime.cs:1636` calls `Physics.SetPosition.Forget(canonical)`
|
|
on EVERY accepted Position, which unconditionally cancels the entity's in-flight
|
|
operation. ACE broadcasts at 5-10 Hz, so any park lasting longer than ~100-200 ms
|
|
is guaranteed to be cancelled before its collision generation commits — exactly
|
|
the far-destination case the deferred path exists to serve.
|
|
|
|
Chain: park -> cancelled -> `RetryDeferred` never runs -> no `Place` receipt ->
|
|
`ReconcileAndAcknowledge` never runs -> **the body is never moved and no ack is
|
|
ever sent.** The old `LocalForcePositionTransaction` / `BlipLocalPlayer` pair
|
|
applied the correction synchronously and unconditionally. Retail's `BlipPlayer`
|
|
@0x00453940 has no "give up quietly" state at all.
|
|
|
|
**Park-and-hope is not a valid mechanism here. Required direction:**
|
|
|
|
1. Do not open a park that can never wake. Before submitting, establish that the
|
|
destination's collision is publishable (R2), and mirror the existing
|
|
C3c-R1-F7 guard shape (`IHeadlessCollisionNeighborhood.IsWithinServiceWindow`,
|
|
`HeadlessSessionWorldProjection.cs:20-27`) rather than inventing a new one.
|
|
2. Where a park still legitimately occurs, the seam must detect that its
|
|
operation was cancelled — `RuntimeSetPositionState.IsPlacementCompletionTracked`
|
|
(`:1245`) is the existing read-only query — and **re-issue the placement from
|
|
the current canonical snapshot** on the next accepted Position or `Advance()`.
|
|
The snapshot already carries the latest accepted pose, so re-issuing is
|
|
correct, not a replay of stale state.
|
|
3. A force correction must never be silently dropped. That is the retail
|
|
invariant this route exists to preserve.
|
|
|
|
Do NOT resolve this with a timeout, a settle window, a retry counter, or by
|
|
exempting ForcePosition from `Forget`. If you conclude the correct answer is a
|
|
deliberate, cited divergence, STOP and report rather than shipping one.
|
|
|
|
### R2 — HIGH — headless lost its destination-collision publication
|
|
|
|
`HeadlessSessionWorldProjection.BlipLocalPlayer` (deleted) called
|
|
`_collision.CenterOn(position.LandblockId)`. The headless collision neighborhood
|
|
is a hard 3x3 window (`BuildPublicationPlan`, `:462-488`) moved ONLY by
|
|
`CenterOn`. The surviving callers are spawn (`:562`), the controller-null login
|
|
branch (`:603`), teleport prep (`:640`) and portal arrival (`:683`) — a
|
|
ForcePosition on a live local player now reaches none of them. `PumpFirstEntry`
|
|
(`:624`) polls the stale `_requestedLocalPlayerCell`, which nothing updates on
|
|
this path either.
|
|
|
|
Restore a real mechanism (re-center plus the `_requestedLocalPlayerCell`
|
|
update), or gate on `IsWithinServiceWindow` and handle out-of-window explicitly.
|
|
The deleted `CenterCount` assertion in `HeadlessSessionHostTests.cs:430` is the
|
|
invariant; restore it rather than the changed number.
|
|
|
|
The in-test justification ("retail's BlipPlayer has no streaming-window
|
|
concept") is true of retail and irrelevant: the window is OUR adaptation, and
|
|
retail has no equivalent because retail has every landblock resident.
|
|
|
|
### R3 — HIGH — login-window ForcePosition now does nothing at all
|
|
|
|
`RuntimeLiveEntitySessionController.cs:229-254`. Previously every accepted local
|
|
Position ran `_worldProjection.ProjectPosition`, whose controller-null branch
|
|
(`HeadlessSessionWorldProjection.cs:593-607`) set `_requestedLocalPlayerCell`,
|
|
called `CenterOn`, and pumped `_firstEntry.DriveAll()`. Now a ForcePosition
|
|
takes the new branch, the drive returns `NotApplicable` (residence active), and
|
|
nothing happens.
|
|
|
|
Restore the pump for the controller-absent case, and delete the comment at
|
|
`:239-240` claiming "there is no legacy fallback to run instead" — there was
|
|
one; it is the `else` branch this change routed around.
|
|
|
|
### R4 — MEDIUM-HIGH — the force-ack steals a receipt the sink declined
|
|
|
|
`RuntimeAcceptedPositionDriveController.cs:366-375` unconditionally calls
|
|
`AcknowledgeProjection(outcome.Projection)`. `RuntimePlacementProjectionSubscription`
|
|
deliberately leaves a declined `Place` at the FIFO head for a later retry
|
|
(`:118-121`); this consumes and destroys it.
|
|
|
|
The sink declines for real production reasons — `!_spatial.IsLoaded(landblock)`
|
|
(`LiveEntityRuntime.cs:1210-1219`) and stale transit authority
|
|
(`RuntimePlacementPresentationSink.cs:90-96`). In those cases `entity.SetPosition`
|
|
/ `Rotation` / `ParentCellId` / `RebucketLiveEntity` / `IsSpatiallyProjected`
|
|
are never written, and because the generic tail is now skipped there is no
|
|
second writer to cover it — the render entity silently stays put while the
|
|
canonical body moved.
|
|
|
|
The `RuntimeFirstEntryDriveController` mirror is safe ONLY because a residence
|
|
makes the sink decline by design and a follow-up `ExecutorCompleted` receipt
|
|
re-binds presentation. Route 2 has no such follow-up. Drop the force-ack and let
|
|
the subscription's retry contract stand, or provide a real follow-up binding.
|
|
|
|
### R5 — MEDIUM-HIGH — `Contention` and `Rejected` silently drop the correction
|
|
|
|
`RuntimeAcceptedPositionDriveController.cs:265-271` returns `Contention` when
|
|
`TryBeginExclusiveAuthoredPlacement` fails; neither status creates a pending
|
|
entry, and both hosts then return without blipping or acking
|
|
(`LiveEntityNetworkUpdateController.cs:1140`). The `Contention` doc comment
|
|
promises "a later accepted Position, or this controller's own Advance pump,
|
|
retries" — the pump provably cannot retry something never recorded, and a later
|
|
Position carries a different pose. Retail always applies.
|
|
|
|
The most likely trigger is R1's parked operation, which makes every subsequent
|
|
ForcePosition `Contention`. Fixing R1 largely fixes this; the status handling
|
|
must still not silently drop.
|
|
|
|
Also fix the `Rejected` enum doc: it claims "No SetPosition ran; no ack was
|
|
sent", which is false at the two `SubmitAndResolve` sites (`:361`, `:415`) where
|
|
a SetPosition ran and was cancelled.
|
|
|
|
### R6 — MEDIUM — `_pending` leaks and can be silently overwritten
|
|
|
|
`RuntimeAcceptedPositionDriveController.cs:289-324`. `Advance()` exits only on
|
|
record-key release or acknowledged completion. A mid-session cancellation
|
|
(supersession, lost-cell deadline, `ParkCollisionResidents`, generation cancel —
|
|
all route through `ForgetPlacementCompletionCore`) leaves `_pending` set
|
|
forever, so `AcceptedPositionDrivePendingCount` keeps `IsConverged` false for
|
|
the rest of the session. `GameWindowLifetime.DisposeGameRuntime:490-498` throws
|
|
on non-convergence.
|
|
|
|
Plan §4c required dropping the pending ack on cancellation, supersession,
|
|
teardown, generation change and reset. Only teardown and reset shipped. Use
|
|
`IsPlacementCompletionTracked`. Also: the `_pending = null` cleanups are all
|
|
guarded by `if (!firstAttempt)`, and `SubmitAndResolve(firstAttempt: true)`
|
|
assigns `_pending` without inspecting an existing one — make it refuse to
|
|
overwrite a live pending.
|
|
|
|
### R7 — MEDIUM — the 0.48 fixture, the changed assertion, and the false doc
|
|
|
|
**This is a test-fixture artifact, NOT a production regression.** The headless
|
|
fixture's `LoadedSetupCollisionSource` returns one sphere
|
|
`(Vector3.Zero, 0.48f)` — centre AT the origin, bottom 0.48 m below the feet.
|
|
The real human Setup `0x02000001` is `(0,0,0.475) r=0.48` plus
|
|
`(0,0,1.350) r=0.48` (`Ts46SphereListConformanceTests.cs:35-39`), so the foot
|
|
sphere's bottom is origin - 0.005 and a settled origin lands on the floor within
|
|
5 mm. The control is in this same changeset: the new Runtime fixture uses the
|
|
dummy sphere (offset == radius) and asserts the origin lands exactly on the
|
|
floor (`RuntimeAcceptedPositionDriveControllerTests.cs:178`).
|
|
|
|
Required:
|
|
1. Give the headless fixture the retail offset `(0f, 0f, 0.475f) r=0.48` and
|
|
**restore the `Z == 50f` assertion**. Changing an assertion to match new
|
|
output is the plan's own forbidden move.
|
|
2. Delete the false comment at `HeadlessSessionHostTests.cs:419-427` — it
|
|
describes the sphere CENTRE and then asserts it about `controller.Position`,
|
|
which is the ORIGIN (`PlayerMovementController.cs:304` -> `PhysicsBody.cs:153`,
|
|
retail `CPhysicsObj::m_position.frame.origin`).
|
|
3. Correct the `docs/ISSUES.md` #285 "retail fidelity gain" paragraph. Retail's
|
|
`BlipPlayer` has never lifted the origin by a sphere radius. Left as-is this
|
|
becomes the citation a future session trusts.
|
|
|
|
### R8 — MEDIUM — acceptance gaps
|
|
|
|
- No App-layer test exists proving the generic tail no longer double-writes the
|
|
local player and that the committed projection is what moves the render
|
|
entity. The plan's acceptance item 2 is unmet; three App tests were deleted
|
|
and replaced with a comment. Given R4, this is precisely the seam that is
|
|
broken.
|
|
- `RuntimeAcceptedPositionDriveControllerTests.cs:310-311` asserts
|
|
`acksAfterFirstResolve <= 1`, so the test **passes with zero acks** — the
|
|
DeferredCell park -> wake -> commit -> single-ack sequence is unverified,
|
|
while `docs/ISSUES.md` claims it is covered. Fix the fixture so the contact
|
|
gate is satisfied and assert exactly one, or state plainly that it is
|
|
unverified. Do not leave the overclaim in the record.
|
|
|
|
### R9 — LOW — hygiene
|
|
|
|
- `RuntimeAcceptedPositionDriveController` is `public sealed` with an internal
|
|
ctor and all-internal members; its template `RuntimeFirstEntryDriveController`
|
|
is `internal sealed`. Make it internal unless the public surface is genuinely
|
|
required (if it is, say why).
|
|
- `PlayerMovementController.cs:632` has a stale `<see cref="BlipPosition"/>` to
|
|
a deleted member. Harmless only while `GenerateDocumentationFile` is off;
|
|
`TreatWarningsAsErrors` is on, so it breaks the build the day docs are enabled.
|
|
- `HeadlessSessionHost._currentSession` is never cleared on teardown.
|
|
- Headless without a content lease leaves the drive controller null, so the
|
|
ForcePosition and its ack are dropped entirely
|
|
(`HeadlessSessionHost.cs:568-581`); previously the ack fired unconditionally.
|
|
- `LiveEntityNetworkUpdateController.cs:1140-1155` fires
|
|
`MarkLiveOwnerPoseDirty` and `ObserveAcceptedLocalPosition` for ANY non-
|
|
`NotApplicable` status including `Rejected` and `Contention` — moving the
|
|
streaming observer to a landblock we explicitly refused to place into.
|
|
|
|
## Gate
|
|
|
|
Unchanged: complete Release solution suite, not a focused subset. Pre-change
|
|
baseline is 10,844 / 4 skipped / 0 failed; the first pass reached 10,848 with
|
|
the defects above, so a green suite is necessary and demonstrably not
|
|
sufficient. Both reviews must be re-run on the fixed diff before commit.
|
|
|
|
---
|
|
|
|
# ROUND 2 — residuals after the R1-R9 fix round (2026-08-03)
|
|
|
|
Both delta reviews returned FAIL again. Suite is green at 10,853 / 4 / 0, which
|
|
again proves nothing. R2, R3, R4, R5, R6, R7 and R9 are confirmed genuinely
|
|
fixed and must not be churned. Three blocking residuals remain, and **two of
|
|
them are defects in the R1 reissue mechanism itself.**
|
|
|
|
**Root of the problem: `_pending` has no single owner and no single lifecycle
|
|
rule.** Round 1 bolted reissue onto ad-hoc per-branch bookkeeping. B1 wants MORE
|
|
reissuing, N1 wants LESS, and N2 wants reissue to be a DIFFERENT route — they
|
|
look contradictory only because there is no unifying rule. There is one.
|
|
|
|
## The unified mechanism (implement exactly this — it replaces the ad-hoc rules)
|
|
|
|
`RuntimeEntityPlacementToken` already carries `PositionAuthorityVersion`
|
|
(`RuntimeSetPositionState.cs:50`). Make that the single decision input.
|
|
|
|
**One rule:** the drive owns at most one in-flight placement for the local
|
|
player. After any terminal outcome, and on every `Advance()`, compare the
|
|
committed/parked token's `PositionAuthorityVersion` against the live record's
|
|
current `PositionAuthorityVersion`:
|
|
|
|
- **Equal** — the canonical accepted authority has not moved since this
|
|
operation began. Nothing is outstanding. Clear `_pending`. Do not reissue.
|
|
- **Advanced** — a newer accepted Position arrived while we were in flight, and
|
|
it may have been the thing that killed our operation. Consult the newest
|
|
accepted event's disposition (do NOT reuse `stale.Route`):
|
|
- still **ForcePosition** — reissue, re-classifying from the current record.
|
|
- now an ordinary **Apply** — clear `_pending` and do NOT reissue. The
|
|
correction was superseded by newer server truth; the ordinary route owns
|
|
that pose. This is not a silent drop: retail applies each event as it
|
|
arrives, and a force correction overtaken by a newer position is moot.
|
|
|
|
Route every terminal branch through one `_pending` funnel. No branch may assign
|
|
or clear it directly.
|
|
|
|
### B1 — BLOCKING — a force correction is still silently dropped (conformance)
|
|
|
|
When a parked operation wakes and its `Place` is ACCEPTED by the sink, the
|
|
operation leaves `_operations` but the completion is retained. `CancelCoreDeferred`
|
|
then returns early at `RuntimeSetPositionState.cs:5141` without reaching
|
|
`ForgetPlacementCompletionCore`, so the retained completion survives. The next
|
|
ForcePosition hits `HasRetainedCompletion` (`:1319`) -> invalid token ->
|
|
`Contention` (`RuntimeAcceptedPositionDriveController.cs:289-295`); both hosts
|
|
return without placing or acking, and the next `Advance()` consumes the OLD
|
|
completion and acks the OLD pose. That packet's correction is lost.
|
|
|
|
One-frame window on the graphical host only (`_session.Tick()` inbound dispatch
|
|
precedes `RetryPending()` in `RetailLiveFrameCoordinator`); headless is immune
|
|
because its pump is adjacent to the readiness check. The DECLINED-`Place`
|
|
variant is unaffected and needs no change.
|
|
|
|
The unified rule fixes this: the new packet advanced `PositionAuthorityVersion`
|
|
past the committed token, and the newest event is a ForcePosition, so it
|
|
reissues.
|
|
|
|
### N1 — BLOCKING — stale `_pending` causes a second placement AND a second ack
|
|
|
|
`SubmitAndResolve(firstAttempt: true)` (`:297`) never inspects `_pending`, and
|
|
only the `!firstAttempt` branches clear it (`:508-511`, `:482-484`, `:549-551`).
|
|
So: park -> `Forget` wipes the watch -> the same packet's ForcePosition Begins
|
|
cleanly and Commits -> ack fires -> `_pending` still holds the dead P1 -> next
|
|
`Advance()` finds the watch dead -> `ReissueFromCanonical` -> **a second
|
|
canonical placement and a second outbound `AutonomousPosition` for one server
|
|
correction.**
|
|
|
|
That is the double-apply/double-ack class this entire slice exists to delete
|
|
(`670f307c`), reintroduced. `AssignPending` does not catch it because it is only
|
|
reached on the DeferredCell/retryable branches. The unified funnel fixes it:
|
|
equal versions -> clear, no reissue.
|
|
|
|
### N2 — BLOCKING — reissue applies force semantics to an ordinary pose
|
|
|
|
`ReissueFromCanonical` (`:418-447`) reuses `stale.Route` verbatim —
|
|
`SetPositionSimple` + `Teleport|Slide|SendPositionEvent` +
|
|
`SendPositionImmediately: true`. But the commonest way a park dies is an
|
|
ordinary `Apply` Position, whose retail route
|
|
(`RuntimeAuthoritativePositionRouteClassifier.cs:368-388`) is
|
|
`Interpolate`/`NoPositionOperation`, `PhysicsSetPositionFlags.None`,
|
|
`ConstrainPhase.BeforePositionOperation`, `SendPositionImmediately: false`.
|
|
|
|
So the reissue converts an ordinary server echo into a hard `Teleport|Slide`
|
|
canonical placement, sends an ack retail would never send on that branch, and
|
|
skips the `ConstrainTo` the ordinary branch runs. My round-1 direction sanctioned
|
|
re-issuing THE CORRECTION; it did not sanction re-classifying a different
|
|
disposition's pose as a force. The unified rule fixes this by consulting the
|
|
newest accepted disposition.
|
|
|
|
If any residual divergence remains after this, it needs a
|
|
`docs/architecture/retail-divergence-register.md` row in the same commit.
|
|
|
|
### B2 — BLOCKING (record accuracy) — the plan claims coverage it does not have
|
|
|
|
`docs/plans/2026-08-02-placement-cutover.md:287` reads "R8 added the App-layer
|
|
double-write source pins the plan's own acceptance item required." That is a
|
|
claim of coverage. The truth, per the adversarial review:
|
|
|
|
- acceptance item 2's first half is **source-pinned, not proven** — the
|
|
`Assert.Single` regex would still pass if a second write were spelled
|
|
differently, and no test exercises the branch;
|
|
- acceptance item 2's second half — **"the committed projection is what moves
|
|
the render entity"** — is **uncovered at any layer**. No test drives a route-2
|
|
ForcePosition through `RuntimePlacementPresentationSink` /
|
|
`TryApplyRuntimePlacementPlace` and asserts the `WorldEntity` moved.
|
|
|
|
Correct the text to record the gap explicitly. A documented gap is acceptable;
|
|
a false claim of coverage is not. Same rule that produced R7.
|
|
|
|
## Non-blocking — record, do not fix in this round
|
|
|
|
- **N3** — headless never calls `RetryPending` after construction (grep finds no
|
|
caller outside `src/AcDream.App/`). R4's fix depends on the subscription's
|
|
retry, so a declined headless `Place` would wedge the ordered stream. Latent,
|
|
not proven reachable. File it.
|
|
- **N4** — `Advance()` lacks the `_driving` reentrancy latch its template
|
|
`RuntimeFirstEntryDriveController.DriveAll:128-148` has. No live re-entrant
|
|
path today. Hygiene.
|
|
- **N5** — `CenterOnAcceptedForcePosition` does not restore
|
|
`controller.LocalEntityId = record.LocalEntityId ?? 0u` (inert today, but an
|
|
unreplaced deletion); `_movementTruthDiagnostics.OnServerEcho` no longer fires
|
|
for a local ForcePosition (diagnostic only).
|
|
- **R9 residue** — `ConstraintManager.cs:25` and `PhysicsBody.cs:442` still cite
|
|
the deleted `BlipPosition` in `<c>` tags (build-safe, but false docs).
|
|
- **Route-1 ack** — the plan required confirming whether the ack fires while an
|
|
initial-Create residence owns the record. It does not; `SendPositionImmediately`
|
|
is consumed only as a trace fact in the continuation executor (`:717`, `:2576`).
|
|
Not a regression, but the plan said file it. File it.
|
|
- **AD register row** — the headless 3x3 collision window is now a named member
|
|
of the Runtime-facing `IRuntimeDirectWorldProjection` contract with an ordering
|
|
requirement retail has no analogue for, and no existing row covers it (AD-6 is
|
|
retired; AD-2 is the graphical reveal barrier). Recommend a row.
|
|
- **Stale comment** — the `HeadlessSessionHostTests` comment references "the
|
|
previous 50.48f assertion", which does not exist at HEAD.
|
|
|
|
---
|
|
|
|
# ROUND 3 — final round (2026-08-03)
|
|
|
|
The round-3 **adversarial** review returned **PASS**. The round-3
|
|
**conformance** review returned **FAIL on one item**. This round closes that
|
|
item and files the adversarial review's non-blocking findings. The round-2
|
|
unified `_pending` funnel was confirmed sound by both reviewers and was NOT
|
|
restructured.
|
|
|
|
## The blocker — a terminal-without-commit sent no ack (CLOSED)
|
|
|
|
`SettlePending`'s Equal branch was reached both by a successful commit and by a
|
|
terminal outcome that never committed (non-retryable prepare failure, the
|
|
`default:` Rejected/Cancelled branch, and both `Advance` death branches). In the
|
|
non-commit case the body never moved AND no outbound `AutonomousPosition` left.
|
|
|
|
The conformance reviewer proposed a `committed` flag plus a
|
|
one-re-issue-per-`Advance` guard. That is more than retail requires, and the
|
|
retail evidence was re-verified from
|
|
`docs/research/named-retail/acclient_2013_pseudo_c.txt` before coding:
|
|
|
|
- `SmartBox::BlipPlayer` @0x00453940 (line 92528) calls
|
|
`CPhysicsObj::SetPositionSimple(this->player, edi_1, 1)` @0x00453968 and
|
|
**discards its return value**. `BlipPlayer` itself returns `void`.
|
|
- `CPhysicsObj::SetPositionSimple` @0x005162B0 (line 284276) is declared
|
|
`enum SetPositionError __thiscall`. Other retail call sites DO test it —
|
|
`if (CPhysicsObj::SetPositionSimple(...) == OK_SPE)` at @0x0055605D and
|
|
@0x00556021 — which proves the discard in `BlipPlayer` is deliberate, not a
|
|
decompiler artifact.
|
|
- `SmartBox::HandleReceivedPosition` @0x00453FD0's FORCE_POSITION branch calls
|
|
`SmartBox::BlipPlayer` @0x00454074, stamps `update_times[0]` @0x00454079, then
|
|
runs `cmdinterp->SendPositionEvent()` @0x00454091 **unconditionally** and
|
|
returns @0x0045409D.
|
|
|
|
Retail's semantics are therefore: **attempt the placement once; if it fails the
|
|
body simply does not move; acknowledge regardless; never retry.** No commit flag
|
|
and no recursion guard are needed to express that.
|
|
|
|
**Implemented.** The ack is now sent exactly once per BEGUN placement, at its
|
|
terminal outcome — from `ReconcileAndAcknowledge` on the commit path, or from
|
|
`SettlePending` on a non-commit terminal. `SettlePending` gained a
|
|
`positionEventOwed` parameter; `Pending` gained a `PositionEventOwed` field so
|
|
the re-issue retry marker (which stands for a packet whose placement was never
|
|
begun) cannot double-ack. The commit paths pass `false` because their ack has
|
|
already left. The non-commit ack runs no reconciliation — the body did not move,
|
|
so there is no committed frame to reconcile — and therefore carries the body's
|
|
unchanged pose, which is exactly what retail's ack carries after a failed
|
|
`SetPositionSimple` and is informative to the server: its force did not take.
|
|
The `CanSendPositionEvent` gate was left untouched; it is retail's own
|
|
(`CommandInterpreter::SendPositionEvent` @0x006B4770 tests the transient-state
|
|
contact bits), so a legitimately airborne body still suppresses the send on both
|
|
paths.
|
|
|
|
`ReconcileAndAcknowledge` was split so both paths share one outbound site,
|
|
`SendPositionEvent`.
|
|
|
|
## Other items closed this round
|
|
|
|
- **AD-62 rewritten.** The `DeferredCell`-park precondition is dropped — it was
|
|
never required, and the never-parked failure paths reach the same outcome. The
|
|
row now leads with the general rule and keeps the named shapes as examples,
|
|
adds the externally-blocked `Contention` shape (nothing recorded, nothing
|
|
pumps it) and the other `PositionAuthorityVersion` advances (`TryApplyPickup`
|
|
`RuntimeEntityObjectLifetime.cs:1116`, `CommitPositionChannelUpdate` `:2041`,
|
|
`AdvanceCreateAuthority` `:2466`), and separates the shapes that now lose only
|
|
the re-apply from the narrower shapes that still lose the ack too.
|
|
- **Overstated doc corrected.** `AcceptedForceObservation`'s comment claimed the
|
|
record's current version equals the recorded force's version *if and only if*
|
|
the newest accepted event was that force. False — `AdvancePositionAuthority`
|
|
has four call sites. It is now stated as the one-way test it actually is.
|
|
- **Vacuous assertion deleted.** The `gameActions.Count <= 2` assertion in
|
|
`Advanced_ReissuesWhenTheNewestAcceptedEventIsStillAForcePosition` could not
|
|
fail: that fixture's `CommitLandblockCollision` adds both landblocks at
|
|
`worldOffsetX/Y: 0f` while the world frame places the deferred landblock at
|
|
+192/+192, so the body lands over no terrain, `InContact` is false, and every
|
|
ack is suppressed — the count is 0. It was also too loose to encode "at most
|
|
one per packet". Deleted rather than shipped; the test's real discriminators
|
|
(body position, `PendingCount`) stay.
|
|
|
|
## Tests
|
|
|
|
Two added, both with a verified discrimination check (the implementation was
|
|
temporarily broken in each direction and the intended test observed to fail,
|
|
then reverted and re-verified):
|
|
|
|
- `TerminalWithoutCommit_SendsExactlyOnePositionEventAndLeavesTheBodyUnmoved` —
|
|
a park retired without committing sends exactly one `AutonomousPosition`,
|
|
performs no placement of its own, and never repeats on further pumps.
|
|
- `Committed_SendsExactlyOnePositionEventAcrossTheCommitAndTheSettle` — the new
|
|
settle-side ack does not become a second ack on the committed path.
|
|
|
|
Two existing tests changed their ack expectation from `Assert.Empty` to
|
|
`Assert.Single`, because they exercise terminal-without-commit paths whose
|
|
`Empty` encoded the defect this round removes:
|
|
`Equal_ClearsPendingWithoutReissuingWhenNoNewerAcceptedAuthorityArrived` and
|
|
`Advanced_DoesNotReissueWhenTheNewestAcceptedEventIsAnOrdinaryApply`. In the
|
|
latter the single ack belongs to the FORCE packet, not to the ordinary echo —
|
|
retail's ordinary branch has no unconditional `SendPositionEvent`.
|
|
|
|
Measurement note, recorded because it corrects an assumption in this file's
|
|
round-2 text: the `DeferredCell` park these fixtures use is the POST-engine
|
|
quiescence park, so `_physics.Engine.SetPosition` has already moved the
|
|
canonical body to the destination while the placement itself is withdrawn and
|
|
parked. The new test therefore captures its unmoved/no-further-placement
|
|
baselines at the park, and asserts them across the terminal settle, which is the
|
|
thing under test.
|
|
|
|
## Filed, not fixed
|
|
|
|
`docs/ISSUES.md` #293 (the `DeferredCell` branch still consumes `Withdraw`
|
|
receipts the sink may have declined — the same shape R4 removed for `Place`),
|
|
#294 (`ReconcileAndAcknowledge` runs before the funnel's currency guard on the
|
|
deferred wake), #295 (the retry marker inflates
|
|
`AcceptedPositionDrivePendingCount`, which is an in-flight-placement counter),
|
|
#296 (a retryable prepare is reported to hosts as `Contention`, conflating a
|
|
retained-and-pumping case with a dropped one).
|
|
|
|
## Gate
|
|
|
|
Complete Release solution suite, unchanged discipline: green is necessary and
|
|
demonstrably not sufficient — all four prior states were green and three were
|
|
defective.
|