Skip to content

fix: held accessor views fail loud in dev — making the docs' fail-loud claim true#53

Merged
andymai merged 1 commit into
mainfrom
fix/stale-view-guard
Jun 6, 2026
Merged

fix: held accessor views fail loud in dev — making the docs' fail-loud claim true#53
andymai merged 1 commit into
mainfrom
fix/stale-view-guard

Conversation

@andymai
Copy link
Copy Markdown
Owner

@andymai andymai commented Jun 6, 2026

The gap (audit finding #4, 2026-06)

core-concepts.md promises pooled-ref misuse "fails loud" — but only the EntityRef-level resolve was guarded. The object read()/write() returns is itself the pooled per-(archetype, component) singleton, and a held view silently re-pointed two ways:

  1. a later world.entity(b).read(C) re-pokes the singleton → the held view reads b's data
  2. the entity dies/migrates and a swap-pop hands its row to a new tenant without a re-poke (__eid still matches; the data underneath changed)

The fix

Dev-mode (IS_DEV) per-resolve Proxy membrane in storage.#resolve, stamped with (handle, boundRow). Every property get/set re-validates the singleton's binding and the row's current tenant (arch.rows[boundRow] === handle, read lazily — the rows view is re-published on growth), throwing an actionable stale accessor view error.

  • Lenient resolves exempt: observer-window reads of dying entities thread __lenient through AccessorResolver.resolveRead/Write(…, lenient) — their rows are expected to be re-tenanted mid-window.
  • Reflect uses target as receiver so prototype getters/setters keep the singleton as this — cached VecViews stay singleton-bound. (Known limit: a held vec element view bypasses the membrane; the .vec access itself is covered.)
  • Production: bare singleton, zero allocation. Query-iteration cursors never pass through the guarded path — the hot path is untouched.
  • Website core-concepts.md updated to state the guarantee precisely (dev guards the view; fix what dev flags before shipping).

Also in this PR

load(bytes, mode) now throws on an unknown mode — a JS caller passing e.g. 'overwrite' previously fell through every mode === 'replace' branch and silently merged on a load-a-save path.

Tests

  • held read view throws after re-point (pre-guard: returned the other entity's value)
  • held write view throws (pre-guard: wrote to the other entity); target verified untouched
  • swap-pop re-tenanting without re-poke throws (the __eid-matches case)
  • same-entity re-resolve keeps older views valid; different-component views coexist
  • lenient observer-window reads of dying entities still work
  • load(bytes, 'overwrite') throws unknown load mode

pnpm test 1210 passed · typecheck:tests clean · docs:check 40/40 · spec §6.5 (accessors.md) updated locally

… is now true

The object read()/write() returns is the pooled per-(archetype,
component) singleton. A held view silently re-pointed when a later
world.entity() resolve re-poked it at another entity, or when a
swap-pop handed its row to a new tenant — core-concepts.md claimed
this misuse 'fails loud' but only the EntityRef-level resolve was
guarded.

In dev mode (NODE_ENV !== 'production') each random-access resolve now
returns a per-call Proxy membrane stamped with (handle, row); every
property access re-validates both the singleton's binding and the
row's current tenant, throwing an actionable error instead of reading
another entity's data. Lenient (observer-window) resolves are exempt —
their rows are expected to be re-tenanted mid-window. Production
returns the bare singleton: zero allocation. Query iteration never
passes through the guarded path.

Also: load(bytes, mode) throws on an unknown mode instead of silently
treating it as merge (a JS caller passing 'overwrite' fell through
every replace branch).
@andymai andymai enabled auto-merge (squash) June 6, 2026 08:14
@andymai andymai merged commit de3dac3 into main Jun 6, 2026
8 checks passed
@andymai andymai deleted the fix/stale-view-guard branch June 6, 2026 08:16
@release-kun release-kun Bot mentioned this pull request Jun 6, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Jun 6, 2026

Greptile Summary

This PR closes two silent-failure gaps: it adds a dev-mode Proxy membrane in storage.#resolve that stamps each returned accessor view with its (handle, row) binding and re-validates on every property access, and it makes load() throw loudly on an unrecognised mode string instead of falling through to a silent merge.

  • guardAccessorView in storage.ts wraps the pooled accessor singleton in a per-resolve Proxy; fresh() checks both a.__eid (re-point guard) and arch.rows[boundRow] (swap-pop/re-tenanting guard), passing arch = null for cold entities to rely solely on the __eid check. Lenient bindings (observer-window reads of dying entities) thread __lenient from EntityRef through resolveRead/Write to skip the guard.
  • deserialize.ts gains an upfront mode !== 'replace' && mode !== 'merge' guard before any cursor work, preventing a JS caller with a typo'd mode from silently merging instead of replacing.

Confidence Score: 4/5

The production path is untouched; the change adds a dev-only Proxy guard and a serialisation mode check. Safe to merge with the noted dev-guard gap understood.

The dev-mode guard's fresh() check correctly catches re-pointing and swap-pop re-tenanting, but silently passes when the entity being despawned or migrating is the last occupant of its archetype — arch.rows[row] is never cleared in that path, so both halves of the condition remain true. A held write view in that window can write into a freed row without throwing.

packages/core/src/storage/storage.ts — specifically the fresh() predicate inside guardAccessorView and the corresponding last-entity despawn/migration path in removeRow.

Important Files Changed

Filename Overview
packages/core/src/storage/storage.ts Adds the dev-mode Proxy membrane via guardAccessorView; guard has a coverage gap for the last-entity-in-archetype case where arch.rows[row] is not zeroed on removal.
packages/core/src/entity/ref.ts Threads this.__lenient through to resolveRead/resolveWrite so observer-window calls skip the new guard; straightforward and correct.
packages/core/test/p05-stale-ref-guard.test.ts Comprehensive suite covering re-point, write-side stale, swap-pop, same-entity re-resolve, cross-component coexistence, and lenient observer path; last-entity-in-archetype despawn is not tested.
packages/serialization/src/deserialize.ts Adds an explicit mode-validity check before processing, correctly preventing silent fall-through to merge for unknown mode strings.
packages/serialization/test/m10-validation-edges.unit.test.ts Adds a test for the unknown-mode rejection; setup and assertion are correct.
website/guide/core-concepts.md Documentation updated to accurately describe the dev-mode field-view guard and production trade-off.

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
packages/core/src/storage/storage.ts:281-282
**Stale-guard miss: last-entity despawn/migration not detected**

`removeRow` only updates `arch.rows[row]` during a swap-pop (when `row !== last`). When the entity being removed IS the last occupant of its archetype, `arch.count` is simply decremented and `arch.rows[row]` is left holding the departing handle. Because neither condition in `fresh()` changes — `a.__eid` is not re-pointed, and `arch.rows[row]` still matches `expectedEid >>> 0` — the proxy appears fresh after the entity is gone. A held write view accessed in this window would silently write into a now-freed row, potentially corrupting data for the next entity allocated there. The test suite only covers the swap-pop path (entity is not last); a test with `world.despawn(only_entity)` would repro this miss.

Reviews (1): Last reviewed commit: "fix: held accessor views fail loud in de..." | Re-trigger Greptile

Comment on lines +281 to +282
const fresh = (): boolean =>
(a.__eid as number) === expectedEid && (arch === null || arch.rows[row] === (expectedEid >>> 0))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Stale-guard miss: last-entity despawn/migration not detected

removeRow only updates arch.rows[row] during a swap-pop (when row !== last). When the entity being removed IS the last occupant of its archetype, arch.count is simply decremented and arch.rows[row] is left holding the departing handle. Because neither condition in fresh() changes — a.__eid is not re-pointed, and arch.rows[row] still matches expectedEid >>> 0 — the proxy appears fresh after the entity is gone. A held write view accessed in this window would silently write into a now-freed row, potentially corrupting data for the next entity allocated there. The test suite only covers the swap-pop path (entity is not last); a test with world.despawn(only_entity) would repro this miss.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/storage/storage.ts
Line: 281-282

Comment:
**Stale-guard miss: last-entity despawn/migration not detected**

`removeRow` only updates `arch.rows[row]` during a swap-pop (when `row !== last`). When the entity being removed IS the last occupant of its archetype, `arch.count` is simply decremented and `arch.rows[row]` is left holding the departing handle. Because neither condition in `fresh()` changes — `a.__eid` is not re-pointed, and `arch.rows[row]` still matches `expectedEid >>> 0` — the proxy appears fresh after the entity is gone. A held write view accessed in this window would silently write into a now-freed row, potentially corrupting data for the next entity allocated there. The test suite only covers the swap-pop path (entity is not last); a test with `world.despawn(only_entity)` would repro this miss.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant