[zyxxoo]

Purpose

When a PD or Store state-machine application throws, the original code can continue to later Raft entries and advance applied progress beyond the failed operation. This change reports the failure to jRaft and stops that apply sequence, while preserving typed Store responses and isolating completion-callback failures.

Before — original code

The diagram describes the original catch-and-continue behavior, before this PR. For example, if committed entry 2 throws, entry 3 may still run and applied progress may reach 3 despite the failed operation.

After — current implementation

• An exception escaping PD or Store application calls setErrorAndRollback(1, ...) and returns before applying later entries. In the example, the last successfully applied position remains 1.
• Completion-callback exceptions are isolated from application failures. Store result delivery unwraps DefaultRaftClosure at the common result-write boundary so the original GrpcClosure receives its typed response.
• Store synchronously records terminal state-machine errors. Restart paths share a lock and check the terminal flag both before shutdown and after the old FSM has joined, preventing automatic replay through reinitialization. Error notification does not take that lock, avoiding a shutdown/join deadlock.

Scope and recovery boundary

This handles exceptions propagated to onApply; existing handlers that convert business errors into FeedbackRes keep their current behavior. jRaft rollback does not undo partial business-data writes. An affected Store replica requires repair of the underlying cause and a Store process restart; this PR does not implement automatic snapshot recovery or guarantee complete cluster recovery.

Production jRaft versions remain unchanged: Server 1.3.11, PD/Store 1.3.13. Version properties are centralized in the root POM, Store tests use their production version, and the dependency inventory is aligned. Cross-component version upgrades remain separate in #3239. Broader health/readiness and recovery work is tracked in #3222.

Validation

Packaging follow-up at d3c148af6: moved the two Node-dependent handler tests into the Node test module, preserving all assertions and production packaging. Java 11 full clean install (tests skipped) passed; the unfiltered Store core CI command passed Node 11/11 and Store core 44/44 tests. An independent review found no actionable issues in this relocation. New-head CI remains the final gate.

Validated at c24e5a329b0a3feb6b68ee8b7f9ce2c601268248:

• Full clean compile and formatting; final core profiles: 189 tests, 187 passed, 2 skipped.
• Failure-before/fix-after regression evidence for applied progress, typed response delivery, and an error arriving during restart shutdown.
• Four actual Store handler/session request types, with normal and throwing callbacks; deterministic shutdown/error and queued-activity interleavings.
• Five independent local review lanes examined the final contracts and lifecycle paths; a weak asynchronous assertion was replaced and re-reviewed.

Storage and the lifecycle-test drain boundary use controlled substitutes; these checks are not a multi-node recovery test. See the current PR checks for CI status.

Documentation

Doc - Done: Store operations guide documents the terminal failure and manual recovery boundary.

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.40%. Comparing base (dbb6663) to head (3e028d3).

Files with missing lines	Patch %	Lines
...va/org/apache/hugegraph/store/PartitionEngine.java	88.23%	1 Missing and 1 partial ⚠️
...he/hugegraph/store/raft/PartitionStateMachine.java	66.66%	1 Missing and 1 partial ⚠️
...pache/hugegraph/store/raft/DefaultRaftClosure.java	85.71%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2871      +/-   ##
============================================
+ Coverage     41.34%   41.40%   +0.05%     
- Complexity     7314     7338      +24     
============================================
  Files           802      802              
  Lines         69770    69792      +22     
  Branches       9309     9312       +3     
============================================
+ Hits          28848    28898      +50     
+ Misses        37626    37601      -25     
+ Partials       3296     3293       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull Request Overview

This PR improves error handling in Raft state machines by ensuring that errors are properly reported to the iterator when exceptions occur during state machine operations.

• Adds error reporting via setErrorAndRollback() when exceptions are caught during state machine operations
• Implements consistent error handling across both store and PD components

Reviewed Changes

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

File	Description
HgStoreStateMachine.java	Adds error reporting call when exceptions occur during state machine operations
RaftStateMachine.java	Adds error reporting call when exceptions occur during state machine operations

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[github-actions]

Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label

[imbajin]

Score: 6.0/10; validation is insufficient for approval. Three independent reviews found no verified material runtime regression, but this change moves PD and Store apply failures into jRaft terminal-error handling without adding failure-path regression tests. Please verify leader apply failure and follower replay failure, that later entries are not applied, closure completion, and the applied-index/snapshot boundary. In PD, the existing done.run(EINTERNAL) and jRaft error propagation both complete the current closure; the present CompletableFuture consumer ignores the second completion, so this is not a claim of duplicate RPC effects or data loss. This review is based on code and jRaft 1.3.13 control-flow inspection, not an executed fault test.

[imbajin]

Score: 8/10 (previously 6/10). The benefit is confirmed: the same fault tests fail before the fix because entry 2 throws but lastApplied advances to 3; after the fix it remains at 1 and later entries are not applied. PD error closures are completed once, and Store no longer publishes the failed entry as applied.

Updated to master dbb6663 and resolved conflicts without rewriting history. Store now targets the active PartitionStateMachine, rather than the older implementation. Seven apply scenarios cover success, leader/follower failure, closure completion and the applied/snapshot boundary; existing snapshot tests remain intact. Independent review also caught and corrected a snapshot assertion that could pass for an unrelated reason.

Validation: formatting, full clean compile and the integrated core profiles passed: 179 tests, 177 passed, 2 skipped, no failures. One earlier compaction-lock test failed on a timing-sensitive state assertion; it passed on clean master and in the final integrated run. New CI is running; no approval or merge is implied.

Remaining boundaries: mocked Node/LogManager, no multi-node restart validation, and no rollback of partial business writes. Store's existing automatic restart can repeat a deterministic failure; PD's existing leader-stop callback can overwrite the ERROR probe label while readiness remains false. These recovery/probe issues remain outside this PR (#3222). Doc - No Need: internal error handling, no API/configuration change.

[imbajin]

Centralized the existing jRaft version declarations in the root POM without upgrading any production component: Server remains on 1.3.11; PD and Store remain on 1.3.13. Store tests reference the same property as their production component. All 18 resolved jRaft dependency entries are identical before and after this declaration refactor.

A single-version upgrade is explicitly deferred to #3239, with a TODO in the root POM. Compatibility must be established separately; the current tests are not evidence that upgrading Server is safe. Full clean compile and the 10 relevant PD/Store tests passed after this change.

[imbajin]

Fixed the Store core CI failure in HgSnapshotHandlerTest. The test treated compactionDone as a terminal state, but the subsequent successful snapshot resets it to compactionCanStart. In the failed CI log, compaction finished at 16:14:54.001 and the successful snapshot released the lock at 16:14:54.024; the test polled every 50 ms and missed that transient state.

The test now directly waits for and asserts path-lock release after interruption, while confirming the snapshot range lock is still owned. The redundant second compaction and transient-state assertion are removed; production code is unchanged. A negative control that removes unlock(path) fails at the retained lock-release assertion, demonstrating that the original regression is still detected.

I had observed the same failure locally earlier but accepted passing reruns without resolving this race. That validation gap is now addressed.

Validation: full clean compile, formatting and the Store core profile passed (37 Store tests, zero failures). The targeted negative control failed as expected.

[Copilot]

Copilot review overview

🟡 Changes recommended

A critical Checkstyle separator violation remains in PartitionStateMachineTest.java.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Add required blank line before new test method · New

[imbajin]

Blocking: yes. Summary: A deterministic apply failure can restart and replay the same committed entry, and a completion callback exception can also fail-stop the FSM after work has started. Evidence: exact-head state-machine and PartitionEngine source plus JRaft 1.3.13 rollback control flow.

[imbajin]

Follow-up review at 181865d: the three new review threads are addressed. Five fresh independent local review lanes examined the complete diff. They found an asynchronous terminal-error notification window; it was fixed with synchronous notification for state-machine errors only, then independently re-reviewed with no remaining actionable findings. The final small monitor-test adjustment also received separate review.

Validation: callback-failure tests reproduced the old incorrect fail-stop behavior and now pass; terminal restart guards and notification ordering are covered. Full clean compile and formatting passed. Final integrated core profiles ran 185 tests: 183 passed, 2 skipped, no failures. A sliding-window double-read assertion found during validation was reproduced across the expiry boundary and corrected without changing its accepted values.

The change adds no retry framework or dependency upgrade. Store remains stopped after a state-machine error until the underlying cause is repaired and the process is restarted. Partial business writes are not undone. New-head CI is still pending; this is a COMMENT review, not approval or merge.

[Copilot]

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Race allows Raft restart after terminal state-machine failure · New
• DefaultRaftClosure causes leader-side ClassCastException · New

Resolved since last review (1)

• Add required blank line before new test method

[imbajin]

The new findings were real gaps in the previous verification: simplified handlers missed the concrete GrpcClosure contract, and the restart tests covered an error already latched before restart rather than one arriving during shutdown. Both now have failing-before/passing-after regression evidence.

At c24e5a3, typed results are preserved at the common result-write boundary; restarts are serialized and the terminal flag is checked after the old FSM has joined. The error callback remains lock-free to avoid a join/notification deadlock. Five independent local review lanes traced the actual handlers and lifecycle calls. One weak asynchronous assertion they identified was replaced with a latch-controlled queued-activity test and re-reviewed.

Validation: four real handler/session request types in normal and throwing-callback cases; deterministic shutdown/error and queued-activity interleavings; full clean compile and formatting; final core profiles 189 tests, 187 passed and 2 skipped. New-head CI is pending.

Scope: fail-stop applies to exceptions propagated to onApply. Existing handlers that convert business errors into FeedbackRes are not reclassified here. Storage and the lifecycle test drain boundary are controlled substitutes; no full-cluster recovery claim is made.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Add PD recovery documentation and ensure the Store gRPC regression test runs in CI.

Review effort: Lite
Findings: None

Resolved since last review (2)

• DefaultRaftClosure causes leader-side ClassCastException
• Race allows Raft restart after terminal state-machine failure

Previously missed (1)

In code that hasn't changed since last review

Add PD operator guidance for terminal apply-failure recovery

hugegraph-pd/​hg-pd-core/​src/​main/​java/​org/​apache/​hugegraph/​pd/​raft/​RaftStateMachine.java:131

This changes PD's apply-failure behavior to stop the FSM and leave the node in an error state, but the PR only adds the corresponding recovery guidance to the Store operations guide. Please add matching PD operator documentation (including the required recovery/restart boundary), so PD deployments are not left without guidance for this new terminal failure mode.

[imbajin]

Blocking: yes. Summary: Error messages are used as jRaft format strings before rollback is recorded, so messages containing format directives can escape the intended state-machine failure path. Evidence: jRaft 1.3.13 Status(RaftError, String, Object...) calls String.format; the same construction appears in both PD and Store.

[imbajin]

⚠️ Status(RaftError, String, Object...) formats its second argument with String.format. If t.getMessage() contains %s or ends in %, construction throws before setErrorAndRollback() runs, so JRaft does not receive the intended iterator error; the PD catch uses the same call. Please pass the message as data (for example, new Status(RaftError.ESTATEMACHINE, "%s", t.getMessage())) in both state machines.

[imbajin]

Confirmed independently against jRaft 1.3.13 and fixed in 3e028d3 in both PD and Store: the exception message is now a data argument to a fixed "%s" format. The existing real-FSM tests now inject "%s" and a trailing "%": PD failed 2/4 before the fix; after the fix PD 4/4 and Store 10/10 passed. A separate SDK check covered plain text, format directives and a null message. Full clean compile passed; new-head CI remains pending.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Raft failure and restart semantics span multiple components and require final CI and human lifecycle validation.

Review effort: Balanced
Findings: None