QA sweep: three silent leaks, and cut the overzoom raster cost in half #37

New issue

Closed

opened 2026-08-10 19:24:09 +00:00 by jeroen · 0 comments

jeroen commented

2026-08-10 19:24:09 +00:00

Owner

Problem

A four-agent QA + efficiency sweep of viberfox (three read-only lanes plus the
simulator lane) found defects and waste across the simulator, the tile pipeline
and the client's per-frame path. The simulator had never been touched by any of
the 18 prior perf commits.

The sweep also confirmed the codebase is in good shape: all 7 invariants
documented in CLAUDE.md's "Known issues / workarounds" still hold in code —
the egui panel .chain(), matching shadow cascades, derived PROTOCOL_VERSION,
avatar ground height across its three sites, fall/climb rates across three,
text_edit_focused keyboard gating, and the Android cfg seams.

Approach

Landed on this branch, 7 commits. Every behavioural fix has a test that fails
without it (verified for the avatar leak by reverting the fix).

crates/simulator/src/net.rs:194 — 23 bare ? in the incoming-frame arm
returned out of handle_connection, jumping over the teardown. One malformed
frame from an authenticated client leaked its avatar, which state.rs:218
then broadcast to every client at the tick rate for the life of the process.
crates/viberfox/src/platform/stream.rs:224 — forget dropped a key from
requested but left it in the dispatch queue, so a re-request enqueued a
second copy. Both dispatched, one released; the concurrency cap shrank
permanently. Fatal where the cap is 1 (far buildings vs. public Overpass):
the lane dies silently and busy() never quiesces.
crates/geo/src/vector_tiles.rs:441 — the overzoom loop built and stroked
every feature of the source tile regardless of the sub-tile window. Measured
on the Groningen fixture: z17 84.9 -> 41.0 ms/tile, z16 123.1 -> 78.5 ms.
179 of a fresh anchor's 424 requests are overzoomed.
crates/geo/src/routing.rs:1686 — corridor_tiles_at walked the full
bounding box (~3e7 hypot iterations for an intercontinental pair) before
refusing, contradicting its own doc comment.
crates/viberfox/src/lib.rs:3100 — the rain shell drew a full-viewport
blended pass with three noise octaves per fragment when dry.
crates/viberfox/src/systems/player/avatar.rs:717 — two allocations per
avatar per frame before the early-out that skips single-holder avatars.

Notes updated in place: tile-raster-cost.md (the overzoom cause + measurement)
and wasm-performance.md, whose check: line had become a false green —
it matched only the comment saying the file was no longer a Value walk.

Acceptance criteria

cargo test green: 273 viberfox, 94 geo, 19 core, 16 simulator, 14 big_space
cargo check --workspace --all-targets passes
cargo fmt --check passes across the five owned crates
CI green on the pushed head
cargo shots on a workstation confirms rain still renders at --rain 1

Verification

cargo check --workspace --all-targets, cargo test -p viberfox_geo,
cargo test -p viberfox, cargo test -p viberfox_simulator,
cargo test -p big_space --lib.

The overzoom cull's gate is culling_off_screen_features_changes_no_pixels,
which renders seven sub-tiles across factors 2/4/8 with the cull on and off and
asserts byte-identical rasters — that is what pins the stroke-reach margin.
bench_overzoom_cull (ignored) reproduces the timing table.

Needs a machine with a GPU: the rain change is visually unverified. This
container has no Vulkan driver, so --rain 1 cannot be rendered here. The logic
mirrors update_clouds in the same file exactly.

Out of scope — follow-up tickets

SimWorld::observer is one global field, last-writer-wins across all
connections (state.rs:34, net.rs:214). A second client 500 m away empties
the first's AoI; a NaN position empties it for everyone.
The per-tick snapshot clones and re-encodes regions and prims the client
discards after the first snapshot (network.rs:374). Same restructure as the
observer fix; changes wire semantics, so it needs a PROTOCOL_HISTORY row.
Auth hardening: any account may DeletePrim any prim (net.rs:225), session
tokens have no TTL/revocation and never re-check disabled (auth.rs:102),
and the pre-auth hello read has no timeout (net.rs:89).
platform/workers.rs:149 — PAUSED/PARKED are checked and pushed
non-atomically, which can strand a job for a whole --shot run.

Branch: perf/37-qa-sweep-leaks-and-overzoom

## Problem A four-agent QA + efficiency sweep of viberfox (three read-only lanes plus the simulator lane) found defects and waste across the simulator, the tile pipeline and the client's per-frame path. The simulator had never been touched by any of the 18 prior `perf` commits. The sweep also confirmed the codebase is in good shape: all 7 invariants documented in CLAUDE.md's "Known issues / workarounds" still hold in code — the egui panel `.chain()`, matching shadow cascades, derived `PROTOCOL_VERSION`, avatar ground height across its three sites, fall/climb rates across three, `text_edit_focused` keyboard gating, and the Android cfg seams. ## Approach Landed on this branch, 7 commits. Every behavioural fix has a test that fails without it (verified for the avatar leak by reverting the fix). - `crates/simulator/src/net.rs:194` — 23 bare `?` in the incoming-frame arm returned out of `handle_connection`, jumping over the teardown. One malformed frame from an authenticated client leaked its avatar, which `state.rs:218` then broadcast to every client at the tick rate for the life of the process. - `crates/viberfox/src/platform/stream.rs:224` — `forget` dropped a key from `requested` but left it in the dispatch queue, so a re-request enqueued a second copy. Both dispatched, one released; the concurrency cap shrank permanently. Fatal where the cap is 1 (far buildings vs. public Overpass): the lane dies silently and `busy()` never quiesces. - `crates/geo/src/vector_tiles.rs:441` — the overzoom loop built and stroked every feature of the source tile regardless of the sub-tile window. Measured on the Groningen fixture: z17 84.9 -> 41.0 ms/tile, z16 123.1 -> 78.5 ms. 179 of a fresh anchor's 424 requests are overzoomed. - `crates/geo/src/routing.rs:1686` — `corridor_tiles_at` walked the full bounding box (~3e7 `hypot` iterations for an intercontinental pair) before refusing, contradicting its own doc comment. - `crates/viberfox/src/lib.rs:3100` — the rain shell drew a full-viewport blended pass with three noise octaves per fragment when dry. - `crates/viberfox/src/systems/player/avatar.rs:717` — two allocations per avatar per frame before the early-out that skips single-holder avatars. Notes updated in place: `tile-raster-cost.md` (the overzoom cause + measurement) and `wasm-performance.md`, whose `check:` line had become a **false green** — it matched only the comment saying the file was no longer a `Value` walk. ## Acceptance criteria - [x] `cargo test` green: 273 viberfox, 94 geo, 19 core, 16 simulator, 14 big_space - [x] `cargo check --workspace --all-targets` passes - [x] `cargo fmt --check` passes across the five owned crates - [ ] CI green on the pushed head - [ ] `cargo shots` on a workstation confirms rain still renders at `--rain 1` ## Verification `cargo check --workspace --all-targets`, `cargo test -p viberfox_geo`, `cargo test -p viberfox`, `cargo test -p viberfox_simulator`, `cargo test -p big_space --lib`. The overzoom cull's gate is `culling_off_screen_features_changes_no_pixels`, which renders seven sub-tiles across factors 2/4/8 with the cull on and off and asserts byte-identical rasters — that is what pins the stroke-reach margin. `bench_overzoom_cull` (ignored) reproduces the timing table. **Needs a machine with a GPU:** the rain change is visually unverified. This container has no Vulkan driver, so `--rain 1` cannot be rendered here. The logic mirrors `update_clouds` in the same file exactly. ## Out of scope — follow-up tickets - `SimWorld::observer` is one global field, last-writer-wins across all connections (`state.rs:34`, `net.rs:214`). A second client 500 m away empties the first's AoI; a NaN position empties it for everyone. - The per-tick snapshot clones and re-encodes regions and prims the client discards after the first snapshot (`network.rs:374`). Same restructure as the observer fix; changes wire semantics, so it needs a `PROTOCOL_HISTORY` row. - Auth hardening: any account may `DeletePrim` any prim (`net.rs:225`), session tokens have no TTL/revocation and never re-check `disabled` (`auth.rs:102`), and the pre-auth hello read has no timeout (`net.rs:89`). - `platform/workers.rs:149` — `PAUSED`/`PARKED` are checked and pushed non-atomically, which can strand a job for a whole `--shot` run. --- Branch: `perf/37-qa-sweep-leaks-and-overzoom`

jeroen added the

driven

label

2026-08-10 19:24:09 +00:00