Skip to content

fix: release record supplier lock on validation failure - #19811

Open
FrankChen021 wants to merge 1 commit into
apache:masterfrom
FrankChen021:agent/codeql-unreleased-lock
Open

fix: release record supplier lock on validation failure#19811
FrankChen021 wants to merge 1 commit into
apache:masterfrom
FrankChen021:agent/codeql-unreleased-lock

Conversation

@FrankChen021

Copy link
Copy Markdown
Member

Root cause

checkOffsetAvailability acquired recordSupplierLock, then validated the record supplier assignment before entering the try/finally that releases the lock. If the assignment did not contain the expected stream partition, the validation threw IllegalStateException and permanently retained the reentrant lock on that thread.

Fix

Move the assignment validation into the existing try block so every path after successful lock acquisition reaches the finally and unlocks.

Add a focused regression test that exercises the assignment-mismatch exception and verifies that recordSupplierLock is no longer locked afterward.

Impact

This prevents a malformed or unexpectedly changed record-supplier assignment from leaking the supervisor's supplier lock and potentially blocking later record-supplier operations. Successful offset-availability checks and lock scope are unchanged.

Validation

  • mvn -ntp -Pskip-static-checks -pl indexing-service -Dtest=SeekableStreamSupervisorStateTest test
    • 60 tests run; 0 failures; 0 errors; 0 skipped
  • mvn -ntp -pl indexing-service -DskipTests checkstyle:check
    • 0 Checkstyle violations
  • git diff --check

@FrankChen021 FrankChen021 changed the title Fix record supplier lock release fix: release record supplier lock on validation failure Jul 30, 2026
@FrankChen021
FrankChen021 marked this pull request as ready for review July 30, 2026 22:26
Copilot AI review requested due to automatic review settings July 30, 2026 22:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes a lock-leak bug in SeekableStreamSupervisor#checkOffsetAvailability where recordSupplierLock could remain held if record-supplier assignment validation failed, potentially blocking later supervisor operations.

Changes:

  • Moved record-supplier assignment validation inside the existing try/finally to guarantee recordSupplierLock.unlock() on all post-lock-acquisition paths.
  • Added a targeted regression test that triggers the assignment-mismatch exception path and asserts the lock is released afterward.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java Ensures recordSupplierLock is always released even when assignment validation throws.
indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorStateTest.java Adds a regression test covering the assignment-mismatch exception and lock-release behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.

Reviewed 2 of 2 changed files.


This is an automated review by Codex GPT-5.6-Sol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants