HBASE-30340: CatalogJanitor can run concurrent scans due to incorrect alreadyRunning lock handling

[sercanCyberVision]

CatalogJanitor.scan() can allow concurrent scans due to incorrect handling of the
alreadyRunning lock.

ROOT CAUSE
Currently, the lock acquisition is performed inside the try block:

    try {
      if (!alreadyRunning.compareAndSet(false, true)) {
        return -1;
      }
      ...
    } finally {
      alreadyRunning.set(false);
    }

When a scan is already running, a concurrent scan fails the compareAndSet() and
returns immediately. However, because the lock acquisition is inside the try block,
the finally block is still executed and resets alreadyRunning to false.

This allows another scan to acquire the lock while the original scan is still running.

SOLUTION
The lock acquisition should be moved before the try block:

    if (!alreadyRunning.compareAndSet(false, true)) {
      return -1;
    }
    try {
      ...
    } finally {
      alreadyRunning.set(false);
    }

This prevents another scan from starting until the current scan has completed.

[guluo2016]

Thanks for the contribution!
Could we add a UT for this scenario @sercanCyberVision

[sercanCyberVision]

@guluo2016, thank you for reviewing the PR.

I have added a UT for this scenario and ran the full TestCatalogJanitor test class. All 11 tests passed:

[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.hadoop.hbase.master.janitor.TestCatalogJanitor
[INFO] Tests run: 11, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 14.94 s -- in org.apache.hadoop.hbase.master.janitor.TestCatalogJanitor
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 11, Failures: 0, Errors: 0, Skipped: 0

I verified that the expected log messages are present in the corresponding TestCatalogJanitor-output.txt file:

2026-08-27T10:53:14,027 INFO  [HBase-Test-TestCatalogJanitor-Main-Thread] janitor.TestCatalogJanitor(721): First catalog janitor scan started and waiting to finish.
2026-08-27T10:53:14,029 INFO  [HBase-Test-TestCatalogJanitor-Main-Thread] janitor.TestCatalogJanitor(725): Second catalog janitor scan attempt returned -1.
2026-08-27T10:53:14,032 INFO  [junit-timeout-thread-1] janitor.TestCatalogJanitor(734): Third catalog janitor scan attempt returned -1.
2026-08-27T10:53:14,032 INFO  [HBase-Test-TestCatalogJanitor-Main-Thread] janitor.TestCatalogJanitor(747): Releasing first catalog janitor scan and waiting for it to complete.

I also executed the new test against the original implementation (before the fix). The test timed out after 60 seconds because the third scan() call did not return -1 as expected:

2026-08-27T10:44:27,867 ERROR [HBase-Test-TestCatalogJanitor-Main-Thread] janitor.TestCatalogJanitor(739): Third catalog janitor scan did not return -1 within 60 seconds; the scan may be running instead of returning -1.

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes a concurrency bug in CatalogJanitor.scan() where a losing compareAndSet path could still clear alreadyRunning, allowing overlapping scans.

Changes:

• Move alreadyRunning.compareAndSet(false, true) outside the try/finally so only the lock holder clears it.
• Add a regression test to ensure a failed concurrent scan attempt does not clear the lock while the first scan is still running.

Reviewed changes

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

File	Description
hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java	Fixes the lock lifecycle so concurrent scan attempts can’t clear alreadyRunning.
hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitor.java	Adds a regression test that exercises the and validates the corrected locking behavior under concurrency.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[sercanCyberVision]

@guluo2016, please find the changes:

1. As you suggested, I set the timeout duration to 5 seconds for the third scan attempt to return.
2. As Copilot suggested, I extended the try/catch/finally block scope to cover all scan attempts, so that if something goes wrong earlier in the test, allowScanToFinish can still be released in the finally block.
3. As Copilot suggested, I set a 15-second timeout for allowScanToFinish in scanForReport(). This gives the test enough time to complete all three scan attempts, including the 5-second timeout of the third scan in the negative test case, before allowScanToFinish is released in the finally block.

I executed both the positive and negative test cases again.

In the negative test case, on the build side:

[ERROR] Tests run: 11, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 19.93 s <<< FAILURE! -- in org.apache.hadoop.hbase.master.janitor.TestCatalogJanitor
[ERROR] org.apache.hadoop.hbase.master.janitor.TestCatalogJanitor.testAlreadyRunningStatusDoesNotClearLock -- Time elapsed: 7.665 s <<< FAILURE!
org.opentest4j.AssertionFailedError: execution timed out after 5000 ms

On the logs side:

2026-08-27T15:10:57,317 ERROR [HBase-Test-TestCatalogJanitor-Main-Thread] janitor.TestCatalogJanitor(743): Catalog janitor scan concurrency test failed; the alreadyRunning lock mechanism may not be behaving as expected.
2026-08-27T15:10:57,318 INFO  [HBase-Test-TestCatalogJanitor-Main-Thread] janitor.TestCatalogJanitor(751): Releasing first catalog janitor scan and waiting for it to complete.

[guluo2016]

I think the assertTimeoutPreemptively could be removed, since spy.scan() should return -1 immediately without needing a background thread.
In addition, the assertTimeoutPreemptively has undesirable side effects in some scenarios，details see：https://docs.junit.org/6.0.0/writing-tests/assertions.html?utm_source=chatgpt.com#:~:text=Preemptive%20Timeouts%20with,lang.ThreadLocal%20storage.

[sercanCyberVision]

You are right. We already have allowScanToFinish.await(15, TimeUnit.SECONDS),, which provides a timeout if the third scan incorrectly acquires the lock and starts running. I have removed assertTimeoutPreemptively

[guluo2016]

I think this catch block also is unnecessary, since it doesn't add any assertion or change the test result.

[sercanCyberVision]

It was only there for logging purposes for the third scan attempt. Since assertTimeoutPreemptively has been removed, it is no longer needed, so I removed it as well.

[guluo2016]

The assertion also is unnecessary
Maybe we can simply log whether the scanThread is alive here.

[sercanCyberVision]

Done.