fix(store-client): stop retrying after DEADLINE_EXCEEDED and honour thread interrupts in NodeTxExecutor

[SebastianGruza]

Purpose of the PR

• close [Bug] A stalled HStore node holds REST workers for minutes: store-client commit retry swallows interrupts #3199

When a store node stops answering, NodeTxExecutor.retryingInvoke() (hg-store-client) retries the failing call up to NODE_MAX_RETRYING_TIMES (10) with a 1, 1, 1, 2, 3, 4, 5, 6, 7, 8 s sleep schedule, and every attempt is a blocking gRPC call bounded only by grpc.timeout.seconds (default 100 s). After a DEADLINE_EXCEEDED the next attempt just waits the full deadline again, so one commit or one point lookup on a stalled partition holds its calling thread for 11 × grpc.timeout.seconds + 38 s (about 19 minutes on defaults). The loop also catches the InterruptedException from Thread.sleep, logs Failed to sleep and continues, so the interrupt sent by restserver.request_timeout (REST worker) or by the Gremlin Server evaluationTimeout is swallowed and neither limit can free the thread. With a writer at 1 request/s, a single stalled store node exhausts the REST worker pool within tens of seconds and the server answers 503 to everything for minutes after the writer stops.

Main Changes

• NodeTxExecutor.retryingInvoke() classifies every failed attempt (classify() → Failure), looking at the cause chain and at the suppressed failures of a parallel commit; the most severe class wins:
  • FATAL — the calling thread was interrupted or the call was CANCELLED: never retried. The loop also stops when the thread is interrupted before an attempt or while sleeping between attempts, restoring the interrupt flag and keeping an InterruptedException as the root cause (HugeException.isInterrupted() / HugeTask.fail() rely on it).
  • DEADLINE — DEADLINE_EXCEEDED: retried once, because the NOT_WORK notice sent for the failed RPC reloads the partition leaders and, on replicated partitions, the next attempt can reach a new raft leader; a second deadline in a row fails the call (any other failure in between resets that budget).
  • RETRYABLE — UNAVAILABLE, NOT_LEADER and other transport failures: retried as before, which is what the store-replacement recovery of refactor(store): recover retries after store replacement #3130 relies on.
• commitSessions(): a parallel commit no longer keeps only the first failure — every session failure is collected and thrown as one HgStoreClientException with the others attached as suppressed, so the retry decision does not depend on which partition happened to report first. The allSuccess gate that this made unreachable is removed.
• Retryability is checked before the attempt budget, and the non-retry warning logs the cause. No change to the attempt count or the sleep schedule.

Verifying these changes

• Unit tests in NodeTxExecutorTest (hg-store-test, store-client-test profile): failure classification (incl. suppressed failures), a deadline is retried exactly once, deadline → new leader succeeds, deadline → other failure → deadline is retried again, a mixed UNAVAILABLE + DEADLINE_EXCEEDED commit makes two attempts with both sessions rolled back, an all-UNAVAILABLE commit stays retryable, an interrupt during a failure or before an attempt stops the loop with InterruptedException as the root cause.

  mvn test -pl hugegraph-store/hg-store-test -am -P store-client-test -Dtest=NodeTxExecutorTest -Dsurefire.failIfNoSpecifiedTests=false
  

  11/11 pass on Temurin 17.

• Live PD + 3-store cluster (master 36811483, default-shard-count: 1, one store frozen with SIGSTOP, one POST /graph/vertices per second for 300 s, a probe GET every 2 s, grpc.timeout.seconds=20 written into the client jar so that a run fits in minutes; the shipped default is 100 s, multiply the after-columns by 5):

  	before (master)	never retry a deadline (920bbbd, superseded)	retry a deadline once (35e0a6d, this head)
  REST unavailable (probe gets 503)	431 of 503 s	12 of 300 s	128 of 300 s, in 20 windows of 2–16 s
  REST still unavailable after the writer stopped	200 s	0 s	0 s
  300 writes	27 × 201, 243 × 503, 23 × 500	131 × 201, 152 × 500, 7 × 503	90 × 201, 117 × 500, 89 × 503
  slowest write	257 s (= 11 × 20 + 38)	20.1 s	45 s (= 2 × 20 + backoff)
  server log	Failed to sleep ×30, reached the upper limit ×30	Not retrying after ×152	retrying once ×119, second deadline ×54, Failed to sleep 0

  The single retry costs availability under sustained load on a cluster without replicas (a stalled partition now holds a worker for two deadlines instead of one), which is the price of keeping the leader-failover recovery on replicated clusters; the server still recovers the moment the writer stops, and the interrupt from request_timeout still ends a request after one deadline. Full logs and the reproduction script: https://github.com/SebastianGruza/hugegraph-validation/blob/master/docs/findings.md#f15

• Trivial rework / code cleanup without any test coverage. (No Need)

• Already covered by existing tests, such as (please modify tests here).

• Need tests and can be verified as shown above.

Does this PR potentially affect the following parts?

• Dependencies (add/update license info)
• Modify configurations
• The public API
• Other affects: retry semantics of the HStore client — CANCELLED/interrupts are never retried, DEADLINE_EXCEEDED at most once in a row, parallel-commit failures are decided together
• Nope

Documentation Status

• Doc - TODO
• Doc - Done
• Doc - No Need

[bitflicker64]

Blocking: no. Summary: the fix holds at 4350ef99 — the exception the retry loop sees really does carry the gRPC status (NotifyingExecutor.invoke() rethrows err(t) = HgStoreClientException.of(msg, t) at NotifyingExecutor.java:74-82, GrpcStoreNodeSessionImpl.commit() wraps it in RuntimeException(t) at :139-141, and doCommit() unwraps one level at NodeTxExecutor.java:154-161), and no recovery path is lost by dropping DEADLINE_EXCEEDED from the retry set, because the node-invalidation notice that would make a retry resolve a different leader only fires for UNAVAILABLE (NotifyingExecutor.java:244-255) while leader moves arrive in-band as PARTITION_FAULT_TYPE_NOT_LEADER on an HgStoreClientException with no gRPC cause (:150-171), which isRetryable() still retries. Interrupt-flag hygiene is safe for pooled callers: Grizzly clears the flag at the top of every task (AbstractThreadPool.Worker.doWork(), grizzly-framework 3.0.1 line 526), as does ThreadPoolExecutor.runWorker. Five comments below, none blocking; the important one is that doCommit() keeps only the first of several parallel commit failures, so on a mixed-failure commit whether the new fail-fast engages is decided by a race. Evidence: static review of the exact-head diff (2 files, +128/-18) against merge-base 36811483; suite wiring checked (ClientSuiteTest lists NodeTxExecutorTest.class, hg-store-test/pom.xml:227-237 includes **/ClientSuiteTest.java, run by pd-store-ci.yml:286). No CI signal at this head — all seven workflows are action_required awaiting maintainer approval, so gh api repos/apache/hugegraph/commits/4350ef99.../status is pending and check-runs is empty. The new tests could not be executed locally: mvn test -pl hugegraph-store/hg-store-test -am -P store-client-test fails in the untouched hugegraph-commons/hugegraph-common module with lombok annotations unprocessed, so the author's 6/6 result is unverified here.

[bitflicker64]

⚠️ On a commit that spans more than one partition, whether this gate fires is decided by a race, so the fix is not guaranteed to engage on the very workload the issue describes.

Evidence:

• doCommit() commits every session in parallel and keeps only the first throwable: sessions.parallelStream().forEach(...) with throwable.compareAndSet(null, t) (NodeTxExecutor.java:136-145). Which ForkJoinPool task wins is nondeterministic.
• That single throwable is the only one rethrown (:154-161), so it is the only input isRetryable() ever sees for the whole commit — the other nodes' failures are discarded, not even suppressed.
• So for a commit touching a stalled node (DEADLINE_EXCEEDED) and a node being replaced (UNAVAILABLE, the refactor(store): recover retries after store replacement #3130 recovery this PR is careful to preserve): if the UNAVAILABLE lands first, the commit is retried up to 11 times and each attempt still blocks on the stalled partition for the full grpc.timeout.seconds (AbstractGrpcClient.java:128). That is 11 × grpc.timeout.seconds of held caller thread — the stall from [Bug] A stalled HStore node holds REST workers for minutes: store-client commit retry swallows interrupts #3199, unchanged.
• Nothing pins this: all four new tests use a supplier with a single failure mode, and testTransientFailureIsStillRetried / testDeadlineExceededIsNotRetried each exercise one node's worth of behaviour.

Requested change: collect every failure instead of the first (a ConcurrentLinkedQueue<Throwable> or Collections.synchronizedList) and make the decision deterministic — retry only when every captured failure is retryable, attaching the rest as suppressed on the thrown HgStoreClientException — and add a test for a two-session commit where one session fails DEADLINE_EXCEEDED and the other UNAVAILABLE, asserting exactly one attempt.

[SebastianGruza]

Done in 8adf522. commitSessions() now collects every session failure in a ConcurrentLinkedQueue and throws one HgStoreClientException (first failure unwrapped one level as before, the others attached as suppressed); isRetryable() walks the suppressed failures as well as the cause chain (identity-set guarded against cycles), so one DEADLINE_EXCEEDED among several failures makes the attempt non-retryable regardless of which partition reported first. Tests: testMixedCommitFailuresAreNotRetried (two sessions, UNAVAILABLE + DEADLINE_EXCEEDED thrown fresh on every call, exactly one attempt, both sessions rolled back, one suppressed) and testAllRetryableCommitFailuresAreRetried.

[bitflicker64]

🧹 This log drops the throwable, on what this change makes the common failure path.

Evidence:

• log.warn("Not retrying after: {}", t.getMessage()) passes only the message, so nothing reaches the logger's throwable slot.
• The branch four lines up keeps it: log.error(maxTryMsg, t) (:393). Before this PR that was the only place a store failure was logged with its cause chain; after it, a stalled node takes this branch on the first attempt and never reaches :393, so the nested StatusRuntimeException and its stack trace are no longer in the server log at all.

Requested change: log.warn("Not retrying after: {}", t.getMessage(), t); — slf4j appends a trailing Throwable argument as the cause.

[SebastianGruza]

Done in 8adf522 — the throwable is passed as the trailing argument.

[bitflicker64]

🧹 Ordering the attempt-budget check before the retryability check makes the last attempt log the wrong reason.

Evidence:

• At i == NODE_MAX_RETRYING_TIMES the loop takes :392-396 first, so a DEADLINE_EXCEEDED on the final attempt is reported as "the number of retries reached the upper limit : 10" (maxTryMsg, :58-60) even though the new gate at :397 is what should have described it.
• This is reachable in the scenario the PR targets: attempts 0-9 fail UNAVAILABLE while a store is being replaced (still retried by design), then attempt 10 hits the deadline on the new node.

Requested change: move the if (!isRetryable(t)) block above the i + 1 > NODE_MAX_RETRYING_TIMES block so each exit logs the reason that actually applied.

[SebastianGruza]

Done in 8adf522 — isRetryable() is checked first, the attempt budget second.

[bitflicker64]

🧹 The new pre-attempt interrupt guard has no coverage; this test exercises only the sleep handler.

Evidence:

• The supplier increments attempts, then sets the flag, then throws (:163-165). So the i == 0 guard at NodeTxExecutor.java:381-387 runs on a clean thread, isRetryable(new RuntimeException(...)) is true, and the abort comes from the catch (InterruptedException) around Thread.sleep at NodeTxExecutor.java:411-418.
• This assertion proves it: assertEquals(1, attempts.get()). Had the guard fired, the supplier would never have run and attempts would be 0.
• The untested branch is the one that matters for a caller whose restserver.request_timeout expired between store calls rather than during one — the case the guard's own comment cites.

Requested change: add a case that sets the flag before the call, e.g. Thread.currentThread().interrupt(); assertThrows(HgStoreClientException.class, () -> executor.retryingInvoke(() -> { attempts.incrementAndGet(); return "ok"; })); assertEquals(0, attempts.get());, with the same finally { Thread.interrupted(); } guard this test already uses.

[SebastianGruza]

Done in 8adf522 — testInterruptBeforeCallSkipsTheAttempt sets the flag before the call and asserts attempts == 0 with the flag still set.

[bitflicker64]

🧹 Two style nits in the new test code.

Evidence:

• This line is 104 characters, over the project's LineLength max of 100 (style/checkstyle.xml:29-32). It is the only added line in the diff that exceeds it.
• import io.grpc.StatusRuntimeException (:43) is unused: the tests only call Status.<CODE>.asRuntimeException() and never name the type. UnusedImports is enabled at style/checkstyle.xml:59.
• Neither breaks the build — the checkstyle plugin is bound only in hugegraph-server/pom.xml:295 and hugegraph-commons/pom.xml:147, not in the store modules — so this is purely to keep the new code inside the shared style.

Requested change: wrap the argument list onto a second line and drop the unused import.

[SebastianGruza]

Done in 8adf522 — line wrapped, unused import removed; no added line exceeds 100 characters now.

[bitflicker64]

Blocking: no. Summary: the earlier follow-ups are in at 8adf522 and the interrupt handling looks right; one inline comment on DEADLINE_EXCEEDED no longer riding through a partition leader failover, which corrects what my earlier summary said about that. Evidence: static trace through NotifyingExecutor, HgStoreNodePartitionerImpl, ClientCache and PartitionEngine at the exact head; mvn test -pl hugegraph-store/hg-store-test -am -P store-client-test -Dtest=NodeTxExecutorTest passes locally on JDK 17 (9 tests, 0 failures). No CI signal at this head: all seven workflows are action_required, waiting for maintainer approval.

[bitflicker64]

⚠️ Not retrying DEADLINE_EXCEEDED also drops a leader-failover recovery. The Javadoc's reasoning (a retry waits the full deadline again) holds when the retry goes to the same store, which is not the case once the partition leader has moved. This corrects my earlier summary, which only looked at node eviction.

Evidence:

• Every RPC error sends a NOT_WORK notice (NotifyingExecutor.java:117-123, :244-255). HgStoreNodePartitionerImpl.notice() then calls pdClient.invalidPartitionCache() (HgStoreNodePartitionerImpl.java:190-195), which reloads shard-group leaders from PD right away (ClientCache.java:220-230, :178-186).
• The retried doCommit() supplier re-runs doAction() for every entry (NodeTxExecutor.java:132-135), so it routes against the reloaded leaders (NodeTxSessionProxy.java:731-734, :862-866).
• A new raft leader is elected after the 3 s election timeout (HgStoreEngineOptions.java:91) and pushes its shard group to PD (PartitionEngine.java:605-681), well inside the 100 s default grpc.timeout.seconds (HgStoreClientConfig.java:30).

So with replicated partitions, attempt 1 used to reach the new leader; now the call fails after attempt 0. Interrupted REST workers still stop after one deadline either way, but callers with no timeout that interrupts them (async jobs are only interrupted on cancel) lose the recovery. Was the SIGSTOP cluster at the shipped default-shard-count: 1 (hugegraph-pd/hg-pd-dist/src/assembly/static/conf/application.yml:95)? With 3 replicas the before run should have recovered on attempt 1.

Requested change: allow at most one DEADLINE_EXCEEDED retry per retryingInvoke() call (keep CANCELLED and InterruptedException non-retryable), update the Javadoc to match, and make testDeadlineExceededIsNotRetried expect two attempts.

[SebastianGruza]

Agreed, and yes — the SIGSTOP cluster runs with the shipped default-shard-count: 1, so it never had a leader to fail over to; with replicas the first retry would indeed have reached the new leader. Done in b588aa8: retryingInvoke() now classifies a failure as FATAL (interrupt, CANCELLED: never retried), DEADLINE (retried exactly once per call, then failed on a second deadline in a row) or RETRYABLE (unchanged); suppressed failures of a parallel commit are classified as well and the most severe class wins. Javadoc updated. Tests: testDeadlineExceededIsRetriedExactlyOnce expects two attempts, testDeadlineThenNewLeaderSucceeds covers the failover case, the mixed-commit test now expects two attempts, testClassifyFailures replaces the old isRetryable test. 10/10 locally on JDK 17.

[bitflicker64]

Blocking: no. Summary: At b588aa8 the retry classification, the commit failure aggregation and the interrupt handling look correct, and every earlier inline point is addressed. One minor issue: two of the three new interrupt exits drop the InterruptedException, and the server's task-cancel path looks for it. Evidence: static review of the full exact-head diff (2 files, +323/-47) against merge-base 60c8803, traced through NotifyingExecutor, GrpcStoreNodeSessionImpl, HugeException, HugeTask and DistributedTaskScheduler; grpc-stub 1.39.0 ClientCalls bytecode checked for the interrupt cancel cause. Tests were not run locally. No CI signal at this head: all seven workflows are action_required, waiting for maintainer approval.

[bitflicker64]

🧹 Two of the new interrupt exits drop the InterruptedException, so the server cannot tell them apart from a store failure.

Evidence:

• :409-410 throws HgStoreClientException.of("Interrupted before retry " + i) with no cause. :456-458 uses the store failure t as the cause and discards e. Neither has an InterruptedException as its root cause.
• The FATAL path keeps one. A blocking stub on an interrupted thread fails with CANCELLED, and the InterruptedException is its cause: grpc-stub 1.39.0 ClientCalls.blockingUnaryCall calls call.cancel("Thread interrupted", e).
• HugeException.isInterrupted() only checks the root cause (HugeException.java:56-61). HugeTask.fail() relies on it so that a cancelled task is not recorded as failed (HugeTask.java:351-355). Take a task that is cancelled while its thread is between store calls or in the retry sleep. It now logs a WARN with a stack trace. If the worker reaches fail() before cancel() sets CANCELLED (:323 interrupts, :336 sets the status), the task is stored as FAILED and cancel() returns false, so DistributedTaskScheduler.cancel() does not save CANCELLED (:317-321). Before this change the loop swallowed the interrupt and carried on, so this path did not exist.

Requested change: make InterruptedException the root cause on both exits. For example, use HgStoreClientException.of("Interrupted before retry " + i, new InterruptedException()). In the sleep handler, use HgStoreClientException.of("Interrupted while waiting to retry: " + t.getMessage(), e) and attach t with addSuppressed. Then extend testInterruptStopsRetrying and testInterruptBeforeCallSkipsTheAttempt to assert the root cause.

[SebastianGruza]

Done in 172b2d0 — both exits now have an InterruptedException as the root cause (HgStoreClientException.of("Interrupted before retry " + i, new InterruptedException()); in the sleep handler the InterruptedException is the cause and the store failure t is attached to it as suppressed). testInterruptStopsRetrying and testInterruptBeforeCallSkipsTheAttempt assert the root cause, the first one also the suppressed store failure. 10/10 locally.

[bitflicker64]

Blocking: no. Summary: at 172b2d0 the retry classification, the parallel-commit failure aggregation and both interrupt exits are correct, and every inline point from the earlier reviews is addressed; three new minor points remain, the main one being that the PR description still describes the pre-b588aa88 behaviour. Evidence: HugeException.isInterrupted() (hugegraph-core/.../HugeException.java:56-61) tests rootCause(), so putting an InterruptedException at the bottom of both exits is what HugeTask.fail() (task/HugeTask.java:351) needs; HgStoreClientException uses the plain RuntimeException constructors (type/HgStoreClientException.java:24-46) so suppression is enabled and aggregate()/classify() really see the other partitions' failures; GrpcStoreNodeSessionImpl.commit() -> NotifyingExecutor.invoke() -> err(t) keeps the StatusRuntimeException in the cause chain; grep -rn 'StatusException' hugegraph-store/ is empty, so matching only StatusRuntimeException is not a live gap; no added line exceeds the 100-char LineLength max (style/checkstyle.xml:29-32). Not verified: the new tests were not executed here (mvn -pl hugegraph-store/hg-store-test -am -P store-client-test -Dtest=NodeTxExecutorTest was not run) and there is no CI signal at this head (combined status pending, zero check runs).

[bitflicker64]

⚠️ The PR description still describes the pre-b588aa88 behaviour, so the numbers reviewers and the squash commit message will carry are wrong for this head.

Evidence:

• The body says "Do not retry a failure whose cause chain carries Status.Code.DEADLINE_EXCEEDED … (isRetryable())". At this head a deadline is retried once (this block, plus the guard four lines down), and isRetryable() no longer exists: git grep -n isRetryable 172b2d0 -- hugegraph-store returns nothing — it is classify() / Failure now.
• The before/after table's slowest write | 20.1 s was measured with grpc.timeout.seconds=20 under the never-retry behaviour, i.e. one deadline. With the single retry the worst case is two deadlines plus the 1 s backoff: about 41 s in that setup, and about 201 s on the shipped default grpc.timeout.seconds=100. That is still a large improvement over 11 × timeout + 38 s, but it is twice what the table claims.

Requested change: update the "Main Changes" bullet to describe the retry-once classification and rename isRetryable() to classify(), and either re-run the SIGSTOP measurement at 172b2d0 or annotate the table as taken at 920bbbdf.

[SebastianGruza]

Done — the description now describes classify() / Failure with the single deadline retry (and the reset after a different failure from 35e0a6d), and I re-ran the SIGSTOP measurement at 35e0a6d instead of annotating: with one deadline retry the REST outage under the 1 request/s load is 128 of 300 s (in 20 short windows) against 12 s for the never-retry head and 431 of 503 s for master, the slowest write is 45 s (two deadlines plus backoff) and the server is still healthy 0 s after the writer stops. The table in the body has all three columns and states that the old column is superseded.

[bitflicker64]

🧹 Extracting commitSessions() removed the only writer of allSuccess, leaving a dead variable and an unreachable branch.

Evidence:

• AtomicBoolean allSuccess = new AtomicBoolean(true) (:131) is now only ever read. The allSuccess.set(false) that used to live in the inline parallel-commit block moved into commitSessions(), which reports through the thrown exception instead.
• So if (!allSuccess.get()) { throw HgStoreClientException.of(msg); } (:136-138) can never fire, and msg is referenced only from that unreachable throw.

Requested change: delete :131 and :136-138 (and the now-unused msg field, plus the java.util.concurrent.atomic.AtomicBoolean import, if nothing else uses them). If the gate was meant to catch a doAction() that returned false, wire the loop's return value into it instead of leaving it dead.

[SebastianGruza]

Done in 35e0a6d — allSuccess, the unreachable throw and the unused msg constant are gone (the doAction() return value was never wired into that gate before either, so nothing changes behaviourally).

[bitflicker64]

🧹 The comment and the javadoc say "a second deadline in a row", but the flag is never reset, so the code means "a second deadline anywhere in this invocation".

Evidence:

• boolean[] deadlineRetried = {false} (:401) is set once at :441 and never cleared, including after an attempt that fails with something else or that succeeds partially.
• Concretely: attempt 0 DEADLINE_EXCEEDED (retried), attempts 1-9 UNAVAILABLE (retried, ~38 s of backoff, partition leaders reloaded several times over), attempt 10 DEADLINE_EXCEEDED — the guard fires and the call aborts, even though these two deadlines are not consecutive and the stated rationale ("the NOT_WORK notice … reloads the partition leaders") applies just as much the second time.
• The same wording is in the Failure javadoc at :478.

Requested change: pick one. Either reset deadlineRetried[0] = false whenever an attempt fails with a non-DEADLINE classification, which makes the code match "in a row"; or reword :433 and :478 to "a second deadline in this invocation" so the budget is honestly described as one deadline retry per call.

[SebastianGruza]

Done in 35e0a6d — I kept the "in a row" semantics and made the code match: the budget flag is reset whenever an attempt fails with a non-DEADLINE classification, so deadline → UNAVAILABLE → deadline retries the second deadline too (testDeadlineBudgetResetsAfterAnotherFailure, 4 attempts). 11/11 locally.