Fix ZookeeperLockRegistry test isolation and interrupt handling

[artembilan]

ZkLock.lockInterruptibly() looped on tryLock(1, SECONDS) until it succeeded.
Since tryLock() returns false (rather than throwing) when the Zookeeper connection probe times out, an unreachable server turned that loop into an endless spin with no deadline, no interrupt check and no logging.

The build had no test timeout configured at all, so such a hang stalled Gradle silently until the CI job limit.

• check Thread.interrupted() per iteration in lockInterruptibly() and throw InterruptedException; log a DEBUG message per retry
• log a WARN in tryLock() when the connection probe times out or reports no connection, instead of silently returning false
• clamp the recomputed waitTime with Math.max(0, ...) so a slow probe cannot pass a negative deadline to InterProcessMutex.acquire()
• set queueCapacity to 0 on the default mutexTaskExecutor: with Spring's defaults (core 1, unbounded queue) the pool never grew past a single thread, so concurrent tryLock() calls serialized behind one connection probe
• give ZkLockRegistryTests.voidLockFailsWhenServerDown() its own TestingServer; it used to stop and restart the one shared by the class, and a failed restart left the six tests declared after it blocked forever in InterProcessMutex.acquire()
• bound the maincountDownLatch.await() in concurrentObtainCapacityTest()
• add a junit.jupiter.execution.timeout.default for all Test tasks (10 m, 30 m for testAll) so a stuck test fails with a stack trace of its thread

Auto-cherry-pick to 7.1.x & 7.0.x

[artembilan]

Reviewed the fix — the core logic (interrupt check + logging in lockInterruptibly(), the Math.max(0, ...) clamp on waitTime, the isolated TestingServer for voidLockFailsWhenServerDown(), the bounded latch, and the JUnit execution timeouts) is sound and addresses the described busy-spin/hang.

Two follow-on concerns worth considering, not blockers:

1. Unbounded thread growth: threadPoolTaskExecutor.setQueueCapacity(0) removes the queue but doesn't bound maxPoolSize (Spring's ThreadPoolTaskExecutor default is Integer.MAX_VALUE). With a SynchronousQueue and no cap, every concurrent tryLock()/lockInterruptibly() connection probe that can't be served by an idle thread spawns a brand-new one. During a sustained ZooKeeper outage with many locks retrying concurrently, this could grow the thread count without bound and risk OutOfMemoryError: unable to create native thread. Might be worth setting an explicit maxPoolSize.

2. Log flooding on outage: tryLock() now logs at WARN on every failed connection probe. Since lockInterruptibly() retries roughly once per second while unreachable, a prolonged outage with active retries will produce continuous per-second WARN lines per lock instead of a one-time signal — exactly when log volume matters most. Consider logging only on state transition (first failure) or throttling.

[artembilan]

Reviewed the full main...fix-zk-busy-spin diff against the actual sources of Curator 5.9.0 and ZooKeeper 3.9.3 (not just the diff). The direction is right and the branch is a net improvement: the per-registry eagerly-created ThreadPoolTaskExecutor was a real liability, the busy-spin in lockInterruptibly() is genuinely fixed for the case that matters, the isolated TestingServer in voidLockFailsWhenServerDown() is a correct fix for a nasty test-ordering hazard, and the deprecation-instead-of-removal call is the right one for a 7.0.6 patch. Two things I would not merge without addressing: the requested tryLock deadline is no longer bounded in the silent connection-loss case (the exact scenario the removed executor existed for), and the connection check mixes two different Curator state machines. Findings below, most severe first.

1. tryLock(time, unit) can block far past time on a silent connection loss — a regression vs. the probe. ZookeeperLockRegistry.java:311-322

client.getZookeeperClient().isConnected() reads curator-client's raw ConnectionState.isConnected AtomicBoolean, which is only flipped when the ZK client notices the connection is gone. On a socket close that is immediate, but on a silent partition (no FIN/RST — the classic ZK outage) ClientCnxn.SendThread only gives up after readTimeout = negotiatedSessionTimeout * 2 / 3 (ClientCnxn.java:1446), i.e. ~40 s with Curator's default 60 s session. For that whole window isConnected() returns true, the guard is skipped, and we fall into this.mutex.acquire(remaining, MILLISECONDS).

That call is not bounded by remaining. InterProcessMutex.acquire → LockInternals.attemptLock → StandardLockInternalsDriver.createsTheLock → CreateBuilderImpl.pathInForeground → RetryLoop.callWithRetry, whose first statement is client.internalBlockUntilConnectedOrTimedOut() (verified in the bytecode of curator-client 5.9.0). That method loops until connected or connectionTimeoutMs elapses — default 15 s (CuratorFrameworkFactory.DEFAULT_CONNECTION_TIMEOUT_MS) — and then just falls through so the ZK op can throw ConnectionLoss, at which point the retry policy sleeps and the whole thing repeats.

Failure scenario: ZK is partitioned away without the socket dropping. lock.tryLock(200, TimeUnit.MILLISECONDS) blocks ~15 s (+ retry-policy sleeps, + a second pass if attemptLock retries its NoNodeException) before returning false. Under the old code the same call returned false in 200 ms, because future.get(waitTime) bounded the probe. This is precisely what the javadoc removed in this commit was talking about: "This must be performed asynchronously so the Lock#tryLock(long, TimeUnit) contract can be honored." Anything polling a lock on a hot path (aggregator/LockRegistryLeaderInitiator) will stall on this.

Note the change is still a clear win for the detected-disconnect case (blockUntilConnected returns false after exactly time, where previously acquire would burn connectionTimeoutMs there too) — the new test covers that path. It is only the stale-state window that regressed.

Suggested fix, in order of preference: (a) if a hard bound is required, the acquisition has to run off-thread, not just the probe — i.e. the executor cannot be dropped outright, only re-scoped; (b) if you accept the trade-off, stop promising a bound you do not deliver: zookeeper.adoc:88-90 currently says the wait happens "within the time requested for a Lock.tryLock(long, TimeUnit)", which is only true of the connection wait. Add a sentence that the effective ceiling is time plus Curator's connectionTimeoutMs and retry-policy budget, and that users needing a tight bound must configure CuratorFrameworkFactory.builder().connectionTimeoutMs(...) and their RetryPolicy below their lock timeout. This caveat belongs in the ZkLock.tryLock javadoc too, since it is a documented Lock contract deviation.

2. The connection check mixes two independent state machines; the pre-check is redundant. ZookeeperLockRegistry.java:311-313

client.getZookeeperClient().isConnected() reads curator-client's raw ConnectionState.isConnected (AtomicBoolean, driven by the ZK watcher). client.blockUntilConnected(...) reads a different source: ConnectionStateManager.currentConnectionState, the framework CONNECTED/SUSPENDED/LOST/READ_ONLY enum, updated by the framework's event processing. These can disagree in both directions.

Failure scenario: the raw client has already flipped to disconnected but the state manager has not yet processed the event, so currentConnectionState is still CONNECTED. blockUntilConnected then returns true immediately (ConnectionStateManager.java:207 — while (!isConnected()) is simply skipped), and we drop straight into the blocking acquire of finding 1 with a full waitTime budget that no longer bounds anything. The guard silently does nothing.

The pre-check also buys nothing: blockUntilConnected already returns immediately when the state manager says connected. Suggested fix — drop the pre-check and just call it:

if (!this.client.blockUntilConnected((int) Math.min(waitTime, Integer.MAX_VALUE), TimeUnit.MILLISECONDS)) {

One behavioural caveat worth knowing before you do: ConnectionStateManager.currentConnectionState is null until the first state change is posted, so isConnected() is false there — the very first tryLock right after client.start() would wait for the state manager (milliseconds) instead of proceeding on the raw flag. That is arguably the more correct behaviour anyway.

3. The headline fix is untested. ZkLockRegistryTests.java

voidLockFailsWhenServerDown() does exercise the new blockUntilConnected path — that part is covered, credit where due. But the actual subject of commit c1c6fe753e — lockInterruptibly() no longer spinning forever and now honouring an interrupt while ZK is unreachable — has no test at all, and SI requires a test for a behaviour change. Nothing would catch a regression that reverts the Thread.interrupted() check.

Suggested: inside the isolated-server block, after server.stop(), submit registry.obtain("baz").lockInterruptibly() to an executor, thread.interrupt() it, and assert the future completes with an InterruptedException within a few seconds.

Second, cheaper suggestion that would have caught finding 1: wrap the existing lock2.tryLock(1, TimeUnit.SECONDS) at line 320 in an elapsed-time assertion (assertThat(elapsed).isLessThan(5000)). One line, and it pins the contract this PR is about.

4. lockInterruptibly() ignores the interrupt status on entry, and the loop has no backoff. ZookeeperLockRegistry.java:277-290

Lock.lockInterruptibly() specifies that InterruptedException is thrown if the thread "has its interrupted status set on entry to this method". Here the check is at the bottom of the loop, so if the lock happens to be free the first tryLock succeeds and the method returns normally with the interrupt still pending — the caller silently loses the interrupt until something else observes it. Fix: hoist a if (Thread.interrupted()) { throw new InterruptedException(...); } above the loop (or restructure as a do/while with the check first).

Separately: the loop still has no sleep between iterations. Today it happens to be paced because blockUntilConnected eats the full second when disconnected, but that is accidental — any path that makes tryLock return false promptly (e.g. the new KeeperException catch firing fast) re-creates the very hot spin this PR removes. A small sleep or backoff would make it correct by construction rather than by coincidence. Low severity, but cheap insurance given the PR title.

5. The exception filter — narrow in one direction, semantically lossy in the other. ZookeeperLockRegistry.java:324-333

The filter does not hide genuine errors: NoAuthException, NodeExistsException and a user-level NoNodeException still propagate as MessagingException. That part is right.

Two nuances. Narrow: LockInternals.attemptLock rethrows KeeperException.NoNodeException once the retry policy is exhausted, and Curator's own comment there reads "this can happen when the session expires, etc." — so a transient session-loss case can still surface as a thrown MessagingException rather than false. Pre-existing, not a regression, but it undercuts the stated goal of "connection trouble means false".

Lossy: swallowing ConnectionLossException into false covers the ambiguous case where the ephemeral lock node was created server-side before the connection dropped. Curator's withProtection() normally reconciles that on the next attempt, so this is acceptable — but it is a change from "throw and let the caller decide" to "silently false", and it deserves a sentence in zookeeper.adoc alongside the isConnected() note.

6. build.gradle:231-238 — the comment over-promises what the JUnit timeout gives you.

junit.jupiter.execution.timeout.thread.mode.default defaults to SAME_THREAD. TimeoutInvocation schedules an interrupt of the test thread and then still waits for the invocation to return. A test wedged in a non-interruptible block — a socket read inside InterProcessMutex.acquire(), i.e. exactly the hang this PR is about — is not aborted and the build stalls anyway. So "Fail a stuck test ... instead of stalling the whole build" only holds for interruptible hangs.

Fix: either add systemProperty 'junit.jupiter.execution.timeout.thread.mode.default', 'SEPARATE_THREAD' (caveat: this changes the executing thread for every test in the build, which can upset ThreadLocal- and Spring-TestContext-sensitive tests — worth a trial run before committing to it), or soften the comment to say the stuck thread is interrupted and its stack trace recorded when it is interruptible. Also: this is a global change to every module's test task riding along in a Zookeeper bugfix PR; for the 7.0.x/7.1.x backports I would split it into its own commit so it can be dropped independently if it turns slow-but-green CI tests red.

7. destroy() as a no-op — fine, no leak. ZookeeperLockRegistry.java:187-194

The only owned resource was the executor and it is gone; the CuratorFramework was always caller-owned. The empty body is correct and the javadoc explaining why is the right call. Two nits: the class now declares DisposableBean purely to expose a no-op (keep it for 7.0.x/7.1.x binary compatibility, but it and the setter are both 8.0 removal candidates — worth tracking together), and the javadoc could note that the locks cache is not cleared either, so expireUnusedOlderThan remains the only eviction hook.

8. Backport safety of the deprecation — this is the right call, no change needed.

Stating it plainly since it was asked. There is no XSD or namespace attribute for mutexTaskExecutor (I checked spring-integration-zookeeper/src/main/resources — the module has no XML config for the lock registry), so only programmatic callers are affected, and for them the method still compiles and still runs. The setter's only documented reason to exist was managed-thread environments (WorkManagerTaskExecutor); since the registry now creates no threads at all, those users are strictly better off. And removing an eagerly-initialised ThreadPoolTaskExecutor per registry instance is a resource reduction in a patch release, which is the safe direction. Retaining "some behaviour" would mean retaining the probe, which is the bug. Two small nits: the setter now silently accepts null where it previously did Assert.notNull (harmless, but it is a contract loosening); and forRemoval = true in a patch commits you to removal in 8.0 — presumably intentional, just confirming it is deliberate rather than reflexive.

9. Verified-correct items, so nobody re-litigates them in review.

• The int narrowing is safe. Math.min(waitTime, Integer.MAX_VALUE) is evaluated in long and clamps at 2147483647 ms (~24.8 days); TimeUnit.toMillis saturates rather than wraps, so no overflow. Negative/zero time is also safe: ConnectionStateManager.blockUntilConnected returns immediately when maxWaitTime <= 0 with a non-null unit (ConnectionStateManager.java:201-219, and the CuratorFramework javadoc states it explicitly).
• Interruptibility is preserved: blockUntilConnected waits via Object.wait and declares InterruptedException, internalLockLoop likewise, and tryLock rethrows.
• The Math.max(0, ...) clamp is harmless but a no-op in practice — LockInternals.internalLockLoop treats millisToWait <= 0 identically to a negative value (break → deleteOurPath → false).
• Importing org.apache.zookeeper.KeeperException into main sources is fine; ZookeeperMetadataStore already does it, and zookeeper is an api-scoped transitive of curator-recipes.
• Checkstyle: AtclauseOrder for METHOD_DEF is @param, @return, @throws, @since, @deprecated, @see, so the setter javadoc order is compliant; EmptyBlock uses the default token set, which excludes METHOD_DEF, so the empty destroy() body will not trip it.

10. Nits.

• Log/LogFactory has plenty of precedent, but newer SI code (DefaultLockRepository, PostgresChannelMessageTableSubscriber, LockRegistryLeaderInitiator) uses org.springframework.core.log.LogAccessor, whose debug(Supplier<String>) overloads would let you drop all three isDebugEnabled() guards and the string concatenation.
• Grammar: "involved into locking" → "involved in locking", in both ZookeeperLockRegistry.java:125 and zookeeper.adoc:88.
• ZkLockRegistryTests.java:332 is 122 chars (SI style guide caps at 120; checkstyle has no LineLength module, so purely cosmetic).
• ZookeeperTestSupport.createNewClient() already builds exactly this client with the same BoundedExponentialBackoffRetry(100, 1000, 3); a connect-string overload there would avoid duplicating it in the test.
• @author/@since: both files already carry the author tag and there is no new public API, so nothing missing.
• Branch hygiene: 770bced644 is an empty commit and 43d64a74cb polishes its immediate predecessor — per CONTRIBUTING both should be squashed into the first commit before merge. Also, the commit trailer points at the PR itself; the guidelines ask for a GH issue reference (Fixes: gh-NNNN), and a real hang bug warrants one for the 7.0.6/7.1.x release notes.

[artembilan]

Self-review of the branch (AI-assisted, verified against Curator 5.9.0 sources)

Claims below were traced to the actual Curator 5.9.0 (gradle/libs.versions.toml) and JUnit Jupiter 6.1.2 sources unpacked from the Gradle cache, not recalled.

Blocking issues

1. The new test does not exercise the fix it is named for.

ZkLockRegistryTests lines ~344-346:

lockThread.start();
lockThread.interrupt();

There is no barrier, so the interrupt lands before or during the first tryLock. Three separate paths then produce InterruptedException, and only one of them is the new in-loop check:

1. the entry guard checkInterruption() in lockInterruptibly();
2. blockUntilConnected() -> ConnectionStateManager.blockUntilConnected (verified: while (!isConnected()) { ... wait(waitTime); }, so Object.wait throws immediately when the flag is already set) -> rethrown from the InterruptedException catch;
3. the in-loop checkInterruption() - the actual subject of the PR.

Because the server is fully stopped, zookeeperClient.isConnected() is false on every iteration and control always reaches the interruptible wait(). Remove the in-loop check and this test still passes.

Fix: have the worker count down a latch after its first failed tryLock and interrupt only then; add a case driving the zero-blocking path of #2 below, the only place the in-loop check is load-bearing. Also lockThread is never joined: if the assertion fails, the thread keeps running through server.restart() and can acquire interruptible.

2. lockInterruptibly() still spins for a non-interrupted caller.

while (!tryLock(1, TimeUnit.SECONDS)) has no delay of its own, so it is throttled only by however long tryLock happens to block. This PR adds two paths where that is not one second:

Durable case - the new KeeperException catch. Both isConnected() checks pass, mutex.acquire throws ConnectionLossException, tryLock returns false. How long that took is entirely the user's RetryPolicy: with the test's BoundedExponentialBackoffRetry(100, 1000, 3) it is ~700 ms per turn (survivable); with a policy whose allowRetry declines immediately it is ~0 ms and the loop spins for the whole outage. Reachable for as long as the connection is down - not a race.

Transient case - the double isConnected() guard. zookeeperClient.isConnected() is false, blockUntilConnected returns true instantly out of ConnectionStateManager (verified: it never waits when its own state says connected), the second check is false, return false after zero blocking. That is exactly the state the new comment says the second check exists to catch - the guard for one symptom creates a zero-delay return the loop has no floor for.

Fix - give the loop a wall-clock slice, which covers both:

long deadline = System.currentTimeMillis() + 1000;
while (!tryLock(1, TimeUnit.SECONDS)) {
    checkInterruption();
    long remaining = deadline - System.currentTimeMillis();
    if (remaining > 0) {
        Thread.sleep(remaining);
    }
    deadline = System.currentTimeMillis() + 1000;
}

Thread.sleep is itself interruptible, so the in-loop check keeps its meaning.

Non-blocking suggestions

3. Should the deprecation be backported? This PR pairs the busy-spin fix with a forRemoval = true deprecation and a behavior change (exception -> false), and it is marked for auto-cherry-pick to 7.1.x & 7.0.x. Every existing @Deprecated(since = ...) in the tree is a two-component minor (7.0 x26, 7.1 x14) - no precedent for deprecating in a patch. Consider cherry-picking only the spin fix and leaving the deprecation on main. (The 7.0.6 @since labels themselves are fine - three-component @since is standard here, 481 occurrences including @since 7.0.5.)

4. SessionMovedException left out of the catch. Curator's retryable set is CONNECTIONLOSS | OPERATIONTIMEOUT | SESSIONMOVED | SESSIONEXPIRED (RetryPolicy), and RetryLoopImpl.takeException rethrows the raw exception once retries are exhausted. Three of the four become false; SessionMoved becomes a MessagingException. Suggest catch (KeeperException e) plus if (this.client.getZookeeperClient().getRetryPolicy().allowRetry(e)) { return false; }, or just add the fourth type.

5. Reentrancy regresses while disconnected. InterProcessMutex.internalLock returns true straight from its threadData map, no ZK round trip, when the calling thread already owns the lock. The new guard runs first, so a reentrant tryLock by the owner now returns false during an outage even though nothing needs the network. Cheap fix: an early if (this.mutex.isOwnedByCurrentThread()) short-circuit before the guard.

6. The timing assertion is both vacuous and flakeable. < 10_000 ms for a tryLock(1, SECONDS) is 10x slack - a 9-second block passes. And CuratorFrameworkFactory.newClient(connectString, retryPolicy) takes the defaults sessionTimeoutMs = 60_000, connectionTimeoutMs = 15_000. If the client has not yet processed the disconnect, isConnected() is still true, the guard is skipped, and RetryLoop.callWithRetry -> internalBlockUntilConnectedOrTimedOut() can burn up to 15 s. Narrow window, but real. Fix: build via CuratorFrameworkFactory.builder() with an explicit small connectionTimeoutMs/sessionTimeoutMs, then tighten the bound to ~3 s.

7. The build.gradle change is scope creep, and half the comment is wrong as configured. SameThreadTimeoutInvocation.proceed() does thread.interrupt() and then throws, so "interrupt a stuck test" is accurate. But "report a stack trace of its thread" is not what happens: the dump comes from PreInterruptThreadDumpPrinter, registered only when junit.jupiter.execution.timeout.thread.dump.enabled=true, which defaults to false - and it prints to System.out, which the build suppresses below -i. Either add that system property or drop the clause; either way a global JUnit timeout for every module belongs in its own PR.

8. Pre-existing copy-paste bug in the two-arg constructor: Assert.notNull(client, "'keyToPath' cannot be null") should assert keyToPath. One-line drive-by.

9. The InterruptedException catch in tryLock(long, TimeUnit) does Thread.currentThread().interrupt(); throw e;, which contradicts the Lock.tryLock(long, TimeUnit) javadoc ("the interrupted status is cleared") and is now inconsistent with the new checkInterruption(). Pre-existing, but this PR is about interrupt semantics.

10. The no-arg tryLock() delegates to tryLock(1, TimeUnit.SECONDS), so it can block ~1 s - the contract says acquire only if free at invocation time. Pre-existing; tryLock(0, TimeUnit.MILLISECONDS) would now behave correctly given the guard handles a zero budget.

11. lock() wraps everything, including InterruptedException from mutex.acquire(), into IllegalStateException without restoring the interrupt flag - and uses IllegalStateException where tryLock/unlock use MessagingException. Pre-existing inconsistency.

12. Wording. The javadoc uses e.g. where the adoc equivalent correctly spells out "for example". The new comment says the CuratorZookeeperClient "reacts to a closed socket immediately, while its ConnectionStateManager may still report a connection for a while" - "for a while" overstates it; the stronger and more accurate justification is that CuratorZookeeperClient.isConnected() is the very flag the Curator retry loop consults.

13. Commit hygiene. Squash before merge as planned: 770bced is a body-only commit pointing at the PR, 43d64a7/a627d72 are "Polish" commits, and c1c6fe7's body describes behavior later reverted. If this lands on main as a 7.2 item rather than a pure backport, it also wants a whats-new.adoc entry.

Verified good

• blockUntilConnected delegation. CuratorFrameworkImpl delegates solely to ConnectionStateManager.blockUntilConnected, which is synchronized and Object.wait()-based - genuinely interruptible, and with units != null plus a non-positive maxWaitTime it returns isConnected() immediately rather than blocking. The TimeUnit.MILLISECONDS argument is therefore safe; the no-arg blockUntilConnected() (units null) would have blocked indefinitely.
• The two state machines really are independent. CuratorZookeeperClient.isConnected() reads an AtomicBoolean in ConnectionState; ConnectionStateManager.isConnected() reads its own currentConnectionState. In ConnectionState.process the atomic is set before parent watchers fire, and ConnectionState.reset() clears it with no WatchedEvent at all. So CZK == false && CSM == true is genuinely reachable, and the guard's logic holds.
• The guard is well targeted. RetryLoop.callWithRetry calls internalBlockUntilConnectedOrTimedOut(), whose loop condition is state.isConnected() - the same flag the new code checks. isConnected() == false is therefore a precise predictor that mutex.acquire would burn connectionTimeoutMs. Worth saying in the comment.
• The KeeperException catch is reachable, not dead code. RetryLoopImpl.takeException rethrows the original exception unwrapped, and LockInternals.attemptLock re-wraps only NoNodeException.
• Returning false does not leak a lock znode. LockInternals.internalLockLoop deletes the candidate node via deleteOurPathQuietly / guaranteed() on both the exception and not-acquired paths.
• The class javadoc caveat is accurate. connectionTimeoutMs (default 15 s) + RetryPolicy budget is exactly the unbounded portion, and "two-thirds of the session timeout" is the right ZK client read timeout.
• Arithmetic. (int) Math.min(waitTime, Integer.MAX_VALUE) is overflow-safe; Math.max(0, ...) correctly stops a negative deadline reaching acquire - a real improvement over the old code; negative/zero time degrades to a non-blocking attempt.
• checkInterruption() uses Thread.interrupted() (clears) rather than isInterrupted() - correct.
• destroy() as a no-op is safe. No internal executor remains, so nothing leaks; the original WorkManagerTaskExecutor motivation is satisfied by spawning no threads at all.
• Giving the test its own TestingServer is a real improvement - the shared stop/restart genuinely could wedge the tests declared after it.

Could not verify

• Whether KeeperException.OperationTimeoutException is actually thrown on this path. Curator lists OPERATIONTIMEOUT as retryable but there is no site in curator-client/curator-framework 5.9.0 that raises it - it would have to come from the ZooKeeper client itself. Catching it is harmless, but the adoc claim may be unfalsifiable in practice.
• The real-world width of the CZK == false && CSM == true window. Structurally confirmed, but in the ordinary disconnect both flags are driven by the same WatchedEvent on the same ZK event thread in a fixed order, so the typical window is microseconds. This does not weaken finding #2, whose durable case is the KeeperException path.
• Runtime behavior - :spring-integration-zookeeper:test was not executed, so findings #1 and #6 are from code reading only.

[artembilan]

Following up on the review above: the setMutexTaskExecutor() deprecation and the blockUntilConnected() rewrite are rolled back in 9f85bc4. They are an API and behavior change rather than a bug fix, and this branch is meant to be cherry-picked to 7.1.x/7.0.x - that is finding #3, taken as the primary point. The executor, its setter, destroy() and the asynchronous checkExists() probe are back exactly as on main, including the default executor configuration; the queueCapacity = 0 tweak from the first commit is dropped too, so the probe serialization stays pre-existing and untouched. The KeeperException -> false mapping, the class JavaDoc caveat and the 7.0.6 reference-manual paragraph go with it.

That resolves the findings that described code which no longer exists: #2 (the delay floor is restored - future.get(1s) blocks the loop for its full second again), #4 (SessionMovedException), #5 (reentrancy while disconnected), and the blockUntilConnected half of #12. #7 is partly addressed - the thread-dump clause was wrong as configured and has been corrected; whether the junit.jupiter timeout itself belongs in this PR is still open.

#1 stands, and is worse than reported. The interrupt checks are a guard, not the mechanism: the restored tryLock() spends its wait inside Future.get(), which throws InterruptedException on its own. Verified by deleting the in-loop checkInterruption() and re-running ZkLockRegistryTests - still green. So main's lockInterruptibly() already terminated on an interrupt, and the "busy-spin hang" in the first commit's title was really just the test-isolation defect (voidLockFailsWhenServerDown stopping the server shared with the rest of the class). The checks are kept as explicit guards, the comment now says so, and the test asserts the contract rather than pretending to cover them. The branch title overclaims and wants renaming before merge.

Also done: #6 - the timing bound is tightened from 10 s to 3 s and the interrupt thread is now joined. Still open: #8 (the Assert.notNull(client, "'keyToPath' cannot be null") copy-paste bug), deliberately left out to keep this branch to one concern, and the pre-existing items #9, #10, #11.

[artembilan]

Second review, post-rollback (AI-assisted, fresh reviewer)

Reviewed the net git diff main...HEAD rather than the commit list, since the rollback made those diverge. Curator claims verified against curator-recipes-5.9.0-sources.jar from the Gradle cache. Build, checkstyle and voidLockFailsWhenServerDown (2.18 s) all green locally.

No blocking issues in the code. The rollback is clean - grepping the touched files for blockUntilConnected, 7.0.6 and Deprecated returns nothing, zookeeper.adoc is byte-identical to main, and the executor, its setter, destroy() and the checkExists() probe match main exactly.

Worth fixing before merge

1. The squashed commit message needs to be hand-written. c1c6fe753e is still the first commit, and on a GitHub squash its body becomes the default message that ships to main, 7.1.x and 7.0.x. It documents three things no longer in the net diff: a WARN in tryLock() (the diff logs DEBUG, and against main this is net-new logging, not a demotion - main has no logger in this class at all), queueCapacity = 0 on the default executor (reverted, the executor is byte-identical to main), and the "busy-spin hang" framing that 9f85bc4409 explicitly concedes was really test isolation. The accurate summary already exists in 9f85bc4409 ("What remains is the fix itself: ..."); use that plus the auto-cherry-pick trailer.

2. The InterruptedException path orphans the probe task. ZookeeperLockRegistry.java:356-358 rethrows without future.cancel(true), while both the !connected path (:341) and the TimeoutException path (:352) cancel. The default ThreadPoolTaskExecutor is corePoolSize = 1 with an unbounded queue, so: server down, thread A interrupted inside future.get(1000), its checkExists() probe stays runnable in Curator's retry loop for up to connectionTimeoutMs (15 s default) holding the single worker - every subsequent tryLock() on the registry then queues behind it and returns false regardless of the real connection state. Pre-existing on main, distinct from the deferred queue-capacity item (that one is capacity, this is a leaked task), one line, backport-safe:

catch (InterruptedException e) {
    future.cancel(true);
    Thread.currentThread().interrupt();
    throw e;
}

3. Two lines exceed 120 columns and CI will not catch them - src/checkstyle/checkstyle.xml has no LineLength module. ZookeeperLockRegistry.java:353 is 121 (the LOGGER.debug(() -> "Timed out ...") line) and ZkLockRegistryTests.java:370 is 122, the latter purely from the three extra tabs of nesting. For the test, collapsing the two try-with-resources into one drops every body line by a tab and fixes it without rewrapping (resources initialise left to right, so client may reference server):

try (var server = new TestingServer();
        var client = CuratorFrameworkFactory.newClient(server.getConnectString(),
                new BoundedExponentialBackoffRetry(100, 1000, 3))) {

4. No test for the Assert.notNull(keyToPath, ...) fix - the branch's only genuine behavior fix, and CONTRIBUTING asks for one. Watch the overload trap: new ZookeeperLockRegistry(this.client, null) is ambiguous with the String root overload and will not compile, so the cast is required:

assertThatIllegalArgumentException()
        .isThrownBy(() -> new ZookeeperLockRegistry(this.client, (KeyToPathStrategy) null))
        .withMessage("'keyToPath' cannot be null");

Wording

5. The Math.max(0, ...) clamp is a behavioral no-op and the comment implies otherwise. Verified in 5.9.0: LockInternals.internalLockLoop does if (millisToWait <= 0) { break; }, so 0 and any negative take the identical branch, and allowRetry uses wall-clock elapsed rather than millisToWait. Nothing else reads it. Keep the clamp, but :346 currently reads "A slow connection check must not pass a negative deadline to the mutex", which reads as a defect being fixed. Suggest "Defensive: never hand the mutex a negative deadline."

6. The test assertion still oversells its coverage. ZkLockRegistryTests.java:346-348 does start() then interrupt() with no barrier, and Thread.start() establishes happens-before, so the flag is set before lockInterruptibly() runs and the entry guard wins essentially every time - the retry loop is never entered. The message "must propagate the interrupt, not swallow it in its loop" describes a path the test does not reach. Suggest "...must propagate the interrupt rather than attempt an acquisition." The main-code comment is already honest about the in-loop check being a guard; the test should match.

7. lockInterruptibly() now leaves the interrupt flag in two different states depending on where the interrupt landed: checkInterruption() uses Thread.interrupted() and clears it (correct per the Lock javadoc), while an InterruptedException bubbling up from tryLock re-sets it at :357. The tryLock re-set is a known deferred item, but the inconsistency inside lockInterruptibly() is new with this diff. No test covers flag state either way. Fine to leave, but the deferred item should not be closed without revisiting this.

8. lock4 at ZkLockRegistryTests.java:331 is declared before lock3 at :368 and is not part of the numbered sequence - interruptibleLock would read better.

Verified good

• The entry checkInterruption() is a real contract improvement, not cosmetics. On main, if the flag was already set and the probe future happened to be complete, FutureTask.get(timeout) returns without an interrupt check, so lockInterruptibly() could acquire the lock on an interrupted thread. The guard closes that.
• The interrupt test is sound and non-flaky. Every loop exit lands on an interruptible point: FutureTask.awaitDone checks Thread.interrupted() first, and the TimeoutException path falls through to checkInterruption(). There is no window where the loop spins with the flag set. The join(10_000) + isAlive() guard correctly stops a failed assertion leaving a thread racing for lock2.
• Test isolation is genuine. ZookeeperTestSupport.testingServer is a static per-class field; the new local TestingServer takes a random port and its own temp dir with no JVM-global state shared. The stated hazard is real - the six tests declared after voidLockFailsWhenServerDown all use this.client against the shared server.
• The 3 s bound is safe. Expected cost ~1 s (future.get(1000) plus one thread spawn on an empty queue); measured 2.18 s for the whole test. 5 s would cost nothing if you want more CI headroom.
• LogAccessor conversion is correct. LogAccessor(Class<?>) and debug(Supplier<? extends CharSequence>) both exist; dropping the guards is right for the Supplier overloads. LOGGER is the dominant in-tree name (18 of 20). All three sites are DEBUG, so net-new logging at the quietest level.
• build.gradle comment is now accurate - SAME_THREAD does schedule an interrupt and then wait for the invocation to return.
• PR title matches the diff, @author/@since need nothing (no new public API), and whats-new.adoc is not needed for a patch-targeted bug fix.

Could not verify

• Whether the 10-minute default timeout is safe across all 40+ modules; only spring-integration-zookeeper was run. Risk direction is a hard failure rather than a silent hang, so it is self-reporting.
• CI-machine timing for the 3 s bound. Local evidence only.