fix: Settle an acquired ledger before reporting it complete

[bthomee]

Part 9/16 of a stack. Base: part 8 (bthomee/shamap-invalid-08-judge-abandoned-map).

High Level Overview of Change

InboundLedger::done() now settles the ledger (setImmutable()) before publishing complete_,
closing a race where a concurrent reader — isComplete() is read without mtx_, including by
InboundLedgers::acquire() — could otherwise take a ledger whose maps are still mid-sync. A mutable
ledger reaching LedgerHistory::insert(), LedgerHolder::set() or LedgerMaster::switchLCL() calls
logicError(), which aborts a Release build.

Context of Change

isComplete() is read without mtx_, by InboundLedgers::acquire() among others, so the flag must not
be published before the ledger it describes is settled. done() therefore owns the publication: it
settles the ledger and only then sets complete_, so no reader can see the flag before the ledger is
immutable, and its docstring and isComplete()'s now say so.

trigger() and receiveNode() report what they hold rather than what it amounts to: each sets the
have-flags and leaves the conclusion to done(), which trigger() reaches once it has every part or
has failed. tryDB() keeps setting complete_ itself, since it settles its own result first, so
done() accepts either a flag already set or all three have-flags. trigger() calls done() with
mtx_ still held; the mutex is recursive, so the call sites that already hold it further up are
unaffected.

testWalkSettlesBeforeReportingComplete drives an acquisition that only its own walk can finish: the
header and every state node except the deepest are local, so checkLocal() leaves it incomplete, and
the last node becomes available only afterwards. It asserts isComplete() together with
getLedger()->isImmutable() — a single thread cannot observe the ordering itself, so what the case pins
is the consequence: that trigger() still completes an acquisition without setting the flag out of
order.

API Impact

None of the checkboxes below apply: this is an internal xrpld correctness fix with no libxrpl,
public-API, or peer-protocol surface.

[Copilot]

Pull request overview

This PR fixes a concurrency ordering issue in InboundLedger by ensuring a ledger is fully settled (made immutable) before the acquisition is published as complete, preventing other threads from observing isComplete() while the ledger’s SHAMaps are still mid-sync.

Changes:

• Update InboundLedger::done() so it settles (setImmutable()) before publishing complete_, and treats “have every part” as the precondition to completion.
• Adjust trigger()/receiveNode() to stop setting complete_ directly; they now only set the “have-*” flags and let done() publish completion.
• Add a unit test to cover the “completed by walk” path and assert that a completed acquisition yields an immutable ledger.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
src/xrpld/app/ledger/InboundLedger.h	Clarifies contracts: isComplete() implies the ledger is settled; done() documents the required ordering and locking.
src/xrpld/app/ledger/detail/InboundLedger.cpp	Enforces “settle first, then publish complete_” and moves completion publication exclusively into done().
src/test/app/InboundLedger_test.cpp	Adds a regression test validating that walk-completed acquisitions are reported complete only after settlement.

---

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

[github-actions]

This PR has conflicts, please resolve them in order for the PR to be reviewed.

[github-actions]

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[xrplf-ai-reviewer]

This diff correctly restructures InboundLedger's completion signaling so that complete_ is only published from inside done() after the ledger has been settled (setImmutable()), closing the described lock-free-read race with isComplete(). trigger() and receiveNode() now only set have-flags and defer the completion decision to done(), which accepts either an already-set complete_ (tryDB path) or all three have-flags (walk path). The accompanying test (testWalkSettlesBeforeReportingComplete) exercises exactly this ordering by forcing completion to happen via trigger()'s walk rather than tryDB(). I traced through all the changed conditionals (done()'s entry condition, trigger()'s bottom done() call condition, and the removed complete_ = true assignments) and they are internally consistent with the new design; I did not find a correctness bug introduced by this diff. The one residual assumption — that haveHeader_ is already true by the time receiveNode() observes haveState_ && haveTransactions_ — is a pre-existing invariant of the acquisition flow (header is always requested before state/transaction nodes) and isn't something this diff changes, so it's not flagged as a new issue.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!