Skip to content

fix: harden scan cancellation and cleanup against pre-existing races#4920

Open
porunov wants to merge 1 commit into
JanusGraph:masterfrom
porunov:fix/standard-scanner-cancellation-hardening
Open

fix: harden scan cancellation and cleanup against pre-existing races#4920
porunov wants to merge 1 commit into
JanusGraph:masterfrom
porunov:fix/standard-scanner-cancellation-hardening

Conversation

@porunov

@porunov porunov commented Jul 16, 2026

Copy link
Copy Markdown
Member

Four long-standing issues in the scan-job framework, independent of any particular backend:

  • StandardScannerExecutor.interruptTask() dereferenced rowsCollector unconditionally, so a cancel(true) arriving before run() built the collector threw NullPointerException from the future's cancellation path.

  • interrupted and rowsCollector crossed the cancelling-thread/scan-thread boundary as plain fields with no happens-before, so a cancel could be lost entirely: interruptTask() might miss the initialized collector while run() missed interrupted=true, letting a cancelled scan run to completion and still invoke finishJob. Both fields are now volatile (whichever way the cancel/setup race goes, one side observes the other) and run() short-circuits after setup when the cancel arrived before the collector existed.

  • StandardScannerExecutor.cleanup() set hasCompleted before running the collector cleanup and the transaction rollback; a cleanup failure therefore skipped storeTx.rollback() permanently (subsequent cleanup calls no-op on hasCompleted), leaking the scan's store transaction. Collector cleanup and rollback completion are now tracked separately: the rollback always runs (in a finally block), a rollback failure does not mask the original collector-cleanup failure (attached as suppressed), and a failed rollback stays retryable by the later cleanup attempt instead of being skipped forever.

  • Threads.waitForCompletion() swallowed InterruptedException, discarded the caller's interrupt status and kept waiting out the full timeout, so a cancelled scan could not shorten the up-to-3-minute processor wait. It now restores the interrupt flag and returns the current completion state immediately (single caller: StandardScannerExecutor, which logs and proceeds to forced termination).

ScanCancellationTest now also covers cancelling while the scan is still in job setup: cancel(true) must not throw, the future completes as cancelled, the pipeline unwinds, no row is processed and finishJob is never invoked.


Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there an issue associated with this PR? Is it referenced in the commit message?
  • Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you written and/or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
  • If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens scan-job cancellation and cleanup paths to avoid race-triggered NPEs, ensure store transactions are rolled back even when cleanup fails, and make scan cancellation more responsive by honoring thread interrupts during processor shutdown waits.

Changes:

  • Preserve interrupt status and stop waiting early in Threads.waitForCompletion(...) when interrupted.
  • Prevent interruptTask() from dereferencing a not-yet-initialized rowsCollector during early cancellation.
  • Ensure storeTx.rollback() executes even if rowsCollector.cleanup() throws by moving rollback into a finally block.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
janusgraph-core/src/main/java/org/janusgraph/util/system/Threads.java Restores interrupt flag and returns early on interruption during completion waits.
janusgraph-core/src/main/java/org/janusgraph/diskstorage/keycolumnvalue/scan/StandardScannerExecutor.java Hardens cancellation and cleanup to avoid NPE and rollback leaks under races/exceptions.

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

Copilot AI left a comment

Copy link
Copy Markdown

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 2 comments.

@porunov
porunov force-pushed the fix/standard-scanner-cancellation-hardening branch from fc38e48 to 6b3ae94 Compare July 17, 2026 11:32
@porunov
porunov requested a review from Copilot July 17, 2026 11:46

Copilot AI left a comment

Copy link
Copy Markdown

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 3 out of 3 changed files in this pull request and generated 4 comments.

@porunov
porunov force-pushed the fix/standard-scanner-cancellation-hardening branch from 6b3ae94 to 010b843 Compare July 17, 2026 12:11
@porunov
porunov requested a review from Copilot July 17, 2026 12:25

Copilot AI left a comment

Copy link
Copy Markdown

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 3 out of 3 changed files in this pull request and generated 1 comment.

@porunov
porunov force-pushed the fix/standard-scanner-cancellation-hardening branch from 010b843 to e74d50d Compare July 17, 2026 13:56
@porunov
porunov requested a review from Copilot July 17, 2026 14:03

Copilot AI left a comment

Copy link
Copy Markdown

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 3 out of 3 changed files in this pull request and generated 2 comments.

@porunov
porunov force-pushed the fix/standard-scanner-cancellation-hardening branch from e74d50d to c2c90e7 Compare July 17, 2026 15:08
@porunov
porunov requested a review from Copilot July 17, 2026 15:15

Copilot AI left a comment

Copy link
Copy Markdown

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 3 out of 3 changed files in this pull request and generated 2 comments.

Four long-standing issues in the scan-job framework, independent of any
particular backend:

- StandardScannerExecutor.interruptTask() dereferenced rowsCollector
  unconditionally, so a cancel(true) arriving before run() built the
  collector threw NullPointerException from the future's cancellation path.

- interrupted and rowsCollector crossed the cancelling-thread/scan-thread
  boundary as plain fields with no happens-before, so a cancel could be
  lost entirely: interruptTask() might miss the initialized collector while
  run() missed interrupted=true, letting a cancelled scan run to completion
  and still invoke finishJob. Both fields are now volatile (so one side of
  the race is always observed) and run() short-circuits after setup when
  the cancel arrived before the collector existed.

- StandardScannerExecutor.cleanup() set hasCompleted before running the
  collector cleanup and the transaction rollback; a cleanup failure
  therefore skipped storeTx.rollback() permanently (subsequent cleanup
  calls no-op on hasCompleted), leaking the scan's store transaction.
  Collector cleanup and rollback completion are now tracked separately:
  the rollback always runs (in a finally block), a rollback failure does
  not mask the original collector-cleanup failure (attached as
  suppressed), and a failed rollback stays retryable by the later
  cleanup attempt instead of being skipped forever.

- Threads.waitForCompletion() swallowed InterruptedException, discarded
  the caller's interrupt status and kept waiting out the full timeout, so
  a cancelled scan could not shorten the up-to-3-minute processor wait.
  It now restores the interrupt flag and returns the current completion
  state immediately (single caller: StandardScannerExecutor, which logs
  and proceeds to forced termination).

ScanCancellationTest now also covers cancelling while the scan is still
in job setup: cancel(true) must not throw, the future completes as
cancelled, the pipeline unwinds, no row is processed and finishJob is
never invoked.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
@porunov
porunov force-pushed the fix/standard-scanner-cancellation-hardening branch from c2c90e7 to 386381b Compare July 17, 2026 15:32
@porunov
porunov requested a review from Copilot July 17, 2026 15:33

Copilot AI left a comment

Copy link
Copy Markdown

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 3 out of 3 changed files in this pull request and generated no new comments.

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