fix(fspy): replace the IPC file lock with an in-mapping close gate - #577
Closed
wan9chi wants to merge 1 commit into
Closed
fix(fspy): replace the IPC file lock with an in-mapping close gate#577wan9chi wants to merge 1 commit into
wan9chi wants to merge 1 commit into
Conversation
This was referenced Jul 28, 2026
fspy benchmarklinuxmacoswindows |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb42117817
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wan9chi
force-pushed
the
fspy-close-gate
branch
2 times, most recently
from
July 30, 2026 04:36
38e7f20 to
5d642d1
Compare
wan9chi
force-pushed
the
fspy-close-gate
branch
2 times, most recently
from
July 31, 2026 07:09
c0720e2 to
8b4e19a
Compare
wan9chi
force-pushed
the
fspy-close-gate
branch
from
August 4, 2026 10:17
8b4e19a to
5bae097
Compare
wan9chi
force-pushed
the
fspy-close-gate
branch
2 times, most recently
from
August 10, 2026 06:59
bbb60ad to
800f15d
Compare
wan9chi
force-pushed
the
fspy-close-gate
branch
from
August 10, 2026 07:06
800f15d to
38c76b1
Compare
wan9chi
force-pushed
the
fspy-close-gate
branch
2 times, most recently
from
August 10, 2026 07:26
5e9003e to
f4e2f6e
Compare
The old quiescence protocol attached "may write" to the shared mapping but "is still writing" to a file-lock descriptor. A descendant that closes descriptors it does not recognize released the lock while keeping full write access to the mapping, so the receiver could read frames while a straggler was mutating them. Put the gate in the shared memory itself, where a writer cannot drop it while still being able to write: one atomic word admits and counts claims, and the runner's close is a single `fetch_or` at root-process exit that fences all future claims and reports whether any write was in flight. Zero in flight proves every admitted claim ran to completion and the memory is frozen; anything else means the run is conservatively not cached. Tracking now stops when the root process exits instead of waiting for lingering descendants, so a task that leaks a daemon no longer blocks the read step, and post-exit accesses are treated as what they are: racy with respect to the task's contract. Closes #544. Closes #396. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
wan9chi
force-pushed
the
fspy-close-gate
branch
from
August 10, 2026 07:32
f4e2f6e to
ee9318c
Compare
wan9chi
marked this pull request as draft
August 10, 2026 07:32
wan9chi
added a commit
that referenced
this pull request
Aug 18, 2026
…ry publication (#675) ## 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: - [#544](#544): a traced process that closes inherited file descriptors (`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. - A descendant that outlives the task blocks collection for as long as it runs. [#577](#577) tried an in-mapping writer count instead, but any process that dies between increment and decrement poisons it forever, since no userspace cleanup runs on `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_io` publishes frames through a descriptor table instead of inline headers. `README.md` in that directory is the full description; the shape is: ```text | counters | descriptor table (one slot per frame) | payloads (the rest, grow up) | ``` - **Claiming is wait-free.** Two `fetch_add`s 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. - **Publishing is one store.** The writer fills a span nobody else knows about, then stores the descriptor with `Release`. Only the writer that claimed a slot ever writes it, so it needs no compare-and-swap. The receiver loads with `Acquire`, so a descriptor it sees brings the payload bytes along. - **Death and abandonment are the same state.** An unfinished slot stays zero and the receiver ignores it. No cleanup code runs because none exists — no exit hooks, PID checks, heartbeats or timeouts. - **Sealing never waits and never writes.** One `swap` puts 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](#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](voidzero-dev/vite-plus#2123 `bunx` self-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::send` still 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 - The lock file is gone. `ChannelConf` carries the shm id and the slot count, and `Receiver::lock` became a consuming, nonblocking `Receiver::close`. - The `ouroboros` self-referencing guard in `fspy::ipc` is gone with it, since frames are borrowed from the mapping the reader owns. - `Receiver::close` 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. - A run whose tracking came up short no longer fails the task. The runner reports it as a not-cached reason, since the task did its work and only the record of it is missing. - Attaching a sender now fails only when the channel is already over. Anything else — a file that will not open, or one that cannot hold the protocol — panics, because a process with no writer cannot tell the receiver it recorded nothing, and a trace that silently omits every access is worse than no trace. Panicking in the preload is not new: the base panics on a failed claim, an empty record, a size that does not fit a `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: `layout` holds the shared shape, the descriptor codec and the three-rule memory-ordering contract that the code cites by rule number; `writer` and `reader` each 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 `access` row. 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 real `tsc` or `vitest` task 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 `/tmp` is 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/shm` on Linux, with `temp_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` / `TerminateProcess` via `Child::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-`fork` and `errno` requirements this does not touch. Supersedes #577. Stacked on #680. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Superseded by #675. |
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
1 participant
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
The current quiescence protocol attaches "may write" to a shared mapping but "is still writing" to a file-lock descriptor. A descendant that closes descriptors it does not recognize releases the lock while it keeps write access to the mapping, so the receiver can read frames while a straggler mutates them. The #544 investigation pinned this as the root cause.
This PR replaces the lock with a gate stored in the shared memory itself, where a writer cannot lose it while it can still write. One atomic word admits and counts each claim. The runner closes with one
fetch_orat root-process exit, which fences all future claims and reports whether a write was still in flight. A zero count proves every claim ran to completion: the memory is frozen, and the runner reads and caches. A nonzero count means the run is not cached. In-flight windows last microseconds and the runner closes milliseconds afterwaitreturns, so a nonzero count is rare. The closed bit and the count share one word on purpose; with two atomics, a writer could check the flag before the close and publish its count after it, which is the file lock's race in new clothes.The close drops the shared memory's keeper, so the attach window ends with the close: a process that starts later fails at
open, and a sender that attached earlier has its claims refused. On Linux, the seccomp supervisor'sstopalso returns without waiting: each listener task hands over what it recorded and a detached task keeps answering notifications, so a live filtered process keeps working while its later accesses go unrecorded. Draining those listeners to EOF used to mean waiting for every filtered descendant to exit, which reintroduced the daemon hang this PR removes.A process may also call
exitwhile another of its threads is mid-send, which would abandon a gate guard and mark the whole run incomplete. The preload now interposesexitand_exitand waits the few microseconds until in-flight sends finish before the process dies. Signals and crashes still skip this, and the runner handles those by not caching the run. Windows parity is a follow-up.Semantics change: fspy no longer waits for lingering descendants after the root process exits, and it does not track accesses made after that exit. Collecting the trace no longer delays
vp run; a task that leaks a daemon no longer blocks the read step. This fixes #396, where the receiver's exclusive flock waited on shared flocks inherited by Playwright/Chromium'ssetsidtree. Stdio pipe draining can still hold a run open and is tracked separately in #485.Closes #544
Closes #396
🤖 Generated with Claude Code