refactor(execute): decompose spawn into single-concern modules#343
Merged
branchseer merged 10 commits intomainfrom Apr 17, 2026
Merged
refactor(execute): decompose spawn into single-concern modules#343branchseer merged 10 commits intomainfrom
branchseer merged 10 commits intomainfrom
Conversation
Decompose the ~200-line `spawn()` that mixed fspy/tokio command
construction, Windows Job Object setup, FD_CLOEXEC workaround, stdio
piping, and cancellation into four focused modules:
- `spawn.rs`: only abstracts fspy vs tokio with a unified cancellation-aware
`wait` future; returns `ChildHandle { stdout, stderr, wait }`.
- `pipe.rs`: drains stdout/stderr with optional cache capture.
- `tracked_accesses.rs`: normalizes raw fspy accesses to workspace-relative
form.
- `win_job.rs`: extracted from `mod.rs`.
`execute_spawn` composes the pieces and measures duration end-to-end
(spawn no longer tracks time).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Compute `spawn_stdio` directly from the cache/suggestion condition instead of going through an intermediate `use_piped: bool`. Bundle the piped-mode state (reporter writers + optional cache capture buffer) into a local `Pipe` enum so related state lives in one variant; the `Inherited` arm drops `stdio_config` eagerly as before. Also derive Copy/Clone/PartialEq/Eq/Debug on `SpawnStdio` so callers can match on it and still pass it by value to `spawn()`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
branchseer
changed the title
The wait future captures `OwnedJobHandle` (a Win32 `HANDLE`, `*mut c_void`) on Windows, which isn't `Send`. `BoxFuture` demands `Send`, breaking the Windows build. The future is only ever awaited on a single thread (`execute_graph` doesn't `tokio::spawn`), so `LocalBoxFuture` is the correct shape — and avoids an `unsafe impl Send` on `OwnedJobHandle`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…` enum
Replace the scattered `fspy_enabled: bool`, `spawn_stdio`, `resolved_negatives`,
`std_outputs: Option<_>`, and the 2-variant `Pipe` enum with a single
`ExecutionMode<'a>` whose shape statically captures the four valid runtime
configurations:
- Cache enabled, fspy enabled, piped (forced)
- Cache enabled, fspy disabled, piped (forced)
- Cache disabled, piped (reporter suggestion)
- Cache disabled, inherited (reporter suggestion)
Invariants encoded in the type:
- fspy lives inside `Cached` only (fspy requires `includes_auto`).
- `Cached` always owns its `StdioConfig` (caching forces piped capture).
- `Uncached { stdio_config: None }` means inherited — the reporter's config
was dropped during mode construction to release `std::io::Stdout`.
`(spawn_stdio, fspy_enabled)` is now derived from `&mode` via a single
inline match at the `spawn()` call site (the pair is never needed apart).
After `spawn()` the mode is consumed to drain pipes and extract the
cached-only state into a `CacheState` that feeds the cache-update phase.
Incidentally, negative globs are now compiled only when fspy is actually
enabled (previously compiled for any cache-enabled task, though only used
for fspy normalization).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`CacheState` duplicated four of the five fields of the old
`ExecutionMode::Cached` variant — the only field that belonged solely
to the mode was `stdio_config` (consumed during drain). Make `CacheState`
a single nested field so the variant is `Cached { stdio_config, state }`
and the drain match can just return `state` unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Split `mode` into `(stdio_config, cache_state)` first, then run the drain once. `stdio_config.is_some()` decides whether to drain; `cache_state`'s `std_outputs` supplies the optional capture buffer. Collapses the two near-identical match arms into one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduce `pipe::PipeIo { stdio_config, capture }` and change `pipe_stdio`'s
signature to take `&mut PipeIo` instead of two writers plus an
`Option<&mut Vec<StdOutput>>`. The capture buffer now lives with the
writers it feeds, owned by `PipeIo`.
`execute_spawn` step 7 builds `(Option<PipeIo>, Option<CacheState>)` from
the mode — the drain call stays a single site, and the captured outputs
are pulled back out of `PipeIo::capture` after drain for cache update.
`std_outputs` no longer lives in `CacheState`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`std_outputs` belongs with the rest of the cached state that feeds the cache update. Add it back to `CacheState` as a non-optional `Vec`, initialize it empty at mode construction, and move the captured buffer out of `PipeIo::capture` into `CacheState::std_outputs` after drain. The invariant (cached mode ⟹ capture was initialized) is asserted via `.expect` at the transfer point. The cache-update destructure now just pulls `std_outputs` straight from `CacheState` — no `Option` unwrapping at the use site. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Preserves the user's split of `StdioConfig` into `{ suggestion, writers:
PipeWriters }` and the borrowed-reference shape of `PipeIos<'a>`.
Renames to `PipeSinks` — "sinks" mirrors `PipeWriters` and reads better
than the awkward "Ios" plural. Propagates the `StdioConfig` → `{ writers:
PipeWriters { .. } }` change through the four reporters (plain, grouped,
interleaved, labeled) that still built the old shape, and cleans up the
absolute-path import in `execute/mod.rs`.
Refreshes doc comments that went stale: the drain no longer moves a
buffer out of `PipeIo::capture` — it writes straight into
`state.std_outputs` via the borrow inside `PipeSinks`. Also adds a
reminder right before the `let mut mode` declaration so anyone adding a
sibling local pauses to consider putting the value inside a variant
instead.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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.
Summary
spawn()insession::execute::spawn(which mixed fspy/tokio command construction, Windows Job Object, FD_CLOEXEC workaround, stdio piping, and cancellation) into four focused files undersession::execute:spawn.rs— now just abstracts fspy vs tokio, returningChildHandle { stdout, stderr, wait }with a cancellation-awarewaitfuture.pipe.rs—pipe_stdio()drains stdout/stderr, with optional capture for cache replay. HousesStdOutput/OutputKind.tracked_accesses.rs—TrackedPathAccesses::from_raw()normalizes raw fspy accesses. HousesPathRead.win_job.rs— Windows Job Object RAII, moved out ofmod.rs.execute_spawncomposes the pieces and measures duration end-to-end (spawn no longer tracks time).Pipeenum (Piped { stdio_config, capture }/Inherited) so related state lives in one variant. TheInheritedarm dropsstdio_configeagerly, matching existing behavior.SpawnStdiodirectly from the cache/suggestion condition (no intermediateuse_piped: bool).SpawnStdionow derivesCopy/Clone/PartialEq/Eq/Debug.Test plan
just checkjust lintcargo test(all library/unit tests, includingmalformed_windows_drive_path_after_workspace_strip_is_ignoredwhich moved withnormalize_tracked_workspace_path)cargo test -p vite_task_bin --test e2e_snapshots— full e2e coverage (148 tests) exercising piped + fspy + cache miss/hit, inherited stdio, cancellation/process-tree kill, and read/write-overlap not-cached pathsjust lint-linux/just lint-windows(CI will cover)🤖 Generated with Claude Code