feat(pipeline-core): add fgumi-pipeline-core, the typed-step pipeline framework - #697
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded ChangesPipeline core
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PipelineBuilder
participant WorkerPool
participant Queue
participant PipelineSignal
Client->>PipelineBuilder: build and configure pipeline
PipelineBuilder->>WorkerPool: create contexts and workers
WorkerPool->>Queue: push and pop items
WorkerPool->>PipelineSignal: report completion, cancellation, or error
PipelineSignal-->>Client: return pipeline result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main-runall #697 +/- ##
==============================================
Coverage ? 93.86%
==============================================
Files ? 206
Lines ? 117683
Branches ? 0
==============================================
Hits ? 110468
Misses ? 7215
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai pause |
✅ Action performedReviews paused. |
b7fffea to
27d5d46
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 24
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fgumi-pipeline-core/src/builder.rs`:
- Around line 1320-1334: Update the monitor limitation documentation and both
referenced test comments to name ensure_monitor_visible_transports instead of
assert_monitor_visible_transports, and describe that it returns
PipelineError::MonitorBlindTransport in every build rather than only enforcing
the invariant through debug assertions. Preserve the existing explanation of
monitor visibility and production transport coverage.
- Around line 2636-2699: Update ParallelCountingSink to retain each popped input
value in shared storage instead of discarding it, then have
pipeline_run_with_threads_1_drains_chain,
pipeline_run_with_threads_4_drains_chain, and
detached_step_runs_and_drains_multithreaded sort the collected values and
compare them with the expected 1..=N sequence. Keep the existing count checks if
useful, but ensure the tests verify every emitted ordinal arrives exactly once.
- Around line 216-256: Register source steps with explicit input arity 0 in both
chain and append_source, instead of relying on register_step’s default arity of
1. Update the register_step calls in chain and append_source while preserving
their existing output arity and return behavior, so wiring an input into a
source is rejected at wire time.
- Around line 2848-2906: Strengthen both panic tests,
pipeline_run_reraises_worker_panic_single_threaded and
pipeline_run_reraises_worker_panic_with_monitor_enabled, by asserting that
catch_unwind returns the expected worker-panic payload rather than merely
checking result.is_err(). Preserve the existing setup while distinguishing the
intended PanickingSink panic from framework, spawn, or monitor-visibility
panics.
In `@crates/fgumi-pipeline-core/src/erased.rs`:
- Around line 851-903: The existing tests do not exercise cached handle
resolution or the Detached step kind. Add a test near the dispatch tests that
builds an AddOne consumer, invokes try_run_erased twice with the same
ErasedStepCtx, and verifies both dispatches succeed and produce the expected
outputs, thereby executing the cached resolve_input and resolve_outputs paths.
Extend both cached_kind_and_sticky_match_profile and
cached_kind_and_sticky_match_profile_step2 with Detached cases for false and
true.
In `@crates/fgumi-pipeline-core/src/handles.rs`:
- Around line 326-341: Split the documentation immediately before edge_metrics:
keep the build_branch description and its # Panics section attached to
build_branch, then start a separate doc comment for edge_metrics containing only
its per-edge metrics behavior. Ensure build_branch is documented and
edge_metrics no longer inherits unrelated panic documentation.
- Around line 629-642: Update the ordered transport path around ReorderStage and
its transport queue so memory is bounded by bytes, using ByteBoundedQueue or
equivalent byte accounting and a configured cap rather than an unbounded or
count-only queue. Keep the existing per-edge budget wiring through
BranchBudgetHandles and reorder_cap, and preserve the next_serial exemption.
In `@crates/fgumi-pipeline-core/src/header.rs`:
- Around line 23-48: Refactor HeaderHandle into producer-side HeaderSetter and
consumer-side HeaderReader halves, preserving one-shot header resolution.
Implement Drop for HeaderSetter so an unresolved slot is poisoned with a
descriptive io::Error, while normal set/poison completion suppresses the
fallback. Update HeaderHandle construction and accessors so consumers receive
HeaderReader and observe Some(Err(..)) instead of polling forever.
In `@crates/fgumi-pipeline-core/src/reorder.rs`:
- Around line 81-97: Ensure single-input Serial, Exclusive, and Detached steps
preserve BranchOrdering::ByItemOrdinal when receiving items from a None edge; do
not collapse the output ordering to None. Keep the reorder stage for these
paths, or explicitly enforce and document an ordered-input precondition where
that behavior is intended.
In `@crates/fgumi-pipeline-core/src/runtime/contexts.rs`:
- Around line 498-519: Strengthen assert_linear_chain_invariants by verifying
downstream handle identity, not only its BranchInputHandle<u32> type: create
distinct per-producer values, push each through ctx.outputs[i], and assert the
matching value is received from ctx.inputs[i + 1]. Import OutputHandles and
Single in the test module as needed, while retaining the existing source and
type assertions.
In `@crates/fgumi-pipeline-core/src/runtime/detached.rs`:
- Around line 78-97: Update DetachedPlaceholder::profile() so it does not
silently return incomplete metadata: either make the method fail loudly when
called or preserve the original detached step’s output_queues and
branch_ordering metadata in the placeholder. Ensure extract_detached_steps
callers cannot receive misleading empty metadata for detached steps that declare
outputs.
In `@crates/fgumi-pipeline-core/src/runtime/drain.rs`:
- Around line 81-93: Update concurrent_decrements_have_exactly_one_winner so the
iterator first spawns and collects all thread handles, then joins those handles
in a separate step. Ensure every worker can execute observe_drain concurrently
before results are collected, preserving the exactly-one-winner assertion.
In `@crates/fgumi-pipeline-core/src/runtime/driver.rs`:
- Around line 1004-1053: Create a separate StepDrainCounter for the
pool-dispatch portion of driver_dispatch_records_detached_busy_by_own_step, and
pass it to the is_driver=false dispatch_one_step call so that both dispatches
exercise the output-close path independently.
In `@crates/fgumi-pipeline-core/src/runtime/pool.rs`:
- Around line 82-94: Update assign_sticky_owners to resolve each sticky step’s
worker through the shared Affinity::target_worker helper instead of matching
Affinity variants locally. Preserve skipping affinities without a target and
only assign sticky[target] when the resolved target is within n_workers and
currently unset, keeping the existing StepIdx assignment behavior.
In `@crates/fgumi-pipeline-core/src/runtime/storage.rs`:
- Around line 136-146: Update the StepKind::Exclusive arm in
build_worker_storage to assert that owner is less than the worker count before
indexing entries[owner], matching the always-on range validation used by the
Serial arm. Keep the existing owner-assignment expectation and worker-entry
replacement behavior unchanged, but provide a clear diagnostic for out-of-range
owners.
In `@crates/fgumi-pipeline-core/src/tests.rs`:
- Around line 540-544: Update the HeapSize implementations for OrderedU32 and
OrderedU64 so each item reports a realistic nonzero heap payload instead of 0,
then adjust the ByteBounded limits in both output queue specifications so the
test’s item count exceeds those limits. Preserve the test setup while ensuring
pushes reach the byte cap and force the step to retry.
- Around line 309-312: The direct Pipeline::run calls in tests.rs need a shared
timeout harness to prevent deadlocks from hanging the test suite. Extract the
spawn-and-recv_timeout logic from run_reports_finished_pipeline into a small
closure-taking helper, then use that helper for the PairSummer run at
crates/fgumi-pipeline-core/src/tests.rs lines 309-312, the OrderedPairSummer run
at lines 686-687, and the existing run_reports_finished_pipeline test; preserve
the "DEADLOCKED or stalled at --threads 4" failure message for the PairSummer
case.
- Around line 314-327: Update both Step2 end-to-end assertions in
crates/fgumi-pipeline-core/src/tests.rs at lines 314-327 and 691-697: remove
sort_unstable, assert the collected values in emit order as [55, 44, 33, 22, 11]
and [10, 8, 6, 4, 2] respectively, and revise the comments to state that Serial
dispatch preserves FIFO pairing and output order. If either sequence is unstable
with threads: 4, leave the test unsorted and report the ordering issue rather
than masking it.
- Around line 142-157: Update
step2_build_two_input_handles_rejects_duplicate_producer to describe the actual
invariant: the same producer index is allowed only when the two branch indices
differ. Add a positive test using a producer output with two branches, call
build_two_input_handles with the same producer index and distinct branches, and
assert it succeeds; retain the existing panic case for identical producer and
branch indices.
- Around line 53-61: Update SumPairStep::try_run to avoid eagerly popping both
branches and discarding a value when only one is available. Reuse the
pending-item buffering approach documented by PairSummer, retaining any
unmatched item until the other branch provides a value while preserving Progress
and NoProgress outcomes.
- Around line 576-582: Update the push handling in OrderedSource and
OrderedPairSummer to assert or otherwise propagate failure from
ctx.outputs.push(...) instead of discarding its result. Preserve the existing
decrement and ordinal behavior only when the item is successfully enqueued,
matching WideQueueSource’s rejected-push handling so queue rejection cannot
produce Progress with a missing item.
- Around line 445-459: Remove the `new_worker_copy` override from the `Serial`
step implementation, and update the stale `DrainReproSource` reference
associated with this logic to reflect that serial steps use one shared instance.
Preserve the existing completion and buffering behavior in the surrounding step
logic.
In `@crates/fgumi-pipeline-core/src/topology.rs`:
- Around line 267-295: Add a test alongside
branch_offsets_route_multi_branch_consumers_correctly that creates two producers
and a two-input consumer, wires them with wire_to_slot into slots 0 and 1, and
asserts consumer_input_slot returns the matching slot for each edge and
input_arity reports 2.
In `@crates/fgumi-pipeline-core/tests/compile_fail.rs`:
- Around line 8-12: Update pipeline_core_compile_fail to verify that
tests/compile-fail/*.rs matches at least one fixture before invoking trybuild,
failing explicitly when the set is empty; also replace the “Phase 0 design (line
530)” reference with the specific document path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3fb309e2-8a42-4191-8363-e1909981d0bc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (43)
.coderabbit.yaml.github/workflows/miri.yml.github/workflows/publish.ymlCLAUDE.mdCargo.tomlcrates/fgumi-cli-macros/Cargo.tomlcrates/fgumi-pipeline-core/Cargo.tomlcrates/fgumi-pipeline-core/src/builder.rscrates/fgumi-pipeline-core/src/erased.rscrates/fgumi-pipeline-core/src/finalize.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-core/src/header.rscrates/fgumi-pipeline-core/src/held.rscrates/fgumi-pipeline-core/src/item.rscrates/fgumi-pipeline-core/src/lib.rscrates/fgumi-pipeline-core/src/outputs.rscrates/fgumi-pipeline-core/src/queues.rscrates/fgumi-pipeline-core/src/reorder.rscrates/fgumi-pipeline-core/src/runtime/contexts.rscrates/fgumi-pipeline-core/src/runtime/detached.rscrates/fgumi-pipeline-core/src/runtime/drain.rscrates/fgumi-pipeline-core/src/runtime/driver.rscrates/fgumi-pipeline-core/src/runtime/fused.rscrates/fgumi-pipeline-core/src/runtime/live.rscrates/fgumi-pipeline-core/src/runtime/metrics.rscrates/fgumi-pipeline-core/src/runtime/mod.rscrates/fgumi-pipeline-core/src/runtime/pool.rscrates/fgumi-pipeline-core/src/runtime/sampler.rscrates/fgumi-pipeline-core/src/runtime/scheduler.rscrates/fgumi-pipeline-core/src/runtime/stats.rscrates/fgumi-pipeline-core/src/runtime/storage.rscrates/fgumi-pipeline-core/src/runtime/worker_core.rscrates/fgumi-pipeline-core/src/signal.rscrates/fgumi-pipeline-core/src/step.rscrates/fgumi-pipeline-core/src/tests.rscrates/fgumi-pipeline-core/src/topology.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.stderrcrates/fgumi-pipeline-core/tests/compile_fail.rs
27d5d46 to
986059e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/miri.yml:
- Around line 72-84: Update the Miri workflow step around the command for the
erased test filter to first run cargo with “-- --list”, count entries ending in
“: test”, and fail the step when the count is zero; only invoke the existing
Miri test command when at least one matching test is discovered.
In `@CLAUDE.md`:
- Around line 225-234: Update the cached-hit handle identity checks described in
the TypedStep cache path so mismatches are rejected unconditionally in release
builds, rather than relying solely on the four debug_assert! checks. Preserve
each debug_assert! as an additional diagnostic, and ensure a different
ChainContexts or handle identity cannot use the stale transmuted reference.
In `@crates/fgumi-pipeline-core/src/handles.rs`:
- Around line 199-207: Update the Ordered branch handling in the Unpushed
conversion logic so the ordinal=None invariant violation always fails loudly in
release builds; remove the self.push(item) fallback that allocates a new
ordinal, while preserving the existing debug assertion context and Direct-branch
behavior.
In `@crates/fgumi-pipeline-core/src/runtime/contexts.rs`:
- Around line 313-328: Update find_producer and/or build_consumer_map to detect
multiple producer branches targeting a consumer whose input_arity is 1, and
reject the graph instead of retaining only the first edge. Preserve the existing
producer lookup for valid graphs while ensuring duplicate incoming edges cannot
leave an unpopped branch during context construction.
In `@crates/fgumi-pipeline-core/src/runtime/driver.rs`:
- Around line 105-149: The sticky loop around dispatch_one_step must not retry
indefinitely when Progress leaves the output held. Bound sticky retries and
ensure a held-output Progress yields to downstream/round-robin scheduling
without immediately restarting the sticky owner; preserve normal sticky
continuation when progress can proceed. Add a one-worker regression test using a
full bounded output and a draining downstream step to verify forward progress.
In `@crates/fgumi-pipeline-core/src/runtime/fused.rs`:
- Around line 129-152: Update build_fused_output_set so fused transport queues
retain each profile’s configured count or byte bound instead of replacing it
with QueueSpec::Unbounded. Preserve the existing no-reorder optimization and
ensure run_fused_single_thread continues using the bounded queue specifications.
In `@crates/fgumi-pipeline-core/src/runtime/stats.rs`:
- Around line 24-33: The module documentation in the “What’s not recorded yet”
section is stale: remove or revise the queue-depth bullet to reflect that
runtime::sampler::sample_once records depth through EdgeMetrics::record_depth
and snapshot_with_edges exposes edge occupancy, without claiming
InputHandle::depth() is still required.
- Around line 641-659: Update StatsSnapshot::detached and
PipelineStats::detached_snapshot to carry the detached step index as the first
tuple field, and adjust write_detached plus its documentation for the new shape.
In the SPIN detection loop, match detached entries by StepIdx rather than step
name so duplicate names remain independent. Update
verdict_skips_spin_for_detached_steps and any affected tuple destructuring to
assert the index-based behavior.
In `@crates/fgumi-pipeline-core/src/tests.rs`:
- Around line 725-727: Update the doc comment for the Step2 merger test near the
ordered-input description to state that the test preserves and verifies the
emitted order. Remove the claims that cross-branch pairing is interleave-based
and that the assertion checks only a multiset, while leaving the exact ordered
assertion unchanged.
- Around line 548-559: Update run_with_deadlock_timeout to distinguish channel
disconnection from an actual timeout: when recv_timeout reports disconnection,
join the worker handle immediately so a panic in build_and_run is propagated;
only report the deadlock/stalled assertion for a true timeout, while preserving
successful completion handling.
- Around line 331-342: Update the completion guards in both the shown step
implementation and OrderedPairSummer so StepOutcome::Finished is returned only
when ctx.a and ctx.b are drained and pending_a and pending_b are both None.
Preserve the existing pair-processing and NoProgress behavior, ensuring any
buffered item is handled before completion.
In
`@crates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.stderr`:
- Around line 12-21: Remove the rustc trait-implementation help blocks from both
compile-fail fixtures: in
crates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.stderr
at lines 12-21 and 39-48, remove both HeapSize inventory copies; in
crates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.stderr
at lines 12-16 and 34-38, remove both Ordered-for-Sequenced copies. Preserve the
E0277 diagnostics and the required by a bound in OrderedBytesSingle notes.
- Around line 12-21: Remove the inventory-sensitive “other types implement trait
HeapSize” lists from both compile-fail diagnostic expectations, while preserving
the E0277 errors and the “required by a bound in OrderedBytesSingle” notes. Keep
the normalized “and $N others” placeholder only if it remains part of the
retained diagnostic output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 461ff2ba-e3c8-490e-aa6d-b73a22d60930
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (43)
.coderabbit.yaml.github/workflows/miri.yml.github/workflows/publish.ymlCLAUDE.mdCargo.tomlcrates/fgumi-cli-macros/Cargo.tomlcrates/fgumi-pipeline-core/Cargo.tomlcrates/fgumi-pipeline-core/src/builder.rscrates/fgumi-pipeline-core/src/erased.rscrates/fgumi-pipeline-core/src/finalize.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-core/src/header.rscrates/fgumi-pipeline-core/src/held.rscrates/fgumi-pipeline-core/src/item.rscrates/fgumi-pipeline-core/src/lib.rscrates/fgumi-pipeline-core/src/outputs.rscrates/fgumi-pipeline-core/src/queues.rscrates/fgumi-pipeline-core/src/reorder.rscrates/fgumi-pipeline-core/src/runtime/contexts.rscrates/fgumi-pipeline-core/src/runtime/detached.rscrates/fgumi-pipeline-core/src/runtime/drain.rscrates/fgumi-pipeline-core/src/runtime/driver.rscrates/fgumi-pipeline-core/src/runtime/fused.rscrates/fgumi-pipeline-core/src/runtime/live.rscrates/fgumi-pipeline-core/src/runtime/metrics.rscrates/fgumi-pipeline-core/src/runtime/mod.rscrates/fgumi-pipeline-core/src/runtime/pool.rscrates/fgumi-pipeline-core/src/runtime/sampler.rscrates/fgumi-pipeline-core/src/runtime/scheduler.rscrates/fgumi-pipeline-core/src/runtime/stats.rscrates/fgumi-pipeline-core/src/runtime/storage.rscrates/fgumi-pipeline-core/src/runtime/worker_core.rscrates/fgumi-pipeline-core/src/signal.rscrates/fgumi-pipeline-core/src/step.rscrates/fgumi-pipeline-core/src/tests.rscrates/fgumi-pipeline-core/src/topology.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.stderrcrates/fgumi-pipeline-core/tests/compile_fail.rs
986059e to
4e849b1
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
4e849b1 to
76a325b
Compare
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/miri.yml:
- Around line 86-112: Add a pre-run location assertion in the Miri workflow step
that verifies every approved unsafe site remains within the erased module,
rather than only checking that the erased test filter matches tests. Validate
the source locations of the four targeted #[allow(unsafe_code)] sites and fail
with a clear error if any moves outside erased; keep the existing test-count
guard and Miri command unchanged.
In `@crates/fgumi-pipeline-core/src/erased.rs`:
- Around line 835-839: Update the chain-building code around
StepOutputs::build_queues to bind self.inner.profile() once, then reuse that
profile for both output_queues and branch_ordering instead of calling profile()
separately. Match the existing single-input adapter’s one-time profile binding
pattern.
In `@crates/fgumi-pipeline-core/src/handles.rs`:
- Around line 1383-1384: Replace every plain tuple builder’s branch construction
with build_branch_byte_aware: update the two calls at
crates/fgumi-pipeline-core/src/handles.rs lines 1383-1384, the three calls at
lines 1451-1453, and the four calls at lines 1533-1536. This must cover all
branches while preserving build_branch behavior for non-byte queue
specifications.
In `@crates/fgumi-pipeline-core/src/queues.rs`:
- Around line 530-548: Update the byte_bounded_slot_cap_reject_is_observable
test to fill all BYTE_BOUNDED_QUEUE_SLOT_CAPACITY slots with nonzero-size Heavy
values, using a sufficiently large byte limit. Capture current_bytes() before
the extra push, assert that the slot-cap rejection occurs, and verify
current_bytes() remains unchanged afterward to cover byte rollback.
In `@crates/fgumi-pipeline-core/src/runtime/sampler.rs`:
- Around line 40-47: The sampler loop currently reads ordered-edge depth
separately through sample_once and write_row, causing two state-lock
acquisitions and potentially inconsistent values. Update the sampler flow around
sample_once and write_row to compute each edge’s buffer depth once per tick,
pass the captured depths to both consumers, and revise the module documentation
to reflect the resulting lock/read behavior; preserve existing sampling and
trace output semantics.
In `@crates/fgumi-pipeline-core/src/runtime/scheduler.rs`:
- Around line 20-22: Correct the module documentation near run_worker_loop to
state that sources and sinks remain on the sticky fast-path while the sticky
owner is still included in the round_robin_dispatch walk; only its priority
restart is suppressed. Remove the claim that sticky steps are unaffected or
skipped by the round-robin scheduling, including under Reverse ordering.
In `@crates/fgumi-pipeline-core/src/runtime/storage.rs`:
- Around line 178-188: Update affinity_in_range to validate the resolved worker
returned by Affinity::target_worker against n_workers, rather than matching
affinity variants directly. Preserve the predicate’s existing boolean behavior
and use target_worker as the sole affinity-to-worker mapping.
- Around line 379-387: Add a test alongside
serial_out_of_range_worker_panics_in_storage that constructs an Exclusive step
with an out-of-range Worker affinity, invokes build_worker_storage with the same
invalid owner index, and uses should_panic to assert the “Exclusive owner-range”
failure message, ensuring the always-on assert remains covered.
In `@crates/fgumi-pipeline-core/src/step.rs`:
- Around line 59-90: Update the `Affinity` enum documentation to state that
affinity hints are ignored for `Parallel`, `Exclusive`, and `Detached` step
kinds, matching the behavior in `build_worker_storage` where affinity is handled
only for `StepKind::Serial`.
In `@crates/fgumi-pipeline-core/src/tests.rs`:
- Around line 724-751: Update OrderedPairSummer::try_run so rejected
ctx.outputs.push operations return StepOutcome::NoProgress instead of
StepOutcome::Contention in both the held-item retry branch and the newly
generated item branch, while preserving the existing held-item retry behavior
and item retention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08cd39cc-a3ee-415c-9058-a1d50057f778
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (43)
.coderabbit.yaml.github/workflows/miri.yml.github/workflows/publish.ymlCLAUDE.mdCargo.tomlcrates/fgumi-cli-macros/Cargo.tomlcrates/fgumi-pipeline-core/Cargo.tomlcrates/fgumi-pipeline-core/src/builder.rscrates/fgumi-pipeline-core/src/erased.rscrates/fgumi-pipeline-core/src/finalize.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-core/src/header.rscrates/fgumi-pipeline-core/src/held.rscrates/fgumi-pipeline-core/src/item.rscrates/fgumi-pipeline-core/src/lib.rscrates/fgumi-pipeline-core/src/outputs.rscrates/fgumi-pipeline-core/src/queues.rscrates/fgumi-pipeline-core/src/reorder.rscrates/fgumi-pipeline-core/src/runtime/contexts.rscrates/fgumi-pipeline-core/src/runtime/detached.rscrates/fgumi-pipeline-core/src/runtime/drain.rscrates/fgumi-pipeline-core/src/runtime/driver.rscrates/fgumi-pipeline-core/src/runtime/fused.rscrates/fgumi-pipeline-core/src/runtime/live.rscrates/fgumi-pipeline-core/src/runtime/metrics.rscrates/fgumi-pipeline-core/src/runtime/mod.rscrates/fgumi-pipeline-core/src/runtime/pool.rscrates/fgumi-pipeline-core/src/runtime/sampler.rscrates/fgumi-pipeline-core/src/runtime/scheduler.rscrates/fgumi-pipeline-core/src/runtime/stats.rscrates/fgumi-pipeline-core/src/runtime/storage.rscrates/fgumi-pipeline-core/src/runtime/worker_core.rscrates/fgumi-pipeline-core/src/signal.rscrates/fgumi-pipeline-core/src/step.rscrates/fgumi-pipeline-core/src/tests.rscrates/fgumi-pipeline-core/src/topology.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.stderrcrates/fgumi-pipeline-core/tests/compile_fail.rs
76a325b to
72c4efa
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fgumi-pipeline-core/src/handles.rs`:
- Around line 282-290: Update BranchInputHandle::pop so record_pop receives
heap_size() only for byte-bounded edges; pass zero for CountBoundedQueue and
UnboundedQueue cases. Preserve the existing record_empty behavior when no item
is popped and reordering is not blocked.
In `@crates/fgumi-pipeline-core/src/held.rs`:
- Around line 78-89: Update the double_put_panics test for HeldSlot::put to
assert the specific expected panic message rather than only checking that
catch_unwind returned an error. Match the established always-on guard pattern,
using the exact duplicate-put message emitted by put while preserving coverage
in both debug and release builds.
In `@crates/fgumi-pipeline-core/src/outputs.rs`:
- Around line 340-354: Add build_queues/mark_all_drained round-trip tests for
the missing Single<T> and OrderedBytesSingle<T> output shapes, alongside the
existing shape tests. In each test, verify one branch is created, its typed
input starts open, and StepOutputs::mark_all_drained closes it; keep the comment
accurate by ensuring all declared shapes are covered.
In `@crates/fgumi-pipeline-core/src/queues.rs`:
- Around line 325-332: Update ByteBoundedQueue::set_limit_bytes to clamp
new_limit to at least 1 before storing it, using the existing atomic limit_bytes
field and preserving the current relaxed ordering.
In `@crates/fgumi-pipeline-core/src/runtime/detached.rs`:
- Around line 578-608: Update detached_two_sided_no_deadlock to retain each
value popped by the consumer instead of only incrementing received, then assert
the sorted collected values match the sorted input multiset. Preserve the
existing completion and deadlock checks, and use the producer’s input values to
validate record identity despite retry-induced reordering.
- Around line 110-114: Update detached_group() to preserve and return the
original DetachedGroup label instead of unconditionally returning PerStep.
Extend the detached placeholder state and the extract_detached_steps swap site
so the real group, including Shared labels, is carried into each placeholder
while retaining the existing behavior for other metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6bcba3f9-2104-46ee-a0af-db9e2aee6edf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (43)
.coderabbit.yaml.github/workflows/miri.yml.github/workflows/publish.ymlCLAUDE.mdCargo.tomlcrates/fgumi-cli-macros/Cargo.tomlcrates/fgumi-pipeline-core/Cargo.tomlcrates/fgumi-pipeline-core/src/builder.rscrates/fgumi-pipeline-core/src/erased.rscrates/fgumi-pipeline-core/src/finalize.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-core/src/header.rscrates/fgumi-pipeline-core/src/held.rscrates/fgumi-pipeline-core/src/item.rscrates/fgumi-pipeline-core/src/lib.rscrates/fgumi-pipeline-core/src/outputs.rscrates/fgumi-pipeline-core/src/queues.rscrates/fgumi-pipeline-core/src/reorder.rscrates/fgumi-pipeline-core/src/runtime/contexts.rscrates/fgumi-pipeline-core/src/runtime/detached.rscrates/fgumi-pipeline-core/src/runtime/drain.rscrates/fgumi-pipeline-core/src/runtime/driver.rscrates/fgumi-pipeline-core/src/runtime/fused.rscrates/fgumi-pipeline-core/src/runtime/live.rscrates/fgumi-pipeline-core/src/runtime/metrics.rscrates/fgumi-pipeline-core/src/runtime/mod.rscrates/fgumi-pipeline-core/src/runtime/pool.rscrates/fgumi-pipeline-core/src/runtime/sampler.rscrates/fgumi-pipeline-core/src/runtime/scheduler.rscrates/fgumi-pipeline-core/src/runtime/stats.rscrates/fgumi-pipeline-core/src/runtime/storage.rscrates/fgumi-pipeline-core/src/runtime/worker_core.rscrates/fgumi-pipeline-core/src/signal.rscrates/fgumi-pipeline-core/src/step.rscrates/fgumi-pipeline-core/src/tests.rscrates/fgumi-pipeline-core/src/topology.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.stderrcrates/fgumi-pipeline-core/tests/compile_fail.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fgumi-pipeline-core/src/lib.rs`:
- Around line 7-10: Update the dependency summary in the crate-level
documentation to include anyhow alongside the existing documented dependencies,
reflecting the anyhow::Result import used by finalize.rs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3b16a72-f6a7-4158-9f9f-91bd660c4ec6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (43)
.coderabbit.yaml.github/workflows/miri.yml.github/workflows/publish.ymlCLAUDE.mdCargo.tomlcrates/fgumi-cli-macros/Cargo.tomlcrates/fgumi-pipeline-core/Cargo.tomlcrates/fgumi-pipeline-core/src/builder.rscrates/fgumi-pipeline-core/src/erased.rscrates/fgumi-pipeline-core/src/finalize.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-core/src/header.rscrates/fgumi-pipeline-core/src/held.rscrates/fgumi-pipeline-core/src/item.rscrates/fgumi-pipeline-core/src/lib.rscrates/fgumi-pipeline-core/src/outputs.rscrates/fgumi-pipeline-core/src/queues.rscrates/fgumi-pipeline-core/src/reorder.rscrates/fgumi-pipeline-core/src/runtime/contexts.rscrates/fgumi-pipeline-core/src/runtime/detached.rscrates/fgumi-pipeline-core/src/runtime/drain.rscrates/fgumi-pipeline-core/src/runtime/driver.rscrates/fgumi-pipeline-core/src/runtime/fused.rscrates/fgumi-pipeline-core/src/runtime/live.rscrates/fgumi-pipeline-core/src/runtime/metrics.rscrates/fgumi-pipeline-core/src/runtime/mod.rscrates/fgumi-pipeline-core/src/runtime/pool.rscrates/fgumi-pipeline-core/src/runtime/sampler.rscrates/fgumi-pipeline-core/src/runtime/scheduler.rscrates/fgumi-pipeline-core/src/runtime/stats.rscrates/fgumi-pipeline-core/src/runtime/storage.rscrates/fgumi-pipeline-core/src/runtime/worker_core.rscrates/fgumi-pipeline-core/src/signal.rscrates/fgumi-pipeline-core/src/step.rscrates/fgumi-pipeline-core/src/tests.rscrates/fgumi-pipeline-core/src/topology.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.stderrcrates/fgumi-pipeline-core/tests/compile_fail.rs
e019868 to
a581ae9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fgumi-pipeline-core/src/tests.rs`:
- Around line 1166-1187: Update the manifest header parsing in the dependency
collection loop so `[dependencies.<name>]` is recognized as a runtime dependency
and the `<name>` segment is added to `dependencies`. Preserve the existing
handling for `[dependencies]` and target-specific `*.dependencies` sections,
while continuing to exclude dev- and build-dependency sections.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08c48695-6c5d-4407-99b2-26ecd4297797
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (43)
.coderabbit.yaml.github/workflows/miri.yml.github/workflows/publish.ymlCLAUDE.mdCargo.tomlcrates/fgumi-cli-macros/Cargo.tomlcrates/fgumi-pipeline-core/Cargo.tomlcrates/fgumi-pipeline-core/src/builder.rscrates/fgumi-pipeline-core/src/erased.rscrates/fgumi-pipeline-core/src/finalize.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-core/src/header.rscrates/fgumi-pipeline-core/src/held.rscrates/fgumi-pipeline-core/src/item.rscrates/fgumi-pipeline-core/src/lib.rscrates/fgumi-pipeline-core/src/outputs.rscrates/fgumi-pipeline-core/src/queues.rscrates/fgumi-pipeline-core/src/reorder.rscrates/fgumi-pipeline-core/src/runtime/contexts.rscrates/fgumi-pipeline-core/src/runtime/detached.rscrates/fgumi-pipeline-core/src/runtime/drain.rscrates/fgumi-pipeline-core/src/runtime/driver.rscrates/fgumi-pipeline-core/src/runtime/fused.rscrates/fgumi-pipeline-core/src/runtime/live.rscrates/fgumi-pipeline-core/src/runtime/metrics.rscrates/fgumi-pipeline-core/src/runtime/mod.rscrates/fgumi-pipeline-core/src/runtime/pool.rscrates/fgumi-pipeline-core/src/runtime/sampler.rscrates/fgumi-pipeline-core/src/runtime/scheduler.rscrates/fgumi-pipeline-core/src/runtime/stats.rscrates/fgumi-pipeline-core/src/runtime/storage.rscrates/fgumi-pipeline-core/src/runtime/worker_core.rscrates/fgumi-pipeline-core/src/signal.rscrates/fgumi-pipeline-core/src/step.rscrates/fgumi-pipeline-core/src/tests.rscrates/fgumi-pipeline-core/src/topology.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.rscrates/fgumi-pipeline-core/tests/compile-fail/chain_input_type_mismatch.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_heapsize.stderrcrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.rscrates/fgumi-pipeline-core/tests/compile-fail/ordered_bytes_single_requires_ordered.stderrcrates/fgumi-pipeline-core/tests/compile_fail.rs
… framework
Adds the `Step`/`Step2` traits, the bounded queue layer, the reorder stage
and the work-stealing runtime that schedules and drives them, as a leaf
crate. Additive: nothing on the branch changes behaviour and the crate has
no in-tree consumer yet.
The crate holds nothing that reads or writes sequencing data, so it depends
only on ahash, crossbeam-queue, parking_lot, log and one noodles::sam type.
Changes made while porting:
- Dependencies moved onto `[workspace.dependencies]`. `trybuild` gains a
second consumer here, so it is centralized too and `fgumi-cli-macros`
switches to `{ workspace = true }`.
- Five `collapsible_if` errors fixed as let-chains rather than suppressed.
- `fgumi-pipeline-core` added to `publish.yml`'s `CRATES` array; it is a
leaf, so any position before its future dependents is valid.
- The crate-root doc described the crate as an extraction re-exported by
`fgumi` as `pipeline::core`, which is true neither now nor after the
series lands; rewritten to describe the crate itself. Two references to
design docs that exist in no branch are dropped, and three pre-extraction
`core/*.rs` module paths become intra-doc links.
- Twenty rustdoc link defects fixed: nine unresolved links now carry crate
paths, eight public-doc links to private items are demoted to code spans,
three redundant explicit link targets shortened. `ci-doc` runs with
`-D warnings`; these never surfaced while this was a private module.
Review tooling follows the code out of `src/lib/`:
- `.coderabbit.yaml`'s pipeline `path_instructions` glob named only the
`src/lib/{unified_pipeline,pipeline}` homes, so extracting the engine
into a crate silently dropped it from review — including the clause about
new `unsafe` in the typed-step dispatch hot path, written for exactly the
sites this commit ships. Widened to cover the crate: 33 of the 42 changed
files are instructed now, against 4 before.
- `miri.yml` gains a step for `fgumi-pipeline-core::erased`, where all four
`#[allow(unsafe_code)]` sites live. It runs clean in ~6s. Both Miri steps
now list the filtered tests first and fail on an empty selection: `cargo
test <filter>` exits 0 when a filter matches nothing, so renaming either
module would have retired the gate silently. The rest of the
crate is excluded deliberately: its runtime tests spawn threads and take
Miri well past ten minutes, and two wedge tests use wall-clock watchdogs
that abort under Miri's slowdown.
The four `#[allow(unsafe_code)]` sites in `erased.rs` cache the per-dispatch
`downcast_ref` of a step's typed handles by widening `&'a` to `&'static` for
storage and narrowing it back on read. The invariant the compiler cannot
check -- that every dispatch passes the same handle box for a given step --
is enforced on the cached-hit path by storing the erased box's address in the
cache slot and asserting it unconditionally -- release included, since a
`debug_assert!` alone would leave release builds reading through a stale
pointer. The check is one load and one compare, not the `downcast_ref` `TypeId`
probe the cache exists to elide; a debug-only `debug_assert!` still re-resolves
the typed pointer as a second diagnostic. CLAUDE.md's
unsafe-code section gains an entry for them; note it documents all four,
where feat-runall's own entry describes only the two `TypedStep` sites and
omits the `TypedStep2` pair.
57 tests added over the 284 that arrive with the crate, all against public
API that had no coverage: `build_queues`/`mark_all_drained` round-trips for
every fan-out output shape (asserting branches are open before and closed
after, so they cannot pass on an inert `mark_all_drained`),
`Chain::into_multi` for the 3-, 4- and ordered-bytes-2 branch cases, the
type-erased `append_source`/`append_step`/`append_step2` assembly API,
`HeapSize` for String/Vec/Option, `BuildError` rendering, the
`PipelineConfig` setters, `Step2`'s trait defaults, and the two input-arity
dispatch guards.
Fixes from review, all in the pipeline-core runtime:
- `driver.rs`: a sticky step's `Progress` also means "held an item", so one
sitting on a full output reported it forever. The sticky fast path is now
bounded at `STICKY_BURST_LIMIT` re-entries AND round-robin no longer takes
the priority restart on the sticky owner's `Progress` -- with only one of
the two, the walk still breaks before reaching the consumer that drains
that output, and a one-worker chain livelocks. Both halves are pinned by
`sticky_holding_source_yields_to_its_draining_consumer`, which wedges
(watchdog abort) if either is reverted.
- `erased.rs`: `build_fused_output_set` forced `QueueSpec::Unbounded` on
every branch. The fused driver runs a producer before its consumer, so a
step emitting more per `try_run` than its consumer removes grew the edge
every pass, without limit. The profile's count/byte bound is kept; only
the ordering is dropped.
- `stats.rs`: the SPIN verdict exempted Detached steps by NAME while the
edge attribution beside it deliberately used `StepIdx`. `StatsSnapshot::
detached` now carries the index and the exemption matches on it, so
duplicate step names stay independent.
- `handles.rs`: `retry` on an `Ordered` branch with `ordinal = None`
re-pushed, allocating a fresh ordinal and abandoning the original -- a
permanent gap that stalls the consumer's `ReorderStage`. It now panics in
every build; the recoverable `Direct` sibling is left as-is.
- `contexts.rs`: `find_producer` returned the first matching edge, so two
producers wired to one single-input consumer left the second branch
unpopped. It now rejects that graph, mirroring the per-slot uniqueness
check `find_all_producers` already had.
- `tests.rs`: `PairSummer`/`OrderedPairSummer` reported `Finished` while a
branch item was still buffered, silently dropping it -- the exact loss
their own doc warns step authors about. `run_with_deadlock_timeout`
reported a panic inside the run as "DEADLOCKED or stalled"; it now
re-raises the original panic and blames a deadlock only on a true timeout.
- `compile_fail.rs`: documents that the `.stderr` fixtures pin rustc's whole
diagnostic, including its trait-impl inventory, and how to tell a re-bless
from a real regression. The inventory blocks cannot just be deleted --
trybuild compares for equality, so that fails the test outright.
Three follow-ons to those fixes, found by re-reading the call sites they
touched rather than by review:
- `queues.rs`: the same `debug_assert!`-with-a-data-losing-release-fallback
shape the `handles.rs` fix removed was live in all three transports'
`try_push` -- a push after `mark_drained` went into a queue the consumer
had already closed, so the item was silently dropped and surfaced only as a
short output. The check is now the unconditional `assert_not_drained`
helper, shared by `CountBoundedQueue`, `ByteBoundedQueue` and
`UnboundedQueue` and naming which transport failed. It loads `Relaxed`
rather than `Acquire`: `drained` is monotonic (its only write anywhere is
`store(true, Release)`), so a `Relaxed` load can miss a detection under a
cross-thread race but can never panic a correct program, and this now sits
on the per-item push path.
- `fused.rs` / `builder.rs`: with fused transports keeping their profile byte
bounds, `--queue-memory-total` was being silently ignored on that path in
favour of the per-step defaults -- the fused contexts are built inside
`run_fused_single_thread`, so `Pipeline::run` could not apply the budget on
their behalf. The budget is now threaded in and applied there;
`apply_initial_queue_budget` becomes `pub(crate)` for it.
- `builder.rs` / `step.rs`: two comments the above invalidated. The fused
fast-path comment justified skipping the rebalancer with "has no bounded
queues to rebalance", which stopped being true; it now gives the real
reason (producer-then-consumer in one pass, so no cross-worker imbalance)
and points at where the budget is applied. `step.rs`'s last-worker-barrier
note still said the `try_push`-after-drain violation was "debug-asserted".
Third review round:
- `handles.rs`: the 2-, 3- and 4-branch fan-out builders used the non-byte-aware
`build_branch`, which panics on `QueueSpec::ByteBounded`, so ANY fan-out step
declaring a byte bound failed to build. An armed deadlock monitor requires
`ByteBounded` on every output transport, so a monitored fan-out chain was
unbuildable. All nine calls now use `build_branch_byte_aware`, which delegates
non-byte specs straight back to `build_branch` -- count/unbounded branches are
unchanged. Three rstest cases assert the declared bound is registered on every
branch of every shape; all three panic without the swap.
- `sampler.rs`: each tick read every ordered edge's reorder stash TWICE -- once
for the histogram, once for the timeline row -- taking the `ReorderStage` state
mutex (held by every must-accept push and `try_pop_in_order`) on both, and the
two reads could disagree about one tick. Depths are now read once per tick via
`read_depths` and shared by both consumers. The module doc claimed "one
`Relaxed` load per edge per tick" and "never perturbs the worker hot path",
which was never true for ordered edges; it now describes the real cost.
- `miri.yml`: both steps now also assert that no `#[allow(unsafe_code)]` site has
moved out of the Miri-scoped module. The `--list` guard proves the filter
selects tests, not that it still covers the crate's `unsafe` -- a site that
moved would leave the filter matching and the site unchecked.
- `storage.rs`: `affinity_in_range` re-derived the affinity->worker mapping by
matching variants; it now routes through `Affinity::target_worker`, the
documented single source of truth, so the range check and the dispatch gate
cannot disagree. Added the missing `should_panic` case for the Exclusive
owner-range assert -- only the Serial arm was covered, so that always-on
`assert!` could have been weakened to a `debug_assert!` with CI still green.
- `queues.rs`: the slot-cap reject test filled with 0-byte items, exercising the
byte-reservation rollback with `size == 0` where a leak or double-subtract is
invisible. Added a nonzero-size case pinning `current_bytes` across the reject.
- Test-fixture steps returned `StepOutcome::Contention` when an output push was
rejected. The driver treats it like `NoProgress`, but it feeds
`contention_count`, from which the bottleneck verdict derives its SPIN ratio --
so ordinary backpressure was inventing mutex contention and could trip a bogus
SPIN finding. Fixed in all six sites: the two flagged in `tests.rs`
(`OrderedSource`, `OrderedPairSummer`) plus the same shape in `detached.rs` and
`builder.rs`, which the review did not name.
- Docs corrected where they contradicted the code: `scheduler.rs` claimed sticky
steps do not take part in the round-robin walk (they do -- only their `Progress`
priority restart is suppressed), `step.rs`'s `Affinity` omitted that `Detached`
also ignores the hint and is never range-checked, and `TypedStep2::
build_output_set` rebuilt `profile()` twice.
Fourth review round:
- `handles.rs`: `BranchInputHandle::pop` reported every popped item's
`heap_size()` to `record_pop`, but a `CountBoundedQueue` / `UnboundedQueue`
records `record_push(0)` by design (no byte accounting on its push path). So
count/unbounded edges carried nonzero `popped_bytes` against zero
`pushed_bytes`, and `compute_edge_stats` reported a `mibytes_per_s` throughput
for an edge that measures no bytes at all. The handle now carries
`record_item_bytes`, gated on the same `transport_handle.is_some()` signal
`ordered_branch` already used for the push side.
- `queues.rs`: `ByteBoundedQueue::set_limit_bytes` accepted 0, which makes
`try_push` reject unconditionally (`current_bytes >= 0`) and wedges the producer
forever. `new` asserts `limit_bytes > 0` for exactly that reason, so the setter
could undo the constructor's invariant. Now floored at 1 -- clamped rather than
asserted because this runs on a live pipeline, where a 1-byte limit still makes
progress and a panic would kill the run over a recoverable arithmetic slip.
- `detached.rs`: `DetachedPlaceholder::detached_group` returned `PerStep`
unconditionally, so a step that declared `DetachedGroup::Shared(..)` read back
as `PerStep`. `extract_detached_steps` is `pub` and leaves these placeholders in
the caller's slice, so an external caller regrouping from it would split one
shared group across separate driver threads. The placeholder now carries the
real label.
- `detached_two_sided_no_deadlock` asserted only that N items arrived, so a step
emitting N copies of one item -- or duplicating the held value while dropping a
popped one -- passed. It now collects the values and asserts the sorted multiset
(sorted, not sequenced: the producer's backpressure branch legitimately reorders
on retry).
- `outputs.rs`: the block comment claimed one `build_queues`/`mark_all_drained`
round-trip per output shape; `Single<T>`, `OrderedBytesSingle<T>` and the plain
`(A, B)` tuple had none -- and `OrderedBytesSingle<T>` is the only shape routing
through `build_single_queues_ordered_bytes`, so its drain path had no coverage
at all. All three added; every shape with a `StepOutputs` impl now has one.
- `held.rs`: `double_put_panics` used `catch_unwind`, which accepts any panic, so
a `put` failing for an unrelated reason would still pass while the double-put
guard itself had gone. Pinned with `#[should_panic(expected = ...)]`, matching
every other always-on guard here.
Fifth review round. Two substantive bugs, both in error/stall handling:
- `signal.rs`: `to_result` mapped a failed run to `Ok(())`. `record_error` wins
the state CAS before it runs `payload.set`, so `state == STATE_ERROR` with
`outcome() == None` is reachable; the `None` arm rescued only
`STATE_CANCELLED`, so the error case fell through to success. The cancel side
of this exact publish window already had a guard and a test -- the error side
was the sibling nobody closed. Now derived from the terminal state, which
cannot regress if a future driver forgets to join a writer. A successful run
never reaches the new arm: `is_done()` is `state != STATE_OK`.
- `fused.rs`: the driver failed the run on the FIRST pass in which no step
reported progress. `NoProgress` is the transient "input momentarily empty but
not drained" outcome -- a source waiting on a background reader returns it --
so a healthy run was aborted and its output truncated, and the
`debug_assert!` turned the same transient pass into a panic under test. Idle
passes now back off and retry against a wall-clock budget, and only a
sustained stall reports the error. Budgeted by time rather than by pass count
because `thread::sleep` granularity varies by platform. The loop also had no
backoff at all, so it span the calling thread at 100%.
Two findings were rejected as false positives, verified rather than assumed:
- An ordered non-byte-bounded branch was said to get an uncapped reorder stash
via `ReorderStage::new`. All seven `ordered_branch` call sites pass
`DEFAULT_REORDER_OVERFLOW_BYTES`, so that arm was unreachable from any branch
builder. Rather than only reply, the `Option<u64>` parameter is now a plain
`u64` and the dead unbounded arm is gone, so the question cannot be asked of
this code again.
- The `HeapSize` compile-fail fixture was said to risk "expected failure but
succeeded" if the bound lived only on the `StepOutputs` impl. It is on the
`OrderedBytesSingle<T>` declaration, and the fixture's pinned `.stderr` shows
the `E0277` plus the `required by a bound in OrderedBytesSingle` note.
The rest were accuracy and test-strength fixes:
- The cached-handle address assert was documented as though it proved box
identity. It compares data addresses only, so allocator address reuse defeats
it; soundness rests on the drop-order invariant. Corrected in `erased.rs` and
in CLAUDE.md's unsafe-code entry, which carried the same overclaim.
- `handles.rs`'s module doc still said tuple branches go through `build_branch`
and panic on `ByteBounded`; they have used `build_branch_byte_aware` since the
third round.
- The same-item-type fan-out shapes (`OrderedBytesTuple2/3<OrdU64, ..>`)
asserted only per-position downcasts and drain state, which an aliased branch
handle satisfies. They now route a distinct value through each branch.
- `maybe_instrumented` -- the constructor the branch builders actually call --
had no coverage on any of the three queue impls; one that dropped its
`Some(metrics)` would have produced a silently unmonitored edge.
- The edge-registry test asserted a count and `is_some()`, which swapped or
duplicated consumer labels satisfy; it now pins both producer->consumer pairs.
- `PipelineStats::record`/`record_error` index directly and panic on an
out-of-range `StepIdx` while the detached recorders no-op; the asymmetry is
deliberate and is now documented under `# Panics`.
- `pool.rs`'s `sticky_exclusive_step` test helper took an `owner_idx` it
discarded, with every call site passing 0.
- `OrderedSource` / `OrderedPairSummer` declare `BranchOrdering::None` on an
`OrderedBytesSingle` output, which maps to a direct branch -- so despite the
shape's name these steps never engage the reorder stage. That is deliberate
(the test reasons from FIFO order under byte backpressure) and is now stated,
with pointers to the three places reorder restoration is actually covered.
Sixth review round, four findings — the fused stall bound added last round, and
three tests that asserted less than their messages claimed:
- `fused.rs`: the stall budget was a hard-coded 5s, so a chain with a genuinely
slow source had no recourse from a library-chosen policy. It now comes from
`PipelineConfig::deadlock_timeout_secs`. `0` (the config default, meaning
"monitor disarmed" on the scheduled path) selects a built-in default here
rather than "unbounded", and the asymmetry is the point: the scheduled path
has a monitor to arm and this driver does not, so a disableable bound would
leave a fused wedge with nothing to detect it. Raising the number buys
patience; there is deliberately no way to remove the bound.
- `tests.rs`: the `build_two_input_handles` branch-mapping test asserted "branch
0 must feed input A and branch 1 input B" using `SumPairStep`, whose `a + b`
is commutative -- `10 + 1` and `1 + 10` are both 11, so a swapped wiring
passed while the message claimed the mapping was proven. Now uses a
non-commutative `a * 1000 + b`; verified by swapping the wiring, which yields
1010 against the expected 10001.
- `tests.rs`: `run_reports_finished_pipeline` sorted before comparing at every
thread count, discarding a real guarantee at `threads == 1`, where
`WideQueueSource` emits `n-1..0` through FIFO transports and one
`ReportsFinishedBuffer` draining its `VecDeque` front-first. The single-worker
case now asserts the exact descending sequence, so a reordering regression in
the flush-first path is visible; above one worker the multiset assertion
stands, because the Serial step's cross-worker re-dispatch interleaving is a
valid scheduling detail. Stability confirmed over 40 consecutive runs.
- `tests.rs`: `OrderedSink` recorded only `item.value`, leaving the out-ordinal
sequence unasserted -- and the ordinal is precisely what
`OrderedPairSummer`'s hold-and-retry path protects, since it assigns
`next_out_ordinal` before the push and must reuse it on a retry. A regression
that skipped or reassigned an ordinal left the value sequence intact and
passed. It now records `(ordinal, value)` and asserts both.
Seventh review round (a full re-review, not incremental), three findings:
- `lib.rs`: the crate-root re-exports omitted `DetachedGroup`, which
`Step::detached_group` returns while `Step` itself is flattened, and
`OrderedBytesTuple2` / `OrderedBytesTuple3`, which sat beside an exported
`Single` and `OrderedBytesSingle`. A step author implementing a `Detached`
step or a 2-/3-way ordered fan-out had to mix flattened paths with
`::step::` / `::outputs::` ones. All three added, pinned by a compile-time
test that names every step-author type at the crate root — it fails to build
if a `pub use` is dropped or a new output shape lands without one.
A sweep for the same shape found two more unexported `pub` types,
`queues::BoundedQueueHandle` and `reorder::ReorderCapHandle`, deliberately
left alone: they are runtime budget plumbing named only through
`runtime::contexts::RegisteredQueue`, which is not flattened either, and
`ByteBoundedQueue` exposes inherent `limit_bytes` / `set_limit_bytes` /
`current_bytes` so a direct user never needs the trait in scope. Promoting
them would make the root surface less coherent, not more. The test records
that decision so the next sweep does not re-litigate it.
- `fused.rs`: `DEFAULT_STALL_BUDGET` raised from 5s to 60s. Because
`deadlock_timeout_secs` defaults to 0, that default is what most fused runs
actually get, and 5s is short enough for a source whose background reader
blocks on a cold page cache or network-backed input to trip it — the idle
timer only resets on progress, so one slow read is enough to fail a healthy
run. The bound exists to catch a permanent wedge, not to police a slow
source, and the costs are asymmetric: a too-tight budget loses output, while
a generous one only delays the report of a wedge that has already hung.
- `tests.rs`: `OrderedSink` carried two doc blocks with `#[derive(Clone)]`
between them — the previous round's edit inserted the new doc after the
attribute and left the stale `/// Records summed values.` line above it.
Merged into one block before the attribute. A sweep for the same shape (a
doc comment following an attribute) found no other site.
a581ae9 to
f76e41e
Compare
… framework (#697) Adds the `Step`/`Step2` traits, the bounded queue layer, the reorder stage and the work-stealing runtime that schedules and drives them, as a leaf crate. Additive: nothing on the branch changes behaviour and the crate has no in-tree consumer yet. The crate holds nothing that reads or writes sequencing data, so it depends only on ahash, crossbeam-queue, parking_lot, log and one noodles::sam type. Changes made while porting: - Dependencies moved onto `[workspace.dependencies]`. `trybuild` gains a second consumer here, so it is centralized too and `fgumi-cli-macros` switches to `{ workspace = true }`. - Five `collapsible_if` errors fixed as let-chains rather than suppressed. - `fgumi-pipeline-core` added to `publish.yml`'s `CRATES` array; it is a leaf, so any position before its future dependents is valid. - The crate-root doc described the crate as an extraction re-exported by `fgumi` as `pipeline::core`, which is true neither now nor after the series lands; rewritten to describe the crate itself. Two references to design docs that exist in no branch are dropped, and three pre-extraction `core/*.rs` module paths become intra-doc links. - Twenty rustdoc link defects fixed: nine unresolved links now carry crate paths, eight public-doc links to private items are demoted to code spans, three redundant explicit link targets shortened. `ci-doc` runs with `-D warnings`; these never surfaced while this was a private module. Review tooling follows the code out of `src/lib/`: - `.coderabbit.yaml`'s pipeline `path_instructions` glob named only the `src/lib/{unified_pipeline,pipeline}` homes, so extracting the engine into a crate silently dropped it from review — including the clause about new `unsafe` in the typed-step dispatch hot path, written for exactly the sites this commit ships. Widened to cover the crate: 33 of the 42 changed files are instructed now, against 4 before. - `miri.yml` gains a step for `fgumi-pipeline-core::erased`, where all four `#[allow(unsafe_code)]` sites live. It runs clean in ~6s. Both Miri steps now list the filtered tests first and fail on an empty selection: `cargo test <filter>` exits 0 when a filter matches nothing, so renaming either module would have retired the gate silently. The rest of the crate is excluded deliberately: its runtime tests spawn threads and take Miri well past ten minutes, and two wedge tests use wall-clock watchdogs that abort under Miri's slowdown. The four `#[allow(unsafe_code)]` sites in `erased.rs` cache the per-dispatch `downcast_ref` of a step's typed handles by widening `&'a` to `&'static` for storage and narrowing it back on read. The invariant the compiler cannot check -- that every dispatch passes the same handle box for a given step -- is enforced on the cached-hit path by storing the erased box's address in the cache slot and asserting it unconditionally -- release included, since a `debug_assert!` alone would leave release builds reading through a stale pointer. The check is one load and one compare, not the `downcast_ref` `TypeId` probe the cache exists to elide; a debug-only `debug_assert!` still re-resolves the typed pointer as a second diagnostic. CLAUDE.md's unsafe-code section gains an entry for them; note it documents all four, where feat-runall's own entry describes only the two `TypedStep` sites and omits the `TypedStep2` pair. 57 tests added over the 284 that arrive with the crate, all against public API that had no coverage: `build_queues`/`mark_all_drained` round-trips for every fan-out output shape (asserting branches are open before and closed after, so they cannot pass on an inert `mark_all_drained`), `Chain::into_multi` for the 3-, 4- and ordered-bytes-2 branch cases, the type-erased `append_source`/`append_step`/`append_step2` assembly API, `HeapSize` for String/Vec/Option, `BuildError` rendering, the `PipelineConfig` setters, `Step2`'s trait defaults, and the two input-arity dispatch guards. Fixes from review, all in the pipeline-core runtime: - `driver.rs`: a sticky step's `Progress` also means "held an item", so one sitting on a full output reported it forever. The sticky fast path is now bounded at `STICKY_BURST_LIMIT` re-entries AND round-robin no longer takes the priority restart on the sticky owner's `Progress` -- with only one of the two, the walk still breaks before reaching the consumer that drains that output, and a one-worker chain livelocks. Both halves are pinned by `sticky_holding_source_yields_to_its_draining_consumer`, which wedges (watchdog abort) if either is reverted. - `erased.rs`: `build_fused_output_set` forced `QueueSpec::Unbounded` on every branch. The fused driver runs a producer before its consumer, so a step emitting more per `try_run` than its consumer removes grew the edge every pass, without limit. The profile's count/byte bound is kept; only the ordering is dropped. - `stats.rs`: the SPIN verdict exempted Detached steps by NAME while the edge attribution beside it deliberately used `StepIdx`. `StatsSnapshot:: detached` now carries the index and the exemption matches on it, so duplicate step names stay independent. - `handles.rs`: `retry` on an `Ordered` branch with `ordinal = None` re-pushed, allocating a fresh ordinal and abandoning the original -- a permanent gap that stalls the consumer's `ReorderStage`. It now panics in every build; the recoverable `Direct` sibling is left as-is. - `contexts.rs`: `find_producer` returned the first matching edge, so two producers wired to one single-input consumer left the second branch unpopped. It now rejects that graph, mirroring the per-slot uniqueness check `find_all_producers` already had. - `tests.rs`: `PairSummer`/`OrderedPairSummer` reported `Finished` while a branch item was still buffered, silently dropping it -- the exact loss their own doc warns step authors about. `run_with_deadlock_timeout` reported a panic inside the run as "DEADLOCKED or stalled"; it now re-raises the original panic and blames a deadlock only on a true timeout. - `compile_fail.rs`: documents that the `.stderr` fixtures pin rustc's whole diagnostic, including its trait-impl inventory, and how to tell a re-bless from a real regression. The inventory blocks cannot just be deleted -- trybuild compares for equality, so that fails the test outright. Three follow-ons to those fixes, found by re-reading the call sites they touched rather than by review: - `queues.rs`: the same `debug_assert!`-with-a-data-losing-release-fallback shape the `handles.rs` fix removed was live in all three transports' `try_push` -- a push after `mark_drained` went into a queue the consumer had already closed, so the item was silently dropped and surfaced only as a short output. The check is now the unconditional `assert_not_drained` helper, shared by `CountBoundedQueue`, `ByteBoundedQueue` and `UnboundedQueue` and naming which transport failed. It loads `Relaxed` rather than `Acquire`: `drained` is monotonic (its only write anywhere is `store(true, Release)`), so a `Relaxed` load can miss a detection under a cross-thread race but can never panic a correct program, and this now sits on the per-item push path. - `fused.rs` / `builder.rs`: with fused transports keeping their profile byte bounds, `--queue-memory-total` was being silently ignored on that path in favour of the per-step defaults -- the fused contexts are built inside `run_fused_single_thread`, so `Pipeline::run` could not apply the budget on their behalf. The budget is now threaded in and applied there; `apply_initial_queue_budget` becomes `pub(crate)` for it. - `builder.rs` / `step.rs`: two comments the above invalidated. The fused fast-path comment justified skipping the rebalancer with "has no bounded queues to rebalance", which stopped being true; it now gives the real reason (producer-then-consumer in one pass, so no cross-worker imbalance) and points at where the budget is applied. `step.rs`'s last-worker-barrier note still said the `try_push`-after-drain violation was "debug-asserted". Third review round: - `handles.rs`: the 2-, 3- and 4-branch fan-out builders used the non-byte-aware `build_branch`, which panics on `QueueSpec::ByteBounded`, so ANY fan-out step declaring a byte bound failed to build. An armed deadlock monitor requires `ByteBounded` on every output transport, so a monitored fan-out chain was unbuildable. All nine calls now use `build_branch_byte_aware`, which delegates non-byte specs straight back to `build_branch` -- count/unbounded branches are unchanged. Three rstest cases assert the declared bound is registered on every branch of every shape; all three panic without the swap. - `sampler.rs`: each tick read every ordered edge's reorder stash TWICE -- once for the histogram, once for the timeline row -- taking the `ReorderStage` state mutex (held by every must-accept push and `try_pop_in_order`) on both, and the two reads could disagree about one tick. Depths are now read once per tick via `read_depths` and shared by both consumers. The module doc claimed "one `Relaxed` load per edge per tick" and "never perturbs the worker hot path", which was never true for ordered edges; it now describes the real cost. - `miri.yml`: both steps now also assert that no `#[allow(unsafe_code)]` site has moved out of the Miri-scoped module. The `--list` guard proves the filter selects tests, not that it still covers the crate's `unsafe` -- a site that moved would leave the filter matching and the site unchecked. - `storage.rs`: `affinity_in_range` re-derived the affinity->worker mapping by matching variants; it now routes through `Affinity::target_worker`, the documented single source of truth, so the range check and the dispatch gate cannot disagree. Added the missing `should_panic` case for the Exclusive owner-range assert -- only the Serial arm was covered, so that always-on `assert!` could have been weakened to a `debug_assert!` with CI still green. - `queues.rs`: the slot-cap reject test filled with 0-byte items, exercising the byte-reservation rollback with `size == 0` where a leak or double-subtract is invisible. Added a nonzero-size case pinning `current_bytes` across the reject. - Test-fixture steps returned `StepOutcome::Contention` when an output push was rejected. The driver treats it like `NoProgress`, but it feeds `contention_count`, from which the bottleneck verdict derives its SPIN ratio -- so ordinary backpressure was inventing mutex contention and could trip a bogus SPIN finding. Fixed in all six sites: the two flagged in `tests.rs` (`OrderedSource`, `OrderedPairSummer`) plus the same shape in `detached.rs` and `builder.rs`, which the review did not name. - Docs corrected where they contradicted the code: `scheduler.rs` claimed sticky steps do not take part in the round-robin walk (they do -- only their `Progress` priority restart is suppressed), `step.rs`'s `Affinity` omitted that `Detached` also ignores the hint and is never range-checked, and `TypedStep2:: build_output_set` rebuilt `profile()` twice. Fourth review round: - `handles.rs`: `BranchInputHandle::pop` reported every popped item's `heap_size()` to `record_pop`, but a `CountBoundedQueue` / `UnboundedQueue` records `record_push(0)` by design (no byte accounting on its push path). So count/unbounded edges carried nonzero `popped_bytes` against zero `pushed_bytes`, and `compute_edge_stats` reported a `mibytes_per_s` throughput for an edge that measures no bytes at all. The handle now carries `record_item_bytes`, gated on the same `transport_handle.is_some()` signal `ordered_branch` already used for the push side. - `queues.rs`: `ByteBoundedQueue::set_limit_bytes` accepted 0, which makes `try_push` reject unconditionally (`current_bytes >= 0`) and wedges the producer forever. `new` asserts `limit_bytes > 0` for exactly that reason, so the setter could undo the constructor's invariant. Now floored at 1 -- clamped rather than asserted because this runs on a live pipeline, where a 1-byte limit still makes progress and a panic would kill the run over a recoverable arithmetic slip. - `detached.rs`: `DetachedPlaceholder::detached_group` returned `PerStep` unconditionally, so a step that declared `DetachedGroup::Shared(..)` read back as `PerStep`. `extract_detached_steps` is `pub` and leaves these placeholders in the caller's slice, so an external caller regrouping from it would split one shared group across separate driver threads. The placeholder now carries the real label. - `detached_two_sided_no_deadlock` asserted only that N items arrived, so a step emitting N copies of one item -- or duplicating the held value while dropping a popped one -- passed. It now collects the values and asserts the sorted multiset (sorted, not sequenced: the producer's backpressure branch legitimately reorders on retry). - `outputs.rs`: the block comment claimed one `build_queues`/`mark_all_drained` round-trip per output shape; `Single<T>`, `OrderedBytesSingle<T>` and the plain `(A, B)` tuple had none -- and `OrderedBytesSingle<T>` is the only shape routing through `build_single_queues_ordered_bytes`, so its drain path had no coverage at all. All three added; every shape with a `StepOutputs` impl now has one. - `held.rs`: `double_put_panics` used `catch_unwind`, which accepts any panic, so a `put` failing for an unrelated reason would still pass while the double-put guard itself had gone. Pinned with `#[should_panic(expected = ...)]`, matching every other always-on guard here. Fifth review round. Two substantive bugs, both in error/stall handling: - `signal.rs`: `to_result` mapped a failed run to `Ok(())`. `record_error` wins the state CAS before it runs `payload.set`, so `state == STATE_ERROR` with `outcome() == None` is reachable; the `None` arm rescued only `STATE_CANCELLED`, so the error case fell through to success. The cancel side of this exact publish window already had a guard and a test -- the error side was the sibling nobody closed. Now derived from the terminal state, which cannot regress if a future driver forgets to join a writer. A successful run never reaches the new arm: `is_done()` is `state != STATE_OK`. - `fused.rs`: the driver failed the run on the FIRST pass in which no step reported progress. `NoProgress` is the transient "input momentarily empty but not drained" outcome -- a source waiting on a background reader returns it -- so a healthy run was aborted and its output truncated, and the `debug_assert!` turned the same transient pass into a panic under test. Idle passes now back off and retry against a wall-clock budget, and only a sustained stall reports the error. Budgeted by time rather than by pass count because `thread::sleep` granularity varies by platform. The loop also had no backoff at all, so it span the calling thread at 100%. Two findings were rejected as false positives, verified rather than assumed: - An ordered non-byte-bounded branch was said to get an uncapped reorder stash via `ReorderStage::new`. All seven `ordered_branch` call sites pass `DEFAULT_REORDER_OVERFLOW_BYTES`, so that arm was unreachable from any branch builder. Rather than only reply, the `Option<u64>` parameter is now a plain `u64` and the dead unbounded arm is gone, so the question cannot be asked of this code again. - The `HeapSize` compile-fail fixture was said to risk "expected failure but succeeded" if the bound lived only on the `StepOutputs` impl. It is on the `OrderedBytesSingle<T>` declaration, and the fixture's pinned `.stderr` shows the `E0277` plus the `required by a bound in OrderedBytesSingle` note. The rest were accuracy and test-strength fixes: - The cached-handle address assert was documented as though it proved box identity. It compares data addresses only, so allocator address reuse defeats it; soundness rests on the drop-order invariant. Corrected in `erased.rs` and in CLAUDE.md's unsafe-code entry, which carried the same overclaim. - `handles.rs`'s module doc still said tuple branches go through `build_branch` and panic on `ByteBounded`; they have used `build_branch_byte_aware` since the third round. - The same-item-type fan-out shapes (`OrderedBytesTuple2/3<OrdU64, ..>`) asserted only per-position downcasts and drain state, which an aliased branch handle satisfies. They now route a distinct value through each branch. - `maybe_instrumented` -- the constructor the branch builders actually call -- had no coverage on any of the three queue impls; one that dropped its `Some(metrics)` would have produced a silently unmonitored edge. - The edge-registry test asserted a count and `is_some()`, which swapped or duplicated consumer labels satisfy; it now pins both producer->consumer pairs. - `PipelineStats::record`/`record_error` index directly and panic on an out-of-range `StepIdx` while the detached recorders no-op; the asymmetry is deliberate and is now documented under `# Panics`. - `pool.rs`'s `sticky_exclusive_step` test helper took an `owner_idx` it discarded, with every call site passing 0. - `OrderedSource` / `OrderedPairSummer` declare `BranchOrdering::None` on an `OrderedBytesSingle` output, which maps to a direct branch -- so despite the shape's name these steps never engage the reorder stage. That is deliberate (the test reasons from FIFO order under byte backpressure) and is now stated, with pointers to the three places reorder restoration is actually covered. Sixth review round, four findings — the fused stall bound added last round, and three tests that asserted less than their messages claimed: - `fused.rs`: the stall budget was a hard-coded 5s, so a chain with a genuinely slow source had no recourse from a library-chosen policy. It now comes from `PipelineConfig::deadlock_timeout_secs`. `0` (the config default, meaning "monitor disarmed" on the scheduled path) selects a built-in default here rather than "unbounded", and the asymmetry is the point: the scheduled path has a monitor to arm and this driver does not, so a disableable bound would leave a fused wedge with nothing to detect it. Raising the number buys patience; there is deliberately no way to remove the bound. - `tests.rs`: the `build_two_input_handles` branch-mapping test asserted "branch 0 must feed input A and branch 1 input B" using `SumPairStep`, whose `a + b` is commutative -- `10 + 1` and `1 + 10` are both 11, so a swapped wiring passed while the message claimed the mapping was proven. Now uses a non-commutative `a * 1000 + b`; verified by swapping the wiring, which yields 1010 against the expected 10001. - `tests.rs`: `run_reports_finished_pipeline` sorted before comparing at every thread count, discarding a real guarantee at `threads == 1`, where `WideQueueSource` emits `n-1..0` through FIFO transports and one `ReportsFinishedBuffer` draining its `VecDeque` front-first. The single-worker case now asserts the exact descending sequence, so a reordering regression in the flush-first path is visible; above one worker the multiset assertion stands, because the Serial step's cross-worker re-dispatch interleaving is a valid scheduling detail. Stability confirmed over 40 consecutive runs. - `tests.rs`: `OrderedSink` recorded only `item.value`, leaving the out-ordinal sequence unasserted -- and the ordinal is precisely what `OrderedPairSummer`'s hold-and-retry path protects, since it assigns `next_out_ordinal` before the push and must reuse it on a retry. A regression that skipped or reassigned an ordinal left the value sequence intact and passed. It now records `(ordinal, value)` and asserts both. Seventh review round (a full re-review, not incremental), three findings: - `lib.rs`: the crate-root re-exports omitted `DetachedGroup`, which `Step::detached_group` returns while `Step` itself is flattened, and `OrderedBytesTuple2` / `OrderedBytesTuple3`, which sat beside an exported `Single` and `OrderedBytesSingle`. A step author implementing a `Detached` step or a 2-/3-way ordered fan-out had to mix flattened paths with `::step::` / `::outputs::` ones. All three added, pinned by a compile-time test that names every step-author type at the crate root — it fails to build if a `pub use` is dropped or a new output shape lands without one. A sweep for the same shape found two more unexported `pub` types, `queues::BoundedQueueHandle` and `reorder::ReorderCapHandle`, deliberately left alone: they are runtime budget plumbing named only through `runtime::contexts::RegisteredQueue`, which is not flattened either, and `ByteBoundedQueue` exposes inherent `limit_bytes` / `set_limit_bytes` / `current_bytes` so a direct user never needs the trait in scope. Promoting them would make the root surface less coherent, not more. The test records that decision so the next sweep does not re-litigate it. - `fused.rs`: `DEFAULT_STALL_BUDGET` raised from 5s to 60s. Because `deadlock_timeout_secs` defaults to 0, that default is what most fused runs actually get, and 5s is short enough for a source whose background reader blocks on a cold page cache or network-backed input to trip it — the idle timer only resets on progress, so one slow read is enough to fail a healthy run. The bound exists to catch a permanent wedge, not to police a slow source, and the costs are asymmetric: a too-tight budget loses output, while a generous one only delays the report of a wedge that has already hung. - `tests.rs`: `OrderedSink` carried two doc blocks with `#[derive(Clone)]` between them — the previous round's edit inserted the new doc after the attribute and left the stale `/// Records summed values.` line above it. Merged into one block before the attribute. A sweep for the same shape (a doc comment following an attribute) found no other site.
… framework (#697) Adds the `Step`/`Step2` traits, the bounded queue layer, the reorder stage and the work-stealing runtime that schedules and drives them, as a leaf crate. Additive: nothing on the branch changes behaviour and the crate has no in-tree consumer yet. The crate holds nothing that reads or writes sequencing data, so it depends only on ahash, crossbeam-queue, parking_lot, log and one noodles::sam type. Changes made while porting: - Dependencies moved onto `[workspace.dependencies]`. `trybuild` gains a second consumer here, so it is centralized too and `fgumi-cli-macros` switches to `{ workspace = true }`. - Five `collapsible_if` errors fixed as let-chains rather than suppressed. - `fgumi-pipeline-core` added to `publish.yml`'s `CRATES` array; it is a leaf, so any position before its future dependents is valid. - The crate-root doc described the crate as an extraction re-exported by `fgumi` as `pipeline::core`, which is true neither now nor after the series lands; rewritten to describe the crate itself. Two references to design docs that exist in no branch are dropped, and three pre-extraction `core/*.rs` module paths become intra-doc links. - Twenty rustdoc link defects fixed: nine unresolved links now carry crate paths, eight public-doc links to private items are demoted to code spans, three redundant explicit link targets shortened. `ci-doc` runs with `-D warnings`; these never surfaced while this was a private module. Review tooling follows the code out of `src/lib/`: - `.coderabbit.yaml`'s pipeline `path_instructions` glob named only the `src/lib/{unified_pipeline,pipeline}` homes, so extracting the engine into a crate silently dropped it from review — including the clause about new `unsafe` in the typed-step dispatch hot path, written for exactly the sites this commit ships. Widened to cover the crate: 33 of the 42 changed files are instructed now, against 4 before. - `miri.yml` gains a step for `fgumi-pipeline-core::erased`, where all four `#[allow(unsafe_code)]` sites live. It runs clean in ~6s. Both Miri steps now list the filtered tests first and fail on an empty selection: `cargo test <filter>` exits 0 when a filter matches nothing, so renaming either module would have retired the gate silently. The rest of the crate is excluded deliberately: its runtime tests spawn threads and take Miri well past ten minutes, and two wedge tests use wall-clock watchdogs that abort under Miri's slowdown. The four `#[allow(unsafe_code)]` sites in `erased.rs` cache the per-dispatch `downcast_ref` of a step's typed handles by widening `&'a` to `&'static` for storage and narrowing it back on read. The invariant the compiler cannot check -- that every dispatch passes the same handle box for a given step -- is enforced on the cached-hit path by storing the erased box's address in the cache slot and asserting it unconditionally -- release included, since a `debug_assert!` alone would leave release builds reading through a stale pointer. The check is one load and one compare, not the `downcast_ref` `TypeId` probe the cache exists to elide; a debug-only `debug_assert!` still re-resolves the typed pointer as a second diagnostic. CLAUDE.md's unsafe-code section gains an entry for them; note it documents all four, where feat-runall's own entry describes only the two `TypedStep` sites and omits the `TypedStep2` pair. 57 tests added over the 284 that arrive with the crate, all against public API that had no coverage: `build_queues`/`mark_all_drained` round-trips for every fan-out output shape (asserting branches are open before and closed after, so they cannot pass on an inert `mark_all_drained`), `Chain::into_multi` for the 3-, 4- and ordered-bytes-2 branch cases, the type-erased `append_source`/`append_step`/`append_step2` assembly API, `HeapSize` for String/Vec/Option, `BuildError` rendering, the `PipelineConfig` setters, `Step2`'s trait defaults, and the two input-arity dispatch guards. Fixes from review, all in the pipeline-core runtime: - `driver.rs`: a sticky step's `Progress` also means "held an item", so one sitting on a full output reported it forever. The sticky fast path is now bounded at `STICKY_BURST_LIMIT` re-entries AND round-robin no longer takes the priority restart on the sticky owner's `Progress` -- with only one of the two, the walk still breaks before reaching the consumer that drains that output, and a one-worker chain livelocks. Both halves are pinned by `sticky_holding_source_yields_to_its_draining_consumer`, which wedges (watchdog abort) if either is reverted. - `erased.rs`: `build_fused_output_set` forced `QueueSpec::Unbounded` on every branch. The fused driver runs a producer before its consumer, so a step emitting more per `try_run` than its consumer removes grew the edge every pass, without limit. The profile's count/byte bound is kept; only the ordering is dropped. - `stats.rs`: the SPIN verdict exempted Detached steps by NAME while the edge attribution beside it deliberately used `StepIdx`. `StatsSnapshot:: detached` now carries the index and the exemption matches on it, so duplicate step names stay independent. - `handles.rs`: `retry` on an `Ordered` branch with `ordinal = None` re-pushed, allocating a fresh ordinal and abandoning the original -- a permanent gap that stalls the consumer's `ReorderStage`. It now panics in every build; the recoverable `Direct` sibling is left as-is. - `contexts.rs`: `find_producer` returned the first matching edge, so two producers wired to one single-input consumer left the second branch unpopped. It now rejects that graph, mirroring the per-slot uniqueness check `find_all_producers` already had. - `tests.rs`: `PairSummer`/`OrderedPairSummer` reported `Finished` while a branch item was still buffered, silently dropping it -- the exact loss their own doc warns step authors about. `run_with_deadlock_timeout` reported a panic inside the run as "DEADLOCKED or stalled"; it now re-raises the original panic and blames a deadlock only on a true timeout. - `compile_fail.rs`: documents that the `.stderr` fixtures pin rustc's whole diagnostic, including its trait-impl inventory, and how to tell a re-bless from a real regression. The inventory blocks cannot just be deleted -- trybuild compares for equality, so that fails the test outright. Three follow-ons to those fixes, found by re-reading the call sites they touched rather than by review: - `queues.rs`: the same `debug_assert!`-with-a-data-losing-release-fallback shape the `handles.rs` fix removed was live in all three transports' `try_push` -- a push after `mark_drained` went into a queue the consumer had already closed, so the item was silently dropped and surfaced only as a short output. The check is now the unconditional `assert_not_drained` helper, shared by `CountBoundedQueue`, `ByteBoundedQueue` and `UnboundedQueue` and naming which transport failed. It loads `Relaxed` rather than `Acquire`: `drained` is monotonic (its only write anywhere is `store(true, Release)`), so a `Relaxed` load can miss a detection under a cross-thread race but can never panic a correct program, and this now sits on the per-item push path. - `fused.rs` / `builder.rs`: with fused transports keeping their profile byte bounds, `--queue-memory-total` was being silently ignored on that path in favour of the per-step defaults -- the fused contexts are built inside `run_fused_single_thread`, so `Pipeline::run` could not apply the budget on their behalf. The budget is now threaded in and applied there; `apply_initial_queue_budget` becomes `pub(crate)` for it. - `builder.rs` / `step.rs`: two comments the above invalidated. The fused fast-path comment justified skipping the rebalancer with "has no bounded queues to rebalance", which stopped being true; it now gives the real reason (producer-then-consumer in one pass, so no cross-worker imbalance) and points at where the budget is applied. `step.rs`'s last-worker-barrier note still said the `try_push`-after-drain violation was "debug-asserted". Third review round: - `handles.rs`: the 2-, 3- and 4-branch fan-out builders used the non-byte-aware `build_branch`, which panics on `QueueSpec::ByteBounded`, so ANY fan-out step declaring a byte bound failed to build. An armed deadlock monitor requires `ByteBounded` on every output transport, so a monitored fan-out chain was unbuildable. All nine calls now use `build_branch_byte_aware`, which delegates non-byte specs straight back to `build_branch` -- count/unbounded branches are unchanged. Three rstest cases assert the declared bound is registered on every branch of every shape; all three panic without the swap. - `sampler.rs`: each tick read every ordered edge's reorder stash TWICE -- once for the histogram, once for the timeline row -- taking the `ReorderStage` state mutex (held by every must-accept push and `try_pop_in_order`) on both, and the two reads could disagree about one tick. Depths are now read once per tick via `read_depths` and shared by both consumers. The module doc claimed "one `Relaxed` load per edge per tick" and "never perturbs the worker hot path", which was never true for ordered edges; it now describes the real cost. - `miri.yml`: both steps now also assert that no `#[allow(unsafe_code)]` site has moved out of the Miri-scoped module. The `--list` guard proves the filter selects tests, not that it still covers the crate's `unsafe` -- a site that moved would leave the filter matching and the site unchecked. - `storage.rs`: `affinity_in_range` re-derived the affinity->worker mapping by matching variants; it now routes through `Affinity::target_worker`, the documented single source of truth, so the range check and the dispatch gate cannot disagree. Added the missing `should_panic` case for the Exclusive owner-range assert -- only the Serial arm was covered, so that always-on `assert!` could have been weakened to a `debug_assert!` with CI still green. - `queues.rs`: the slot-cap reject test filled with 0-byte items, exercising the byte-reservation rollback with `size == 0` where a leak or double-subtract is invisible. Added a nonzero-size case pinning `current_bytes` across the reject. - Test-fixture steps returned `StepOutcome::Contention` when an output push was rejected. The driver treats it like `NoProgress`, but it feeds `contention_count`, from which the bottleneck verdict derives its SPIN ratio -- so ordinary backpressure was inventing mutex contention and could trip a bogus SPIN finding. Fixed in all six sites: the two flagged in `tests.rs` (`OrderedSource`, `OrderedPairSummer`) plus the same shape in `detached.rs` and `builder.rs`, which the review did not name. - Docs corrected where they contradicted the code: `scheduler.rs` claimed sticky steps do not take part in the round-robin walk (they do -- only their `Progress` priority restart is suppressed), `step.rs`'s `Affinity` omitted that `Detached` also ignores the hint and is never range-checked, and `TypedStep2:: build_output_set` rebuilt `profile()` twice. Fourth review round: - `handles.rs`: `BranchInputHandle::pop` reported every popped item's `heap_size()` to `record_pop`, but a `CountBoundedQueue` / `UnboundedQueue` records `record_push(0)` by design (no byte accounting on its push path). So count/unbounded edges carried nonzero `popped_bytes` against zero `pushed_bytes`, and `compute_edge_stats` reported a `mibytes_per_s` throughput for an edge that measures no bytes at all. The handle now carries `record_item_bytes`, gated on the same `transport_handle.is_some()` signal `ordered_branch` already used for the push side. - `queues.rs`: `ByteBoundedQueue::set_limit_bytes` accepted 0, which makes `try_push` reject unconditionally (`current_bytes >= 0`) and wedges the producer forever. `new` asserts `limit_bytes > 0` for exactly that reason, so the setter could undo the constructor's invariant. Now floored at 1 -- clamped rather than asserted because this runs on a live pipeline, where a 1-byte limit still makes progress and a panic would kill the run over a recoverable arithmetic slip. - `detached.rs`: `DetachedPlaceholder::detached_group` returned `PerStep` unconditionally, so a step that declared `DetachedGroup::Shared(..)` read back as `PerStep`. `extract_detached_steps` is `pub` and leaves these placeholders in the caller's slice, so an external caller regrouping from it would split one shared group across separate driver threads. The placeholder now carries the real label. - `detached_two_sided_no_deadlock` asserted only that N items arrived, so a step emitting N copies of one item -- or duplicating the held value while dropping a popped one -- passed. It now collects the values and asserts the sorted multiset (sorted, not sequenced: the producer's backpressure branch legitimately reorders on retry). - `outputs.rs`: the block comment claimed one `build_queues`/`mark_all_drained` round-trip per output shape; `Single<T>`, `OrderedBytesSingle<T>` and the plain `(A, B)` tuple had none -- and `OrderedBytesSingle<T>` is the only shape routing through `build_single_queues_ordered_bytes`, so its drain path had no coverage at all. All three added; every shape with a `StepOutputs` impl now has one. - `held.rs`: `double_put_panics` used `catch_unwind`, which accepts any panic, so a `put` failing for an unrelated reason would still pass while the double-put guard itself had gone. Pinned with `#[should_panic(expected = ...)]`, matching every other always-on guard here. Fifth review round. Two substantive bugs, both in error/stall handling: - `signal.rs`: `to_result` mapped a failed run to `Ok(())`. `record_error` wins the state CAS before it runs `payload.set`, so `state == STATE_ERROR` with `outcome() == None` is reachable; the `None` arm rescued only `STATE_CANCELLED`, so the error case fell through to success. The cancel side of this exact publish window already had a guard and a test -- the error side was the sibling nobody closed. Now derived from the terminal state, which cannot regress if a future driver forgets to join a writer. A successful run never reaches the new arm: `is_done()` is `state != STATE_OK`. - `fused.rs`: the driver failed the run on the FIRST pass in which no step reported progress. `NoProgress` is the transient "input momentarily empty but not drained" outcome -- a source waiting on a background reader returns it -- so a healthy run was aborted and its output truncated, and the `debug_assert!` turned the same transient pass into a panic under test. Idle passes now back off and retry against a wall-clock budget, and only a sustained stall reports the error. Budgeted by time rather than by pass count because `thread::sleep` granularity varies by platform. The loop also had no backoff at all, so it span the calling thread at 100%. Two findings were rejected as false positives, verified rather than assumed: - An ordered non-byte-bounded branch was said to get an uncapped reorder stash via `ReorderStage::new`. All seven `ordered_branch` call sites pass `DEFAULT_REORDER_OVERFLOW_BYTES`, so that arm was unreachable from any branch builder. Rather than only reply, the `Option<u64>` parameter is now a plain `u64` and the dead unbounded arm is gone, so the question cannot be asked of this code again. - The `HeapSize` compile-fail fixture was said to risk "expected failure but succeeded" if the bound lived only on the `StepOutputs` impl. It is on the `OrderedBytesSingle<T>` declaration, and the fixture's pinned `.stderr` shows the `E0277` plus the `required by a bound in OrderedBytesSingle` note. The rest were accuracy and test-strength fixes: - The cached-handle address assert was documented as though it proved box identity. It compares data addresses only, so allocator address reuse defeats it; soundness rests on the drop-order invariant. Corrected in `erased.rs` and in CLAUDE.md's unsafe-code entry, which carried the same overclaim. - `handles.rs`'s module doc still said tuple branches go through `build_branch` and panic on `ByteBounded`; they have used `build_branch_byte_aware` since the third round. - The same-item-type fan-out shapes (`OrderedBytesTuple2/3<OrdU64, ..>`) asserted only per-position downcasts and drain state, which an aliased branch handle satisfies. They now route a distinct value through each branch. - `maybe_instrumented` -- the constructor the branch builders actually call -- had no coverage on any of the three queue impls; one that dropped its `Some(metrics)` would have produced a silently unmonitored edge. - The edge-registry test asserted a count and `is_some()`, which swapped or duplicated consumer labels satisfy; it now pins both producer->consumer pairs. - `PipelineStats::record`/`record_error` index directly and panic on an out-of-range `StepIdx` while the detached recorders no-op; the asymmetry is deliberate and is now documented under `# Panics`. - `pool.rs`'s `sticky_exclusive_step` test helper took an `owner_idx` it discarded, with every call site passing 0. - `OrderedSource` / `OrderedPairSummer` declare `BranchOrdering::None` on an `OrderedBytesSingle` output, which maps to a direct branch -- so despite the shape's name these steps never engage the reorder stage. That is deliberate (the test reasons from FIFO order under byte backpressure) and is now stated, with pointers to the three places reorder restoration is actually covered. Sixth review round, four findings — the fused stall bound added last round, and three tests that asserted less than their messages claimed: - `fused.rs`: the stall budget was a hard-coded 5s, so a chain with a genuinely slow source had no recourse from a library-chosen policy. It now comes from `PipelineConfig::deadlock_timeout_secs`. `0` (the config default, meaning "monitor disarmed" on the scheduled path) selects a built-in default here rather than "unbounded", and the asymmetry is the point: the scheduled path has a monitor to arm and this driver does not, so a disableable bound would leave a fused wedge with nothing to detect it. Raising the number buys patience; there is deliberately no way to remove the bound. - `tests.rs`: the `build_two_input_handles` branch-mapping test asserted "branch 0 must feed input A and branch 1 input B" using `SumPairStep`, whose `a + b` is commutative -- `10 + 1` and `1 + 10` are both 11, so a swapped wiring passed while the message claimed the mapping was proven. Now uses a non-commutative `a * 1000 + b`; verified by swapping the wiring, which yields 1010 against the expected 10001. - `tests.rs`: `run_reports_finished_pipeline` sorted before comparing at every thread count, discarding a real guarantee at `threads == 1`, where `WideQueueSource` emits `n-1..0` through FIFO transports and one `ReportsFinishedBuffer` draining its `VecDeque` front-first. The single-worker case now asserts the exact descending sequence, so a reordering regression in the flush-first path is visible; above one worker the multiset assertion stands, because the Serial step's cross-worker re-dispatch interleaving is a valid scheduling detail. Stability confirmed over 40 consecutive runs. - `tests.rs`: `OrderedSink` recorded only `item.value`, leaving the out-ordinal sequence unasserted -- and the ordinal is precisely what `OrderedPairSummer`'s hold-and-retry path protects, since it assigns `next_out_ordinal` before the push and must reuse it on a retry. A regression that skipped or reassigned an ordinal left the value sequence intact and passed. It now records `(ordinal, value)` and asserts both. Seventh review round (a full re-review, not incremental), three findings: - `lib.rs`: the crate-root re-exports omitted `DetachedGroup`, which `Step::detached_group` returns while `Step` itself is flattened, and `OrderedBytesTuple2` / `OrderedBytesTuple3`, which sat beside an exported `Single` and `OrderedBytesSingle`. A step author implementing a `Detached` step or a 2-/3-way ordered fan-out had to mix flattened paths with `::step::` / `::outputs::` ones. All three added, pinned by a compile-time test that names every step-author type at the crate root — it fails to build if a `pub use` is dropped or a new output shape lands without one. A sweep for the same shape found two more unexported `pub` types, `queues::BoundedQueueHandle` and `reorder::ReorderCapHandle`, deliberately left alone: they are runtime budget plumbing named only through `runtime::contexts::RegisteredQueue`, which is not flattened either, and `ByteBoundedQueue` exposes inherent `limit_bytes` / `set_limit_bytes` / `current_bytes` so a direct user never needs the trait in scope. Promoting them would make the root surface less coherent, not more. The test records that decision so the next sweep does not re-litigate it. - `fused.rs`: `DEFAULT_STALL_BUDGET` raised from 5s to 60s. Because `deadlock_timeout_secs` defaults to 0, that default is what most fused runs actually get, and 5s is short enough for a source whose background reader blocks on a cold page cache or network-backed input to trip it — the idle timer only resets on progress, so one slow read is enough to fail a healthy run. The bound exists to catch a permanent wedge, not to police a slow source, and the costs are asymmetric: a too-tight budget loses output, while a generous one only delays the report of a wedge that has already hung. - `tests.rs`: `OrderedSink` carried two doc blocks with `#[derive(Clone)]` between them — the previous round's edit inserted the new doc after the attribute and left the stale `/// Records summed values.` line above it. Merged into one block before the attribute. A sweep for the same shape (a doc comment following an attribute) found no other site.
… framework (#697) Adds the `Step`/`Step2` traits, the bounded queue layer, the reorder stage and the work-stealing runtime that schedules and drives them, as a leaf crate. Additive: nothing on the branch changes behaviour and the crate has no in-tree consumer yet. The crate holds nothing that reads or writes sequencing data, so it depends only on ahash, crossbeam-queue, parking_lot, log and one noodles::sam type. Changes made while porting: - Dependencies moved onto `[workspace.dependencies]`. `trybuild` gains a second consumer here, so it is centralized too and `fgumi-cli-macros` switches to `{ workspace = true }`. - Five `collapsible_if` errors fixed as let-chains rather than suppressed. - `fgumi-pipeline-core` added to `publish.yml`'s `CRATES` array; it is a leaf, so any position before its future dependents is valid. - The crate-root doc described the crate as an extraction re-exported by `fgumi` as `pipeline::core`, which is true neither now nor after the series lands; rewritten to describe the crate itself. Two references to design docs that exist in no branch are dropped, and three pre-extraction `core/*.rs` module paths become intra-doc links. - Twenty rustdoc link defects fixed: nine unresolved links now carry crate paths, eight public-doc links to private items are demoted to code spans, three redundant explicit link targets shortened. `ci-doc` runs with `-D warnings`; these never surfaced while this was a private module. Review tooling follows the code out of `src/lib/`: - `.coderabbit.yaml`'s pipeline `path_instructions` glob named only the `src/lib/{unified_pipeline,pipeline}` homes, so extracting the engine into a crate silently dropped it from review — including the clause about new `unsafe` in the typed-step dispatch hot path, written for exactly the sites this commit ships. Widened to cover the crate: 33 of the 42 changed files are instructed now, against 4 before. - `miri.yml` gains a step for `fgumi-pipeline-core::erased`, where all four `#[allow(unsafe_code)]` sites live. It runs clean in ~6s. Both Miri steps now list the filtered tests first and fail on an empty selection: `cargo test <filter>` exits 0 when a filter matches nothing, so renaming either module would have retired the gate silently. The rest of the crate is excluded deliberately: its runtime tests spawn threads and take Miri well past ten minutes, and two wedge tests use wall-clock watchdogs that abort under Miri's slowdown. The four `#[allow(unsafe_code)]` sites in `erased.rs` cache the per-dispatch `downcast_ref` of a step's typed handles by widening `&'a` to `&'static` for storage and narrowing it back on read. The invariant the compiler cannot check -- that every dispatch passes the same handle box for a given step -- is enforced on the cached-hit path by storing the erased box's address in the cache slot and asserting it unconditionally -- release included, since a `debug_assert!` alone would leave release builds reading through a stale pointer. The check is one load and one compare, not the `downcast_ref` `TypeId` probe the cache exists to elide; a debug-only `debug_assert!` still re-resolves the typed pointer as a second diagnostic. CLAUDE.md's unsafe-code section gains an entry for them; note it documents all four, where feat-runall's own entry describes only the two `TypedStep` sites and omits the `TypedStep2` pair. 57 tests added over the 284 that arrive with the crate, all against public API that had no coverage: `build_queues`/`mark_all_drained` round-trips for every fan-out output shape (asserting branches are open before and closed after, so they cannot pass on an inert `mark_all_drained`), `Chain::into_multi` for the 3-, 4- and ordered-bytes-2 branch cases, the type-erased `append_source`/`append_step`/`append_step2` assembly API, `HeapSize` for String/Vec/Option, `BuildError` rendering, the `PipelineConfig` setters, `Step2`'s trait defaults, and the two input-arity dispatch guards. Fixes from review, all in the pipeline-core runtime: - `driver.rs`: a sticky step's `Progress` also means "held an item", so one sitting on a full output reported it forever. The sticky fast path is now bounded at `STICKY_BURST_LIMIT` re-entries AND round-robin no longer takes the priority restart on the sticky owner's `Progress` -- with only one of the two, the walk still breaks before reaching the consumer that drains that output, and a one-worker chain livelocks. Both halves are pinned by `sticky_holding_source_yields_to_its_draining_consumer`, which wedges (watchdog abort) if either is reverted. - `erased.rs`: `build_fused_output_set` forced `QueueSpec::Unbounded` on every branch. The fused driver runs a producer before its consumer, so a step emitting more per `try_run` than its consumer removes grew the edge every pass, without limit. The profile's count/byte bound is kept; only the ordering is dropped. - `stats.rs`: the SPIN verdict exempted Detached steps by NAME while the edge attribution beside it deliberately used `StepIdx`. `StatsSnapshot:: detached` now carries the index and the exemption matches on it, so duplicate step names stay independent. - `handles.rs`: `retry` on an `Ordered` branch with `ordinal = None` re-pushed, allocating a fresh ordinal and abandoning the original -- a permanent gap that stalls the consumer's `ReorderStage`. It now panics in every build; the recoverable `Direct` sibling is left as-is. - `contexts.rs`: `find_producer` returned the first matching edge, so two producers wired to one single-input consumer left the second branch unpopped. It now rejects that graph, mirroring the per-slot uniqueness check `find_all_producers` already had. - `tests.rs`: `PairSummer`/`OrderedPairSummer` reported `Finished` while a branch item was still buffered, silently dropping it -- the exact loss their own doc warns step authors about. `run_with_deadlock_timeout` reported a panic inside the run as "DEADLOCKED or stalled"; it now re-raises the original panic and blames a deadlock only on a true timeout. - `compile_fail.rs`: documents that the `.stderr` fixtures pin rustc's whole diagnostic, including its trait-impl inventory, and how to tell a re-bless from a real regression. The inventory blocks cannot just be deleted -- trybuild compares for equality, so that fails the test outright. Three follow-ons to those fixes, found by re-reading the call sites they touched rather than by review: - `queues.rs`: the same `debug_assert!`-with-a-data-losing-release-fallback shape the `handles.rs` fix removed was live in all three transports' `try_push` -- a push after `mark_drained` went into a queue the consumer had already closed, so the item was silently dropped and surfaced only as a short output. The check is now the unconditional `assert_not_drained` helper, shared by `CountBoundedQueue`, `ByteBoundedQueue` and `UnboundedQueue` and naming which transport failed. It loads `Relaxed` rather than `Acquire`: `drained` is monotonic (its only write anywhere is `store(true, Release)`), so a `Relaxed` load can miss a detection under a cross-thread race but can never panic a correct program, and this now sits on the per-item push path. - `fused.rs` / `builder.rs`: with fused transports keeping their profile byte bounds, `--queue-memory-total` was being silently ignored on that path in favour of the per-step defaults -- the fused contexts are built inside `run_fused_single_thread`, so `Pipeline::run` could not apply the budget on their behalf. The budget is now threaded in and applied there; `apply_initial_queue_budget` becomes `pub(crate)` for it. - `builder.rs` / `step.rs`: two comments the above invalidated. The fused fast-path comment justified skipping the rebalancer with "has no bounded queues to rebalance", which stopped being true; it now gives the real reason (producer-then-consumer in one pass, so no cross-worker imbalance) and points at where the budget is applied. `step.rs`'s last-worker-barrier note still said the `try_push`-after-drain violation was "debug-asserted". Third review round: - `handles.rs`: the 2-, 3- and 4-branch fan-out builders used the non-byte-aware `build_branch`, which panics on `QueueSpec::ByteBounded`, so ANY fan-out step declaring a byte bound failed to build. An armed deadlock monitor requires `ByteBounded` on every output transport, so a monitored fan-out chain was unbuildable. All nine calls now use `build_branch_byte_aware`, which delegates non-byte specs straight back to `build_branch` -- count/unbounded branches are unchanged. Three rstest cases assert the declared bound is registered on every branch of every shape; all three panic without the swap. - `sampler.rs`: each tick read every ordered edge's reorder stash TWICE -- once for the histogram, once for the timeline row -- taking the `ReorderStage` state mutex (held by every must-accept push and `try_pop_in_order`) on both, and the two reads could disagree about one tick. Depths are now read once per tick via `read_depths` and shared by both consumers. The module doc claimed "one `Relaxed` load per edge per tick" and "never perturbs the worker hot path", which was never true for ordered edges; it now describes the real cost. - `miri.yml`: both steps now also assert that no `#[allow(unsafe_code)]` site has moved out of the Miri-scoped module. The `--list` guard proves the filter selects tests, not that it still covers the crate's `unsafe` -- a site that moved would leave the filter matching and the site unchecked. - `storage.rs`: `affinity_in_range` re-derived the affinity->worker mapping by matching variants; it now routes through `Affinity::target_worker`, the documented single source of truth, so the range check and the dispatch gate cannot disagree. Added the missing `should_panic` case for the Exclusive owner-range assert -- only the Serial arm was covered, so that always-on `assert!` could have been weakened to a `debug_assert!` with CI still green. - `queues.rs`: the slot-cap reject test filled with 0-byte items, exercising the byte-reservation rollback with `size == 0` where a leak or double-subtract is invisible. Added a nonzero-size case pinning `current_bytes` across the reject. - Test-fixture steps returned `StepOutcome::Contention` when an output push was rejected. The driver treats it like `NoProgress`, but it feeds `contention_count`, from which the bottleneck verdict derives its SPIN ratio -- so ordinary backpressure was inventing mutex contention and could trip a bogus SPIN finding. Fixed in all six sites: the two flagged in `tests.rs` (`OrderedSource`, `OrderedPairSummer`) plus the same shape in `detached.rs` and `builder.rs`, which the review did not name. - Docs corrected where they contradicted the code: `scheduler.rs` claimed sticky steps do not take part in the round-robin walk (they do -- only their `Progress` priority restart is suppressed), `step.rs`'s `Affinity` omitted that `Detached` also ignores the hint and is never range-checked, and `TypedStep2:: build_output_set` rebuilt `profile()` twice. Fourth review round: - `handles.rs`: `BranchInputHandle::pop` reported every popped item's `heap_size()` to `record_pop`, but a `CountBoundedQueue` / `UnboundedQueue` records `record_push(0)` by design (no byte accounting on its push path). So count/unbounded edges carried nonzero `popped_bytes` against zero `pushed_bytes`, and `compute_edge_stats` reported a `mibytes_per_s` throughput for an edge that measures no bytes at all. The handle now carries `record_item_bytes`, gated on the same `transport_handle.is_some()` signal `ordered_branch` already used for the push side. - `queues.rs`: `ByteBoundedQueue::set_limit_bytes` accepted 0, which makes `try_push` reject unconditionally (`current_bytes >= 0`) and wedges the producer forever. `new` asserts `limit_bytes > 0` for exactly that reason, so the setter could undo the constructor's invariant. Now floored at 1 -- clamped rather than asserted because this runs on a live pipeline, where a 1-byte limit still makes progress and a panic would kill the run over a recoverable arithmetic slip. - `detached.rs`: `DetachedPlaceholder::detached_group` returned `PerStep` unconditionally, so a step that declared `DetachedGroup::Shared(..)` read back as `PerStep`. `extract_detached_steps` is `pub` and leaves these placeholders in the caller's slice, so an external caller regrouping from it would split one shared group across separate driver threads. The placeholder now carries the real label. - `detached_two_sided_no_deadlock` asserted only that N items arrived, so a step emitting N copies of one item -- or duplicating the held value while dropping a popped one -- passed. It now collects the values and asserts the sorted multiset (sorted, not sequenced: the producer's backpressure branch legitimately reorders on retry). - `outputs.rs`: the block comment claimed one `build_queues`/`mark_all_drained` round-trip per output shape; `Single<T>`, `OrderedBytesSingle<T>` and the plain `(A, B)` tuple had none -- and `OrderedBytesSingle<T>` is the only shape routing through `build_single_queues_ordered_bytes`, so its drain path had no coverage at all. All three added; every shape with a `StepOutputs` impl now has one. - `held.rs`: `double_put_panics` used `catch_unwind`, which accepts any panic, so a `put` failing for an unrelated reason would still pass while the double-put guard itself had gone. Pinned with `#[should_panic(expected = ...)]`, matching every other always-on guard here. Fifth review round. Two substantive bugs, both in error/stall handling: - `signal.rs`: `to_result` mapped a failed run to `Ok(())`. `record_error` wins the state CAS before it runs `payload.set`, so `state == STATE_ERROR` with `outcome() == None` is reachable; the `None` arm rescued only `STATE_CANCELLED`, so the error case fell through to success. The cancel side of this exact publish window already had a guard and a test -- the error side was the sibling nobody closed. Now derived from the terminal state, which cannot regress if a future driver forgets to join a writer. A successful run never reaches the new arm: `is_done()` is `state != STATE_OK`. - `fused.rs`: the driver failed the run on the FIRST pass in which no step reported progress. `NoProgress` is the transient "input momentarily empty but not drained" outcome -- a source waiting on a background reader returns it -- so a healthy run was aborted and its output truncated, and the `debug_assert!` turned the same transient pass into a panic under test. Idle passes now back off and retry against a wall-clock budget, and only a sustained stall reports the error. Budgeted by time rather than by pass count because `thread::sleep` granularity varies by platform. The loop also had no backoff at all, so it span the calling thread at 100%. Two findings were rejected as false positives, verified rather than assumed: - An ordered non-byte-bounded branch was said to get an uncapped reorder stash via `ReorderStage::new`. All seven `ordered_branch` call sites pass `DEFAULT_REORDER_OVERFLOW_BYTES`, so that arm was unreachable from any branch builder. Rather than only reply, the `Option<u64>` parameter is now a plain `u64` and the dead unbounded arm is gone, so the question cannot be asked of this code again. - The `HeapSize` compile-fail fixture was said to risk "expected failure but succeeded" if the bound lived only on the `StepOutputs` impl. It is on the `OrderedBytesSingle<T>` declaration, and the fixture's pinned `.stderr` shows the `E0277` plus the `required by a bound in OrderedBytesSingle` note. The rest were accuracy and test-strength fixes: - The cached-handle address assert was documented as though it proved box identity. It compares data addresses only, so allocator address reuse defeats it; soundness rests on the drop-order invariant. Corrected in `erased.rs` and in CLAUDE.md's unsafe-code entry, which carried the same overclaim. - `handles.rs`'s module doc still said tuple branches go through `build_branch` and panic on `ByteBounded`; they have used `build_branch_byte_aware` since the third round. - The same-item-type fan-out shapes (`OrderedBytesTuple2/3<OrdU64, ..>`) asserted only per-position downcasts and drain state, which an aliased branch handle satisfies. They now route a distinct value through each branch. - `maybe_instrumented` -- the constructor the branch builders actually call -- had no coverage on any of the three queue impls; one that dropped its `Some(metrics)` would have produced a silently unmonitored edge. - The edge-registry test asserted a count and `is_some()`, which swapped or duplicated consumer labels satisfy; it now pins both producer->consumer pairs. - `PipelineStats::record`/`record_error` index directly and panic on an out-of-range `StepIdx` while the detached recorders no-op; the asymmetry is deliberate and is now documented under `# Panics`. - `pool.rs`'s `sticky_exclusive_step` test helper took an `owner_idx` it discarded, with every call site passing 0. - `OrderedSource` / `OrderedPairSummer` declare `BranchOrdering::None` on an `OrderedBytesSingle` output, which maps to a direct branch -- so despite the shape's name these steps never engage the reorder stage. That is deliberate (the test reasons from FIFO order under byte backpressure) and is now stated, with pointers to the three places reorder restoration is actually covered. Sixth review round, four findings — the fused stall bound added last round, and three tests that asserted less than their messages claimed: - `fused.rs`: the stall budget was a hard-coded 5s, so a chain with a genuinely slow source had no recourse from a library-chosen policy. It now comes from `PipelineConfig::deadlock_timeout_secs`. `0` (the config default, meaning "monitor disarmed" on the scheduled path) selects a built-in default here rather than "unbounded", and the asymmetry is the point: the scheduled path has a monitor to arm and this driver does not, so a disableable bound would leave a fused wedge with nothing to detect it. Raising the number buys patience; there is deliberately no way to remove the bound. - `tests.rs`: the `build_two_input_handles` branch-mapping test asserted "branch 0 must feed input A and branch 1 input B" using `SumPairStep`, whose `a + b` is commutative -- `10 + 1` and `1 + 10` are both 11, so a swapped wiring passed while the message claimed the mapping was proven. Now uses a non-commutative `a * 1000 + b`; verified by swapping the wiring, which yields 1010 against the expected 10001. - `tests.rs`: `run_reports_finished_pipeline` sorted before comparing at every thread count, discarding a real guarantee at `threads == 1`, where `WideQueueSource` emits `n-1..0` through FIFO transports and one `ReportsFinishedBuffer` draining its `VecDeque` front-first. The single-worker case now asserts the exact descending sequence, so a reordering regression in the flush-first path is visible; above one worker the multiset assertion stands, because the Serial step's cross-worker re-dispatch interleaving is a valid scheduling detail. Stability confirmed over 40 consecutive runs. - `tests.rs`: `OrderedSink` recorded only `item.value`, leaving the out-ordinal sequence unasserted -- and the ordinal is precisely what `OrderedPairSummer`'s hold-and-retry path protects, since it assigns `next_out_ordinal` before the push and must reuse it on a retry. A regression that skipped or reassigned an ordinal left the value sequence intact and passed. It now records `(ordinal, value)` and asserts both. Seventh review round (a full re-review, not incremental), three findings: - `lib.rs`: the crate-root re-exports omitted `DetachedGroup`, which `Step::detached_group` returns while `Step` itself is flattened, and `OrderedBytesTuple2` / `OrderedBytesTuple3`, which sat beside an exported `Single` and `OrderedBytesSingle`. A step author implementing a `Detached` step or a 2-/3-way ordered fan-out had to mix flattened paths with `::step::` / `::outputs::` ones. All three added, pinned by a compile-time test that names every step-author type at the crate root — it fails to build if a `pub use` is dropped or a new output shape lands without one. A sweep for the same shape found two more unexported `pub` types, `queues::BoundedQueueHandle` and `reorder::ReorderCapHandle`, deliberately left alone: they are runtime budget plumbing named only through `runtime::contexts::RegisteredQueue`, which is not flattened either, and `ByteBoundedQueue` exposes inherent `limit_bytes` / `set_limit_bytes` / `current_bytes` so a direct user never needs the trait in scope. Promoting them would make the root surface less coherent, not more. The test records that decision so the next sweep does not re-litigate it. - `fused.rs`: `DEFAULT_STALL_BUDGET` raised from 5s to 60s. Because `deadlock_timeout_secs` defaults to 0, that default is what most fused runs actually get, and 5s is short enough for a source whose background reader blocks on a cold page cache or network-backed input to trip it — the idle timer only resets on progress, so one slow read is enough to fail a healthy run. The bound exists to catch a permanent wedge, not to police a slow source, and the costs are asymmetric: a too-tight budget loses output, while a generous one only delays the report of a wedge that has already hung. - `tests.rs`: `OrderedSink` carried two doc blocks with `#[derive(Clone)]` between them — the previous round's edit inserted the new doc after the attribute and left the stale `/// Records summed values.` line above it. Merged into one block before the attribute. A sweep for the same shape (a doc comment following an attribute) found no other site.
… framework (#697) Adds the `Step`/`Step2` traits, the bounded queue layer, the reorder stage and the work-stealing runtime that schedules and drives them, as a leaf crate. Additive: nothing on the branch changes behaviour and the crate has no in-tree consumer yet. The crate holds nothing that reads or writes sequencing data, so it depends only on ahash, crossbeam-queue, parking_lot, log and one noodles::sam type. Changes made while porting: - Dependencies moved onto `[workspace.dependencies]`. `trybuild` gains a second consumer here, so it is centralized too and `fgumi-cli-macros` switches to `{ workspace = true }`. - Five `collapsible_if` errors fixed as let-chains rather than suppressed. - `fgumi-pipeline-core` added to `publish.yml`'s `CRATES` array; it is a leaf, so any position before its future dependents is valid. - The crate-root doc described the crate as an extraction re-exported by `fgumi` as `pipeline::core`, which is true neither now nor after the series lands; rewritten to describe the crate itself. Two references to design docs that exist in no branch are dropped, and three pre-extraction `core/*.rs` module paths become intra-doc links. - Twenty rustdoc link defects fixed: nine unresolved links now carry crate paths, eight public-doc links to private items are demoted to code spans, three redundant explicit link targets shortened. `ci-doc` runs with `-D warnings`; these never surfaced while this was a private module. Review tooling follows the code out of `src/lib/`: - `.coderabbit.yaml`'s pipeline `path_instructions` glob named only the `src/lib/{unified_pipeline,pipeline}` homes, so extracting the engine into a crate silently dropped it from review — including the clause about new `unsafe` in the typed-step dispatch hot path, written for exactly the sites this commit ships. Widened to cover the crate: 33 of the 42 changed files are instructed now, against 4 before. - `miri.yml` gains a step for `fgumi-pipeline-core::erased`, where all four `#[allow(unsafe_code)]` sites live. It runs clean in ~6s. Both Miri steps now list the filtered tests first and fail on an empty selection: `cargo test <filter>` exits 0 when a filter matches nothing, so renaming either module would have retired the gate silently. The rest of the crate is excluded deliberately: its runtime tests spawn threads and take Miri well past ten minutes, and two wedge tests use wall-clock watchdogs that abort under Miri's slowdown. The four `#[allow(unsafe_code)]` sites in `erased.rs` cache the per-dispatch `downcast_ref` of a step's typed handles by widening `&'a` to `&'static` for storage and narrowing it back on read. The invariant the compiler cannot check -- that every dispatch passes the same handle box for a given step -- is enforced on the cached-hit path by storing the erased box's address in the cache slot and asserting it unconditionally -- release included, since a `debug_assert!` alone would leave release builds reading through a stale pointer. The check is one load and one compare, not the `downcast_ref` `TypeId` probe the cache exists to elide; a debug-only `debug_assert!` still re-resolves the typed pointer as a second diagnostic. CLAUDE.md's unsafe-code section gains an entry for them; note it documents all four, where feat-runall's own entry describes only the two `TypedStep` sites and omits the `TypedStep2` pair. 57 tests added over the 284 that arrive with the crate, all against public API that had no coverage: `build_queues`/`mark_all_drained` round-trips for every fan-out output shape (asserting branches are open before and closed after, so they cannot pass on an inert `mark_all_drained`), `Chain::into_multi` for the 3-, 4- and ordered-bytes-2 branch cases, the type-erased `append_source`/`append_step`/`append_step2` assembly API, `HeapSize` for String/Vec/Option, `BuildError` rendering, the `PipelineConfig` setters, `Step2`'s trait defaults, and the two input-arity dispatch guards. Fixes from review, all in the pipeline-core runtime: - `driver.rs`: a sticky step's `Progress` also means "held an item", so one sitting on a full output reported it forever. The sticky fast path is now bounded at `STICKY_BURST_LIMIT` re-entries AND round-robin no longer takes the priority restart on the sticky owner's `Progress` -- with only one of the two, the walk still breaks before reaching the consumer that drains that output, and a one-worker chain livelocks. Both halves are pinned by `sticky_holding_source_yields_to_its_draining_consumer`, which wedges (watchdog abort) if either is reverted. - `erased.rs`: `build_fused_output_set` forced `QueueSpec::Unbounded` on every branch. The fused driver runs a producer before its consumer, so a step emitting more per `try_run` than its consumer removes grew the edge every pass, without limit. The profile's count/byte bound is kept; only the ordering is dropped. - `stats.rs`: the SPIN verdict exempted Detached steps by NAME while the edge attribution beside it deliberately used `StepIdx`. `StatsSnapshot:: detached` now carries the index and the exemption matches on it, so duplicate step names stay independent. - `handles.rs`: `retry` on an `Ordered` branch with `ordinal = None` re-pushed, allocating a fresh ordinal and abandoning the original -- a permanent gap that stalls the consumer's `ReorderStage`. It now panics in every build; the recoverable `Direct` sibling is left as-is. - `contexts.rs`: `find_producer` returned the first matching edge, so two producers wired to one single-input consumer left the second branch unpopped. It now rejects that graph, mirroring the per-slot uniqueness check `find_all_producers` already had. - `tests.rs`: `PairSummer`/`OrderedPairSummer` reported `Finished` while a branch item was still buffered, silently dropping it -- the exact loss their own doc warns step authors about. `run_with_deadlock_timeout` reported a panic inside the run as "DEADLOCKED or stalled"; it now re-raises the original panic and blames a deadlock only on a true timeout. - `compile_fail.rs`: documents that the `.stderr` fixtures pin rustc's whole diagnostic, including its trait-impl inventory, and how to tell a re-bless from a real regression. The inventory blocks cannot just be deleted -- trybuild compares for equality, so that fails the test outright. Three follow-ons to those fixes, found by re-reading the call sites they touched rather than by review: - `queues.rs`: the same `debug_assert!`-with-a-data-losing-release-fallback shape the `handles.rs` fix removed was live in all three transports' `try_push` -- a push after `mark_drained` went into a queue the consumer had already closed, so the item was silently dropped and surfaced only as a short output. The check is now the unconditional `assert_not_drained` helper, shared by `CountBoundedQueue`, `ByteBoundedQueue` and `UnboundedQueue` and naming which transport failed. It loads `Relaxed` rather than `Acquire`: `drained` is monotonic (its only write anywhere is `store(true, Release)`), so a `Relaxed` load can miss a detection under a cross-thread race but can never panic a correct program, and this now sits on the per-item push path. - `fused.rs` / `builder.rs`: with fused transports keeping their profile byte bounds, `--queue-memory-total` was being silently ignored on that path in favour of the per-step defaults -- the fused contexts are built inside `run_fused_single_thread`, so `Pipeline::run` could not apply the budget on their behalf. The budget is now threaded in and applied there; `apply_initial_queue_budget` becomes `pub(crate)` for it. - `builder.rs` / `step.rs`: two comments the above invalidated. The fused fast-path comment justified skipping the rebalancer with "has no bounded queues to rebalance", which stopped being true; it now gives the real reason (producer-then-consumer in one pass, so no cross-worker imbalance) and points at where the budget is applied. `step.rs`'s last-worker-barrier note still said the `try_push`-after-drain violation was "debug-asserted". Third review round: - `handles.rs`: the 2-, 3- and 4-branch fan-out builders used the non-byte-aware `build_branch`, which panics on `QueueSpec::ByteBounded`, so ANY fan-out step declaring a byte bound failed to build. An armed deadlock monitor requires `ByteBounded` on every output transport, so a monitored fan-out chain was unbuildable. All nine calls now use `build_branch_byte_aware`, which delegates non-byte specs straight back to `build_branch` -- count/unbounded branches are unchanged. Three rstest cases assert the declared bound is registered on every branch of every shape; all three panic without the swap. - `sampler.rs`: each tick read every ordered edge's reorder stash TWICE -- once for the histogram, once for the timeline row -- taking the `ReorderStage` state mutex (held by every must-accept push and `try_pop_in_order`) on both, and the two reads could disagree about one tick. Depths are now read once per tick via `read_depths` and shared by both consumers. The module doc claimed "one `Relaxed` load per edge per tick" and "never perturbs the worker hot path", which was never true for ordered edges; it now describes the real cost. - `miri.yml`: both steps now also assert that no `#[allow(unsafe_code)]` site has moved out of the Miri-scoped module. The `--list` guard proves the filter selects tests, not that it still covers the crate's `unsafe` -- a site that moved would leave the filter matching and the site unchecked. - `storage.rs`: `affinity_in_range` re-derived the affinity->worker mapping by matching variants; it now routes through `Affinity::target_worker`, the documented single source of truth, so the range check and the dispatch gate cannot disagree. Added the missing `should_panic` case for the Exclusive owner-range assert -- only the Serial arm was covered, so that always-on `assert!` could have been weakened to a `debug_assert!` with CI still green. - `queues.rs`: the slot-cap reject test filled with 0-byte items, exercising the byte-reservation rollback with `size == 0` where a leak or double-subtract is invisible. Added a nonzero-size case pinning `current_bytes` across the reject. - Test-fixture steps returned `StepOutcome::Contention` when an output push was rejected. The driver treats it like `NoProgress`, but it feeds `contention_count`, from which the bottleneck verdict derives its SPIN ratio -- so ordinary backpressure was inventing mutex contention and could trip a bogus SPIN finding. Fixed in all six sites: the two flagged in `tests.rs` (`OrderedSource`, `OrderedPairSummer`) plus the same shape in `detached.rs` and `builder.rs`, which the review did not name. - Docs corrected where they contradicted the code: `scheduler.rs` claimed sticky steps do not take part in the round-robin walk (they do -- only their `Progress` priority restart is suppressed), `step.rs`'s `Affinity` omitted that `Detached` also ignores the hint and is never range-checked, and `TypedStep2:: build_output_set` rebuilt `profile()` twice. Fourth review round: - `handles.rs`: `BranchInputHandle::pop` reported every popped item's `heap_size()` to `record_pop`, but a `CountBoundedQueue` / `UnboundedQueue` records `record_push(0)` by design (no byte accounting on its push path). So count/unbounded edges carried nonzero `popped_bytes` against zero `pushed_bytes`, and `compute_edge_stats` reported a `mibytes_per_s` throughput for an edge that measures no bytes at all. The handle now carries `record_item_bytes`, gated on the same `transport_handle.is_some()` signal `ordered_branch` already used for the push side. - `queues.rs`: `ByteBoundedQueue::set_limit_bytes` accepted 0, which makes `try_push` reject unconditionally (`current_bytes >= 0`) and wedges the producer forever. `new` asserts `limit_bytes > 0` for exactly that reason, so the setter could undo the constructor's invariant. Now floored at 1 -- clamped rather than asserted because this runs on a live pipeline, where a 1-byte limit still makes progress and a panic would kill the run over a recoverable arithmetic slip. - `detached.rs`: `DetachedPlaceholder::detached_group` returned `PerStep` unconditionally, so a step that declared `DetachedGroup::Shared(..)` read back as `PerStep`. `extract_detached_steps` is `pub` and leaves these placeholders in the caller's slice, so an external caller regrouping from it would split one shared group across separate driver threads. The placeholder now carries the real label. - `detached_two_sided_no_deadlock` asserted only that N items arrived, so a step emitting N copies of one item -- or duplicating the held value while dropping a popped one -- passed. It now collects the values and asserts the sorted multiset (sorted, not sequenced: the producer's backpressure branch legitimately reorders on retry). - `outputs.rs`: the block comment claimed one `build_queues`/`mark_all_drained` round-trip per output shape; `Single<T>`, `OrderedBytesSingle<T>` and the plain `(A, B)` tuple had none -- and `OrderedBytesSingle<T>` is the only shape routing through `build_single_queues_ordered_bytes`, so its drain path had no coverage at all. All three added; every shape with a `StepOutputs` impl now has one. - `held.rs`: `double_put_panics` used `catch_unwind`, which accepts any panic, so a `put` failing for an unrelated reason would still pass while the double-put guard itself had gone. Pinned with `#[should_panic(expected = ...)]`, matching every other always-on guard here. Fifth review round. Two substantive bugs, both in error/stall handling: - `signal.rs`: `to_result` mapped a failed run to `Ok(())`. `record_error` wins the state CAS before it runs `payload.set`, so `state == STATE_ERROR` with `outcome() == None` is reachable; the `None` arm rescued only `STATE_CANCELLED`, so the error case fell through to success. The cancel side of this exact publish window already had a guard and a test -- the error side was the sibling nobody closed. Now derived from the terminal state, which cannot regress if a future driver forgets to join a writer. A successful run never reaches the new arm: `is_done()` is `state != STATE_OK`. - `fused.rs`: the driver failed the run on the FIRST pass in which no step reported progress. `NoProgress` is the transient "input momentarily empty but not drained" outcome -- a source waiting on a background reader returns it -- so a healthy run was aborted and its output truncated, and the `debug_assert!` turned the same transient pass into a panic under test. Idle passes now back off and retry against a wall-clock budget, and only a sustained stall reports the error. Budgeted by time rather than by pass count because `thread::sleep` granularity varies by platform. The loop also had no backoff at all, so it span the calling thread at 100%. Two findings were rejected as false positives, verified rather than assumed: - An ordered non-byte-bounded branch was said to get an uncapped reorder stash via `ReorderStage::new`. All seven `ordered_branch` call sites pass `DEFAULT_REORDER_OVERFLOW_BYTES`, so that arm was unreachable from any branch builder. Rather than only reply, the `Option<u64>` parameter is now a plain `u64` and the dead unbounded arm is gone, so the question cannot be asked of this code again. - The `HeapSize` compile-fail fixture was said to risk "expected failure but succeeded" if the bound lived only on the `StepOutputs` impl. It is on the `OrderedBytesSingle<T>` declaration, and the fixture's pinned `.stderr` shows the `E0277` plus the `required by a bound in OrderedBytesSingle` note. The rest were accuracy and test-strength fixes: - The cached-handle address assert was documented as though it proved box identity. It compares data addresses only, so allocator address reuse defeats it; soundness rests on the drop-order invariant. Corrected in `erased.rs` and in CLAUDE.md's unsafe-code entry, which carried the same overclaim. - `handles.rs`'s module doc still said tuple branches go through `build_branch` and panic on `ByteBounded`; they have used `build_branch_byte_aware` since the third round. - The same-item-type fan-out shapes (`OrderedBytesTuple2/3<OrdU64, ..>`) asserted only per-position downcasts and drain state, which an aliased branch handle satisfies. They now route a distinct value through each branch. - `maybe_instrumented` -- the constructor the branch builders actually call -- had no coverage on any of the three queue impls; one that dropped its `Some(metrics)` would have produced a silently unmonitored edge. - The edge-registry test asserted a count and `is_some()`, which swapped or duplicated consumer labels satisfy; it now pins both producer->consumer pairs. - `PipelineStats::record`/`record_error` index directly and panic on an out-of-range `StepIdx` while the detached recorders no-op; the asymmetry is deliberate and is now documented under `# Panics`. - `pool.rs`'s `sticky_exclusive_step` test helper took an `owner_idx` it discarded, with every call site passing 0. - `OrderedSource` / `OrderedPairSummer` declare `BranchOrdering::None` on an `OrderedBytesSingle` output, which maps to a direct branch -- so despite the shape's name these steps never engage the reorder stage. That is deliberate (the test reasons from FIFO order under byte backpressure) and is now stated, with pointers to the three places reorder restoration is actually covered. Sixth review round, four findings — the fused stall bound added last round, and three tests that asserted less than their messages claimed: - `fused.rs`: the stall budget was a hard-coded 5s, so a chain with a genuinely slow source had no recourse from a library-chosen policy. It now comes from `PipelineConfig::deadlock_timeout_secs`. `0` (the config default, meaning "monitor disarmed" on the scheduled path) selects a built-in default here rather than "unbounded", and the asymmetry is the point: the scheduled path has a monitor to arm and this driver does not, so a disableable bound would leave a fused wedge with nothing to detect it. Raising the number buys patience; there is deliberately no way to remove the bound. - `tests.rs`: the `build_two_input_handles` branch-mapping test asserted "branch 0 must feed input A and branch 1 input B" using `SumPairStep`, whose `a + b` is commutative -- `10 + 1` and `1 + 10` are both 11, so a swapped wiring passed while the message claimed the mapping was proven. Now uses a non-commutative `a * 1000 + b`; verified by swapping the wiring, which yields 1010 against the expected 10001. - `tests.rs`: `run_reports_finished_pipeline` sorted before comparing at every thread count, discarding a real guarantee at `threads == 1`, where `WideQueueSource` emits `n-1..0` through FIFO transports and one `ReportsFinishedBuffer` draining its `VecDeque` front-first. The single-worker case now asserts the exact descending sequence, so a reordering regression in the flush-first path is visible; above one worker the multiset assertion stands, because the Serial step's cross-worker re-dispatch interleaving is a valid scheduling detail. Stability confirmed over 40 consecutive runs. - `tests.rs`: `OrderedSink` recorded only `item.value`, leaving the out-ordinal sequence unasserted -- and the ordinal is precisely what `OrderedPairSummer`'s hold-and-retry path protects, since it assigns `next_out_ordinal` before the push and must reuse it on a retry. A regression that skipped or reassigned an ordinal left the value sequence intact and passed. It now records `(ordinal, value)` and asserts both. Seventh review round (a full re-review, not incremental), three findings: - `lib.rs`: the crate-root re-exports omitted `DetachedGroup`, which `Step::detached_group` returns while `Step` itself is flattened, and `OrderedBytesTuple2` / `OrderedBytesTuple3`, which sat beside an exported `Single` and `OrderedBytesSingle`. A step author implementing a `Detached` step or a 2-/3-way ordered fan-out had to mix flattened paths with `::step::` / `::outputs::` ones. All three added, pinned by a compile-time test that names every step-author type at the crate root — it fails to build if a `pub use` is dropped or a new output shape lands without one. A sweep for the same shape found two more unexported `pub` types, `queues::BoundedQueueHandle` and `reorder::ReorderCapHandle`, deliberately left alone: they are runtime budget plumbing named only through `runtime::contexts::RegisteredQueue`, which is not flattened either, and `ByteBoundedQueue` exposes inherent `limit_bytes` / `set_limit_bytes` / `current_bytes` so a direct user never needs the trait in scope. Promoting them would make the root surface less coherent, not more. The test records that decision so the next sweep does not re-litigate it. - `fused.rs`: `DEFAULT_STALL_BUDGET` raised from 5s to 60s. Because `deadlock_timeout_secs` defaults to 0, that default is what most fused runs actually get, and 5s is short enough for a source whose background reader blocks on a cold page cache or network-backed input to trip it — the idle timer only resets on progress, so one slow read is enough to fail a healthy run. The bound exists to catch a permanent wedge, not to police a slow source, and the costs are asymmetric: a too-tight budget loses output, while a generous one only delays the report of a wedge that has already hung. - `tests.rs`: `OrderedSink` carried two doc blocks with `#[derive(Clone)]` between them — the previous round's edit inserted the new doc after the attribute and left the stale `/// Records summed values.` line above it. Merged into one block before the attribute. A sweep for the same shape (a doc comment following an attribute) found no other site.
… framework (#697) Adds the `Step`/`Step2` traits, the bounded queue layer, the reorder stage and the work-stealing runtime that schedules and drives them, as a leaf crate. Additive: nothing on the branch changes behaviour and the crate has no in-tree consumer yet. The crate holds nothing that reads or writes sequencing data, so it depends only on ahash, crossbeam-queue, parking_lot, log and one noodles::sam type. Changes made while porting: - Dependencies moved onto `[workspace.dependencies]`. `trybuild` gains a second consumer here, so it is centralized too and `fgumi-cli-macros` switches to `{ workspace = true }`. - Five `collapsible_if` errors fixed as let-chains rather than suppressed. - `fgumi-pipeline-core` added to `publish.yml`'s `CRATES` array; it is a leaf, so any position before its future dependents is valid. - The crate-root doc described the crate as an extraction re-exported by `fgumi` as `pipeline::core`, which is true neither now nor after the series lands; rewritten to describe the crate itself. Two references to design docs that exist in no branch are dropped, and three pre-extraction `core/*.rs` module paths become intra-doc links. - Twenty rustdoc link defects fixed: nine unresolved links now carry crate paths, eight public-doc links to private items are demoted to code spans, three redundant explicit link targets shortened. `ci-doc` runs with `-D warnings`; these never surfaced while this was a private module. Review tooling follows the code out of `src/lib/`: - `.coderabbit.yaml`'s pipeline `path_instructions` glob named only the `src/lib/{unified_pipeline,pipeline}` homes, so extracting the engine into a crate silently dropped it from review — including the clause about new `unsafe` in the typed-step dispatch hot path, written for exactly the sites this commit ships. Widened to cover the crate: 33 of the 42 changed files are instructed now, against 4 before. - `miri.yml` gains a step for `fgumi-pipeline-core::erased`, where all four `#[allow(unsafe_code)]` sites live. It runs clean in ~6s. Both Miri steps now list the filtered tests first and fail on an empty selection: `cargo test <filter>` exits 0 when a filter matches nothing, so renaming either module would have retired the gate silently. The rest of the crate is excluded deliberately: its runtime tests spawn threads and take Miri well past ten minutes, and two wedge tests use wall-clock watchdogs that abort under Miri's slowdown. The four `#[allow(unsafe_code)]` sites in `erased.rs` cache the per-dispatch `downcast_ref` of a step's typed handles by widening `&'a` to `&'static` for storage and narrowing it back on read. The invariant the compiler cannot check -- that every dispatch passes the same handle box for a given step -- is enforced on the cached-hit path by storing the erased box's address in the cache slot and asserting it unconditionally -- release included, since a `debug_assert!` alone would leave release builds reading through a stale pointer. The check is one load and one compare, not the `downcast_ref` `TypeId` probe the cache exists to elide; a debug-only `debug_assert!` still re-resolves the typed pointer as a second diagnostic. CLAUDE.md's unsafe-code section gains an entry for them; note it documents all four, where feat-runall's own entry describes only the two `TypedStep` sites and omits the `TypedStep2` pair. 57 tests added over the 284 that arrive with the crate, all against public API that had no coverage: `build_queues`/`mark_all_drained` round-trips for every fan-out output shape (asserting branches are open before and closed after, so they cannot pass on an inert `mark_all_drained`), `Chain::into_multi` for the 3-, 4- and ordered-bytes-2 branch cases, the type-erased `append_source`/`append_step`/`append_step2` assembly API, `HeapSize` for String/Vec/Option, `BuildError` rendering, the `PipelineConfig` setters, `Step2`'s trait defaults, and the two input-arity dispatch guards. Fixes from review, all in the pipeline-core runtime: - `driver.rs`: a sticky step's `Progress` also means "held an item", so one sitting on a full output reported it forever. The sticky fast path is now bounded at `STICKY_BURST_LIMIT` re-entries AND round-robin no longer takes the priority restart on the sticky owner's `Progress` -- with only one of the two, the walk still breaks before reaching the consumer that drains that output, and a one-worker chain livelocks. Both halves are pinned by `sticky_holding_source_yields_to_its_draining_consumer`, which wedges (watchdog abort) if either is reverted. - `erased.rs`: `build_fused_output_set` forced `QueueSpec::Unbounded` on every branch. The fused driver runs a producer before its consumer, so a step emitting more per `try_run` than its consumer removes grew the edge every pass, without limit. The profile's count/byte bound is kept; only the ordering is dropped. - `stats.rs`: the SPIN verdict exempted Detached steps by NAME while the edge attribution beside it deliberately used `StepIdx`. `StatsSnapshot:: detached` now carries the index and the exemption matches on it, so duplicate step names stay independent. - `handles.rs`: `retry` on an `Ordered` branch with `ordinal = None` re-pushed, allocating a fresh ordinal and abandoning the original -- a permanent gap that stalls the consumer's `ReorderStage`. It now panics in every build; the recoverable `Direct` sibling is left as-is. - `contexts.rs`: `find_producer` returned the first matching edge, so two producers wired to one single-input consumer left the second branch unpopped. It now rejects that graph, mirroring the per-slot uniqueness check `find_all_producers` already had. - `tests.rs`: `PairSummer`/`OrderedPairSummer` reported `Finished` while a branch item was still buffered, silently dropping it -- the exact loss their own doc warns step authors about. `run_with_deadlock_timeout` reported a panic inside the run as "DEADLOCKED or stalled"; it now re-raises the original panic and blames a deadlock only on a true timeout. - `compile_fail.rs`: documents that the `.stderr` fixtures pin rustc's whole diagnostic, including its trait-impl inventory, and how to tell a re-bless from a real regression. The inventory blocks cannot just be deleted -- trybuild compares for equality, so that fails the test outright. Three follow-ons to those fixes, found by re-reading the call sites they touched rather than by review: - `queues.rs`: the same `debug_assert!`-with-a-data-losing-release-fallback shape the `handles.rs` fix removed was live in all three transports' `try_push` -- a push after `mark_drained` went into a queue the consumer had already closed, so the item was silently dropped and surfaced only as a short output. The check is now the unconditional `assert_not_drained` helper, shared by `CountBoundedQueue`, `ByteBoundedQueue` and `UnboundedQueue` and naming which transport failed. It loads `Relaxed` rather than `Acquire`: `drained` is monotonic (its only write anywhere is `store(true, Release)`), so a `Relaxed` load can miss a detection under a cross-thread race but can never panic a correct program, and this now sits on the per-item push path. - `fused.rs` / `builder.rs`: with fused transports keeping their profile byte bounds, `--queue-memory-total` was being silently ignored on that path in favour of the per-step defaults -- the fused contexts are built inside `run_fused_single_thread`, so `Pipeline::run` could not apply the budget on their behalf. The budget is now threaded in and applied there; `apply_initial_queue_budget` becomes `pub(crate)` for it. - `builder.rs` / `step.rs`: two comments the above invalidated. The fused fast-path comment justified skipping the rebalancer with "has no bounded queues to rebalance", which stopped being true; it now gives the real reason (producer-then-consumer in one pass, so no cross-worker imbalance) and points at where the budget is applied. `step.rs`'s last-worker-barrier note still said the `try_push`-after-drain violation was "debug-asserted". Third review round: - `handles.rs`: the 2-, 3- and 4-branch fan-out builders used the non-byte-aware `build_branch`, which panics on `QueueSpec::ByteBounded`, so ANY fan-out step declaring a byte bound failed to build. An armed deadlock monitor requires `ByteBounded` on every output transport, so a monitored fan-out chain was unbuildable. All nine calls now use `build_branch_byte_aware`, which delegates non-byte specs straight back to `build_branch` -- count/unbounded branches are unchanged. Three rstest cases assert the declared bound is registered on every branch of every shape; all three panic without the swap. - `sampler.rs`: each tick read every ordered edge's reorder stash TWICE -- once for the histogram, once for the timeline row -- taking the `ReorderStage` state mutex (held by every must-accept push and `try_pop_in_order`) on both, and the two reads could disagree about one tick. Depths are now read once per tick via `read_depths` and shared by both consumers. The module doc claimed "one `Relaxed` load per edge per tick" and "never perturbs the worker hot path", which was never true for ordered edges; it now describes the real cost. - `miri.yml`: both steps now also assert that no `#[allow(unsafe_code)]` site has moved out of the Miri-scoped module. The `--list` guard proves the filter selects tests, not that it still covers the crate's `unsafe` -- a site that moved would leave the filter matching and the site unchecked. - `storage.rs`: `affinity_in_range` re-derived the affinity->worker mapping by matching variants; it now routes through `Affinity::target_worker`, the documented single source of truth, so the range check and the dispatch gate cannot disagree. Added the missing `should_panic` case for the Exclusive owner-range assert -- only the Serial arm was covered, so that always-on `assert!` could have been weakened to a `debug_assert!` with CI still green. - `queues.rs`: the slot-cap reject test filled with 0-byte items, exercising the byte-reservation rollback with `size == 0` where a leak or double-subtract is invisible. Added a nonzero-size case pinning `current_bytes` across the reject. - Test-fixture steps returned `StepOutcome::Contention` when an output push was rejected. The driver treats it like `NoProgress`, but it feeds `contention_count`, from which the bottleneck verdict derives its SPIN ratio -- so ordinary backpressure was inventing mutex contention and could trip a bogus SPIN finding. Fixed in all six sites: the two flagged in `tests.rs` (`OrderedSource`, `OrderedPairSummer`) plus the same shape in `detached.rs` and `builder.rs`, which the review did not name. - Docs corrected where they contradicted the code: `scheduler.rs` claimed sticky steps do not take part in the round-robin walk (they do -- only their `Progress` priority restart is suppressed), `step.rs`'s `Affinity` omitted that `Detached` also ignores the hint and is never range-checked, and `TypedStep2:: build_output_set` rebuilt `profile()` twice. Fourth review round: - `handles.rs`: `BranchInputHandle::pop` reported every popped item's `heap_size()` to `record_pop`, but a `CountBoundedQueue` / `UnboundedQueue` records `record_push(0)` by design (no byte accounting on its push path). So count/unbounded edges carried nonzero `popped_bytes` against zero `pushed_bytes`, and `compute_edge_stats` reported a `mibytes_per_s` throughput for an edge that measures no bytes at all. The handle now carries `record_item_bytes`, gated on the same `transport_handle.is_some()` signal `ordered_branch` already used for the push side. - `queues.rs`: `ByteBoundedQueue::set_limit_bytes` accepted 0, which makes `try_push` reject unconditionally (`current_bytes >= 0`) and wedges the producer forever. `new` asserts `limit_bytes > 0` for exactly that reason, so the setter could undo the constructor's invariant. Now floored at 1 -- clamped rather than asserted because this runs on a live pipeline, where a 1-byte limit still makes progress and a panic would kill the run over a recoverable arithmetic slip. - `detached.rs`: `DetachedPlaceholder::detached_group` returned `PerStep` unconditionally, so a step that declared `DetachedGroup::Shared(..)` read back as `PerStep`. `extract_detached_steps` is `pub` and leaves these placeholders in the caller's slice, so an external caller regrouping from it would split one shared group across separate driver threads. The placeholder now carries the real label. - `detached_two_sided_no_deadlock` asserted only that N items arrived, so a step emitting N copies of one item -- or duplicating the held value while dropping a popped one -- passed. It now collects the values and asserts the sorted multiset (sorted, not sequenced: the producer's backpressure branch legitimately reorders on retry). - `outputs.rs`: the block comment claimed one `build_queues`/`mark_all_drained` round-trip per output shape; `Single<T>`, `OrderedBytesSingle<T>` and the plain `(A, B)` tuple had none -- and `OrderedBytesSingle<T>` is the only shape routing through `build_single_queues_ordered_bytes`, so its drain path had no coverage at all. All three added; every shape with a `StepOutputs` impl now has one. - `held.rs`: `double_put_panics` used `catch_unwind`, which accepts any panic, so a `put` failing for an unrelated reason would still pass while the double-put guard itself had gone. Pinned with `#[should_panic(expected = ...)]`, matching every other always-on guard here. Fifth review round. Two substantive bugs, both in error/stall handling: - `signal.rs`: `to_result` mapped a failed run to `Ok(())`. `record_error` wins the state CAS before it runs `payload.set`, so `state == STATE_ERROR` with `outcome() == None` is reachable; the `None` arm rescued only `STATE_CANCELLED`, so the error case fell through to success. The cancel side of this exact publish window already had a guard and a test -- the error side was the sibling nobody closed. Now derived from the terminal state, which cannot regress if a future driver forgets to join a writer. A successful run never reaches the new arm: `is_done()` is `state != STATE_OK`. - `fused.rs`: the driver failed the run on the FIRST pass in which no step reported progress. `NoProgress` is the transient "input momentarily empty but not drained" outcome -- a source waiting on a background reader returns it -- so a healthy run was aborted and its output truncated, and the `debug_assert!` turned the same transient pass into a panic under test. Idle passes now back off and retry against a wall-clock budget, and only a sustained stall reports the error. Budgeted by time rather than by pass count because `thread::sleep` granularity varies by platform. The loop also had no backoff at all, so it span the calling thread at 100%. Two findings were rejected as false positives, verified rather than assumed: - An ordered non-byte-bounded branch was said to get an uncapped reorder stash via `ReorderStage::new`. All seven `ordered_branch` call sites pass `DEFAULT_REORDER_OVERFLOW_BYTES`, so that arm was unreachable from any branch builder. Rather than only reply, the `Option<u64>` parameter is now a plain `u64` and the dead unbounded arm is gone, so the question cannot be asked of this code again. - The `HeapSize` compile-fail fixture was said to risk "expected failure but succeeded" if the bound lived only on the `StepOutputs` impl. It is on the `OrderedBytesSingle<T>` declaration, and the fixture's pinned `.stderr` shows the `E0277` plus the `required by a bound in OrderedBytesSingle` note. The rest were accuracy and test-strength fixes: - The cached-handle address assert was documented as though it proved box identity. It compares data addresses only, so allocator address reuse defeats it; soundness rests on the drop-order invariant. Corrected in `erased.rs` and in CLAUDE.md's unsafe-code entry, which carried the same overclaim. - `handles.rs`'s module doc still said tuple branches go through `build_branch` and panic on `ByteBounded`; they have used `build_branch_byte_aware` since the third round. - The same-item-type fan-out shapes (`OrderedBytesTuple2/3<OrdU64, ..>`) asserted only per-position downcasts and drain state, which an aliased branch handle satisfies. They now route a distinct value through each branch. - `maybe_instrumented` -- the constructor the branch builders actually call -- had no coverage on any of the three queue impls; one that dropped its `Some(metrics)` would have produced a silently unmonitored edge. - The edge-registry test asserted a count and `is_some()`, which swapped or duplicated consumer labels satisfy; it now pins both producer->consumer pairs. - `PipelineStats::record`/`record_error` index directly and panic on an out-of-range `StepIdx` while the detached recorders no-op; the asymmetry is deliberate and is now documented under `# Panics`. - `pool.rs`'s `sticky_exclusive_step` test helper took an `owner_idx` it discarded, with every call site passing 0. - `OrderedSource` / `OrderedPairSummer` declare `BranchOrdering::None` on an `OrderedBytesSingle` output, which maps to a direct branch -- so despite the shape's name these steps never engage the reorder stage. That is deliberate (the test reasons from FIFO order under byte backpressure) and is now stated, with pointers to the three places reorder restoration is actually covered. Sixth review round, four findings — the fused stall bound added last round, and three tests that asserted less than their messages claimed: - `fused.rs`: the stall budget was a hard-coded 5s, so a chain with a genuinely slow source had no recourse from a library-chosen policy. It now comes from `PipelineConfig::deadlock_timeout_secs`. `0` (the config default, meaning "monitor disarmed" on the scheduled path) selects a built-in default here rather than "unbounded", and the asymmetry is the point: the scheduled path has a monitor to arm and this driver does not, so a disableable bound would leave a fused wedge with nothing to detect it. Raising the number buys patience; there is deliberately no way to remove the bound. - `tests.rs`: the `build_two_input_handles` branch-mapping test asserted "branch 0 must feed input A and branch 1 input B" using `SumPairStep`, whose `a + b` is commutative -- `10 + 1` and `1 + 10` are both 11, so a swapped wiring passed while the message claimed the mapping was proven. Now uses a non-commutative `a * 1000 + b`; verified by swapping the wiring, which yields 1010 against the expected 10001. - `tests.rs`: `run_reports_finished_pipeline` sorted before comparing at every thread count, discarding a real guarantee at `threads == 1`, where `WideQueueSource` emits `n-1..0` through FIFO transports and one `ReportsFinishedBuffer` draining its `VecDeque` front-first. The single-worker case now asserts the exact descending sequence, so a reordering regression in the flush-first path is visible; above one worker the multiset assertion stands, because the Serial step's cross-worker re-dispatch interleaving is a valid scheduling detail. Stability confirmed over 40 consecutive runs. - `tests.rs`: `OrderedSink` recorded only `item.value`, leaving the out-ordinal sequence unasserted -- and the ordinal is precisely what `OrderedPairSummer`'s hold-and-retry path protects, since it assigns `next_out_ordinal` before the push and must reuse it on a retry. A regression that skipped or reassigned an ordinal left the value sequence intact and passed. It now records `(ordinal, value)` and asserts both. Seventh review round (a full re-review, not incremental), three findings: - `lib.rs`: the crate-root re-exports omitted `DetachedGroup`, which `Step::detached_group` returns while `Step` itself is flattened, and `OrderedBytesTuple2` / `OrderedBytesTuple3`, which sat beside an exported `Single` and `OrderedBytesSingle`. A step author implementing a `Detached` step or a 2-/3-way ordered fan-out had to mix flattened paths with `::step::` / `::outputs::` ones. All three added, pinned by a compile-time test that names every step-author type at the crate root — it fails to build if a `pub use` is dropped or a new output shape lands without one. A sweep for the same shape found two more unexported `pub` types, `queues::BoundedQueueHandle` and `reorder::ReorderCapHandle`, deliberately left alone: they are runtime budget plumbing named only through `runtime::contexts::RegisteredQueue`, which is not flattened either, and `ByteBoundedQueue` exposes inherent `limit_bytes` / `set_limit_bytes` / `current_bytes` so a direct user never needs the trait in scope. Promoting them would make the root surface less coherent, not more. The test records that decision so the next sweep does not re-litigate it. - `fused.rs`: `DEFAULT_STALL_BUDGET` raised from 5s to 60s. Because `deadlock_timeout_secs` defaults to 0, that default is what most fused runs actually get, and 5s is short enough for a source whose background reader blocks on a cold page cache or network-backed input to trip it — the idle timer only resets on progress, so one slow read is enough to fail a healthy run. The bound exists to catch a permanent wedge, not to police a slow source, and the costs are asymmetric: a too-tight budget loses output, while a generous one only delays the report of a wedge that has already hung. - `tests.rs`: `OrderedSink` carried two doc blocks with `#[derive(Clone)]` between them — the previous round's edit inserted the new doc after the attribute and left the stale `/// Records summed values.` line above it. Merged into one block before the attribute. A sweep for the same shape (a doc comment following an attribute) found no other site.
… framework (#697) Adds the `Step`/`Step2` traits, the bounded queue layer, the reorder stage and the work-stealing runtime that schedules and drives them, as a leaf crate. Additive: nothing on the branch changes behaviour and the crate has no in-tree consumer yet. The crate holds nothing that reads or writes sequencing data, so it depends only on ahash, crossbeam-queue, parking_lot, log and one noodles::sam type. Changes made while porting: - Dependencies moved onto `[workspace.dependencies]`. `trybuild` gains a second consumer here, so it is centralized too and `fgumi-cli-macros` switches to `{ workspace = true }`. - Five `collapsible_if` errors fixed as let-chains rather than suppressed. - `fgumi-pipeline-core` added to `publish.yml`'s `CRATES` array; it is a leaf, so any position before its future dependents is valid. - The crate-root doc described the crate as an extraction re-exported by `fgumi` as `pipeline::core`, which is true neither now nor after the series lands; rewritten to describe the crate itself. Two references to design docs that exist in no branch are dropped, and three pre-extraction `core/*.rs` module paths become intra-doc links. - Twenty rustdoc link defects fixed: nine unresolved links now carry crate paths, eight public-doc links to private items are demoted to code spans, three redundant explicit link targets shortened. `ci-doc` runs with `-D warnings`; these never surfaced while this was a private module. Review tooling follows the code out of `src/lib/`: - `.coderabbit.yaml`'s pipeline `path_instructions` glob named only the `src/lib/{unified_pipeline,pipeline}` homes, so extracting the engine into a crate silently dropped it from review — including the clause about new `unsafe` in the typed-step dispatch hot path, written for exactly the sites this commit ships. Widened to cover the crate: 33 of the 42 changed files are instructed now, against 4 before. - `miri.yml` gains a step for `fgumi-pipeline-core::erased`, where all four `#[allow(unsafe_code)]` sites live. It runs clean in ~6s. Both Miri steps now list the filtered tests first and fail on an empty selection: `cargo test <filter>` exits 0 when a filter matches nothing, so renaming either module would have retired the gate silently. The rest of the crate is excluded deliberately: its runtime tests spawn threads and take Miri well past ten minutes, and two wedge tests use wall-clock watchdogs that abort under Miri's slowdown. The four `#[allow(unsafe_code)]` sites in `erased.rs` cache the per-dispatch `downcast_ref` of a step's typed handles by widening `&'a` to `&'static` for storage and narrowing it back on read. The invariant the compiler cannot check -- that every dispatch passes the same handle box for a given step -- is enforced on the cached-hit path by storing the erased box's address in the cache slot and asserting it unconditionally -- release included, since a `debug_assert!` alone would leave release builds reading through a stale pointer. The check is one load and one compare, not the `downcast_ref` `TypeId` probe the cache exists to elide; a debug-only `debug_assert!` still re-resolves the typed pointer as a second diagnostic. CLAUDE.md's unsafe-code section gains an entry for them; note it documents all four, where feat-runall's own entry describes only the two `TypedStep` sites and omits the `TypedStep2` pair. 57 tests added over the 284 that arrive with the crate, all against public API that had no coverage: `build_queues`/`mark_all_drained` round-trips for every fan-out output shape (asserting branches are open before and closed after, so they cannot pass on an inert `mark_all_drained`), `Chain::into_multi` for the 3-, 4- and ordered-bytes-2 branch cases, the type-erased `append_source`/`append_step`/`append_step2` assembly API, `HeapSize` for String/Vec/Option, `BuildError` rendering, the `PipelineConfig` setters, `Step2`'s trait defaults, and the two input-arity dispatch guards. Fixes from review, all in the pipeline-core runtime: - `driver.rs`: a sticky step's `Progress` also means "held an item", so one sitting on a full output reported it forever. The sticky fast path is now bounded at `STICKY_BURST_LIMIT` re-entries AND round-robin no longer takes the priority restart on the sticky owner's `Progress` -- with only one of the two, the walk still breaks before reaching the consumer that drains that output, and a one-worker chain livelocks. Both halves are pinned by `sticky_holding_source_yields_to_its_draining_consumer`, which wedges (watchdog abort) if either is reverted. - `erased.rs`: `build_fused_output_set` forced `QueueSpec::Unbounded` on every branch. The fused driver runs a producer before its consumer, so a step emitting more per `try_run` than its consumer removes grew the edge every pass, without limit. The profile's count/byte bound is kept; only the ordering is dropped. - `stats.rs`: the SPIN verdict exempted Detached steps by NAME while the edge attribution beside it deliberately used `StepIdx`. `StatsSnapshot:: detached` now carries the index and the exemption matches on it, so duplicate step names stay independent. - `handles.rs`: `retry` on an `Ordered` branch with `ordinal = None` re-pushed, allocating a fresh ordinal and abandoning the original -- a permanent gap that stalls the consumer's `ReorderStage`. It now panics in every build; the recoverable `Direct` sibling is left as-is. - `contexts.rs`: `find_producer` returned the first matching edge, so two producers wired to one single-input consumer left the second branch unpopped. It now rejects that graph, mirroring the per-slot uniqueness check `find_all_producers` already had. - `tests.rs`: `PairSummer`/`OrderedPairSummer` reported `Finished` while a branch item was still buffered, silently dropping it -- the exact loss their own doc warns step authors about. `run_with_deadlock_timeout` reported a panic inside the run as "DEADLOCKED or stalled"; it now re-raises the original panic and blames a deadlock only on a true timeout. - `compile_fail.rs`: documents that the `.stderr` fixtures pin rustc's whole diagnostic, including its trait-impl inventory, and how to tell a re-bless from a real regression. The inventory blocks cannot just be deleted -- trybuild compares for equality, so that fails the test outright. Three follow-ons to those fixes, found by re-reading the call sites they touched rather than by review: - `queues.rs`: the same `debug_assert!`-with-a-data-losing-release-fallback shape the `handles.rs` fix removed was live in all three transports' `try_push` -- a push after `mark_drained` went into a queue the consumer had already closed, so the item was silently dropped and surfaced only as a short output. The check is now the unconditional `assert_not_drained` helper, shared by `CountBoundedQueue`, `ByteBoundedQueue` and `UnboundedQueue` and naming which transport failed. It loads `Relaxed` rather than `Acquire`: `drained` is monotonic (its only write anywhere is `store(true, Release)`), so a `Relaxed` load can miss a detection under a cross-thread race but can never panic a correct program, and this now sits on the per-item push path. - `fused.rs` / `builder.rs`: with fused transports keeping their profile byte bounds, `--queue-memory-total` was being silently ignored on that path in favour of the per-step defaults -- the fused contexts are built inside `run_fused_single_thread`, so `Pipeline::run` could not apply the budget on their behalf. The budget is now threaded in and applied there; `apply_initial_queue_budget` becomes `pub(crate)` for it. - `builder.rs` / `step.rs`: two comments the above invalidated. The fused fast-path comment justified skipping the rebalancer with "has no bounded queues to rebalance", which stopped being true; it now gives the real reason (producer-then-consumer in one pass, so no cross-worker imbalance) and points at where the budget is applied. `step.rs`'s last-worker-barrier note still said the `try_push`-after-drain violation was "debug-asserted". Third review round: - `handles.rs`: the 2-, 3- and 4-branch fan-out builders used the non-byte-aware `build_branch`, which panics on `QueueSpec::ByteBounded`, so ANY fan-out step declaring a byte bound failed to build. An armed deadlock monitor requires `ByteBounded` on every output transport, so a monitored fan-out chain was unbuildable. All nine calls now use `build_branch_byte_aware`, which delegates non-byte specs straight back to `build_branch` -- count/unbounded branches are unchanged. Three rstest cases assert the declared bound is registered on every branch of every shape; all three panic without the swap. - `sampler.rs`: each tick read every ordered edge's reorder stash TWICE -- once for the histogram, once for the timeline row -- taking the `ReorderStage` state mutex (held by every must-accept push and `try_pop_in_order`) on both, and the two reads could disagree about one tick. Depths are now read once per tick via `read_depths` and shared by both consumers. The module doc claimed "one `Relaxed` load per edge per tick" and "never perturbs the worker hot path", which was never true for ordered edges; it now describes the real cost. - `miri.yml`: both steps now also assert that no `#[allow(unsafe_code)]` site has moved out of the Miri-scoped module. The `--list` guard proves the filter selects tests, not that it still covers the crate's `unsafe` -- a site that moved would leave the filter matching and the site unchecked. - `storage.rs`: `affinity_in_range` re-derived the affinity->worker mapping by matching variants; it now routes through `Affinity::target_worker`, the documented single source of truth, so the range check and the dispatch gate cannot disagree. Added the missing `should_panic` case for the Exclusive owner-range assert -- only the Serial arm was covered, so that always-on `assert!` could have been weakened to a `debug_assert!` with CI still green. - `queues.rs`: the slot-cap reject test filled with 0-byte items, exercising the byte-reservation rollback with `size == 0` where a leak or double-subtract is invisible. Added a nonzero-size case pinning `current_bytes` across the reject. - Test-fixture steps returned `StepOutcome::Contention` when an output push was rejected. The driver treats it like `NoProgress`, but it feeds `contention_count`, from which the bottleneck verdict derives its SPIN ratio -- so ordinary backpressure was inventing mutex contention and could trip a bogus SPIN finding. Fixed in all six sites: the two flagged in `tests.rs` (`OrderedSource`, `OrderedPairSummer`) plus the same shape in `detached.rs` and `builder.rs`, which the review did not name. - Docs corrected where they contradicted the code: `scheduler.rs` claimed sticky steps do not take part in the round-robin walk (they do -- only their `Progress` priority restart is suppressed), `step.rs`'s `Affinity` omitted that `Detached` also ignores the hint and is never range-checked, and `TypedStep2:: build_output_set` rebuilt `profile()` twice. Fourth review round: - `handles.rs`: `BranchInputHandle::pop` reported every popped item's `heap_size()` to `record_pop`, but a `CountBoundedQueue` / `UnboundedQueue` records `record_push(0)` by design (no byte accounting on its push path). So count/unbounded edges carried nonzero `popped_bytes` against zero `pushed_bytes`, and `compute_edge_stats` reported a `mibytes_per_s` throughput for an edge that measures no bytes at all. The handle now carries `record_item_bytes`, gated on the same `transport_handle.is_some()` signal `ordered_branch` already used for the push side. - `queues.rs`: `ByteBoundedQueue::set_limit_bytes` accepted 0, which makes `try_push` reject unconditionally (`current_bytes >= 0`) and wedges the producer forever. `new` asserts `limit_bytes > 0` for exactly that reason, so the setter could undo the constructor's invariant. Now floored at 1 -- clamped rather than asserted because this runs on a live pipeline, where a 1-byte limit still makes progress and a panic would kill the run over a recoverable arithmetic slip. - `detached.rs`: `DetachedPlaceholder::detached_group` returned `PerStep` unconditionally, so a step that declared `DetachedGroup::Shared(..)` read back as `PerStep`. `extract_detached_steps` is `pub` and leaves these placeholders in the caller's slice, so an external caller regrouping from it would split one shared group across separate driver threads. The placeholder now carries the real label. - `detached_two_sided_no_deadlock` asserted only that N items arrived, so a step emitting N copies of one item -- or duplicating the held value while dropping a popped one -- passed. It now collects the values and asserts the sorted multiset (sorted, not sequenced: the producer's backpressure branch legitimately reorders on retry). - `outputs.rs`: the block comment claimed one `build_queues`/`mark_all_drained` round-trip per output shape; `Single<T>`, `OrderedBytesSingle<T>` and the plain `(A, B)` tuple had none -- and `OrderedBytesSingle<T>` is the only shape routing through `build_single_queues_ordered_bytes`, so its drain path had no coverage at all. All three added; every shape with a `StepOutputs` impl now has one. - `held.rs`: `double_put_panics` used `catch_unwind`, which accepts any panic, so a `put` failing for an unrelated reason would still pass while the double-put guard itself had gone. Pinned with `#[should_panic(expected = ...)]`, matching every other always-on guard here. Fifth review round. Two substantive bugs, both in error/stall handling: - `signal.rs`: `to_result` mapped a failed run to `Ok(())`. `record_error` wins the state CAS before it runs `payload.set`, so `state == STATE_ERROR` with `outcome() == None` is reachable; the `None` arm rescued only `STATE_CANCELLED`, so the error case fell through to success. The cancel side of this exact publish window already had a guard and a test -- the error side was the sibling nobody closed. Now derived from the terminal state, which cannot regress if a future driver forgets to join a writer. A successful run never reaches the new arm: `is_done()` is `state != STATE_OK`. - `fused.rs`: the driver failed the run on the FIRST pass in which no step reported progress. `NoProgress` is the transient "input momentarily empty but not drained" outcome -- a source waiting on a background reader returns it -- so a healthy run was aborted and its output truncated, and the `debug_assert!` turned the same transient pass into a panic under test. Idle passes now back off and retry against a wall-clock budget, and only a sustained stall reports the error. Budgeted by time rather than by pass count because `thread::sleep` granularity varies by platform. The loop also had no backoff at all, so it span the calling thread at 100%. Two findings were rejected as false positives, verified rather than assumed: - An ordered non-byte-bounded branch was said to get an uncapped reorder stash via `ReorderStage::new`. All seven `ordered_branch` call sites pass `DEFAULT_REORDER_OVERFLOW_BYTES`, so that arm was unreachable from any branch builder. Rather than only reply, the `Option<u64>` parameter is now a plain `u64` and the dead unbounded arm is gone, so the question cannot be asked of this code again. - The `HeapSize` compile-fail fixture was said to risk "expected failure but succeeded" if the bound lived only on the `StepOutputs` impl. It is on the `OrderedBytesSingle<T>` declaration, and the fixture's pinned `.stderr` shows the `E0277` plus the `required by a bound in OrderedBytesSingle` note. The rest were accuracy and test-strength fixes: - The cached-handle address assert was documented as though it proved box identity. It compares data addresses only, so allocator address reuse defeats it; soundness rests on the drop-order invariant. Corrected in `erased.rs` and in CLAUDE.md's unsafe-code entry, which carried the same overclaim. - `handles.rs`'s module doc still said tuple branches go through `build_branch` and panic on `ByteBounded`; they have used `build_branch_byte_aware` since the third round. - The same-item-type fan-out shapes (`OrderedBytesTuple2/3<OrdU64, ..>`) asserted only per-position downcasts and drain state, which an aliased branch handle satisfies. They now route a distinct value through each branch. - `maybe_instrumented` -- the constructor the branch builders actually call -- had no coverage on any of the three queue impls; one that dropped its `Some(metrics)` would have produced a silently unmonitored edge. - The edge-registry test asserted a count and `is_some()`, which swapped or duplicated consumer labels satisfy; it now pins both producer->consumer pairs. - `PipelineStats::record`/`record_error` index directly and panic on an out-of-range `StepIdx` while the detached recorders no-op; the asymmetry is deliberate and is now documented under `# Panics`. - `pool.rs`'s `sticky_exclusive_step` test helper took an `owner_idx` it discarded, with every call site passing 0. - `OrderedSource` / `OrderedPairSummer` declare `BranchOrdering::None` on an `OrderedBytesSingle` output, which maps to a direct branch -- so despite the shape's name these steps never engage the reorder stage. That is deliberate (the test reasons from FIFO order under byte backpressure) and is now stated, with pointers to the three places reorder restoration is actually covered. Sixth review round, four findings — the fused stall bound added last round, and three tests that asserted less than their messages claimed: - `fused.rs`: the stall budget was a hard-coded 5s, so a chain with a genuinely slow source had no recourse from a library-chosen policy. It now comes from `PipelineConfig::deadlock_timeout_secs`. `0` (the config default, meaning "monitor disarmed" on the scheduled path) selects a built-in default here rather than "unbounded", and the asymmetry is the point: the scheduled path has a monitor to arm and this driver does not, so a disableable bound would leave a fused wedge with nothing to detect it. Raising the number buys patience; there is deliberately no way to remove the bound. - `tests.rs`: the `build_two_input_handles` branch-mapping test asserted "branch 0 must feed input A and branch 1 input B" using `SumPairStep`, whose `a + b` is commutative -- `10 + 1` and `1 + 10` are both 11, so a swapped wiring passed while the message claimed the mapping was proven. Now uses a non-commutative `a * 1000 + b`; verified by swapping the wiring, which yields 1010 against the expected 10001. - `tests.rs`: `run_reports_finished_pipeline` sorted before comparing at every thread count, discarding a real guarantee at `threads == 1`, where `WideQueueSource` emits `n-1..0` through FIFO transports and one `ReportsFinishedBuffer` draining its `VecDeque` front-first. The single-worker case now asserts the exact descending sequence, so a reordering regression in the flush-first path is visible; above one worker the multiset assertion stands, because the Serial step's cross-worker re-dispatch interleaving is a valid scheduling detail. Stability confirmed over 40 consecutive runs. - `tests.rs`: `OrderedSink` recorded only `item.value`, leaving the out-ordinal sequence unasserted -- and the ordinal is precisely what `OrderedPairSummer`'s hold-and-retry path protects, since it assigns `next_out_ordinal` before the push and must reuse it on a retry. A regression that skipped or reassigned an ordinal left the value sequence intact and passed. It now records `(ordinal, value)` and asserts both. Seventh review round (a full re-review, not incremental), three findings: - `lib.rs`: the crate-root re-exports omitted `DetachedGroup`, which `Step::detached_group` returns while `Step` itself is flattened, and `OrderedBytesTuple2` / `OrderedBytesTuple3`, which sat beside an exported `Single` and `OrderedBytesSingle`. A step author implementing a `Detached` step or a 2-/3-way ordered fan-out had to mix flattened paths with `::step::` / `::outputs::` ones. All three added, pinned by a compile-time test that names every step-author type at the crate root — it fails to build if a `pub use` is dropped or a new output shape lands without one. A sweep for the same shape found two more unexported `pub` types, `queues::BoundedQueueHandle` and `reorder::ReorderCapHandle`, deliberately left alone: they are runtime budget plumbing named only through `runtime::contexts::RegisteredQueue`, which is not flattened either, and `ByteBoundedQueue` exposes inherent `limit_bytes` / `set_limit_bytes` / `current_bytes` so a direct user never needs the trait in scope. Promoting them would make the root surface less coherent, not more. The test records that decision so the next sweep does not re-litigate it. - `fused.rs`: `DEFAULT_STALL_BUDGET` raised from 5s to 60s. Because `deadlock_timeout_secs` defaults to 0, that default is what most fused runs actually get, and 5s is short enough for a source whose background reader blocks on a cold page cache or network-backed input to trip it — the idle timer only resets on progress, so one slow read is enough to fail a healthy run. The bound exists to catch a permanent wedge, not to police a slow source, and the costs are asymmetric: a too-tight budget loses output, while a generous one only delays the report of a wedge that has already hung. - `tests.rs`: `OrderedSink` carried two doc blocks with `#[derive(Clone)]` between them — the previous round's edit inserted the new doc after the attribute and left the stale `/// Records summed values.` line above it. Merged into one block before the attribute. A sweep for the same shape (a doc comment following an attribute) found no other site.
… framework (#697) Adds the `Step`/`Step2` traits, the bounded queue layer, the reorder stage and the work-stealing runtime that schedules and drives them, as a leaf crate. Additive: nothing on the branch changes behaviour and the crate has no in-tree consumer yet. The crate holds nothing that reads or writes sequencing data, so it depends only on ahash, crossbeam-queue, parking_lot, log and one noodles::sam type. Changes made while porting: - Dependencies moved onto `[workspace.dependencies]`. `trybuild` gains a second consumer here, so it is centralized too and `fgumi-cli-macros` switches to `{ workspace = true }`. - Five `collapsible_if` errors fixed as let-chains rather than suppressed. - `fgumi-pipeline-core` added to `publish.yml`'s `CRATES` array; it is a leaf, so any position before its future dependents is valid. - The crate-root doc described the crate as an extraction re-exported by `fgumi` as `pipeline::core`, which is true neither now nor after the series lands; rewritten to describe the crate itself. Two references to design docs that exist in no branch are dropped, and three pre-extraction `core/*.rs` module paths become intra-doc links. - Twenty rustdoc link defects fixed: nine unresolved links now carry crate paths, eight public-doc links to private items are demoted to code spans, three redundant explicit link targets shortened. `ci-doc` runs with `-D warnings`; these never surfaced while this was a private module. Review tooling follows the code out of `src/lib/`: - `.coderabbit.yaml`'s pipeline `path_instructions` glob named only the `src/lib/{unified_pipeline,pipeline}` homes, so extracting the engine into a crate silently dropped it from review — including the clause about new `unsafe` in the typed-step dispatch hot path, written for exactly the sites this commit ships. Widened to cover the crate: 33 of the 42 changed files are instructed now, against 4 before. - `miri.yml` gains a step for `fgumi-pipeline-core::erased`, where all four `#[allow(unsafe_code)]` sites live. It runs clean in ~6s. Both Miri steps now list the filtered tests first and fail on an empty selection: `cargo test <filter>` exits 0 when a filter matches nothing, so renaming either module would have retired the gate silently. The rest of the crate is excluded deliberately: its runtime tests spawn threads and take Miri well past ten minutes, and two wedge tests use wall-clock watchdogs that abort under Miri's slowdown. The four `#[allow(unsafe_code)]` sites in `erased.rs` cache the per-dispatch `downcast_ref` of a step's typed handles by widening `&'a` to `&'static` for storage and narrowing it back on read. The invariant the compiler cannot check -- that every dispatch passes the same handle box for a given step -- is enforced on the cached-hit path by storing the erased box's address in the cache slot and asserting it unconditionally -- release included, since a `debug_assert!` alone would leave release builds reading through a stale pointer. The check is one load and one compare, not the `downcast_ref` `TypeId` probe the cache exists to elide; a debug-only `debug_assert!` still re-resolves the typed pointer as a second diagnostic. CLAUDE.md's unsafe-code section gains an entry for them; note it documents all four, where feat-runall's own entry describes only the two `TypedStep` sites and omits the `TypedStep2` pair. 57 tests added over the 284 that arrive with the crate, all against public API that had no coverage: `build_queues`/`mark_all_drained` round-trips for every fan-out output shape (asserting branches are open before and closed after, so they cannot pass on an inert `mark_all_drained`), `Chain::into_multi` for the 3-, 4- and ordered-bytes-2 branch cases, the type-erased `append_source`/`append_step`/`append_step2` assembly API, `HeapSize` for String/Vec/Option, `BuildError` rendering, the `PipelineConfig` setters, `Step2`'s trait defaults, and the two input-arity dispatch guards. Fixes from review, all in the pipeline-core runtime: - `driver.rs`: a sticky step's `Progress` also means "held an item", so one sitting on a full output reported it forever. The sticky fast path is now bounded at `STICKY_BURST_LIMIT` re-entries AND round-robin no longer takes the priority restart on the sticky owner's `Progress` -- with only one of the two, the walk still breaks before reaching the consumer that drains that output, and a one-worker chain livelocks. Both halves are pinned by `sticky_holding_source_yields_to_its_draining_consumer`, which wedges (watchdog abort) if either is reverted. - `erased.rs`: `build_fused_output_set` forced `QueueSpec::Unbounded` on every branch. The fused driver runs a producer before its consumer, so a step emitting more per `try_run` than its consumer removes grew the edge every pass, without limit. The profile's count/byte bound is kept; only the ordering is dropped. - `stats.rs`: the SPIN verdict exempted Detached steps by NAME while the edge attribution beside it deliberately used `StepIdx`. `StatsSnapshot:: detached` now carries the index and the exemption matches on it, so duplicate step names stay independent. - `handles.rs`: `retry` on an `Ordered` branch with `ordinal = None` re-pushed, allocating a fresh ordinal and abandoning the original -- a permanent gap that stalls the consumer's `ReorderStage`. It now panics in every build; the recoverable `Direct` sibling is left as-is. - `contexts.rs`: `find_producer` returned the first matching edge, so two producers wired to one single-input consumer left the second branch unpopped. It now rejects that graph, mirroring the per-slot uniqueness check `find_all_producers` already had. - `tests.rs`: `PairSummer`/`OrderedPairSummer` reported `Finished` while a branch item was still buffered, silently dropping it -- the exact loss their own doc warns step authors about. `run_with_deadlock_timeout` reported a panic inside the run as "DEADLOCKED or stalled"; it now re-raises the original panic and blames a deadlock only on a true timeout. - `compile_fail.rs`: documents that the `.stderr` fixtures pin rustc's whole diagnostic, including its trait-impl inventory, and how to tell a re-bless from a real regression. The inventory blocks cannot just be deleted -- trybuild compares for equality, so that fails the test outright. Three follow-ons to those fixes, found by re-reading the call sites they touched rather than by review: - `queues.rs`: the same `debug_assert!`-with-a-data-losing-release-fallback shape the `handles.rs` fix removed was live in all three transports' `try_push` -- a push after `mark_drained` went into a queue the consumer had already closed, so the item was silently dropped and surfaced only as a short output. The check is now the unconditional `assert_not_drained` helper, shared by `CountBoundedQueue`, `ByteBoundedQueue` and `UnboundedQueue` and naming which transport failed. It loads `Relaxed` rather than `Acquire`: `drained` is monotonic (its only write anywhere is `store(true, Release)`), so a `Relaxed` load can miss a detection under a cross-thread race but can never panic a correct program, and this now sits on the per-item push path. - `fused.rs` / `builder.rs`: with fused transports keeping their profile byte bounds, `--queue-memory-total` was being silently ignored on that path in favour of the per-step defaults -- the fused contexts are built inside `run_fused_single_thread`, so `Pipeline::run` could not apply the budget on their behalf. The budget is now threaded in and applied there; `apply_initial_queue_budget` becomes `pub(crate)` for it. - `builder.rs` / `step.rs`: two comments the above invalidated. The fused fast-path comment justified skipping the rebalancer with "has no bounded queues to rebalance", which stopped being true; it now gives the real reason (producer-then-consumer in one pass, so no cross-worker imbalance) and points at where the budget is applied. `step.rs`'s last-worker-barrier note still said the `try_push`-after-drain violation was "debug-asserted". Third review round: - `handles.rs`: the 2-, 3- and 4-branch fan-out builders used the non-byte-aware `build_branch`, which panics on `QueueSpec::ByteBounded`, so ANY fan-out step declaring a byte bound failed to build. An armed deadlock monitor requires `ByteBounded` on every output transport, so a monitored fan-out chain was unbuildable. All nine calls now use `build_branch_byte_aware`, which delegates non-byte specs straight back to `build_branch` -- count/unbounded branches are unchanged. Three rstest cases assert the declared bound is registered on every branch of every shape; all three panic without the swap. - `sampler.rs`: each tick read every ordered edge's reorder stash TWICE -- once for the histogram, once for the timeline row -- taking the `ReorderStage` state mutex (held by every must-accept push and `try_pop_in_order`) on both, and the two reads could disagree about one tick. Depths are now read once per tick via `read_depths` and shared by both consumers. The module doc claimed "one `Relaxed` load per edge per tick" and "never perturbs the worker hot path", which was never true for ordered edges; it now describes the real cost. - `miri.yml`: both steps now also assert that no `#[allow(unsafe_code)]` site has moved out of the Miri-scoped module. The `--list` guard proves the filter selects tests, not that it still covers the crate's `unsafe` -- a site that moved would leave the filter matching and the site unchecked. - `storage.rs`: `affinity_in_range` re-derived the affinity->worker mapping by matching variants; it now routes through `Affinity::target_worker`, the documented single source of truth, so the range check and the dispatch gate cannot disagree. Added the missing `should_panic` case for the Exclusive owner-range assert -- only the Serial arm was covered, so that always-on `assert!` could have been weakened to a `debug_assert!` with CI still green. - `queues.rs`: the slot-cap reject test filled with 0-byte items, exercising the byte-reservation rollback with `size == 0` where a leak or double-subtract is invisible. Added a nonzero-size case pinning `current_bytes` across the reject. - Test-fixture steps returned `StepOutcome::Contention` when an output push was rejected. The driver treats it like `NoProgress`, but it feeds `contention_count`, from which the bottleneck verdict derives its SPIN ratio -- so ordinary backpressure was inventing mutex contention and could trip a bogus SPIN finding. Fixed in all six sites: the two flagged in `tests.rs` (`OrderedSource`, `OrderedPairSummer`) plus the same shape in `detached.rs` and `builder.rs`, which the review did not name. - Docs corrected where they contradicted the code: `scheduler.rs` claimed sticky steps do not take part in the round-robin walk (they do -- only their `Progress` priority restart is suppressed), `step.rs`'s `Affinity` omitted that `Detached` also ignores the hint and is never range-checked, and `TypedStep2:: build_output_set` rebuilt `profile()` twice. Fourth review round: - `handles.rs`: `BranchInputHandle::pop` reported every popped item's `heap_size()` to `record_pop`, but a `CountBoundedQueue` / `UnboundedQueue` records `record_push(0)` by design (no byte accounting on its push path). So count/unbounded edges carried nonzero `popped_bytes` against zero `pushed_bytes`, and `compute_edge_stats` reported a `mibytes_per_s` throughput for an edge that measures no bytes at all. The handle now carries `record_item_bytes`, gated on the same `transport_handle.is_some()` signal `ordered_branch` already used for the push side. - `queues.rs`: `ByteBoundedQueue::set_limit_bytes` accepted 0, which makes `try_push` reject unconditionally (`current_bytes >= 0`) and wedges the producer forever. `new` asserts `limit_bytes > 0` for exactly that reason, so the setter could undo the constructor's invariant. Now floored at 1 -- clamped rather than asserted because this runs on a live pipeline, where a 1-byte limit still makes progress and a panic would kill the run over a recoverable arithmetic slip. - `detached.rs`: `DetachedPlaceholder::detached_group` returned `PerStep` unconditionally, so a step that declared `DetachedGroup::Shared(..)` read back as `PerStep`. `extract_detached_steps` is `pub` and leaves these placeholders in the caller's slice, so an external caller regrouping from it would split one shared group across separate driver threads. The placeholder now carries the real label. - `detached_two_sided_no_deadlock` asserted only that N items arrived, so a step emitting N copies of one item -- or duplicating the held value while dropping a popped one -- passed. It now collects the values and asserts the sorted multiset (sorted, not sequenced: the producer's backpressure branch legitimately reorders on retry). - `outputs.rs`: the block comment claimed one `build_queues`/`mark_all_drained` round-trip per output shape; `Single<T>`, `OrderedBytesSingle<T>` and the plain `(A, B)` tuple had none -- and `OrderedBytesSingle<T>` is the only shape routing through `build_single_queues_ordered_bytes`, so its drain path had no coverage at all. All three added; every shape with a `StepOutputs` impl now has one. - `held.rs`: `double_put_panics` used `catch_unwind`, which accepts any panic, so a `put` failing for an unrelated reason would still pass while the double-put guard itself had gone. Pinned with `#[should_panic(expected = ...)]`, matching every other always-on guard here. Fifth review round. Two substantive bugs, both in error/stall handling: - `signal.rs`: `to_result` mapped a failed run to `Ok(())`. `record_error` wins the state CAS before it runs `payload.set`, so `state == STATE_ERROR` with `outcome() == None` is reachable; the `None` arm rescued only `STATE_CANCELLED`, so the error case fell through to success. The cancel side of this exact publish window already had a guard and a test -- the error side was the sibling nobody closed. Now derived from the terminal state, which cannot regress if a future driver forgets to join a writer. A successful run never reaches the new arm: `is_done()` is `state != STATE_OK`. - `fused.rs`: the driver failed the run on the FIRST pass in which no step reported progress. `NoProgress` is the transient "input momentarily empty but not drained" outcome -- a source waiting on a background reader returns it -- so a healthy run was aborted and its output truncated, and the `debug_assert!` turned the same transient pass into a panic under test. Idle passes now back off and retry against a wall-clock budget, and only a sustained stall reports the error. Budgeted by time rather than by pass count because `thread::sleep` granularity varies by platform. The loop also had no backoff at all, so it span the calling thread at 100%. Two findings were rejected as false positives, verified rather than assumed: - An ordered non-byte-bounded branch was said to get an uncapped reorder stash via `ReorderStage::new`. All seven `ordered_branch` call sites pass `DEFAULT_REORDER_OVERFLOW_BYTES`, so that arm was unreachable from any branch builder. Rather than only reply, the `Option<u64>` parameter is now a plain `u64` and the dead unbounded arm is gone, so the question cannot be asked of this code again. - The `HeapSize` compile-fail fixture was said to risk "expected failure but succeeded" if the bound lived only on the `StepOutputs` impl. It is on the `OrderedBytesSingle<T>` declaration, and the fixture's pinned `.stderr` shows the `E0277` plus the `required by a bound in OrderedBytesSingle` note. The rest were accuracy and test-strength fixes: - The cached-handle address assert was documented as though it proved box identity. It compares data addresses only, so allocator address reuse defeats it; soundness rests on the drop-order invariant. Corrected in `erased.rs` and in CLAUDE.md's unsafe-code entry, which carried the same overclaim. - `handles.rs`'s module doc still said tuple branches go through `build_branch` and panic on `ByteBounded`; they have used `build_branch_byte_aware` since the third round. - The same-item-type fan-out shapes (`OrderedBytesTuple2/3<OrdU64, ..>`) asserted only per-position downcasts and drain state, which an aliased branch handle satisfies. They now route a distinct value through each branch. - `maybe_instrumented` -- the constructor the branch builders actually call -- had no coverage on any of the three queue impls; one that dropped its `Some(metrics)` would have produced a silently unmonitored edge. - The edge-registry test asserted a count and `is_some()`, which swapped or duplicated consumer labels satisfy; it now pins both producer->consumer pairs. - `PipelineStats::record`/`record_error` index directly and panic on an out-of-range `StepIdx` while the detached recorders no-op; the asymmetry is deliberate and is now documented under `# Panics`. - `pool.rs`'s `sticky_exclusive_step` test helper took an `owner_idx` it discarded, with every call site passing 0. - `OrderedSource` / `OrderedPairSummer` declare `BranchOrdering::None` on an `OrderedBytesSingle` output, which maps to a direct branch -- so despite the shape's name these steps never engage the reorder stage. That is deliberate (the test reasons from FIFO order under byte backpressure) and is now stated, with pointers to the three places reorder restoration is actually covered. Sixth review round, four findings — the fused stall bound added last round, and three tests that asserted less than their messages claimed: - `fused.rs`: the stall budget was a hard-coded 5s, so a chain with a genuinely slow source had no recourse from a library-chosen policy. It now comes from `PipelineConfig::deadlock_timeout_secs`. `0` (the config default, meaning "monitor disarmed" on the scheduled path) selects a built-in default here rather than "unbounded", and the asymmetry is the point: the scheduled path has a monitor to arm and this driver does not, so a disableable bound would leave a fused wedge with nothing to detect it. Raising the number buys patience; there is deliberately no way to remove the bound. - `tests.rs`: the `build_two_input_handles` branch-mapping test asserted "branch 0 must feed input A and branch 1 input B" using `SumPairStep`, whose `a + b` is commutative -- `10 + 1` and `1 + 10` are both 11, so a swapped wiring passed while the message claimed the mapping was proven. Now uses a non-commutative `a * 1000 + b`; verified by swapping the wiring, which yields 1010 against the expected 10001. - `tests.rs`: `run_reports_finished_pipeline` sorted before comparing at every thread count, discarding a real guarantee at `threads == 1`, where `WideQueueSource` emits `n-1..0` through FIFO transports and one `ReportsFinishedBuffer` draining its `VecDeque` front-first. The single-worker case now asserts the exact descending sequence, so a reordering regression in the flush-first path is visible; above one worker the multiset assertion stands, because the Serial step's cross-worker re-dispatch interleaving is a valid scheduling detail. Stability confirmed over 40 consecutive runs. - `tests.rs`: `OrderedSink` recorded only `item.value`, leaving the out-ordinal sequence unasserted -- and the ordinal is precisely what `OrderedPairSummer`'s hold-and-retry path protects, since it assigns `next_out_ordinal` before the push and must reuse it on a retry. A regression that skipped or reassigned an ordinal left the value sequence intact and passed. It now records `(ordinal, value)` and asserts both. Seventh review round (a full re-review, not incremental), three findings: - `lib.rs`: the crate-root re-exports omitted `DetachedGroup`, which `Step::detached_group` returns while `Step` itself is flattened, and `OrderedBytesTuple2` / `OrderedBytesTuple3`, which sat beside an exported `Single` and `OrderedBytesSingle`. A step author implementing a `Detached` step or a 2-/3-way ordered fan-out had to mix flattened paths with `::step::` / `::outputs::` ones. All three added, pinned by a compile-time test that names every step-author type at the crate root — it fails to build if a `pub use` is dropped or a new output shape lands without one. A sweep for the same shape found two more unexported `pub` types, `queues::BoundedQueueHandle` and `reorder::ReorderCapHandle`, deliberately left alone: they are runtime budget plumbing named only through `runtime::contexts::RegisteredQueue`, which is not flattened either, and `ByteBoundedQueue` exposes inherent `limit_bytes` / `set_limit_bytes` / `current_bytes` so a direct user never needs the trait in scope. Promoting them would make the root surface less coherent, not more. The test records that decision so the next sweep does not re-litigate it. - `fused.rs`: `DEFAULT_STALL_BUDGET` raised from 5s to 60s. Because `deadlock_timeout_secs` defaults to 0, that default is what most fused runs actually get, and 5s is short enough for a source whose background reader blocks on a cold page cache or network-backed input to trip it — the idle timer only resets on progress, so one slow read is enough to fail a healthy run. The bound exists to catch a permanent wedge, not to police a slow source, and the costs are asymmetric: a too-tight budget loses output, while a generous one only delays the report of a wedge that has already hung. - `tests.rs`: `OrderedSink` carried two doc blocks with `#[derive(Clone)]` between them — the previous round's edit inserted the new doc after the attribute and left the stale `/// Records summed values.` line above it. Merged into one block before the attribute. A sweep for the same shape (a doc comment following an attribute) found no other site.
What this is
PR 3 of the
feat-runalllanding series. It addsfgumi-pipeline-core, the typed-step pipeline framework: theStep/Step2traits, the bounded queue layer, the reorder stage, and the work-stealing runtime that schedules and drives them. Like the crates in #672 and #674 it is additive — nothing onmain-runallchanges behaviour, and the crate has no in-tree consumer yet. Thechains/stepslayers that drive these primitives arrive in a later PR.The crate is deliberately free of anything that reads or writes sequencing data: it depends on
ahash,crossbeam-queue,parking_lot,log, and exactly onenoodles::samtype (the shared header handle). It compiles in ~8s standalone.Ported as-is, with these changes
The source is
feat-runall'scrates/fgumi-pipeline-coreverbatim, plus:Dependencies centralized (H7). The branch declared
ahash = "0.8",log = "0",rstest = "0"etc. inline; all nine now come from[workspace.dependencies].trybuildgained a second consumer with this crate (fgumi-cli-macroswas the first), so per the rootCargo.toml's own rule — "third-party crates shared by two or more members declared once here" — it moves to the workspace table andfgumi-cli-macrosswitches to{ workspace = true }.WS-LINT (H9). Five
collapsible_iferrors, fixed as let-chains rather than suppressed, perCLAUDE.md's "adopt idioms the newer compiler unlocks" rule. This matches the tracker's prediction of 5 for this crate exactly.publish.yml (H11).
fgumi-pipeline-coreadded to theCRATESarray afterfgumi-cli-common. It is a leaf, so any position before its future dependents is topologically valid. Verified locally with the workflow's own completeness, stale-entry and topological-order checks.Documentation that described a state this repo is not in. The crate root's module doc opened by explaining that the crate had been "extracted from the
fgumicrate'spipeline::coremodule" and that "thefgumicrate re-exports this aspipeline::core" — neither is true here, and both would still be wrong after the series lands. Rewritten to describe what the crate is. Two doc comments referenced design docs that exist in no branch (docs/design/unified-pipeline-step-dsl.md,docs/design/unified-pipeline-typed-step-migration.md); those pointers are dropped. Three others used the pre-extractioncore/queues.rsmodule paths, now intra-doc links.Twenty rustdoc link defects.
cargo ci-docruns with-D warnings; as a private module insidefgumithese never surfaced. Nine unresolved links (StepCtx2,OutputQueueSet,RawOccupancy,TypedStep/TypedStep2/ErasedStep,Step/Step2) now carry crate paths; eight public-doc links to private items (retry_held_impl,Self::to_result,Self::push_metrics,DetachedPlaceholder) are demoted to code spans; three redundant explicit link targets are shortened.Review tooling follows the code out of
src/lib/Two things silently stopped covering this code the moment it became a crate, and both are fixed here rather than left for whoever notices:
.coderabbit.yaml. The pipelinepath_instructionsentry — the one naming this code's actual failure modes (deadlock, lost output, unbounded memory, the must-acceptnext_serialexemption,StepDrainCounteroutput-close accounting, and "any newunsafein the typed-step dispatch hot path not matching the documented#[allow(unsafe_code)]sites") — matched onlysrc/lib/{unified_pipeline,pipeline}/**/*.rs. Nothing in this PR matched it. Of 42 changed files, 4 were instructed; 38 were not — including the file carrying theunsafethat clause was written for. The glob now covers the crate too, bringing it to 33 of 42 (the 9 remaining are yaml/toml/md/lock/.stderr, correctly outside a*.rsrule). The config's own MAINTENANCE comment asks for exactly this on a rename.miri.yml. The workflow's stated inclusion criterion is "contains the raw-pointer … walks and has no FFI", which this crate meets — and Stacked Borrows is precisely what checks the lifetime-extension transmutes below. Added a step forfgumi-pipeline-core::erased, where all four sites live: 25 tests, clean, ~6s. The rest of the crate is excluded deliberately, and the workflow comment says why: thebuilder/runtimetests spawn threads and run full pipelines, which takes Miri well past ten minutes even with-Zmiri-disable-isolation, and two wedge tests (detached_two_sided_no_deadlock,driver_round_robins_all_live_before_parking) guard against a hang with a wall-clock watchdog thatprocess::abort()s — under Miri's slowdown that fires on a healthy run. Widening the scope means giving those watchdogs a#[cfg(miri)]budget first.The one thing worth a close look: four
unsafesiteserased.rscarries four#[allow(unsafe_code)]transmutes. This is hazard H4 arriving at P1 rather than P3 where the tracker expected it.The runtime hands each step its input/output handles as
&dyn Any, so the adapter mustdowncast_refthem back per dispatch. That sits on the per-item hot path — the crate docs record ≈1.6% of samples onTypeIdcompares in a 4-thread CODEC 8M run — so the adapter resolves once and caches. Caching a reference whose real lifetime the struct cannot name is what theunsafebuys:&'a Handleis widened to&'staticfor storage and narrowed back to&'aon read. No pointer is ever dereferenced through the'staticform.Soundness rests on three invariants documented on
TypedStep: the handle boxes are owned byChainContexts(alive for the wholePipeline::run), every step instance is dropped before those contexts are, and every dispatch passes the same box for a givenstep_idx. The third is the one the compiler cannot check, so this PR adds adebug_assert!on the cached-hit path that re-resolves the handle and compares pointers — a violation now panics in tests instead of becoming a use-after-free in release. Release builds are unchanged.CLAUDE.md's unsafe-code section requires everyunsafesite to be listed with a justification, so it gains an entry for these four. Worth knowing:feat-runallhas its own entry for this ("Approved typed-step DSL hot path"), but it documents two sites where the code has four — it predates theTypedStep2pair and was never updated. The entry added here covers all four, so it supersedes rather than duplicates the branch's version.If you would rather not take the
unsafeat all, the alternative is dropping the cache and paying thedowncast_refper dispatch. That is a one-function change and I am happy to make it — the ≈1.6% figure comes from the source branch and has not been re-measured here.Tests
284 tests arrive with the crate (283 unit + 1
trybuildsuite of 3 compile-fail goldens, which pass unmodified under rustc 1.93). Workspace total goes 7,169 → 7,456, all passing, 27 skipped.I added 23 more to clear the 90% patch-coverage gate, all against public API that arrived with zero coverage rather than to chase the number:
arity()was the only thing tested for(A,B,C),(A,B,C,D),OrderedBytesTuple2/3and()— nothing built their queues. Each now round-tripsbuild_queues→ per-branch typed input →mark_all_drained, which catches abuild_tupleN_queuesthat wires a branch twice or drops one. This is whatarity()alone cannot see. All four assert branches are open before and closed after, so they cannot pass against an inertmark_all_drained.Chain::into_multifor 3 and 4 branches. Only the 2-branch case was covered. Each branch must get its ownBranchIdx; the tests assert both that wiring all branches builds and that dropping one is reported by index.append_source/append_step/append_step2API — the assembly path the parent crate'sChainBuilderwill drive, previously untested end to end. Includes thatappend_step2consumes both upstream branches.HeapSizeforString/Vec<T>/Option<T>. These decide how much budget aByteBoundedQueuecharges; the nested-Veccase (spine + per-element heap) is the one whose absence would let a queue blow past its byte budget.BuildErrorrendering, thePipelineConfigbuilder setters,Step2's trait defaults, and the two framework-bug panics inerased.rsthat guard input-arity dispatch. The setter test is a 6-case#[rstest]table: each case asserts the field its setter targets took the value and that a field it does not target is still at its default — and asserts that second probe againstPipelineConfig::default()first, so a case cannot silently encode a wrong baseline.with_scheduler's case asserts the trait's observablename(), not allocation identity, since both schedulers are zero-sized and a pointer comparison would pass even if the setter stored the wrong one.Fidelity vs
feat-runallAudited rather than assumed. The 37-file set is identical to
origin/feat-runall, and every one of the 67 removed lines is a dependency version string, a doc comment rewritten above, or a re-indented let-chain — no functional code was dropped. Theunsafesurface diffs byte-identical: four sites on both sides, none added, none removed. No#[ignore],todo!,unimplemented!, or feature gate is hiding anything.Gates
ci-fmt/ci-lint/ci-test/ci-doctest/ci-doc--no-default-features/--all-featureschecksCargo.lockEvery non-new file in the diff is a registration:
Cargo.toml,Cargo.lock,publish.yml,crates/fgumi-cli-macros/Cargo.toml,CLAUDE.md.Note on the base
main-runallis 2 commits behindorigin/main(#676, #681). Rebasing the shared branch is a separate step; this PR targetsmain-runallas it stands.Summary by CodeRabbit
New Features
Documentation
Tests
Chores