HBASE-30357 OpenRegionProcedure#restoreSucceedState ignores persisted transitionCode, forcing OPEN even after a real FAILED_OPEN

[mnpoonia]

What changes were proposed in this pull request?

OpenRegionProcedure#restoreSucceedState() is called on master-failover restore, once
per region, via RegionRemoteProcedureBase#stateLoaded(). It unconditionally forced the
region into OPEN, regardless of the actual persisted transitionCode, which can be
FAILED_OPEN. The method's signature only received seqId, not transitionCode, so it
was structurally unable to branch on the real outcome.

Concretely: an RS reports FAILED_OPEN; the master persists
state=REPORT_SUCCEED, transitionCode=FAILED_OPEN on the OpenRegionProcedure, but the
in-memory RegionState.State is left untouched (still OPENING, since
AssignmentManager#regionFailedOpen(regionNode, false) on the live path does not update
RegionState.State). If the master crashes/restarts before this reaches confirmOpened()
and before hbase:meta is updated, restoreSucceedState() sees the region isn't OPEN
yet and force-transitions it to OPEN anyway, then persists that (false) OPEN state to
hbase:meta. There is no rollback anywhere in this procedure chain (by design -
forward-only), so nothing downstream can detect or correct it; the region silently looks
healthy in meta while no RegionServer is actually serving it.

By contrast, CloseRegionProcedure#restoreSucceedState() is safe doing the equivalent
unconditional force, because CLOSE has no failure-variant transition code at the master
side (checkTransition/updateTransitionWithoutPersistingToMeta both assert
transitionCode == CLOSED).

This is not a regression - the logic is unchanged (modulo spotless formatting) since it
was introduced in HBASE-22365 (2019-05-10).

The fix

• Widen RegionRemoteProcedureBase#restoreSucceedState() to also receive the persisted
  transitionCode, passed through from stateLoaded().
• OpenRegionProcedure#restoreSucceedState() now branches: FAILED_OPEN calls
  AssignmentManager#regionFailedOpen(regionNode, false), mirroring exactly what the live
  reportTransition/updateTransitionWithoutPersistingToMeta path already does for the
  same transition code; the existing OPENED handling is unchanged.
• CloseRegionProcedure#restoreSucceedState() accepts the new parameter and ignores it,
  since CLOSE has no failure variant.

After the fix, a restore-time FAILED_OPEN puts the region back into the same
retryable path (OPENING, cleared location) that TransitRegionStateProcedure#confirmOpened()
already drives on the live path - the region gets reassigned/retried through the normal
flow instead of being falsely marked OPEN.

Why are the changes needed?

To prevent a region from being silently, durably marked OPEN in hbase:meta after a
master restart, when in reality no RegionServer opened it. Since HBase has no rollback
mechanism for these forward-only assignment procedures, this bug is otherwise
unrecoverable except by manual detection and intervention.

Does this PR introduce any user-facing change?

No.

Is there a corresponding Apache JIRA?

Yes: HBASE-30357

How was this patch tested?

Added TestOpenRegionProcedureRestoreFailedOpen, extending TestAssignmentManagerBase. A
custom mock RS executor reports FAILED_OPEN on the first open attempt, then - while
holding the RegionStateNode lock, simulating the point right after a crash where the
child procedure has not yet resumed its own execute() - directly invokes the
TransitRegionStateProcedure#stateLoaded() hook that a real master restart would trigger,
and records the region's state immediately after. Confirmed the test fails against
unmodified code with the exact predicted OPEN state, and passes after the fix. Also ran
TestAssignmentManager, TestTransitRegionStateProcedure, TestOpenRegionProcedureHang,
TestOpenRegionProcedureBackoff, TestRollbackSCP, and TestSCPGetRegionsRace with no
regressions.

[mnpoonia]

@virajjasani @apurtell @Apache9 Can you please help with review.

[mnpoonia]

@Apache9 can you please help in reviewing.

[virajjasani]

Nice one, this is worth fixing

[virajjasani]

Is FAILED_OPEN the only case where we have this problem? What about split/merge reverted states?

[mnpoonia]

Good question — I checked both cases:

CloseRegionProcedure: no analogous bug possible. CloseRegionProcedure.checkTransition only ever accepts CLOSED (CloseRegionProcedure.java:99-106) — there's no FAILED_CLOSE/revert code in the protocol at all (the RS aborts instead of reporting a close failure). So transitionCode is structurally guaranteed to be CLOSED whenever it's persisted for this procedure, and restoreSucceedState can't mis-restore something that only ever has one valid value.

Split/merge: these don't go through RegionRemoteProcedureBase/OpenRegionProcedure/CloseRegionProcedure/restoreSucceedState at all — they're driven by SplitTableRegionProcedure/MergeTableRegionsProcedure and reported via a separate switch in AssignmentManager.reportRegionStateTransition, which routes READY_TO_SPLIT/READY_TO_MERGE to their own handlers. Those handlers reject SPLIT/MERGED/SPLIT_REVERTED/MERGE_REVERTED outright — those codes predate AMv2 (from the 1.x ZK-less-assignment era, HBASE-11059) and exist only as a rolling-upgrade compatibility guard against an old (<2.0) RS still reporting the legacy codes. No current RS code ever sends them.

So FAILED_OPEN is the only live instance of this restore-path gap — nothing further to fix here.

[Apache9]

So how do we simulate the master restart here? I haven't seen any restart in the test.

[mnpoonia]

Good question. We don’t actually restart the master in this test.

The test simulates the relevant part of a master restart by directly invoking TransitRegionStateProcedure#stateLoaded(), which is the hook reached when the procedure state is loaded during master recovery.

The important sequence we want to reproduce is:

1. RS reports FAILED_OPEN.
2. The procedure state containing REPORT_SUCCEED + FAILED_OPEN is persisted.
3. Master crashes before the child procedure gets a chance to continue and update the region state/meta.
4. On recovery, stateLoaded() restores the procedure and calls restoreSucceedState().

The test holds the RegionStateNode lock after reporting FAILED_OPEN and invokes stateLoaded() directly, so the child procedure cannot continue normally. This puts us at the same point in the procedure lifecycle as the restore path after a master restart.

I can make the test/comment more explicit about this, e.g. rename the comment from “simulates a master restart” to “simulates procedure recovery after master restart”, to avoid suggesting that the test actually restarts the master.