fix(sort): address CodeRabbit review + de-duplicate cli-common helpers (#446) - #449
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
WalkthroughConsolidates error types, logging utilities, system detection, and validation parsers from the umbrella crate into Changesfgumi-cli-common consolidation and umbrella re-exports
Pipeline correctness fixes, defensive bounds, and test coverage
Sequence Diagram(s)sequenceDiagram
participant RunLoop as Pipeline::run
participant WorkerJoin as worker join()
participant HelperStop as helper stop+join
participant Caller
RunLoop->>WorkerJoin: join each worker thread
WorkerJoin-->>RunLoop: Err(panic) → store in worker_panic
RunLoop->>HelperStop: stop deadlock monitor + rebalancer
HelperStop-->>RunLoop: helpers cleaned up
RunLoop->>Caller: resume_unwind(worker_panic) if Some
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat-runall #449 +/- ##
==============================================
Coverage ? 93.97%
==============================================
Files ? 109
Lines ? 48483
Branches ? 0
==============================================
Hits ? 45564
Misses ? 2919
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nh13
force-pushed
the
446/nh/fix-coderabbit-findings
branch
from
June 20, 2026 17:16
5409934 to
146b50d
Compare
nh13
force-pushed
the
446/nh/feat-sort-cli
branch
2 times, most recently
from
June 20, 2026 22:45
2ae025e to
e382c96
Compare
nh13
force-pushed
the
446/nh/fix-coderabbit-findings
branch
from
June 21, 2026 01:22
146b50d to
939bcbc
Compare
Member
Author
|
@coderabbitai resume |
Member
Author
|
@coderabbitai review |
✅ Action performedReviews resumed. |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 906-908: The worker panic handling needs to defer unwinding across
all worker execution paths to ensure helper thread cleanup completes and prevent
deadlocks. Wrap each worker execution (in the n_threads == 1 path and all
multi-worker paths referenced at lines 936-943, 977-983, 1021-1024) with
std::panic::catch_unwind to capture panics. When catch_unwind returns an Err,
immediately set the pipeline terminal/cancel signal, store the panic payload in
the worker_panic variable, and continue execution to reach the common
monitor/rebalancer shutdown code. After all cleanup completes, call
std::panic::resume_unwind with the stored panic payload if it exists, ensuring
deferred panics unwind only after helper threads are properly cleaned up.
In `@crates/fgumi-pipeline-core/src/runtime/pool.rs`:
- Around line 223-302: Consolidate the multiple test functions
(sticky_exclusive_sets_owner_slot,
sticky_exclusive_out_of_range_owner_is_skipped,
sticky_exclusive_occupied_slot_not_overwritten,
sticky_serial_reader_affinity_targets_worker_0,
sticky_serial_writer_affinity_targets_last_worker,
sticky_serial_worker_affinity_targets_specific_worker,
sticky_serial_worker_out_of_range_is_skipped,
sticky_serial_affinity_none_is_skipped, and
sticky_exclusive_beats_sticky_serial_on_same_slot) into parameterized rstest
tests. Create separate parameterized test cases for exclusive owner assignment
scenarios and for serial affinity variants (Reader, Writer, Worker with specific
indices, Worker out of range, and None), grouping each family of tests under a
single rstest function with appropriate case inputs rather than individual test
functions.
In `@crates/fgumi-pipeline-core/src/runtime/storage.rs`:
- Around line 292-334: Refactor the four affinity eligibility test functions
(serial_reader_affinity_eligible_only_for_worker_0,
serial_writer_affinity_eligible_only_for_last_worker,
serial_worker_idx_affinity_eligible_only_for_target, and
serial_out_of_range_worker_panics_in_storage) into a single parameterized test
using the rstest framework. Use #[rstest] with a table of test cases that define
the affinity type (Reader, Writer, Worker(index)), the expected number of
workers (3), and the expected WorkerStepEntry outcomes for each worker position
(Shared or Skip). For the panic scenario, add a separate parameterized case with
#[should_panic] that tests the out-of-range worker index.
In `@crates/fgumi-pipeline-core/src/topology.rs`:
- Around line 94-100: The assertion checking consumer_input_slot validity uses
consumer_arity.max(1) which incorrectly allows slot 0 to be valid even for
source steps that are registered with input_arity = 0 (since 0.max(1) evaluates
to 1). This causes invalid producer→source edges to bypass topology validation.
Remove the max(1) call and compare consumer_input_slot directly against
consumer_arity to properly reject wiring attempts into zero-arity source steps.
In `@src/lib/logging.rs`:
- Around line 5-10: The `format_count` function is missing from the re-export
statement in the `pub use` line. Add `format_count` to the list of imports being
re-exported from `fgumi_cli_common` alongside `OperationTimer`,
`format_duration`, and `format_rate`. Additionally, check if there is a local
definition or separate import of `format_count` elsewhere in the logging module
and remove it, since the re-export from `fgumi_cli_common` should serve as the
single source of truth for this function.
🪄 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: 3f22a036-81ee-45e1-be9a-d5ecf8689b35
📒 Files selected for processing (16)
crates/fgumi-cli-common/src/lib.rscrates/fgumi-pipeline-core/src/builder.rscrates/fgumi-pipeline-core/src/erased.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-core/src/queues.rscrates/fgumi-pipeline-core/src/runtime/contexts.rscrates/fgumi-pipeline-core/src/runtime/pool.rscrates/fgumi-pipeline-core/src/runtime/storage.rscrates/fgumi-pipeline-core/src/runtime/worker_core.rscrates/fgumi-pipeline-core/src/step.rscrates/fgumi-pipeline-core/src/topology.rscrates/fgumi-pipeline-io/src/sort/and_spill.rssrc/lib/errors.rssrc/lib/logging.rssrc/lib/system.rssrc/lib/validation.rs
nh13
force-pushed
the
446/nh/feat-sort-cli
branch
from
June 21, 2026 06:11
e382c96 to
4e34b85
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
nh13
force-pushed
the
446/nh/feat-sort-cli
branch
4 times, most recently
from
June 21, 2026 21:09
6af43c3 to
b2a13ae
Compare
nh13
force-pushed
the
446/nh/fix-coderabbit-findings
branch
from
June 22, 2026 01:29
939bcbc to
0657455
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fgumi-pipeline-core/src/runtime/contexts.rs (1)
334-405: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDuplicate scenario tests increase drift risk; parameterize and add an invariant property test.
Line 334 and Line 362 are the same test family expressed as hand-written cases; convert these to a single
#[rstest]table and add oneproptestthat varies linear chain length/shape and asserts typed input-handle/downcast invariants plusbounded_queuesexpectations.As per coding guidelines, "Use
rstestfor parameterized tests in Rust test files" and "Useproptestfor property-based testing in Rust test files."🤖 Prompt for 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. In `@crates/fgumi-pipeline-core/src/runtime/contexts.rs` around lines 334 - 405, The test functions build_chain_contexts_for_two_step_chain and build_chain_contexts_three_step_linear contain duplicate test scenarios that test similar functionality. Consolidate these into a single parameterized test using #[rstest] that provides different chain lengths as input parameters to avoid code duplication. Additionally, add a property-based test using proptest that generates linear chains of varying lengths and shapes, asserting invariants such as correct typed input-handle downcasts and bounded_queues expectations. This approach reduces drift risk and improves test coverage.Source: Coding guidelines
♻️ Duplicate comments (2)
crates/fgumi-pipeline-core/src/topology.rs (1)
96-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winZero-arity consumers are still wireable via slot 0; compare against raw arity.
consumer_arity.max(1)admits invalid producer→source edges (input_arity = 0). Compare directly toconsumer_arity.Proposed fix
- assert!( - consumer_input_slot < consumer_arity.max(1), + assert!( + consumer_input_slot < consumer_arity, "consumer_input_slot {consumer_input_slot} out of range for step '{}' \ with input_arity {consumer_arity}", self.step_names[consumer.0] );🤖 Prompt for 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. In `@crates/fgumi-pipeline-core/src/topology.rs` around lines 96 - 97, The assertion checking consumer_input_slot bounds incorrectly uses consumer_arity.max(1), which allows invalid connections for zero-arity consumers. Remove the .max(1) call and compare consumer_input_slot directly against the raw consumer_arity value to properly reject slot 0 connections when the consumer has zero arity.crates/fgumi-cli-common/src/lib.rs (1)
398-398:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame
is_multiple_ofstability issue.Line 398, 400, 402 also use
is_multiple_ofonusize. Apply the same% K == 0fix.🤖 Prompt for 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. In `@crates/fgumi-cli-common/src/lib.rs` at line 398, Replace all three instances of the `is_multiple_of` method calls with the modulo operator pattern. On line 398, 400, and 402 where `is_multiple_of(G)`, `is_multiple_of(M)`, and `is_multiple_of(K)` are used respectively, replace each occurrence with the equivalent `% [divisor] == 0` check (e.g., `bytes % G == 0` instead of `bytes.is_multiple_of(G)`).
🤖 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-io/src/sort/and_spill.rs`:
- Line 170: The panic contract documentation at line 170 is incomplete. It
currently only documents panics for spill slot overflow but does not mention the
memory chunk count overflow condition. Update the `# Panics` section of the doc
comment to include both conditions: panics if the number of spill slots exceeds
`u32::MAX` AND panics if the memory chunk count exceeds `u32::MAX`. Apply the
same documentation update to the panic contract mentioned at lines 193-194.
---
Outside diff comments:
In `@crates/fgumi-pipeline-core/src/runtime/contexts.rs`:
- Around line 334-405: The test functions
build_chain_contexts_for_two_step_chain and
build_chain_contexts_three_step_linear contain duplicate test scenarios that
test similar functionality. Consolidate these into a single parameterized test
using #[rstest] that provides different chain lengths as input parameters to
avoid code duplication. Additionally, add a property-based test using proptest
that generates linear chains of varying lengths and shapes, asserting invariants
such as correct typed input-handle downcasts and bounded_queues expectations.
This approach reduces drift risk and improves test coverage.
---
Duplicate comments:
In `@crates/fgumi-cli-common/src/lib.rs`:
- Line 398: Replace all three instances of the `is_multiple_of` method calls
with the modulo operator pattern. On line 398, 400, and 402 where
`is_multiple_of(G)`, `is_multiple_of(M)`, and `is_multiple_of(K)` are used
respectively, replace each occurrence with the equivalent `% [divisor] == 0`
check (e.g., `bytes % G == 0` instead of `bytes.is_multiple_of(G)`).
In `@crates/fgumi-pipeline-core/src/topology.rs`:
- Around line 96-97: The assertion checking consumer_input_slot bounds
incorrectly uses consumer_arity.max(1), which allows invalid connections for
zero-arity consumers. Remove the .max(1) call and compare consumer_input_slot
directly against the raw consumer_arity value to properly reject slot 0
connections when the consumer has zero arity.
🪄 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: 8b928c58-3890-438c-938d-6abda930d493
📒 Files selected for processing (16)
crates/fgumi-cli-common/src/lib.rscrates/fgumi-pipeline-core/src/builder.rscrates/fgumi-pipeline-core/src/erased.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-core/src/queues.rscrates/fgumi-pipeline-core/src/runtime/contexts.rscrates/fgumi-pipeline-core/src/runtime/pool.rscrates/fgumi-pipeline-core/src/runtime/storage.rscrates/fgumi-pipeline-core/src/runtime/worker_core.rscrates/fgumi-pipeline-core/src/step.rscrates/fgumi-pipeline-core/src/topology.rscrates/fgumi-pipeline-io/src/sort/and_spill.rssrc/lib/errors.rssrc/lib/logging.rssrc/lib/system.rssrc/lib/validation.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fgumi-pipeline-core/src/runtime/contexts.rs (1)
334-405: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDuplicate scenario tests increase drift risk; parameterize and add an invariant property test.
Line 334 and Line 362 are the same test family expressed as hand-written cases; convert these to a single
#[rstest]table and add oneproptestthat varies linear chain length/shape and asserts typed input-handle/downcast invariants plusbounded_queuesexpectations.As per coding guidelines, "Use
rstestfor parameterized tests in Rust test files" and "Useproptestfor property-based testing in Rust test files."🤖 Prompt for 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. In `@crates/fgumi-pipeline-core/src/runtime/contexts.rs` around lines 334 - 405, The test functions build_chain_contexts_for_two_step_chain and build_chain_contexts_three_step_linear contain duplicate test scenarios that test similar functionality. Consolidate these into a single parameterized test using #[rstest] that provides different chain lengths as input parameters to avoid code duplication. Additionally, add a property-based test using proptest that generates linear chains of varying lengths and shapes, asserting invariants such as correct typed input-handle downcasts and bounded_queues expectations. This approach reduces drift risk and improves test coverage.Source: Coding guidelines
♻️ Duplicate comments (2)
crates/fgumi-pipeline-core/src/topology.rs (1)
96-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winZero-arity consumers are still wireable via slot 0; compare against raw arity.
consumer_arity.max(1)admits invalid producer→source edges (input_arity = 0). Compare directly toconsumer_arity.Proposed fix
- assert!( - consumer_input_slot < consumer_arity.max(1), + assert!( + consumer_input_slot < consumer_arity, "consumer_input_slot {consumer_input_slot} out of range for step '{}' \ with input_arity {consumer_arity}", self.step_names[consumer.0] );🤖 Prompt for 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. In `@crates/fgumi-pipeline-core/src/topology.rs` around lines 96 - 97, The assertion checking consumer_input_slot bounds incorrectly uses consumer_arity.max(1), which allows invalid connections for zero-arity consumers. Remove the .max(1) call and compare consumer_input_slot directly against the raw consumer_arity value to properly reject slot 0 connections when the consumer has zero arity.crates/fgumi-cli-common/src/lib.rs (1)
398-398:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame
is_multiple_ofstability issue.Line 398, 400, 402 also use
is_multiple_ofonusize. Apply the same% K == 0fix.🤖 Prompt for 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. In `@crates/fgumi-cli-common/src/lib.rs` at line 398, Replace all three instances of the `is_multiple_of` method calls with the modulo operator pattern. On line 398, 400, and 402 where `is_multiple_of(G)`, `is_multiple_of(M)`, and `is_multiple_of(K)` are used respectively, replace each occurrence with the equivalent `% [divisor] == 0` check (e.g., `bytes % G == 0` instead of `bytes.is_multiple_of(G)`).
🤖 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-io/src/sort/and_spill.rs`:
- Line 170: The panic contract documentation at line 170 is incomplete. It
currently only documents panics for spill slot overflow but does not mention the
memory chunk count overflow condition. Update the `# Panics` section of the doc
comment to include both conditions: panics if the number of spill slots exceeds
`u32::MAX` AND panics if the memory chunk count exceeds `u32::MAX`. Apply the
same documentation update to the panic contract mentioned at lines 193-194.
---
Outside diff comments:
In `@crates/fgumi-pipeline-core/src/runtime/contexts.rs`:
- Around line 334-405: The test functions
build_chain_contexts_for_two_step_chain and
build_chain_contexts_three_step_linear contain duplicate test scenarios that
test similar functionality. Consolidate these into a single parameterized test
using #[rstest] that provides different chain lengths as input parameters to
avoid code duplication. Additionally, add a property-based test using proptest
that generates linear chains of varying lengths and shapes, asserting invariants
such as correct typed input-handle downcasts and bounded_queues expectations.
This approach reduces drift risk and improves test coverage.
---
Duplicate comments:
In `@crates/fgumi-cli-common/src/lib.rs`:
- Line 398: Replace all three instances of the `is_multiple_of` method calls
with the modulo operator pattern. On line 398, 400, and 402 where
`is_multiple_of(G)`, `is_multiple_of(M)`, and `is_multiple_of(K)` are used
respectively, replace each occurrence with the equivalent `% [divisor] == 0`
check (e.g., `bytes % G == 0` instead of `bytes.is_multiple_of(G)`).
In `@crates/fgumi-pipeline-core/src/topology.rs`:
- Around line 96-97: The assertion checking consumer_input_slot bounds
incorrectly uses consumer_arity.max(1), which allows invalid connections for
zero-arity consumers. Remove the .max(1) call and compare consumer_input_slot
directly against the raw consumer_arity value to properly reject slot 0
connections when the consumer has zero arity.
🪄 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: 8b928c58-3890-438c-938d-6abda930d493
📒 Files selected for processing (16)
crates/fgumi-cli-common/src/lib.rscrates/fgumi-pipeline-core/src/builder.rscrates/fgumi-pipeline-core/src/erased.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-core/src/queues.rscrates/fgumi-pipeline-core/src/runtime/contexts.rscrates/fgumi-pipeline-core/src/runtime/pool.rscrates/fgumi-pipeline-core/src/runtime/storage.rscrates/fgumi-pipeline-core/src/runtime/worker_core.rscrates/fgumi-pipeline-core/src/step.rscrates/fgumi-pipeline-core/src/topology.rscrates/fgumi-pipeline-io/src/sort/and_spill.rssrc/lib/errors.rssrc/lib/logging.rssrc/lib/system.rssrc/lib/validation.rs
🛑 Comments failed to post (1)
crates/fgumi-pipeline-io/src/sort/and_spill.rs (1)
170-170:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPanic contract is incomplete after adding
checked_addformemory_chunk_count.Update the
# Panicsdocs to include memory-chunk-count overflow (u32::MAX), not only spill-slot overflow.Proposed doc fix
- /// Panics if the number of spill slots exceeds `u32::MAX`. + /// Panics if the number of spill slots or non-empty memory chunks + /// exceeds `u32::MAX`.Also applies to: 193-194
🤖 Prompt for 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. In `@crates/fgumi-pipeline-io/src/sort/and_spill.rs` at line 170, The panic contract documentation at line 170 is incomplete. It currently only documents panics for spill slot overflow but does not mention the memory chunk count overflow condition. Update the `# Panics` section of the doc comment to include both conditions: panics if the number of spill slots exceeds `u32::MAX` AND panics if the memory chunk count exceeds `u32::MAX`. Apply the same documentation update to the panic contract mentioned at lines 193-194.
…hims Make fgumi-cli-common the authoritative home for the helpers the sort extraction had duplicated (FgumiError/Result, validate_file_exists, parse_memory_size, OperationTimer, format_duration/rate/count, detect_total_memory/detect_cpu_count). The umbrella errors.rs, logging.rs, system.rs, and validation.rs become re-export shims so existing call-site paths resolve unchanged, leaving one FgumiError type and no duplicated helper bodies. Also harden detect_total_memory's 32-bit fallback and add resolve_memory_budget Auto-path test coverage (CodeRabbit).
nh13
force-pushed
the
446/nh/fix-coderabbit-findings
branch
from
June 22, 2026 03:59
0657455 to
5262951
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/topology.rs`:
- Around line 87-100: The wire_to_slot method accesses
self.input_arities[consumer.0] without first validating that the consumer index
is within bounds, which can result in an opaque out-of-bounds panic. Add an
explicit assertion at the beginning of the method to validate that consumer.0 is
within the valid range (less than the length of self.input_arities and
self.step_names) before attempting to access these arrays. This ensures any
topology validation failure produces a clear, deterministic error message rather
than a panic.
🪄 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: 25f8197d-58e6-4381-af7f-f233c302feb7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (17)
crates/fgumi-cli-common/src/lib.rscrates/fgumi-pipeline-core/Cargo.tomlcrates/fgumi-pipeline-core/src/builder.rscrates/fgumi-pipeline-core/src/erased.rscrates/fgumi-pipeline-core/src/handles.rscrates/fgumi-pipeline-core/src/queues.rscrates/fgumi-pipeline-core/src/runtime/contexts.rscrates/fgumi-pipeline-core/src/runtime/pool.rscrates/fgumi-pipeline-core/src/runtime/storage.rscrates/fgumi-pipeline-core/src/runtime/worker_core.rscrates/fgumi-pipeline-core/src/step.rscrates/fgumi-pipeline-core/src/topology.rscrates/fgumi-pipeline-io/src/sort/and_spill.rssrc/lib/errors.rssrc/lib/logging.rssrc/lib/system.rssrc/lib/validation.rs
CodeRabbit findings: fix the ByteBoundedQueue byte-counter race (reserve-before-push, roll back on failure) that could underflow and wedge backpressure; bound the reorder overflow stash; defer worker-panic re-raise until monitor/rebalancer threads are joined; add topology wire bounds checks; cap the worker backoff; and correct the Step/Affinity docs.
…, and chain wiring Add unit coverage for assign_sticky_owners, the affinity-gated Serial dispatch eligibility, and build_chain_contexts wiring (CodeRabbit).
Use checked_add for memory_chunk_count so an overflow panics rather than silently wrapping and corrupting the AllAnnounced count (CodeRabbit).
nh13
force-pushed
the
446/nh/fix-coderabbit-findings
branch
from
June 22, 2026 05:00
5262951 to
c8c116f
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #447. Addresses the CodeRabbit review of the sort-CLI extraction, plus a duplication cleanup in the new
fgumi-cli-commoncrate. Base this on #447, notfeat-runall— review/merge #447 first.All of CodeRabbit's 20 findings were pre-existing (in
fgumi-pipeline-corefrom #440, or in the verbatim-moved sort steps), not introduced by the extraction — #447 is byte-identical to the prior binary. This PR fixes the still-valid ones and skips the two that need a larger framework change.Commits (read in order)
refactor(cli-common): single-source shared CLI helpers via umbrella shims— the extraction had copiedFgumiError/Result,validate_file_exists,parse_memory_size,OperationTimer,format_*, anddetect_total_memory/detect_cpu_countintofgumi-cli-commonwhile the umbrella kept its originals. This makesfgumi-cli-commonthe single source and turnserrors.rs/logging.rs/system.rs/validation.rsinto re-export shims (the patterncommon.rs/command.rsalready use), so there is now oneFgumiErrorand no duplicated bodies (net −385 lines). Also hardensdetect_total_memory's 32-bit fallback and addsresolve_memory_budgetAuto-path tests.fix(pipeline-core): address correctness and robustness review findings— fixes theByteBoundedQueuebyte-counter race (reserve-before-push, roll back on failure — it could underflow and wedge backpressure), bounds the reorder overflow stash, defers worker-panic re-raise until monitor/rebalancer threads join, adds topology wire bounds checks, caps the worker backoff, and correctsStep/Affinitydocs.test(pipeline-core): cover sticky-owner assignment, affinity dispatch, and chain wiring.fix(pipeline-io): guard sort memory-chunk count against u32 overflow—checked_addformemory_chunk_countso it panics rather than silently wrapping and corrupting theAllAnnouncedcount.Per-finding disposition (20 CodeRabbit findings)
build_two_input_handlesrejoin, the twostep.rsdoc fixes, and the four test-coverage additions (pool, storage, contexts, resolve_memory_budget), plus the two cli-common findings (detect_total_memory fallback, resolve_memory_budget tests).CountBounded → ByteBoundedpair (and_spill.rs,spill_decompress.rs): both sort steps useOutputs = Single<T>, whose build path rejectsByteBounded("requires byte-aware build path"), so the suggested change panics every fused sort pipeline. The intent (memory-bound rather than count-bound backpressure on largeMemoryChunkevents) is legitimate but needs migrating the steps to theOrderedBytesSingleoutput shape — non-trivial. Left asCountBounded; worth a follow-up issue. (The u32 overflow guard from the same finding is applied.)ByItemOrdinalfor Serial/Exclusive": applying it kept a reorder stage alive on already-ordered output and deadlocked everyextractrun. The original collapse-to-Noneis correct; kept it.Verification
cargo build --release,cargo ci-test(2130 passed, 0 failed, 33 skipped),cargo ci-lint(clippy pedantic),cargo ci-fmt, andcargo check --workspace --no-default-features --all-targetsall green. Sort output is unaffected (the changes are memory-accounting / docs / tests); the 80 sort tests including the fused coordinate and template-coordinate paths pass.Summary by CodeRabbit