fix(fspy): replace quiescence locking with crash-tolerant shared-memory publication - #675
Open
wan9chi wants to merge 87 commits into
Open
fix(fspy): replace quiescence locking with crash-tolerant shared-memory publication#675wan9chi wants to merge 87 commits into
wan9chi wants to merge 87 commits into
Conversation
…cation The IPC channel previously required writer quiescence before reading: a file lock (or #577's active-writer gate) had to drain before the receiver could parse the inline frame stream. A traced process that closed the lock descriptor while keeping the mapping writable corrupted parsing (#544), and one that never exited (a daemon) or died mid-record could block collection or poison the writer count forever. The shared memory now uses a two-ended layout: an allocator word admits claims and closes the channel, a descriptor table grows from the front, and payloads grow from the back. Each frame commits by publishing its descriptor with a release CAS; closing atomically aborts every unfinished slot and copies committed payloads out with relaxed atomic loads, so the receiver never waits for a writer, never trusts payload bytes for traversal, and never holds a reference into memory another process may mutate. Loss of a record by a live writer (capacity, abandonment) flags the trace incomplete so the run is not cached from an under-reporting trace; process death needs no cleanup because records are published before the recorded operation is performed. Closes #544. Supersedes #577. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fspy benchmarklinuxmacoswindows |
Temporary stderr phase timings to locate the CI launch regression on the Linux benchmark runner. Will be dropped before merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This reverts commit d2fa1de.
…path The crash-tolerant close moved two hidden costs into the tracked child's launch window on journalling filesystems: the first write to the sparse 4 GiB backing file (a millisecond-scale block allocation, previously paid lazily or never) and unmapping the receiver's view (previously after access collection). The Linux benchmark runner priced them at ~2.2 ms and ~0.6 ms per launch. Pre-fault the header page on a background thread at channel creation — a protocol-neutral compare-exchange of zero with zero, run concurrently with process startup — and release the receiver's mapping on a detached thread after the frames are copied out. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the packed allocator word and its compare-and-swap loop with two monotonic counters over a fixed table/payload partition. A claim is two wait-free fetch_adds validated against the fixed region bounds; failed claims overshoot the counters harmlessly because committed descriptors are self-describing and readers clamp to the region capacities. The close boundary becomes a snapshot load: claims that arrive later land in slots the receiver never visits and are dropped under the same publish-before-perform argument as freeze-race losses. The CLOSED gate — whose write materializes the counter page, a millisecond-scale first-block allocation on journalling filesystems when the trace is empty — moves onto the deferred teardown thread, which lets the pre-fault machinery from the previous commit be deleted outright. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The wait-free rework deleted the pre-fault thread on the theory that a write-free close no longer needs the page. The benchmark disagreed: on the Linux runner the first touch of the sparse backing file costs milliseconds whether it is a write (a sender's first claim) or a read (close's snapshot), so Linux launches regressed right back. Windows meanwhile improved once the thread was gone — its first touch is cheap and the spawn was the cost. Restore the concurrent header-page warm-up, gated to Linux. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… a thread Reserving one block at the header and one at the payload-region start with fallocate(KEEP_SIZE) is a cheap metadata-only operation at channel creation, so the milliseconds of journalled block allocation that some filesystems charge for the first touch of each area no longer need a background thread to hide them — and the payload area, which the thread could not safely touch, is now covered too. KEEP_SIZE because growing the file would desynchronize mapping sizes across processes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lting on a thread" This reverts commit 528d1d1.
The fallocate experiment showed the first-touch cost is in the fault path, not block allocation, so only a real touch helps. Give the pre-fault thread a second target: a protocol-owned warm word between the table and the payload data, so the page where the first payloads land is materialized without racing any writer's payload bytes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This reverts commit c4ba8dd.
…opying Frames now owns the mapping and lazily hands out per-span borrows of the validated committed payloads. A committed span is immutable under the protocol and disjoint from everything a live writer may still touch, so the borrows are sound without a copy; the trust argument lives in the reader module docs. This also collapses the close-time machinery: the CLOSED gate returns inline into close (its page is pre-warmed on Linux where first touches are expensive), the deferred-teardown thread is gone, and the mapping is released when Frames drops — naturally off the collection path. The receiver-side frame validation pass in the supervisor is dropped with it; committed frames are complete by protocol. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Nothing is collected anymore — frames are borrowed in place — and the async wrapper descended from the file-lock era, when acquiring the trace could block until every sender exited. Closing is now bounded by the number of reported records and runs inline, so the type becomes ChannelAccesses with a TryFrom<Receiver> conversion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
wan9chi
force-pushed
the
claude/fspy-shm-publication-design-76917a
branch
from
August 15, 2026 01:05
e560a84 to
0a80d2b
Compare
The protocol layer should not know its consumer: describe the publish- before-perform rule as the intended usage contract and the incomplete flag as a property of the channel, with no mention of what sits on top. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The descriptor table's length becomes the protocol's const-generic parameter, so the header and table are one repr(C) struct — offsets become field accesses, the table a real array, and the per-accessor unsafe pointer derivations collapse into one borrow. The payload area stays outside the struct deliberately: writers hold exclusive borrows into it that must not alias the shared region borrow. The channel names its layout the same way: channel::<SLOTS>() sizes the backing file to capacity_for_slots(SLOTS) (the exact inverse of the slots_for_capacity sizing rule), every process names one shared SHM_SLOTS constant, and a sender now rejects a region whose size disagrees with the layout instead of panicking inside geometry assertions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The const-generic table size taxed every signature and call site, and it bought a property the mapping already provides: with the layout derived from the mapping length alone, the region is self-describing — writers and the receiver compute identical bounds from the size of the file they mapped, with no shared constant to agree on and no size handshake to get wrong. What the struct experiment taught survives: one unsafe borrow now builds three typed views — the repr(C) header, the descriptor table as a slice of atomics, and the raw payload area — so counters are named fields, slots are bounds-checked indexes, and only payload spans remain pointer arithmetic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The incomplete flag had one real writer — capacity exhaustion — and the counters already record that: a failed claim's bumps push a counter past its limit, counters never move backwards, and the bump precedes the operation whose record was lost, so the close snapshot either sees the overshoot or the loss belongs past the boundary. The flag's other writers were bug-only paths. So the flag word, FrameMut's Drop, and the write_encoded flagging wrapper are gone; abandoning a frame now leaves exactly what dying does — an unfinished slot the receiver ignores — and acting on an abandoned record is outside the usage contract. Oversized frames become an asserted precondition: a caller error, and the one loss counters could not record. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The protocol had two ways to write a record — write_encoded, with an error enum only one caller half-used, and the hand-rolled claim/serialize/finish sequence in the Unix client. Both callers want the same thing: serialize the record into a frame and skip it on any failure, because an intercepted call must proceed no matter what. That helper now lives once, on the channel's Sender; shm_io keeps only claim, fill, and finish. Also: a plain-words README section on how a full region is handled, and a mermaid dependency graph of the module files as a reading order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The six-way split was designed around machinery the simplifications have since deleted, and its narrative had drifted: writer and reader derive their own payload references, so state was never the only file touching shared memory. Merge to the boundary that still earns its keep — pure integer math versus code that touches the mapping: - layout.rs absorbs the descriptor codec (both plain arithmetic) - shared.rs is state + writer + reader in reading order, with the reservation types and SharedState going private to it - mod.rs keeps the surface, overview docs, and integration tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Completeness by counter overshoot had two holes. A frame larger than a descriptor can describe (> i32::MAX bytes) could not be reported at all, so claiming one panicked — reachable in the preloads, whose record lengths come from path strings the traced program controls, breaking the promise that a preload never panics its host. And a writer killed inside a failed claim left an overshot counter behind, marking a channel incomplete over a record whose operation never ran. Replace the overshoot rule with a loss flag in the header's reserved space: every failed claim stores it before the writer moves on, and the receiver reads it once at close. The non-overflow path is untouched — the flag's cache line is only written by a claim that is already failing. The report-before-perform order gives the same rule-1 guarantee as commit-before-perform: a report the receiver misses belongs to an operation performed after close, and a writer that dies before reporting never performed the operation at all. Oversized frames now fail like any other refused claim, without poisoning the counters, so the channel stays usable for the records after them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
HEADER_LEN predates the typed header: when layout.rs was offset math only, there was no struct to measure, so the size was a literal and a const assert tied the struct to it after the fact. Move the Header struct into layout.rs — it describes the region's shape, which is that file's job — and derive HEADER_LEN from size_of. The sizing math now follows the struct automatically; the one remaining literal is an assert pinning the header to a single cache line, which is a design intent no struct can express. CLOSED moves along with it, keeping all the bit meanings next to the slot codec. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every one of these is a leftover from a deleted design, kept alive only by habit or by tests: - ReserveError duplicated ClaimError variant for variant; try_claim now returns ClaimError directly and the mapping in claim_frame goes away. - Reservation was a named pair passed once between two functions in the same file; a destructured tuple says the same thing. - SharedState::mapping_len wrapped a field its one caller can read. - The close/pre_fault wrappers in mod.rs re-stated shared's docs to delegate one call; the functions are now re-exported like the rest of the surface, with the wrapper's doc text folded into the real ones. - Sender's Deref to ShmWriter served only tests, which now reach the writer field directly; FrameMut, ClaimError, and ProtocolError are no longer nameable outside the channel (production never names them), the error types staying test-visible for assertions. - into_memory is gated to the non-miri test that is its only caller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SlotState classified every slot value the receiver could read, but its only consumer treats all invalid classes identically, and the validation that followed already refuses every pattern the classification singled out: an unfinished zero decodes a zero length, and any value carrying the aborted bit decodes a length beyond the 31-bit limit. Collapse decode and validate into one step returning Option<PayloadSpan>; the freeze loop keeps a slot's span, skips ABORTED, and calls everything else corrupt. The three-state table stays as the comment documenting what slot values mean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The free unsafe close made the module's unsafe boundary uneven: the writer pays its contract once at attach and operates safely, while the receiver re-asserted the same contract at every close call — in the channel, far from the creation-time facts the SAFETY comment cites. ShmReceiver mirrors ShmWriter: one unsafe constructor with the identical contract, eager geometry validation, and a safe consuming close. The channel constructs it where the region is created, so Receiver::close is now safe code. pre_fault stays a free function: it is a creator-side warm-up on a throwaway view, belonging to neither endpoint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adding a contended row to the benchmark priced what the loops actually cost, and it is 2.4x to 2.7x per claim at four to eight threads — the two-thread rows had shown nothing, which is why they looked free. Neither wrap they guarded against can be reached. The payload counter can only pass the region once a claim has already failed, and that failure sets the gate; every later claim is then refused at the claim counter, which is read after the reservation and before any span is built, so a wrapped payload counter never gets to name an offset. The claim count needs 2^63 increments to reach the gate bit — thousands of years at the rate claims measure, for a channel that lives one command — and a gate that read as set by accident would only fail the seal, which is the cautious answer rather than a wrong one. The reasoning now sits beside the counters instead of in the code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A code review turned up three ways this PR's own guarantee could fail quietly, plus the docs it had left behind. `Sender::send` could give up on a record — a serialization mismatch, or a frame it had already claimed — and return with nothing said. A failed claim reports itself, but a writer that abandons a frame is indistinguishable from one that died, and a writer that died never performed its operation. So there was no way to report the one case where a live writer skips a record and goes on to act. `ShmWriter` gains `report_lost_record` for exactly that, and `send` uses it. `Receiver::close` removed the backing file before sealing. A process starting up in that window failed to attach, which is the one loss the gate cannot see, and could then act before the seal's snapshot. Sealing first means it attaches, finds the channel closed, and gives up cleanly — and its records are legitimately past the boundary. The rest is documentation that stopped being true when the seal stopped freezing slots: `Receiver::close` and `ChannelAccesses` both still promised unfinished frames were "atomically aborted", the seal is one load and a bit rather than two loads, and the claim counter counts claims that reached the payload reservation rather than every attempt. Tests: `Sender::send` had no coverage at all — every test reached past it into `claim_frame`, so a disagreement between `serialized_size` and `serialize_into` would have dropped every record silently. It now round trips real records. Also pinned: that a payload-capacity failure costs no slot, and that a reported loss fails the seal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The new sender test spelled its record paths as `&str`, which only becomes an `IpcPath` on unix — Windows carries UTF-16 and needs `from_wide`. My Windows lint had run with `--lib`, so it never compiled the tests and CI caught it instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
It warmed the region's first page on a background thread so the first claim would not pay for the backing file's first block allocation — a couple of milliseconds on Linux where the temporary directory is a journalling filesystem, and near nothing where it is tmpfs. That is once per channel, so once per task execution, against tasks that run for hundreds of milliseconds to minutes. It does not pay for a thread per channel and a Linux-only branch through channel creation. The benchmark's launch row will get much worse and should be read with that in mind: it times a target that opens nothing, so a two-millisecond constant is most of what it measures. The number to watch is whether the access rows move, and they should not. If the fault cost is ever worth removing rather than hiding, the way to do it is to stop putting an IPC region on a journalling filesystem. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The seal read the claim counter and then set the gate in a second operation. A `swap` does both: it returns the count and shuts the gate in one step, so the boundary and the gate are one point in that counter's modification order rather than two with a window between them. Rule 1 loses the case it had to explain — the claim that lands after the snapshot but before the gate — because there is no longer anywhere for it to land. Replacing the count rather than or-ing into it is fine. Nothing reads it after a seal: a later claim fails on the gate before its slot index is used, and a later seal only tests the bit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The README and the module docs told the same story twice, at length. The README keeps it; `mod.rs` now says what the module is, states the one rule its users must follow, and points at the README for the rest. Cut from the README: the argument against designs nobody wrote, the lifecycle diagram that repeated the paragraph above it, and a performance section restating mechanics already covered. `layout.rs` loses its account of the counters, which the README gives, and keeps the ordering contract the code cites by rule number. Rewrote the prose throughout: active voice, no em dashes, fewer adverbs, and the specific thing named instead of gestured at. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Each paragraph is one line now, so editors and viewers wrap it to whatever width the reader has, and a reworded sentence no longer reflows the lines under it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The comment claimed the counter might read higher than the table without saying how, which made the clamp look like padding. The cause is ordinary: a writer bumps the counter, finds its index out of range, and only then sets the gate, so a seal landing in that window reads the higher count with the gate still clear. Slicing the table on the raw count would panic in the receiver. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A count past the table means a claim was refused, which invites the question of whether the seal should fail on it. It should not: that writer performs the operation it could not record only after the boundary, and if it died first it performed none. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s on The CLOSED gate now means one thing: the region had no room. That is the only failure `shm_io` owns, since the region's size is the only thing it controls. `report_lost_record` goes with that, and the gate loses its second meaning. Everything else a sender can hit is a defect in this crate, so the channel layer panics instead of reporting. `Sender::send` panics when a record's serialized size disagrees with the bytes it writes, and `sender()` panics when the region is there but cannot be opened, mapped, or attached to. A missing backing file stays an error, because it is not a failure at all: the receiver removed it, so it has already stopped collecting and this process is working past the boundary. That distinction is what lets the rest abort. A process that cannot attach has no way to tell the receiver it recorded nothing, and a trace that silently omits every access a process made is worse than a build that stops. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Four of the five panics were a `let ... else` or a closure wrapped around the value they were unwrapping. `expect` says the same thing in one line and prints the underlying error with it, which the hand-written messages had to interpolate by hand. Splitting the serialize check in two also names the failures separately: one for a codec that refuses its own frame, one for a codec that fills less of it than it asked for. The open path keeps its `match`, since a missing file returns rather than panics and there is no unwrap to hang an `expect` on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The gate was set with `fetch_or`, which is a locked read-modify-write on a line every writer is contending for. A plain store does the job: it drops the claim count along with setting the bit, and nothing reads that count once the gate is set. The seal fails on the bit before it looks at the count, and a claim that reads the cleared count reads the gate with it, so it gives up before using a slot index. The two tests that asserted a count next to the gate now assert the gate alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both sides checked an index against `SLOTS` and then indexed the table on the strength of that check. `get` does both at once, so the bound comes from the slice being indexed rather than from a constant that has to agree with it. The writer takes the slot it claimed or reports the loss, and holds the reference instead of the index. The seal takes the admitted prefix or falls back to the whole table, which says what the old `min` meant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
wan9chi
force-pushed
the
claude/fspy-shm-publication-design-76917a
branch
from
August 17, 2026 05:51
085f438 to
08150ee
Compare
The shared memory a tracked run reports its file accesses through was a constant in `fspy`, four gibibytes wide. How many accesses a program makes is the runner's business rather than the tracer's, and nothing could ask for a different size, so no test could put a task in front of a channel too small for it. The size becomes an argument to `fspy::Command::new`, and the runner reads `VP_RUN_INTERNAL_FSPY_SHM_CAPACITY` for it, keeping the same four gibibytes when it is unset. The variable is internal: it exists so a test can shrink the channel until a task overruns it, and nothing outside this repository should set it. The e2e case that comes with it stats one 2 MiB path, the largest single record tracking can be asked to hold, under a 64 MiB channel. That leaves room to spare, so the run caches like any other, which is what tells us the size arrived. The interesting case, a channel with no room for the record, has to wait: today it aborts the task process, and the panic it prints carries a thread id, a toolchain path, a backtrace and a platform's own abort code, none of which snapshot the same way twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The descriptor table's length was a constant in the channel, one slot per 64 bytes of a 4 GiB region. Both ends read it from that constant, so it had to be a compile-time answer, and it rode on a `SLOTS` const generic through `Meta`, `MappedLayout`, `ShmWriter` and `ShmReader::seal`. Nothing about the protocol needs it decided that early. Both ends need only to agree, so the receiver passes the count when it creates the region and every sender reads it back out of the channel's own configuration. `Meta` becomes the two counters alone, and the table becomes a slice pointer beside them, which is what the reader already kept. That lets a caller size a channel for what it expects to record, rather than taking a number this crate picked. The runner names both halves now, and its own `VP_RUN_INTERNAL_FSPY_SHM_CAPACITY` can shrink a channel far enough for a test to overrun it, which needs the table to shrink with it. Two smaller things follow from the same change. `Receiver::close` now reports only the one failure a caller can act on, a record a sender could not write, and panics on a region that cannot hold the protocol, which `channel` proved it could before any sender saw it. And a run whose tracking came up short no longer fails the task: the runner reports it as a not-cached reason, since the task itself did its work and only the record of it is missing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The channel's size and its slot count both arrive from the runner now. The merged branch settled the capacity half; this side adds the slot count. The e2e case it brought along can now do what it was written for. A channel with no room for a record no longer aborts the task process, so the snapshot shows the task exiting cleanly and the run reported as not cached. Its `vtt stat-many` helper is gone: one 2 MiB path overruns a small channel more precisely than twenty thousand short ones, and `stat_long_filename` already made one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The base moved the size onto a builder method, so this side follows: the channel takes a `ChannelSize`, whose slot count the descriptor table needs and which no longer has a compile-time answer. `shm_for_capacity` splits a byte budget between the table and payload room, which is what the runner calls with its own capacity, and what the default size is built from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two CI fixes from the base. The musl exemption on the size field goes with them: the setter reads it there too, so claiming it is dead only made the expectation unfulfilled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The case now drives the channel by record count rather than record size, because a Windows path record cannot get large enough to overrun one. With the counting lever it can do what it was written for: a 64 KiB channel holds a thousand records and the task makes twenty thousand, so the snapshot shows the task printing its last line and exiting cleanly, with the run reported as not cached. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…error The base moved the channel's size to a single read beside the `channel` call, so `Command` keeps nothing about it and `fspy::ipc` picks the slot count out of the byte count there. A lost record was an `Option<FrameReader>` behind an `is_complete()` that callers were free to ignore. It is a `TrackingIncomplete` error now, carried in `ChildTermination::path_accesses`, so nothing reads the accesses without meeting the failure first. The runner still declines to fail the task over it: it turns the error into a not-cached reason, which is the one place that policy belongs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both entries described the mechanism rather than what a user sees. Crashing "mid-record", closing "inherited file descriptors" and keeping "every completed record intact" are things the tracker does; what a user hit was `vp run` hanging or failing, and a task getting killed partway through. The second entry also cites #533, the report of the abort. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`sender` decided for itself: a missing backing file came back as an error, and every other failure panicked inside the call. That put the policy in the one place that cannot know the caller's situation. It now returns every failure. Two error kinds say the channel is simply over — the receiver removed the file, or sealed it just before this call — and a caller meeting those has lost nothing by recording nothing. Both preload clients skip on those and panic on anything else, which is the same behaviour as before, now written where the decision belongs. Neither client prints on the skip. A preload library writing to the traced process's stderr corrupts whatever that process is printing, and a channel that closed before this process started is not news. The benchmark launcher gains a small shim over `ChildTermination`'s accesses field. The benchmark compiles that one source against both this revision's fspy and the merge base's, so a change to the field's type stops the base arm building; the shim spans both shapes. CI caught this, and the fix is verified by compiling the head launcher against main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The table's `SAFETY` comment said its alignment "follows from the start address being aligned for `Counters`, whose size is a multiple of that alignment". The claim is true, but its second half was nowhere asserted: it came out of `size_of::<Counters>() == 2 * size_of::<AtomicU64>()` and the fact that an `AtomicU64` is never aligned more strictly than its own width. A reader had to reconstruct that, and a later field could quietly break it. There is now an assert for exactly the step in question, and the comment names both asserts it stands on rather than restating the argument. A test checks the pointers `new` actually builds, across slot counts, since the asserts argue about the types and not the arithmetic. `meta_len` was left over from the `Meta` struct that used to hold both parts. It is `payloads_at` now, which is what it measures. `table_len` became `table_bytes`, so it stops reading like the slot count that `table().len()` returns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Handing every failure to the caller gave both preload clients the same fifteen lines: skip on two error kinds, panic on the rest. Two copies of one policy, and a signature that made a caller re-derive it from `io::ErrorKind` before it could act. `sender` returns `Option<Sender>` now and decides for itself. `None` means the channel is already over, which is the only outcome a caller can do anything about, and it does the same thing either way: record nothing. Everything else stops the process where the cause is known, with the error in the message. Both clients are one line and a comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…region `channel` attaches a throwaway writer to prove the region can host the protocol. Nothing tested it, and two panics depend on it: `sender` and `Receiver::close` both treat a region that cannot hold the protocol as impossible, on the grounds that creation already refused it. Removing the check and running this test shows what it buys: `channel` accepts the size, and the panic lands in `sender` instead — which runs in every traced process's preload, where the cause is furthest from the config that caused it. The size comes from an environment variable, so that is reachable by configuration, not only by a bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`tracking_fell_short` re-derived a condition its own caller already implied. fspy is attached only when `input` or `output` asks for inferred paths (`CacheState::new`), and the spawn flag is that same `fspy.is_some()`, so `path_accesses` is `Some` exactly when the task infers. The function tested `fspy.is_some() && infers && ...` where the first two are one fact. That mattered beyond the redundancy: because the check claimed to let a short trace past for a task that declares everything, `observe_fspy` had to be ready for an `Err` it could never see, and answered it by treating a lost trace as an empty one. A wrong trace and no trace are not the same thing, and nothing should have to decide that twice. One `let ... else` takes the accesses and turns the run away if they are not all of them. `observe_fspy` now receives what it uses, an `Option<&PathAccessIterable>`, and has nothing left to interpret. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Collecting a run's file accesses required every writer to hold still. The receiver could only read the frame stream once each sender had released a file lock, and that coupling broke in two ways:
python-daemon, for one) releases the lock while keeping the shared memory writable. The reader then races live writers and reads path bytes as a frame header, panicking the runner.SIGKILL.Correctness cannot depend on how long a file descriptor lives, or on writers running cleanup while the task tears down.
The protocol
fspy_shared::ipc::channel::shm_iopublishes frames through a descriptor table instead of inline headers.README.mdin that directory is the full description; the shape is:fetch_adds reserve payload bytes and a slot, with no retry loop and no lock. Each writer checks what the counter returned against a fixed limit, so overshooting costs nothing: no counter says where data is, since every committed descriptor carries its own offset and length.Release. Only the writer that claimed a slot ever writes it, so it needs no compare-and-swap. The receiver loads withAcquire, so a descriptor it sees brings the payload bytes along.swapputs the CLOSED gate into the claim counter and reads the old value, drawing the boundary and shutting the gate at a single point in that counter's modification order. Sealing a channel holding ten million frames costs what sealing an empty one costs. Frames are then read in place, straight out of shared memory, with no copies.The channel asks one thing of its users: publish a record before performing the action it describes. A dead writer's missing record then describes an action that never happened, and a record refused after the seal describes one performed after the channel closed. The receiver drops both, and both answers are truthful. This holds across the Unix preload and the Windows detours; the Linux seccomp path collects supervisor-side and is unaffected.
Running out of room
A claim with nowhere to go sets the CLOSED gate before returning, and the writer skips that record and carries on — recording must never stop the program doing the work. The gate is also what tells the receiver: a seal that finds it already set hands back nothing, so the run is reported as untracked rather than as having touched only the paths that fit.
Setting the bit first matters for the same reason publishing before acting does. If the seal misses the bit, the writer set it after the boundary, so the skipped record describes an action performed after the channel closed; and a writer that died before setting it never performed its action.
This replaces a panic, and that panic is #533: on the base of this PR a full region aborts the traced process, because the panic fires inside an interposed
extern "C"function that cannot unwind. It is reachable by workload rather than by any bug — #533 was hit as a SIGABRT storm when vite-plus#2123'sbunxself-recursion flooded the channel — so a large enough build could kill the compiler doing the work. It now costs an uncached task instead, which is the fail-open behaviour that issue asks for, down to flagging the trace incomplete so nothing caches from it.One clause of #533 is not met, deliberately. It asks for no panic in a no-unwind context at all, and
Sender::sendstill has asserts that only a disagreement between this crate and its codec can trip. Attaching a sender is a further, deliberate exception in the other direction: see the last point below.Consequences elsewhere
ChannelConfcarries the shm id and the slot count, andReceiver::lockbecame a consuming, nonblockingReceiver::close.ouroborosself-referencing guard infspy::ipcis gone with it, since frames are borrowed from the mapping the reader owns.Receiver::closereports only the one failure a caller can act on — a record a sender could not write — and panics on a region that cannot hold the protocol, whichchannelproved it could before any sender saw it.usize, and an underfilled frame. This PR removes the reachable one and adds one that only a broken channel reaches.Each file carries one argument:
layoutholds the shared shape, the descriptor codec and the three-rule memory-ordering contract that the code cites by rule number;writerandreadereach carry a single aliasing justification.Known regression: Linux task launch
The benchmark's Linux launch rows read +158% dynamic and +221% static, against roughly 0% on macOS and Windows and roughly 0% on every
accessrow. Read that as an absolute number rather than a ratio: it is the one-off cost of first-touching the region's first pages, a millisecond or two, and the benchmark's launch target opens nothing at all, so a couple of milliseconds is most of what it measures. Against a realtscorvitesttask it is not visible. It is also per channel, which is per tracked spawn, so a build spawning hundreds of tasks pays it hundreds of times.Two things about it are settled by earlier work on this branch: the cost is in the fault path rather than block allocation, so
fallocate(KEEP_SIZE)does not help (measured — it went backwards), and reads of holes on this runner cost the same as writes. A Linux-gated background pre-fault thread used to hide it and brought the row to ~+21-28%; it was removed on this branch because it buys nothing where/tmpis tmpfs and cost more than it saved on the other platforms.The fix that would remove the cost rather than mask it is to put the region on a filesystem that does not journal —
/dev/shmon Linux, withtemp_dir()as the fallback elsewhere. That is a separate change, and this project's history says to measure it rather than reason about it.Everything else measured flat:
access+1.83%/+0.34% on Linux, +0.96% macOS, +0.17% Windows, all inside the benchmark's ~2.3pp noise floor.Verification
Miri covers the protocol tests, including the slot state machine, seal races and concurrent writers. Cross-process tests cover real shared memory and a writer hard-killed mid-frame (
SIGKILL/TerminateProcessviaChild::kill).The e2e case from the base PR now does what it was written for. A 64 KiB channel holds a thousand records — one descriptor slot per 64 bytes of the region — and the task makes twenty thousand accesses, so the snapshot shows the task printing its last line and exiting cleanly, with the run reported as not cached, twice over: the second run does not replay an entry built from part of a trace. The case skips musl, which has no preload and collects through the seccomp supervisor, so there is no channel there to fill.
Closes #544. Fixes #533. Partly addresses #605, whose signal-handler, post-
forkanderrnorequirements this does not touch. Supersedes #577. Stacked on #680.🤖 Generated with Claude Code