Refactor mutex handling, continued

[achamayou]

Follow up to #7744, updated against head of the repo

Scope

This report assesses PR #7744, "TSAN fixes: Refactor mutex handling" against main at commit 0f8233cc, inspected on 2026-08-06.

PR #7744 is still an open draft and was never merged. Its current head is da6f6bfa. git cherry reports that none of its non-merge commits landed patch-equivalently on main, although later PRs independently addressed parts of the same problem space.

The assessment is based on the PR description, commits, review comments, current code, and git history. It is a static analysis; a reduced-suppression TSAN run is still needed to confirm each lock-cycle report dynamically.

Summary

Result	Count	Meaning
Still applicable	6	The problematic ownership, synchronization, or lock ordering remains on main.
Partially superseded	1	A later PR removed the unsafe visibility window, but the nested startup lock ordering remains.
Fixed independently	1	A later merged PR removed the problem.

The three targeted suppressions also remain in tsan_env_suppressions: two deadlock suppressions covering store.h and untyped_map.h, and one race suppression covering node/rpc/frontend.h.

Still Applicable

1. LedgerSecrets takes its mutex before acquiring a KV dependency

Status: Still applicable.

get_latest(), get_latest_and_penultimate(), and get() still acquire LedgerSecrets::lock before calling take_dependency_on_secrets(). The latter opens and reads a KV handle, establishing the order LedgerSecrets::lock -> KV locks.

Evidence: ledger_secrets.h lines 171-215.

PR #7744's focused change is still appropriate: establish the transaction dependency first, then take the local ledger-secrets mutex only while reading the in-memory map.

2. RpcFrontend races on consensus and history

Status: Still applicable.

Both fields remain mutable raw pointers. update_consensus() and update_history() write them without synchronization, while request and tick paths read them concurrently. process(), process_forwarded(), and tick() can all call update_consensus().

Evidence:

• frontend.h lines 41-70
• frontend.h lines 1126-1161
• The matching TSAN race suppression remains at tsan_env_suppressions line 12.

The final #7744 direction is sound: explicitly publish consensus/history to each frontend and use synchronized pointer publication. The implementation must load each pointer once per operation and handle nullptr, especially in redirect resolution.

3. Certificate state has both lock-order and ownership problems

Status: Still applicable.

self_signed_node_cert and endorsed_node_cert are protected by the broad NodeState::lock. Certificate map/global hooks take that lock while executing from KV hook paths. This contributes to the KV maps_lock -> NodeState::lock side of the reported lock inversion.

Separately, NodeClient stores references to both mutable certificate objects, and HTTPNodeClient::make_request() reads them without taking NodeState::lock. Certificate renewal can replace those objects concurrently.

Evidence:

• Certificate fields: node_state.h lines 401-416
• Certificate hooks taking the main lock: node_state.h lines 3241-3363
• Reference members: node_client.h lines 12-30
• Unsynchronized read: http_node_client.h lines 25-35

Use a dedicated certificate mutex for both mutable certificate fields, return copies from locked accessors, and make NodeClient own immutable certificate values rather than references. Keep the certificate lock scope narrow and do not hold it while deserializing snapshots, opening frontends, or performing RPCs.

4. Member and user frontends are opened synchronously from KV global hooks

Status: Still applicable.

The endorsed-certificate global hook opens the member frontend while holding NodeState::lock; the service global hook opens the user frontend directly. Global hooks execute during KV commit/compaction paths, where the store may hold maps_lock. RpcFrontend::open() takes open_lock and initializes endpoint handlers, so these calls extend an already sensitive lock chain.

Evidence:

• Member frontend opening: node_state.h lines 3291-3363
• User frontend opening: node_state.h lines 3366-3403
• Store compaction takes maps_lock: store.h lines 569-588

Defer member/user frontend opening to an idempotent task after the hook returns. Keep the node frontend synchronous: #7744 briefly made it asynchronous, but boot and in-process node RPCs require it to be open before their tasks run.

5. transition_service_to_open() takes NodeState::lock before using a transaction

Status: Still applicable.

The method takes NodeState::lock and then accesses several KV maps through the supplied transaction, establishing NodeState::lock -> KV locks. Governance execution can reach the same objects from the opposite direction.

Evidence: node_state.h lines 2464-2533.

The #7744 audit found that this function's member access is transaction-owned, immutable after startup, or independently synchronized. Remove the broad lock after revalidating that assumption against current code; add narrower synchronization only for any mutable member that now requires it.

6. The snapshot-evidence global hook takes NodeState::lock

Status: Still applicable. This was found during review of #7744 rather than in its initial description.

The SnapshotEvidence global hook takes NodeState::lock to inspect and replace backup_snapshot_fetch_task. Since the hook can run under Store::maps_lock, this recreates the same maps_lock -> NodeState::lock ordering that #7744 was trying to remove. The task destructor also uses the main lock when clearing the owner pointer.

Evidence:

• Task cleanup: node_state.h lines 250-279
• Global hook: node_state.h lines 3586-3625

Protect only backup_snapshot_fetch_task with a dedicated mutex. Create/update the pointer under that mutex, then enqueue work after releasing it. The task completion path should use the same dedicated mutex.

Partially Superseded

Consensus role initialization after publication

Status: The unsafe observation window is largely fixed by PR #8060, but the nested startup locking remains relevant to TSAN cleanup.

Current startup still constructs and publishes consensus before calling force_become_primary() or init_as_backup():

• New service: node_state.h lines 1179-1184
• Joining backup: node_state.h lines 1628-1719
• Recovery primary: node_state.h lines 2139-2146
• Publication occurs in setup_consensus().

#8060 added store readiness and node-state gates so ordinary transactions, ticks, and inbound consensus messages cannot use the object before role initialization completes. That removes the strongest correctness argument for moving role activation into the constructor.

However, startup still takes NodeState::lock and then the AFT state lock. If removing the deadlock suppressions still reports this ordering, use a two-phase factory or initialization API:

1. Construct an unpublished AFT object.
2. Seed passive term/index/history/ledger state without externally visible side effects.
3. Publish it to the store and frontends.
4. Activate leader-only behavior after wiring is complete and without holding NodeState::lock.

Do not call become_leader(true) from the constructor. Review correctly noted that it can enqueue retired-node cleanup and perform leader-side effects before Store::set_consensus() and frontend setup are complete.

Fixed Independently

Snapshot fetch version - 1 underflow

Status: Fixed by PR #7775.

The #7744 review found that snapshot_evidence.version - 1 could underflow at zero. #7775 changed backup snapshot fetching to closed/inclusive semantics and now passes snapshot_evidence.version directly.

Current evidence: node_state.h lines 3615-3622.

No further #7744 work is needed for this item.

Draft-Only Hazards

These review findings apply to #7744's proposed implementation, not to current main, but any replacement must avoid them:

Hazard in #7744	Required constraint
Calling become_leader(true) in the AFT constructor runs leader side effects before consensus is fully wired.	Separate passive state seeding from leader activation.
Making the node frontend asynchronous lets boot/JWT/cleanup tasks issue in-process RPCs before it opens.	Keep the node frontend synchronous; defer only member/user frontends opened by hooks.
An unsafe self-signed-certificate accessor was used outside a provably stable startup region.	Expose copy-returning locked accessors and minimize any explicitly unsafe API.
The atomic-pointer draft dereferenced consensus in redirect resolution without a null check.	Load once, check for null, and return no redirect when consensus is unavailable.

The review's NodeClient reference-race and snapshot-hook lock findings are not draft-only; they remain current-main issues and are listed above.

Recommended Plan

Phase 1: Establish focused TSAN baselines

1. Create a dedicated TSAN build using the CI configuration:

   cmake -S . -B build-tsan -GNinja \
     -DCMAKE_BUILD_TYPE=Debug \
     -DLONG_TESTS=OFF \
     -DTSAN=ON \
     -DUSE_SNMALLOC=OFF \
     -DWORKER_THREADS=2
   ninja -C build-tsan

2. Run frontend_test, governance_test, and the partition tests through build-tsan/tests.sh, first with the existing suppressions and then with only the relevant suppression removed.

3. Save one reproducer or TSAN stack pair for each suppression before changing code. This prevents unrelated lock-order reports from being conflated.

Phase 2: Land narrow lock-order fixes

1. Move the three ledger-secret dependency reads before LedgerSecrets::lock.
2. Remove the broad lock from transition_service_to_open() after a current member-access audit.
3. Add a dedicated mutex for backup_snapshot_fetch_task and remove NodeState::lock from its hook/completion paths.
4. Rerun the focused TSAN tests after each change.

Phase 3: Fix ownership and publication races

1. Introduce a dedicated certificate mutex for both node certificate fields.
2. Make NodeClient own immutable certificate snapshots rather than references.
3. Publish RpcFrontend consensus/history pointers explicitly, with atomic or otherwise synchronized access.
4. Add tests for null consensus during startup, forwarding on primary/backup, certificate renewal, and retired-node cleanup.

Phase 4: Remove hook-side work and revisit AFT startup

1. Defer member/user frontend opening until after global hooks return; keep node frontend opening synchronous.
2. Re-run TSAN without the deadlock suppressions.
3. Only if AFT startup ordering is still reported, introduce passive pre-publication state seeding plus post-publication leader activation. Do not port TSAN fixes: Refactor mutex handling #7744's constructor-side leader effects unchanged.

Phase 5: Remove suppressions

Remove the three targeted entries one at a time, with the corresponding focused test green before each removal. Finish with the long-test TSAN configuration and the normal C++ unit/e2e checks affected by startup, governance, recovery, snapshot fetching, and forwarding.

Suggested Change Split

Keep this as several reviewable PRs rather than reviving #7744 wholesale:

1. Ledger-secret ordering and its suppression evidence.
2. Frontend consensus/history publication and race suppression removal.
3. Certificate locking plus NodeClient value ownership.
4. Snapshot-task and governance lock-order cleanup.
5. Deferred member/user frontend opening.
6. AFT startup changes only if the preceding fixes do not eliminate the remaining deadlock reports.

This ordering starts with local, behavior-preserving changes and leaves the highest-risk consensus startup work until TSAN demonstrates that it is still necessary.