Fixes from a full-workspace code review: correctness, error handling, robustness - #171
Merged
Conversation
…decoder aliases Both Decoder associated-type aliases instantiated ConstCodesDecoder without the OUTDEGREES/REFERENCES/BLOCKS/INTERVALS/RESIDUALS const parameters, so any Static dispatch with non-default codes validated the compression flags and then silently decoded with the default gamma/unary/gamma/gamma/zeta3 codes, producing garbage.
…zes a vertex In symmetric mode backwards_step_sum_sweep sets the exact (forward) eccentricity of the start vertex but, unlike forward_step_sum_sweep, never lowered radius_high, so ExactSumSweep could certify and return a non-minimal radius (e.g. 3 instead of 2 on the 9-node graph in the new regression test).
…ition return
run_with_logging returned early when one side of the bipartition was
empty, before copying the preference vector into rank, so a fresh
BiRank with num_sources in {0, n} silently reported all-zero ranks.
An empty .graph file (valid for a 0-node graph) made new_mmap request a zero-length anonymous mapping, which mmap-rs rejects; it now keeps at least one 16-byte block. The vec_len computation is also simplified with size_of.
… helper split::seq::Iter never advanced the lender to cutpoints[0], so split_iter_at([10, 20]) on sequential-helper implementations (BvGraphSeq, ArcListGraph, PermutedGraph, ...) returned nodes 0..10 instead of 10..20, violating the SplitLabeling contract.
Granularity::Nodes(0) and Granularity::Arcs(0) are reachable from the CLI (--node-granularity 0 / --arc-granularity 0) and flowed into par_node_apply/par_apply as a zero divisor or zero chunk size, panicking or spinning; PageRank/BiRank clamped locally but HyperBall and the generic traits did not.
…ters ParSortPairs and ParSortIters only bounds-checked the source of each pair, so out-of-range destinations (an out-of-range map value in transform::map_*, a non-bijective permutation in permute_*, or a bad arc target in 'webgraph from arcs') silently produced graphs whose successors exceed num_nodes.
…past the end iter_from unwrapped advance_by, so a from greater than num_nodes panicked instead of yielding an empty lender like the range-based graph implementations.
Cleanup ran remove_dir_all on the caller-supplied tmp_dir, deleting any preexisting content of that directory. Each compression now works in a fresh tempfile subdirectory of the configured base (removing the stateful owned_tmp_dir caching, which would also have reused a deleted path on a second compression).
The offsets loop emitted the end offset of node i as the i-th entry, so the cumulative offsets pointed every node at its successor's start (with a bogus trailing zero entry): any .ef built from a converted graph corrupted random access. The stream now starts with the leading zero and contains exactly num_nodes + 1 entries, matching 'build offsets'.
pagerank and birank dispatched on the endianness read from the .properties file but then loaded the graphs with the default (big-endian) loader, so little-endian inputs always failed to load.
…gerank Both 'threshold' and the flattened NumThreadsArg claimed -t, which panics clap's debug assertions on any debug-build invocation and is ambiguous in release builds; -t stays with --num-threads as in the other commands.
LeftSucc and RightSucc implemented ExactSizeIterator::len but kept the default (0, None) size_hint, violating the ExactSizeIterator contract and defeating consumer preallocation.
The constructors only checked the requested width, so FixedWidth::<i128>::with_bits(5) was accepted even though deserialization sign-extends through 64 bits, turning -1 into 2^64 - 1. The documented at-most-64-bit type limit is now enforced.
split_iter_at forwarded union-sized cutpoints to both children, so
splitting a union of graphs with different node counts panicked on the
smaller child ('last cutpoint must be <= num_nodes'); the cutpoints are
now clamped per child, as iter_from already did.
…raphs FairChunks yields no ranges when the target chunk size is zero (and its last range can end before trailing zero-outdegree nodes), so with_dcf could produce cutpoints that do not reach num_nodes or contain a single element, panicking later in split_iter_at.
CompFlags::from_properties unwrapped the little-endian version parse and indexed/unwrapped compression-flag tokens, so user-editable .properties content such as version=abc, compressionflags=OUTDEGREES, or OUTDEGREES_BOGUS panicked instead of returning the contextual error the load path promises.
push unwrapped push_label inside the successor iterator, turning fallible label storage into a panic; BvComp::push already propagated the same error. The error is now captured while RaggedArray::push eagerly drains the iterator and returned to the caller.
The unordered path fed a bounded channel from the calling thread while the scoped workers waited for a free pool thread; when the caller occupied the only pool thread (num_threads = 1), no worker could drain the channel and the send blocked forever. A single effective worker is now run inline.
Only the low-level with_transpose checked that the transpose has the same number of nodes and arcs as the graph; the high-level constructors (used by the CLI) accepted any graph, panicking or silently skipping systolic updates once iterations started.
…icking 'build ef', 'build dcf', and 'check ef' unwrapped the 'nodes'/'arcs' keys of user-editable .properties files, and 'from arcs --labels' unwrapped the creation of and writes to the .nodes mapping file; all now return contextual errors.
All 26 command dispatchers panicked on an unrecognized 'endianness' property value (user-editable input); they now bail with the same message, matching the write-side dispatcher.
A 0-node graph produced sentinel and NaN statistics (minoutdegree=18446744073709551615, avgoutdegree=NaN, percdangling=NaN) and an empty --scc-sizes file underflowed when indexing the largest component; both now fail with a clear error.
all_cc_upper_bound unconditionally added three iterations, but the symmetric branch performs a single BFS plus a linear scan (the directed branch performs two BFSes plus the scan), so the public iteration counts of run_symm results were over-reported.
The file was never declared in the module tree (no 'mod comp' in traits/mod.rs) and ends mid-sentence; it was an abandoned draft that misleads readers about where compression traits live.
- StoreLabelsConf::label_serializer_name returns "()" (not an empty string) for unlabeled graphs, which is the sentinel the compressor checks; - PermutedGraph applies the permutation old-to-new (node x becomes perm[x]), not new-to-old as documented; - BTreeGraph::add_arc/remove_arc panic on missing endpoints, now documented; - par_map_fold2/par_map_fold2_with require A::default() to be a fold identity, since idle workers contribute default accumulators; - the README advertised pre-rename transform functions (transpose, permute_split, map, ...) with dead docs.rs links.
The help text claimed labels are stored as Java big-endian 64-bit integers, but llp/llp-combine write and read eps-serde labels_*.bin plus labels_*.gap files.
…at load time The random-access decoders seek via unchecked Elias-Fano indexing, so a stale or truncated .ef (e.g., built for a different graph) caused out-of-bounds accesses in release builds; both random-access load paths now verify that the offsets contain num_nodes + 1 entries.
num_arcs_hint forwarded the underlying graph's arc count even though the type documentation states no exact count is available once self-loops are filtered; consumers treating the hint as exact (DCF construction, statistics, the Zip cross-check) saw an inflated value.
…hods add_arcs/from_arcs sorted the input and fed it to add_arc, which panicked on duplicate arcs with a misleading 'successor is not increasing' message; the bulk methods now deduplicate (keeping the label of the last occurrence, as LabeledBTreeGraph does) and the add_arc panic message names duplicates explicitly.
…y output --lines-to-skip counted comment lines, contrary to its documented behavior, and an input whose lines were all skipped as malformed logged an error but exited successfully without producing a graph.
'perm comp' indexed through user-provided sequences without checking that they are permutations, panicking on out-of-range values and silently emitting non-permutations for duplicates; each input is now verified. 'check ef' compared only the num_nodes node-start offsets against the graph, never the final total-length entry.
…mands - 'to arcs --labels' lost an empty label on the last line because str::lines drops the final empty field, failing legitimate label files written by 'from arcs --labels'; - 'bench bvgraph' panicked on empty node ranges and divided by zero arcs; empty graphs are now rejected up front; - '--max-ref-count -2' passed parsing and panicked in the CompFlags conversion; values below -1 are now rejected by clap.
'run llp --perm' on a zero-node graph failed with 'No labels were found' because the label stage stores no files for zero nodes; the permutation is now empty.
The DCF-based work split applies only to the permutation path, so --dcf had no effect without --permutation. clap now requires --permutation whenever --dcf is passed, turning the silent no-op into an argument error instead of a warning that could be lost in the logs.
Indexing checked only the flat Vec bound, so an out-of-range column could silently alias an element of the next row. Index and IndexMut now debug-assert that the column is within bounds.
The six copies of the boundary computation (uniform splits in SplitLabeling/IntoParLenders/ParGraph and the partitioning in ParSortPairs/ParSortIters) multiplied the segment index by the rounded-up step before clamping, which overflows for node counts near usize::MAX; the multiplications now saturate and split_iter uses an inclusive range.
The first gap used isize casts whose difference wraps for node ids above isize::MAX; the distance is now computed with a lossless u128 widening.
The BvGraph-level convenience wrapper was implemented only for the default const code parameters, although the underlying factory method is fully generic.
The end-of-sort flush covers every partition of every worker state, so sparse inputs created and memory-mapped up to threads x partitions empty batch files.
ParGraph::with_cutpoints and ParSortedLabeledGraph::from_parts stored whatever they received, so invalid sequences produced wrong metadata (e.g., par_comp trusting a wrong final boundary as num_nodes) or panicked only later inside into_par_lenders; the documented invariants are now checked up front.
Zero-node and zero-arc graphs produced NaN/inf values for avgref, avgdist, bitsperlink, bitspernode, and compratio (the latter also for complete graphs, whose theoretical bound is degenerate), which Java-compatible property consumers may reject; the ratios are now emitted only when meaningful. The version-0 zetak error message now explains the Java-compatibility constraint.
PageRank checked stochasticity only under cfg(test) and BiRank checked only the length, so production callers (including the CLI, which loads user files) could pass negative or NaN vectors: NaN norm deltas never satisfy threshold predicates, turning runs into busy loops. PageRank now always asserts stochasticity and BiRank rejects negative or non-finite entries; the divergence from the Java LAW default mode (weakly preferential there, strongly preferential here) is documented.
…ar_comp Compression workers unwrapped writer creation, pushes, and flushes, panicking inside the rayon scope on I/O or label-store failures, and an empty lender sent no message at all. Workers now send an id-carrying Result (with an explicit empty-segment variant) so the ordered merge surfaces failures as contextual errors in job order, and legal interior empty segments compress correctly instead of erroring out.
Only bvcomp_and_read had corpus replay, so the higher-level roundtrip harness (which exercises BvComp and BvCompZ::par_comp) could bitrot without CI coverage; a small deterministic seed corpus is now replayed.
A MemoryUsage of BatchSize(0), or a MemorySize smaller than one element, produced zero-capacity sort buffers whose effective batch size was then whatever Vec's growth policy picked on the first push. All three batch size computations (parallel pairs, parallel iters, sequential iters) now clamp to one element per buffer.
Fixes clippy::useless_conversion; BvComp::push and BvCompZ::push take any IntoIterator.
Fixes clippy::explicit_counter_loop; degs_iter.get_pos() remains usable after the loop for the final-entry check.
fs::copy uses copy_file_range, an unsupported syscall under Miri; read plus write exercises the same stale-offsets rejection.
The default and explicit LoadMmap load paths mprotect anonymous maps, which Miri does not support; the zero-node roundtrip now runs only its LoadMem loads under Miri. The new CLI integration tests are excluded wholesale like the preexisting ones, as the CLI loads graphs and Elias-Fano structures through mmap.
The CI clippy step runs with -D warnings, and stable clippy flags the if-let/else-return-None in TaskQueue::next (pre-existing on main) as clippy::question_mark. The ? operator is equivalent.
Co-scheduled with the ordered par_map_fold tests in test_utils_coverage, this test's one-thread Rayon pool perturbs Miri's cooperative scheduler enough to trip experimental Tree Borrows in rayon-core's in_place_scope teardown (reproduced at -Zmiri-seed=6 --test-threads=4; the ordered tests pass once it is isolated). Moving it to its own test binary keeps the deadlock regression under both normal CI and Miri without the cross-test interaction. cargo miri test --workspace now passes.
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.
58 commits of fixes from a systematic review of the workspace (
webgraph,webgraph-algo,webgraph-cli), each scoped to one semantic change with a regression test.Highlights
BVGraph compression & offsets
par_compworkers propagate I/O and label-store failures through the ordered merge as contextual errors instead of panicking inside the rayon scope; legal empty interior segments now compress correctly (previously they silently truncated the output while stamping the full node count).check_offsetsverifies the final length entry; random-access loads reject offsets files with a wrong entry count (e.g., stale.effiles).to endiannessno longer writes offsets shifted by one node.Labelings & views
Zipzips labels strictly (panics on mismatched per-node label counts instead of silently truncating), checks node streams in release builds, and forwardsnum_arcs_hint(as doesUnitLabelGraph); theSortedIteratorclaim ofRightprojections is narrowed to sound cases.NoSelfLoopsGraph::num_arcs_hintno longer overstates the filtered count.VecGraph/LabeledVecGraphbulk construction deduplicates arcs (last-wins, matchingBTreeGraph).Algorithms
HyperBallseeds the radius-0 neighborhood function with the total weight; empty graphs and malformed weights produce clean errors.PageRankvalidates preference-vector stochasticity in production builds;BiRankrejects negative/non-finite query vectors; divergence from the Java LAW default mode is documented.ExactSumSweepradius fixes; LLP gap-cost computed without signed casts.Overflow & bounds
Matrixindexing checks column bounds; partition-boundary computations no longer overflow nearusize::MAX; sorter batch size clamps to at least one element per buffer.CLI
from arcscounts--lines-to-skipafter comments and fails on zero arcs;to arcs --labelskeeps a trailing empty label;perm compvalidates permutations;check efcompares the final entry; empty-graph guards inbenchandrun llp --perm;--max-ref-countvalidated at parse time.Tests & docs
roundtripfuzz target gained corpus replay; changelogs updated across all three crates.Verification
cargo test --workspace: 58 suites greencargo test --workspace --features slow_tests: 58 suites greencargo doc --workspace --no-deps: clean (pre-existingindex.htmlcollision warning only)