fix(fspy): replace the IPC file lock with an in-mapping close gate - #577
fix(fspy): replace the IPC file lock with an in-mapping close gate#577wan9chi wants to merge 1 commit into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
4f029ee to
cfb9f15
Compare
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".
| // Close the ipc channel now that the child has exited. We are not | ||
| // interested in path accesses from descendants after the main child | ||
| // has exited, and we do not wait for them either: closing is a | ||
| // single atomic operation that also fences out later writers. |
There was a problem hiding this comment.
On Linux, when the root leaves a descendant alive under the inherited seccomp filter, this close is never reached at root exit: the immediately preceding supervisor.stop().await calls Supervisor::stop, whose handling loop drains its JoinSet until every notify listener reaches EOF (crates/fspy_seccomp_unotify/src/supervisor/mod.rs:52-55,123-125). A leaked daemon therefore still blocks the task indefinitely even when its stdio is redirected, so the supervisor handlers must be cancelled or detached rather than awaited before closing the channel.
AGENTS.md reference: AGENTS.md:L147-L153
Useful? React with 👍 / 👎.
| # Changelog | ||
|
|
||
| - **Fixed** Automatic file-access tracking now backs its shared memory with a sparse temporary file on every platform, so tasks are still tracked inside coding-agent sandboxes that deny POSIX shared memory and Unix domain sockets ([#563](https://github.com/voidzero-dev/vite-task/issues/563), [#576](https://github.com/voidzero-dev/vite-task/pull/576)). | ||
| - **Changed** Automatic file-access tracking now stops the moment a task's root process exits. Accesses made by leftover descendants afterwards are no longer recorded and never delay `vp run`, and a task that still had a traced process mid-write at that instant is conservatively not cached ([#544](https://github.com/voidzero-dev/vite-task/issues/544), [#396](https://github.com/voidzero-dev/vite-task/issues/396)). |
There was a problem hiding this comment.
When a background descendant inherits the task's stdout or stderr, the claim that it can never delay vp run is not true for cached tasks: ExecutionMode::spawn_config forces cached executions to SpawnStdio::Piped (crates/vite_task/src/session/execute/mod.rs:218), and run_child awaits pipe_stdio to EOF before awaiting ChildHandle.wait (:593-608). The descendant keeps the pipe write end open after the root exits, so the run still hangs even though fspy's background wait has closed the gate; pipe draining needs to be coordinated with root-process termination.
Useful? React with 👍 / 👎.
cfb9f15 to
e67d3fa
Compare
e67d3fa to
b0ab415
Compare
b0ab415 to
73b40b8
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>
73b40b8 to
941a231
Compare
1 participant
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 keeping full write access to the mapping, so the receiver can read frames while a straggler mutates them — the root cause pinned in the #544 investigation.
This replaces the lock with a gate stored in the shared memory itself, where a writer cannot drop it while still being able to write: a claim is admitted and counted by one atomic word, and the runner's close is one
fetch_orat root-process exit that atomically fences all future claims and reports whether any write was still in flight. If none was — the overwhelmingly common case, since in-flight windows are microseconds and the runner closes milliseconds after the exit it observes viawait— every claim provably ran to completion and the memory is frozen. Otherwise the run is conservatively not cached. The closed bit and the count sharing one word is load-bearing: splitting them into two atomics would reintroduce the very race the file lock had.Semantics change: fspy no longer waits for lingering descendants after the root process exits, and accesses made after that exit are not tracked. This makes
vp run's post-task timing predictable — a task that leaks a daemon no longer blocks the read step — and treats post-exit accesses as what they are, racy with respect to the task's contract.That also fixes the hang where a cached task's tool leaves detached descendants: the receiver's exclusive flock used to wait on shared flocks inherited by Playwright/Chromium's
setsidtree. The runner never waits now, the browser tree's post-exit accesses are dropped by design, and the run caches normally.Closes #544
Closes #396
🤖 Generated with Claude Code