fspy: shm frame reader panics parsing traced accesses (shm_io.rs:328)

[wan9chi]

fspy: shm frame reader panics parsing traced accesses (shm_io.rs:328)

vt run crashes with a slice-bounds panic while collecting fspy path accesses
after the task itself completed successfully. The shared-memory frame
iterator reads file-path bytes as a frame header and tries to skip ~1.9 GB
forward.

Pre-existing bug: reproduced at branch HEAD (a8920e66,
agent/real-package-read-write-overlap-tests) with no local changes. Not
caused by, and not fixed by, the ignore-report removal work.

Panic

thread 'main' panicked at crates/fspy_shared/src/ipc/channel/shm_io.rs:328:47:
range start index 1919954688 out of range for slice of length 102756

The panic site is the 1.. ("partially written frame — skip it") arm of
ShmReader::iter_frames:

1.. => {
    // Partially written frame - skip it and continue
    let size = usize::try_from(frame_header).unwrap();
    remaining_content = &remaining_content[roundup_to_align_frame_header(size)..]; // <- panics
}

Trimmed backtrace (debug build, macOS arm64):

10: core::slice::index::slice_index_fail
12: fspy_shared::ipc::channel::shm_io::ShmReader<&[u8]>::iter_frames::{closure}
14: fspy::ipc::OwnedReceiverLockGuard::iter_path_accesses::{closure}
17: <Chain<...> as Iterator>::next            (PathAccessIterable::iter)
18: vite_task::session::execute::tracked_accesses::TrackedPathAccesses::from_raw
21: vite_task::session::execute::cache_update::observe_fspy
22: vite_task::session::execute::cache_update::update_cache
25: vite_task::session::execute::execute_spawn

So the child ran to completion; the crash happens in the post-run cache-update
phase, on the receiver side, while iterating shm frames.

Decoding the numbers

• 1919954688 = 0x72702F00. As the little-endian i32 frame header, the four
  bytes at the header position are 00 2F 70 72 = "\0/pr" — a NUL byte
  followed by the beginning of an absolute path (/private/tmp/..., the
  staging directory). The reader is interpreting frame content (a path
  string) as a frame header, meaning reader position and writer layout have
  diverged at some earlier point in the buffer.
• The slice length (102740 / 102756 / 104368 across observed panics) is
  the claimed end offset read from the shm header — roughly 100 KiB used.
  SHM_CAPACITY is 4 GiB sparse (crates/fspy/src/ipc.rs:12), so this is not
  capacity exhaustion.
• Every observed panic had the same bogus header value — when the failure
  hits, the buffer layout is identical run to run (same workload, same paths).

Reproduction

Environment: macOS Darwin 25.5.0 (arm64), debug build, upstream vite 8.1.3
from packages/tools (vite-task-tools>vite override), Node via
packages/tools/node_modules/.bin.

# 1) Stage the vite fixture under a LONG absolute path (~95+ chars).
#    Short harness tmpdirs do not trip it; the long path changes frame layout.
STAGE=/private/tmp/<something-deep-enough>/vitestage
mkdir -p "$STAGE"
cp -r crates/vite_task_bin/tests/e2e_snapshots/fixtures/vite_build_cache "$STAGE/ws"
rm -rf "$STAGE"/ws/{portable_clone,portable_origin,snapshots,snapshots.toml}
ln -s "$PWD/packages/tools/node_modules" "$STAGE/node_modules"

# 2) Freshly relink vt — this is the reliable trigger knob (see below).
touch crates/vite_task_bin/src/main.rs
cargo build -p vite_task_bin --bin vt

# 3) Run the task once from the staged workspace.
cd "$STAGE/ws"
PATH="<repo>/target-or-cache/debug:<repo>/packages/tools/node_modules/.bin:$PATH" \
  vt run --cache -v build

Observed frequency:

Condition	Result
First run(s) right after a fresh cargo build of vt	panic 4/4 (incl. once at clean branch HEAD)
Same staging, warm binary, idle machine	ok 14/14
Same staging, warm binary, 10 CPU spinners	ok 8/8

The fresh-relink correlation is reliable: touch main.rs && cargo build
followed immediately by a run panicked on the first attempt; the same command
repeated with a warm binary did not. A freshly linked (re-signed) binary
changes first-run timing on macOS (code-signature validation, cold page-ins),
which points at a timing-sensitive window rather than a purely deterministic
arithmetic bug — while the constant bogus header value shows the buffer
content itself is stable.

Both knobs matter: the e2e harness also runs freshly built binaries but stages
fixtures under short tmpdir paths and has never hit this (195/195 green), so
frame layout (path lengths) must also line up for the bad read to land on a
path byte.

Protocol background

Writer (ShmWriter::claim_frame, shm_io.rs ~line 148):

1. fetch_update on the leading AtomicUsize bumps the end offset by
   roundup4(4 + size) — this is the only cross-writer coordination.
2. Store +size into the frame's AtomicI32 header (Relaxed), fence(Release).
3. Write content bytes.
4. On completion (FrameMut finish): fence(Release), store -size (Relaxed).

Reader (ShmReader::iter_frames, ~line 278), which runs in vt after the
main child exits and OwnedReceiverLockGuard::lock_async returns:

• Reads the end offset once, then walks frames with plain (non-atomic)
  reads ("ShmReader reads headers by simple pointer dereferences").
• Header 0: "claimed but never written" — advance 4 bytes and keep scanning.
• Header > 0: "partially written" — skip roundup4(size).
• Header < 0: complete frame — yield size bytes.

The reader/writer advance arithmetic is mutually consistent (all offsets stay
4-aligned, roundup4(P + 4 + size) = P + 4 + roundup4(size)), so a divergence
requires either a not-yet-visible/in-flight write or a frame state the scan
rules don't actually cover.

Hypotheses (unconfirmed)

1. Reader overlaps a still-active writer. The design assumes quiescence
   ("The ShmReader must be created after all writing … is done and visible").
   If any descendant of the task (e.g. an esbuild service or worker spawned by
   vite that dies asynchronously after the main child exits) can still execute
   claim_frame/content writes after the receiver lock is acquired, the 0
   arm becomes unsound: the reader sees a zero header for a just-claimed frame,
   4-steps into its extent, the writer's content lands behind it, and the
   next 4-byte read hits path bytes ("\0/pr") — exactly what the panic shows.
   Worth auditing OwnedReceiverLockGuard: does every process that can write
   hold the shared flock for the entire lifetime of its mapping (exec'd
   descendants, forked children, threads still running during process
   teardown)?
2. Insufficient memory ordering for a non-quiescent reader. Headers are
   written with Relaxed atomics + Release fences, but the reader uses plain
   dereferences with no Acquire pairing. On arm64 this is only correct if true
   quiescence is guaranteed; combined with (1), the reader can observe a
   completion header without the content, or vice versa.
3. Writer-side accounting edge under specific cumulative frame sizes.
   Considered less likely (the arithmetic reviews clean), but not excluded —
   the constant panic offset would also be consistent with a size-dependent
   boundary bug that only certain path-length mixes reach.

Impact

• User-visible crash of vp run after the task already succeeded (exit via
  panic, no summary, no cache update).
• The scarier variant is the one that doesn't panic: a misparsed frame that
  stays in bounds silently yields garbage PathAccess records or swallows real
  ones. That corrupts automatic input/output inference — wrong cache
  fingerprints or missing outputs — with no diagnostic at all. The 0 and 1..
  scan arms can both skip over valid data if the layout assumption is broken.

Suggested next steps

1. On this panic path, dump the shm region to a file plus the offsets of the
   last few successfully parsed frames — one hexdump around the divergence
   point should immediately distinguish hypothesis (1)/(2) from (3).
2. Instrument claim_frame (behind an env flag) to log (pid, offset, size)
   per claim; compare against the reader's walk.
3. Audit receiver-lock coverage for late writers (exec/fork descendants,
   at-exit writes). A cheap experiment: have the traced child spawn a
   grandchild that keeps writing accesses for 500 ms after the parent exits,
   and see if the panic becomes reliable.
4. Regardless of root cause, make the reader defensive: treat an
   out-of-bounds skip (size > remaining) as "truncated tail — stop iterating
   and warn" instead of panicking, and consider Acquire loads for headers. The
   cache-update path already tolerates missing accesses gracefully
   (FspyUnsupported-style bail) — a truncated read should degrade the same
   way, not abort the run.

[wan9chi]

Root-cause investigation results

The earlier report guessed the receiver's file lock might be broken. Investigated; the answer is more precise:

The file lock works exactly as specified — and the corruption reproduces anyway, because the protocol's quiescence guarantee is attached to the lock fd, while write capability is attached to the mmap.

1. The lock itself is sound (verified)

A stress experiment with real processes in the exact runner topology — a detached grandchild acquires a Sender (shared flock) and keeps writing after the direct child (the only process the parent waits on) exits — showed Receiver::lock() blocking for the straggler's full lifetime in 100/100 rounds, with zero corruption and a stable end offset under the lock.

2. Lock-independent reproduction (the actual hole)

If the straggler closes the lock file descriptor — as fd-hygiene sweeps, daemonizers, and closefrom()-style cleanup in real tools do — the shared flock is released with the fd, but the sender's shared-memory mapping keeps full write access. Re-running the same topology with the grandchild closing its lock fd first:

• Receiver::lock() returned in microseconds while the grandchild was actively writing;
• the claimed end offset grew under the exclusive lock in 48/100 rounds;
• the reader walked into in-flight frames repeatedly (24 zero-header walks, 2 partial skips), and in 14/100 rounds hit exactly the crash signature from this issue — a positive garbage "frame size" parsed from path bytes, which the pre-fix reader turned into the out-of-bounds panic.

3. Why the crash needs no memory-model exotica

The per-frame in-flight window (between the end-offset fetch_update, which is immediately visible, and the +size header store) is only nanoseconds wide — but once an unlocked writer overlaps the reader at all, the reader's cursor can pass a frame's header before the store lands, never re-reads it, and then dereferences frame content as headers. Deterministically pinned by the new unit test via a writer-side stall hook (simulated preemption inside that window).

Landed on main (pending PR)

• stalled_writer_content_is_not_parsed_as_frame_headers (shm_io): deterministic parse-level repro; red without the fix (verified 3/3, same panic value as the field crash), green with it.
• reader_survives_straggler_writing_after_lock_fd_closed (channel): end-to-end topology repro using a Sender::close_lock_file_for_test hook; asserts reader soundness (terminates, yields all pre-lock frames, never yields garbage).
• Reader hardening in iter_frames: a frame size extending past the claimed end offset cannot be a real header → stop and drop the unreadable tail instead of panicking (also fixes a latent -i32::MIN negation overflow).
• Doc updates on Receiver::lock / module comments recording the fd-lifetime caveat.

Still open

1. Which process closes the fd in the wild (or whether another lock-discard path exists) — the vite-build field trigger was timing-sensitive and stopped reproducing before this was pinned; the fd-closure mechanism is demonstrated, not attributed.
2. Silent truncation: the hardened reader drops the unreadable tail without telling the caller. Since the tail is written by post-exit stragglers the runner already intends to ignore, this is usually benign — but plumbing a "truncated" signal up to update_cache (skip caching that run) would remove the residual risk.
3. Protocol hardening options if quiescence should be enforced rather than tolerated: per-frame sequence/checksum, a PID registry in the shm, or accepting the current tolerate-and-truncate stance as the design.

[wan9chi]

Proposed fix: replace the lock file and shm names with a handle handshake

Handoff spec — everything needed to implement is below. (Edited: an earlier revision included a frame-index protocol and a read timeout; timeouts and daemon-spawning tasks are out of this issue's scope, so those parts are deferred — see the last section.)

Why checksums/validation can't work

The reader finds frame N+1 by reading frame N's size bytes. A still-running writer can change those bytes after any check and before use. So no validation of the shared memory fixes this. The file lock can't fix it either: a tool that closes file descriptors it doesn't recognize releases the lock but keeps its mapping, and the mapping is what writes (proven in the comment above).

The fix direction: the runner must wait until every writer process has exited. A dead writer's memory is settled — the header is stored before the content (with release fences), so a process that died at any point leaves only the three frame states the current parser already handles (all-zero, +size partial, -size complete). Process exit is the one release the tool cannot fake, skip, or trigger early by closing fds. This is the same waiting behavior the flock has today — block until the writers are gone — just tied to something a writer cannot drop while still being able to write.

The handshake

Today: senders find the shm by name and the lock by file path, both from the payload env. Replace both with one connection. The runner listens on a unix socket (macOS/Linux) / named pipe (Windows); only the socket path goes in the payload env. The preload ctor connects and two things are exchanged:

Writer → runner: a watch on the writer process (so the runner can wait for it to exit; the watch lives in the runner's fd/handle table, so the tool closing its own fds changes nothing):

• Linux: writer sends pidfd_open(getpid()) via SCM_RIGHTS. A pidfd keeps pointing at that exact process; pid reuse is impossible by construction.
• Windows: writer calls OpenProcess(PROCESS_DUP_HANDLE, runner_pid) — using the runner's pid is safe because the runner is alive for the whole exchange; if the runner died, the pipe is gone and the writer just gives up — then DuplicateHandle of its own process pseudo-handle into the runner, and sends the resulting handle value over the pipe. The runner treats it as an opaque waitable. Optional cross-check: GetProcessId(handle) == GetNamedPipeClientProcessId(pipe).
  • Why this direction: a HANDLE value is an index into the owning process's handle table; the number means nothing in another process. DuplicateHandle is the only way to install a handle into another table, and it needs a handle to the destination — so someone has to bootstrap from a pid, and the runner's pid is the only one guaranteed not to be recycled mid-exchange. Handle inheritance would also work but node/libuv spawn children with restricted handle lists (PROC_THREAD_ATTRIBUTE_HANDLE_LIST), which breaks the chain.
• macOS: no unprivileged handle transfer. The kernel reports the peer pid on the socket (LOCAL_PEERPID); the runner registers kqueue EVFILT_PROC NOTE_EXIT on it, then verifies pbsi_uniqueid from proc_pidinfo(PROC_PIDT_SHORTBSDINFO) — a 64-bit per-boot, never-reused process id — against the value the writer sends. Mismatch or ESRCH → the writer already died → its memory is settled, nothing to wait for.

Runner → writer: the shared memory itself (no global name anywhere):

• Linux: memfd_create fd via SCM_RIGHTS.
• macOS: shm_open + immediate shm_unlink, then send the fd via SCM_RIGHTS.
• Windows: unnamed section (CreateFileMapping with no name); the runner already holds the writer's process handle from the step above, so it duplicates the section handle into the writer and sends the value.

The writer mmaps and may close the socket; nothing depends on it afterwards. Crash cleanup becomes automatic: anonymous/unlinked memory disappears with the last mapping, no stale /dev/shm/fspy-* or lock files.

Fail closed: if a watch cannot be established or verified (e.g. a garbage handle value), the runner must not assume the writer is dead. Mark tracking incomplete for the run; see below.

Reading at end of task

1. Stop accepting connections. This is the fence against new senders (a late process fails to connect → runs untracked, same as today's "channel closed").
2. Wait for every collected watch to fire — i.e. every writer has exited. Same blocking behavior as today's flock, without its holes.
3. Memory is now settled; read it with the existing parser. Keep the bounds guard that landed with the regression tests — it costs nothing and stops any future gap in the watch set from turning into misparsing.
4. If tracking was marked incomplete (watch failure): skip reading, report "tracking incomplete" to update_cache, which must skip caching that run — never cache from a shortened access list.

Edge cases

• fork() without exec: the child inherits the mapping but never did a handshake, so the runner has no watch on it. The preload caches the pid that performed the handshake; on write, if getpid() differs, do a new handshake first; if the runner no longer accepts, drop the access (fail open, matches today).
• exec chains: the ctor runs per image and re-handshakes, replacing today's per-exec sender recreation.
• Compatibility: none needed. The shm protocol is per-run and the preload dylib is embedded in the vt binary, so writer and reader always ship together.
• iter_path_accesses currently unwrap()s wincode decoding; replace with skip + debug log while in there — a panic is never worth it.

Definition of done

• The lock file, try_lock_shared, the flock in Receiver::lock, and Sender::close_lock_file_for_test are gone. No shm name or lock path exists anywhere (check /dev/shm on Linux and ls /dev/shm | grep fspy in a test; on macOS the name is unlinked before any child sees it).
• Senders obtain the shm and prove their identity through the handshake; the runner waits on process watches before reading.
• Watch-establishment failure ⇒ "tracking incomplete" ⇒ run not cached, summary says why.
• The fork() re-handshake path works (test with a real fork() child writing after the parent exits).
• stalled_writer_content_is_not_parsed_as_frame_headers (shm_io) passes unchanged — it tests the parser backstop, which stays.
• reader_survives_straggler_writing_after_lock_fd_closed (channel) is rewritten: there is no lock fd to close anymore. Keep the topology (grandchild writing after the child exits) and assert the runner's read happens only after the grandchild exited and yields all of its frames.
• New test: a sender created after the runner stopped accepting fails cleanly (replaces forbid_new_senders_after_locked).

Deferred (separate issue)

Bounded waiting. This design blocks until every writer exits, like the flock does today; a task that spawns a long-lived process will block vp run at the read step. Fixing that needs a timeout, and reading before all writers exited needs a frame protocol where frame positions don't come from frame bytes (fixed-stride index of atomically-published (offset, len) slots; committed frames are immutable so they stay readable next to live writers). Both are deliberately out of this issue's scope.