Skip to content

Always capture segments flusher first for consistency#7882

Merged
agourlay merged 1 commit intodevfrom
always-capture-flushers-first
Jan 8, 2026
Merged

Always capture segments flusher first for consistency#7882
agourlay merged 1 commit intodevfrom
always-capture-flushers-first

Conversation

@agourlay
Copy link
Copy Markdown
Member

@agourlay agourlay commented Jan 8, 2026

Small change for the sake of consistency and flushing hygiene.

It unfortunately does not fix our data inconsistency issue but makes sure we capture the internal states at roughly the same time.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 8, 2026

📝 Walkthrough

Walkthrough

This pull request refactors the flusher collection logic in the segment holder's flush path. A comment is added to document the intent of improving data consistency. More significantly, flusher callbacks are pre-collected once before the sync/async branching logic, eliminating the need to iterate through segment_reads multiple times. The pre-collected flushers are then reused in both the synchronous execution path and the background flush thread, consolidating the collection mechanism.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • timvisee
  • generall
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Always capture segments flusher first for consistency' directly and accurately summarizes the main change: precomputing flusher callbacks before both sync and async flush paths to improve consistency.
Description check ✅ Passed The description is related to the changeset, explaining that the PR improves flushing hygiene and consistency by capturing internal states at roughly the same time, which aligns with the pre-collection of flushers.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a2ceb2 and 0b8e238.

📒 Files selected for processing (2)
  • lib/segment/src/segment/entry.rs
  • lib/shard/src/segment_holder/flush.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (.github/review-rules.md)

**/*.rs: Prefer explicit SomeType::from(x) over implicit x.into() in Rust
In new code, don't use transmute_from_u8, transmute_to_u8, transmute_from_u8_to_slice, transmute_from_u8_to_mut_slice, transmute_to_u8_slice - use bytemuck or zerocopy crates instead
Prefer explicit exhaustive matches over catch-all _ arm in Rust match expressions, so we don't miss handling new enum variants when they are added later (exceptions: tests and benchmarks, or when sure new variants won't affect logic)
Prefer explicit field ignoring using : _ over using .. in Rust struct destructuring (exceptions: tests and benchmarks, or when sure new fields won't affect logic)

Files:

  • lib/shard/src/segment_holder/flush.rs
  • lib/segment/src/segment/entry.rs
🧠 Learnings (9)
📓 Common learnings 
Learnt from: timvisee
Repo: qdrant/qdrant PR: 7388
File: lib/shard/src/segment_holder/flush.rs:73-76
Timestamp: 2025-10-13T09:34:22.740Z
Learning: In Qdrant's SegmentHolder flush logic, when `sync=false` and a background flush is already running, returning early is acceptable because: (1) flushes normally only start if there are changes, (2) the lowest version number is acknowledged in the WAL when changes exist, and (3) missed operations will replay on restart. However, an edge case exists when the `force` flag is used: if the first forced flush has no changes but a follow-up non-force flush with changes is ignored, the system might acknowledge a version that is too recent.

Learnt from: ffuugoo
Repo: qdrant/qdrant PR: 6352
File: lib/segment/src/id_tracker/mutable_id_tracker.rs:289-299
Timestamp: 2025-04-17T10:52:59.516Z
Learning: In Qdrant's MutableIdTracker, FileVersionTracker flushers are designed to capture the pending version at creation time (not at flush time) to ensure they remain in sync with pending_mappings flusher. This design ensures consistency between data and version information during flush operations.

Learnt from: timvisee
Repo: qdrant/qdrant PR: 7157
File: lib/shard/src/segment_holder/mod.rs:808-814
Timestamp: 2025-09-01T11:42:06.964Z
Learning: In Qdrant's segment holder, panicking when no segments exist during flush_all is intentional and preferred over graceful error handling, as having zero segments could permanently corrupt the WAL by acknowledging u64::MAX. The maintainers consider this condition impossible and use the panic as a fail-fast safety mechanism to prevent data corruption.

Learnt from: generall
Repo: qdrant/qdrant PR: 7388
File: lib/shard/src/segment_holder/flush.rs:167-191
Timestamp: 2025-10-12T22:22:11.435Z
Learning: In Qdrant's SegmentHolder flush logic, when calculating the minimum unsaved version for WAL acknowledgment, the code intentionally returns `segment_persisted_version` (not `segment_persisted_version + 1`) when there are unsaved changes. This conservative approach assumes the last persisted version might not have been fully applied and allows the system to safely retry operations when versions match, ensuring WAL consistency.
📚 Learning: 2025-10-13T09:34:22.740Z 
Learnt from: timvisee
Repo: qdrant/qdrant PR: 7388
File: lib/shard/src/segment_holder/flush.rs:73-76
Timestamp: 2025-10-13T09:34:22.740Z
Learning: In Qdrant's SegmentHolder flush logic, when `sync=false` and a background flush is already running, returning early is acceptable because: (1) flushes normally only start if there are changes, (2) the lowest version number is acknowledged in the WAL when changes exist, and (3) missed operations will replay on restart. However, an edge case exists when the `force` flag is used: if the first forced flush has no changes but a follow-up non-force flush with changes is ignored, the system might acknowledge a version that is too recent.

Applied to files:

  • lib/shard/src/segment_holder/flush.rs
  • lib/segment/src/segment/entry.rs
📚 Learning: 2025-09-01T11:42:06.964Z 
Learnt from: timvisee
Repo: qdrant/qdrant PR: 7157
File: lib/shard/src/segment_holder/mod.rs:808-814
Timestamp: 2025-09-01T11:42:06.964Z
Learning: In Qdrant's segment holder, panicking when no segments exist during flush_all is intentional and preferred over graceful error handling, as having zero segments could permanently corrupt the WAL by acknowledging u64::MAX. The maintainers consider this condition impossible and use the panic as a fail-fast safety mechanism to prevent data corruption.

Applied to files:

  • lib/shard/src/segment_holder/flush.rs
  • lib/segment/src/segment/entry.rs
📚 Learning: 2025-04-17T10:52:59.516Z 
Learnt from: ffuugoo
Repo: qdrant/qdrant PR: 6352
File: lib/segment/src/id_tracker/mutable_id_tracker.rs:289-299
Timestamp: 2025-04-17T10:52:59.516Z
Learning: In Qdrant's MutableIdTracker, FileVersionTracker flushers are designed to capture the pending version at creation time (not at flush time) to ensure they remain in sync with pending_mappings flusher. This design ensures consistency between data and version information during flush operations.

Applied to files:

  • lib/shard/src/segment_holder/flush.rs
  • lib/segment/src/segment/entry.rs
📚 Learning: 2025-08-11T00:37:34.100Z 
Learnt from: IvanPleshkov
Repo: qdrant/qdrant PR: 6986
File: lib/segment/src/vector_storage/quantized/quantized_ram_storage.rs:46-55
Timestamp: 2025-08-11T00:37:34.100Z
Learning: In `lib/segment/src/vector_storage/quantized/quantized_ram_storage.rs`, the `ChunkedVectors` used in `QuantizedRamStorage` is intentionally designed to be non-persistent during updates. The `push_vector` method only updates the in-memory vectors without flushing to disk, and this is expected behavior rather than a bug.

Applied to files:

  • lib/segment/src/segment/entry.rs
📚 Learning: 2025-09-01T11:19:26.371Z 
Learnt from: IvanPleshkov
Repo: qdrant/qdrant PR: 7193
File: lib/segment/src/vector_storage/quantized/quantized_chunked_mmap_storage.rs:17-30
Timestamp: 2025-09-01T11:19:26.371Z
Learning: In lib/segment/src/vector_storage/quantized/quantized_chunked_mmap_storage.rs, the ChunkedMmapVectors underlying implementation does not validate against zero-sized vectors, so adding such validation in QuantizedChunkedMmapStorage::new is unnecessary and would be redundant.

Applied to files:

  • lib/segment/src/segment/entry.rs
📚 Learning: 2025-08-15T11:42:00.297Z 
Learnt from: IvanPleshkov
Repo: qdrant/qdrant PR: 7043
File: lib/segment/src/vector_storage/quantized/quantized_mmap_storage.rs:86-90
Timestamp: 2025-08-15T11:42:00.297Z
Learning: In lib/segment/src/vector_storage/quantized/quantized_mmap_storage.rs, overflow protection for encoded_storage_size computation (quantized_vector_size * vectors_count) is implemented in PR #7048, using checked_mul with u64 casting to prevent silent overflow.

Applied to files:

  • lib/segment/src/segment/entry.rs
📚 Learning: 2025-10-13T22:58:03.121Z 
Learnt from: generall
Repo: qdrant/qdrant PR: 7400
File: lib/segment/src/id_tracker/simple_id_tracker.rs:234-241
Timestamp: 2025-10-13T22:58:03.121Z
Learning: SimpleIdTracker in lib/segment/src/id_tracker/simple_id_tracker.rs is being deprecated and should not receive fixes related to version tracking or recovery logic, as it has a different version storage structure that is incompatible with newer trackers.

Applied to files:

  • lib/segment/src/segment/entry.rs
📚 Learning: 2025-08-15T11:41:10.629Z 
Learnt from: IvanPleshkov
Repo: qdrant/qdrant PR: 7043
File: lib/segment/src/vector_storage/quantized/quantized_vectors.rs:857-857
Timestamp: 2025-08-15T11:41:10.629Z
Learning: In `lib/segment/src/vector_storage/quantized/quantized_vectors.rs`, the multivector offset storage has two different patterns: for RAM storage, offsets are collected into Vec<MultivectorOffset> and used directly; for MMAP storage, offsets are consumed to create a file via create_offsets_file_from_iter, then the file is loaded back as MultivectorOffsetsStorageMmap. The direct consumption of offsets iterator in the MMAP case is intentional.

Applied to files:

  • lib/segment/src/segment/entry.rs
🧬 Code graph analysis (1)
lib/shard/src/segment_holder/flush.rs (4)
lib/shard/src/operations/mod.rs (1)
  • force (160-163)
lib/segment/src/segment/entry.rs (1)
  • flusher (622-768)
lib/segment/src/index/struct_payload_index.rs (1)
  • flusher (1107-1145)
lib/shard/src/proxy_segment/segment_entry.rs (1)
  • flusher (639-643)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: test-consistency
  • GitHub Check: integration-tests-consensus
  • GitHub Check: test-shard-snapshot-api-s3-minio
  • GitHub Check: integration-tests
  • GitHub Check: test-consensus-compose
  • GitHub Check: e2e-tests
  • GitHub Check: rust-tests (ubuntu-latest)
  • GitHub Check: rust-tests (macos-latest)
  • GitHub Check: rust-tests (windows-latest)
  • GitHub Check: Test Python bindings
  • GitHub Check: lint
🔇 Additional comments (2)
lib/segment/src/segment/entry.rs (1)

640-640: LGTM: Documentation improvement aligns with refactoring.

The comment clarifies the intent of capturing flushers at a single point in time to improve data consistency. This aligns well with the actual refactoring in lib/shard/src/segment_holder/flush.rs and with the design principle that flushers capture state at creation time rather than flush time.

Based on learnings: FileVersionTracker flushers capture pending versions at creation time to maintain sync with other flushers.

lib/shard/src/segment_holder/flush.rs (1)

77-99: LGTM: Excellent refactoring that improves data consistency.

Pre-collecting all flushers before the sync/async branching is a solid improvement:

  1. Better consistency: All flusher callbacks capture their internal state at the same moment, reducing the risk of inconsistencies between segments. This aligns with the design principle that flushers capture pending state at creation time (not at flush time).

  2. Eliminates redundancy: Previously, flushers would have been collected separately in both sync and async branches, leading to redundant iterations over segment_reads.

  3. Maintains correctness: The pre-collection happens after all critical locks are acquired (segment locks, flush lock) and after background flush checks, so the same ordering and safety guarantees are preserved.

  4. Clean separation: Flusher creation (state capture) is now clearly separated from flusher execution, making the code easier to reason about.

Based on learnings: This change leverages the fact that flushers capture pending versions at creation time to ensure they remain in sync.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@agourlay agourlay requested a review from timvisee January 8, 2026 10:34
timvisee
timvisee approved these changes Jan 8, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@timvisee timvisee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe it matters, but I like both now use the same approach.

@agourlay agourlay merged commit e62d447 into dev Jan 8, 2026
15 checks passed
@agourlay agourlay deleted the always-capture-flushers-first branch January 8, 2026 10:50
@coderabbitai coderabbitai Bot mentioned this pull request Jan 20, 2026
Merged
generall pushed a commit that referenced this pull request Feb 9, 2026
@agourlay @generall
Always capture segments flusher first for consistency (#7882)
45bc203
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timvisee timvisee timvisee approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@agourlay @timvisee