Skip to content

Finish mutex lock-order cleanup - #8154

Open
Amaury Chamayou (achamayou) wants to merge 6 commits into
mainfrom
achamayou-finish-mutex-cleanup
Open

Finish mutex lock-order cleanup#8154
Amaury Chamayou (achamayou) wants to merge 6 commits into
mainfrom
achamayou-finish-mutex-cleanup

Conversation

@achamayou

Copy link
Copy Markdown
Member

Summary

  • defer member and user frontend initialization until after their KV global hooks return, while keeping the node frontend synchronous
  • publish frontend readiness atomically after handler initialization so hook-side readiness checks do not take open_lock
  • add focused atomic-open regression coverage
  • enable suppression-free TSAN deadlock detection for the frontend, governance, logging, and partition tests
  • remove the broad store.h and untyped_map.h deadlock suppressions

Why

The endorsed-certificate and service global hooks could call RpcFrontend::open() while KV locks were held. open() takes open_lock and initializes endpoint handlers, extending the hook-side lock chain. The endorsed-certificate hook also queried frontend readiness by taking the same mutex.

The hooks now enqueue idempotent frontend initialization on the task system. RpcFrontend::open() retains its mutex for one-time initialization and release-publishes readiness only after init_handlers() completes; readers acquire-load readiness without taking that mutex.

The node frontend remains synchronous because boot and in-process node RPC tasks depend on it being available immediately.

TSAN exit criteria

DETECT_DEADLOCKS is now available to e2e tests and is applied to the focused startup/governance/partition coverage as well as frontend_test. These tests run with deadlock detection enabled, halt on the first report, and do not load repository-wide suppressions.

The long TSAN workflow will determine whether AFT startup still reports a lock-order inversion. If it does, this PR will add the two-phase startup change described in #8123 before the suppressions remain removed.

Testing

  • TSAN build: frontend_test, js_generic, and logging
  • suppression-free TSAN: frontend_test and ledger_secrets_test
  • C++ and CMake formatting
  • include, copyright, and ASCII checks
  • suppression-free e2e startup and governance runs reached the deferred member-frontend path locally, but could not complete because WSL rejected test TCP listeners; long TSAN CI is requested for authoritative e2e and partition coverage

Closes #8123

Defer member and user frontend initialization out of KV global hooks, publish frontend readiness atomically, and run focused e2e coverage with TSAN deadlock detection before removing the broad KV suppressions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@achamayou Amaury Chamayou (achamayou) added the run-long-test Run Long Test job label Aug 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Defers member/user frontend initialization to avoid KV lock-order inversions and strengthens suppression-free TSAN coverage.

Changes:

  • Atomically publishes frontend readiness after handler initialization.
  • Opens member/user frontends asynchronously.
  • Enables deadlock detection and removes broad suppressions.

Custom instructions used:

  • .github/copilot-instructions.md
  • .github/instructions/reviewing.instructions.md

Reviewed changes

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

Show a summary per file
File Description
tsan_env_suppressions Removes broad KV deadlock suppressions.
src/node/rpc/test/frontend_test.cpp Tests atomic frontend opening.
src/node/rpc/frontend.h Atomically publishes readiness.
src/node/node_state.h Defers member/user frontend opening.
CMakeLists.txt Enables focused deadlock detection.
cmake/gersemi_definitions.cmake Registers the new CMake option.
cmake/common.cmake Propagates deadlock detection to e2e tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/node/rpc/frontend.h
Prevent endpoint registry ticks from running while deferred handler initialization is still in progress, and extend the atomic-open regression test to cover tick dispatch.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor mutex handling, continued

2 participants