Skip to content

Fix scroll lock ordering#7203

Merged
timvisee merged 1 commit intodevfrom
scroll-lock-ordering
Sep 2, 2025
Merged

Fix scroll lock ordering#7203
timvisee merged 1 commit intodevfrom
scroll-lock-ordering

Conversation

@agourlay
Copy link
Member

@agourlay agourlay commented Sep 2, 2025

Always take the scroll lock and segment locks in the same order.

The update path takes the scroll lock in the opposite order as the read paths.

https://github.com/qdrant/qdrant/blob/dev/lib/collection/src/collection_manager/collection_updater.rs#L55-L70

Now it is always

  1. scroll_lock
  2. segments lock
  3. segment lock

@agourlay agourlay marked this pull request as ready for review September 2, 2025 11:15
@agourlay agourlay requested review from ffuugoo and timvisee September 2, 2025 11:16
coderabbitai[bot]

This comment was marked as resolved.

@timvisee timvisee merged commit 74a37e5 into dev Sep 2, 2025
16 checks passed
@timvisee timvisee deleted the scroll-lock-ordering branch September 2, 2025 11:28
@qdrant qdrant deleted a comment from coderabbitai bot Sep 2, 2025
@agourlay
Copy link
Member Author

agourlay commented Sep 2, 2025

For the record, I still get a deadlock on segments with this fix.

    #[allow(clippy::too_many_arguments)]
    pub async fn scroll_by_id(
        &self,
        offset: Option<ExtendedPointId>,
        limit: usize,
        with_payload_interface: &WithPayloadInterface,
        with_vector: &WithVector,
        filter: Option<&Filter>,
        search_runtime_handle: &Handle,
        timeout: Option<Duration>,
        hw_measurement_acc: HwMeasurementAcc,
    ) -> CollectionResult<Vec<RecordInternal>> {
        let start = Instant::now();
        let timeout = timeout.unwrap_or(self.shared_storage_config.search_timeout);
        let stopping_guard = StoppingGuard::new();
        let segments = self.segments.clone();

        let scroll_lock = self.scroll_read_lock.read().await;
        let (non_appendable, appendable) = segments.read().split_segments(); <---- stuck here

timvisee pushed a commit that referenced this pull request Sep 29, 2025
@timvisee timvisee mentioned this pull request Sep 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants