Add monad-mpt --rescan-devices

[maxkozlovsky]

Growing a node's database meant --archive then --restore into a freshly created larger pool: scratch space, plus a full dump and restore. Extending the device the pool owns was worse than unsupported -- the metadata saying a device belongs to a pool lives at its very end, so lvextend left it unopenable, with no resize2fs step to follow.

--rescan-devices is that step: one offline run takes up the space an extended device now offers, under one confirmation and one metadata growth.

Taking up an extend needs the previous size of the device, which the extend itself strands mid-device, so db_metadata now records that size on each writable open, in padding it already reserves -- no magic bump, no migration. That recording is the only source, and is acted on only where the stranded footer's config_hash confirms it, so an extend must be preceded by a writable open under a release carrying this code.

With one device the relocated footer is the whole commit record: there is no sibling whose config_hash has to be durable first, so a crash at any point either leaves the old footer stranded and the run repeatable, or the new one in place and the run complete.

[github-actions]

Review summary

This is a carefully built change and the crash-safety story holds up under scrutiny: the bytes-used array is written and made durable before the footer that commits the relocation, the overlap refusal (info.size >= previous_size + region) exactly guarantees the new array clears the stranded region, the recorded size is pinned against the stranded footer's own config_hash before it is acted on, and the PENDING_OP_RESCAN record plus the db_map_size_of_ healing fix close the metadata-layer windows. I verified the hash refactor is behavior-preserving against the old inline formulae (including the device_t const * reopen path, where the hash is still taken from the source device), that read_device_info_'s hash_dev_no (st_ino for files, 0 for block devices) matches what open_device_ feeds make_device_, and that the CLI's raw read of copy 0 at device offset 0 is right because cnv chunk 0 starts at offset 0. The test coverage — golden config-hash value, both crash windows, resume, wrong/missing recorded size, bounds — is thorough.

Findings (all non-blocking): one P2 on the abort message an operator actually hits after lvextend + daemon restart (it never names --rescan-devices), and three P3s (defaulted preview_rescan args that can pass where the open asserts, a "Nothing to do" CLI message that is wrong on the resume path, and a duplicated grow test).

Note on CI: the red build jobs fail in category/statesync/test/fuzz_statesync.cpp (6-argument call to monad_statesync_client_context_create vs the 5-argument declaration in statesync_client.h). That mismatch is present in the base commit of max/remove-multi-device (#2562) and this PR does not touch statesync, so it is inherited from the stacked base, not introduced here — but it needs to be fixed in #2562 before this can land green.

PR hygiene is otherwise good: single commit, no merge commits, scope is coherent.

Verdict: CORRECT

🤖 Generated with Claude Code

[maxkozlovsky]

@claude re-review

[github-actions]

[P2] This guard runs after UpdateAux aux(io) has already stamped a fresh database onto the device: UpdateAux::init calls init_new_pool() when is_new_pool() (update_aux.cpp:445), which writes the metadata header and ends in record_device_size_() → sync_metadata_to_disk_(), so by the time this refusal prints "refusing to initialise one", the initialisation is already durable. That also defeats the guard on the very next attempt: re-running the identical command — which the --rescan-devices help text tells the operator to do for an interrupted run — now finds valid db_metadata, is_new_pool() is false, and the run prints "Rescan complete." on a freshly initialised empty database instead of the refusal. To refuse before anything is stamped, probe for the db_metadata magic before constructing UpdateAux (the tool already reads copy 0 raw in recorded_device_size), rather than asking is_new_pool() after the fact.

[github-actions]

Re-review summary

All four findings from the previous review are addressed in this push:

• The open_existing abort in make_device_ now explains that a device extended in place presents this way and names monad-mpt --rescan-devices as the remedy, so the daemon-restart-after-lvextend path reaches an actionable message.
• preview_rescan lost its defaulted arguments; a caller must now supply both the recorded size and the budget, so the preview cannot silently apply fewer refusals than the real mode::rescan open.
• The confirmation prompt for grown_previous_size == 0 no longer claims completion; it now says the pool already spans the device and that an incomplete metadata growth will be finished, which is accurate on the resume path.
• device_resize_test.cpp is gone; its unique recorded_device_size assertions were folded into rescan_devices_test.cpp's grows_chunk_info_and_free_list.

One new finding this pass (inline, P2): the "holds no database after the rescan" guard in cli_tool_impl.cpp runs after UpdateAux's constructor has already initialised and synced fresh db_metadata, so the refusal message is inaccurate and the guard self-defeats — an identical re-run (which the help text prescribes for interrupted runs) finds the freshly stamped metadata, passes the guard, and reports "Rescan complete." on an empty database.

Re-verified on this push: extend_chunk_info_ assigns all seven chunk_info_t bitfields before the atomic store; the unique_hash refactor still takes the hash from the source device on the device_t const * reopen path; the dirty-copy and corrupt-copy heals are both sized by db_map_size_of_ on the clean source copy; and the overlap refusal still guarantees the new bytes-used array clears the stranded region before the commit footer is written.

Verdict: CORRECT

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Critical footer and device-identity issues plus metadata recovery and validation issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds offline --rescan-devices support to reclaim capacity after in-place device expansion by relocating storage metadata and growing database metadata.

Changes:

• Records device sizes on writable opens.
• Adds rescan validation, metadata relocation, growth, and replay.
• Adds CLI confirmation and recovery-oriented test coverage.

Unresolved findings:

• Critical: Make footer commits crash-safe and bind block-device hashes to stable device identity.
• Moderate: Validate existing footer hashes and reject zero capacities before deriving chunks.
• Moderate: Make recorded-size updates recoverable across metadata copies.

File summaries

File	Description
category/mpt/update_aux.cpp	Improves mismatch diagnostics
category/mpt/test/update_aux_test.cpp	Corrects chunk-count test setup
category/mpt/test/rescan_devices_test.cpp	Tests rescan behavior and replay
category/mpt/test/rescan_devices_test_util.hpp	Provides rescan test fixtures
category/mpt/test/rescan_death_no_mode.cpp	Tests refusal without rescan mode
category/mpt/test/db_metadata_test.cpp	Tests metadata layout
category/mpt/test/db_metadata_test_access.hpp	Provides metadata test access
category/mpt/test/CMakeLists.txt	Registers rescan tests
category/mpt/test/cli_tool_test.cpp	Tests end-to-end CLI behavior
category/mpt/detail/db_metadata.hpp	Adds recorded-size and pending-operation metadata
category/mpt/db_metadata_context.hpp	Declares growth and recovery helpers
category/mpt/db_metadata_context.cpp	Records device sizes and grows metadata
category/mpt/cli_tool_impl.cpp	Implements the rescan CLI workflow
category/async/test/storage_pool.cpp	Tests rescan validation and recovery
category/async/test/storage_pool_test_access.hpp	Provides storage-pool test access
category/async/storage_pool.hpp	Adds rescan APIs and device tracking
category/async/storage_pool.cpp	Implements inspection, validation, relocation, and rescan

Review details

Suppressed comments (3)

category/async/storage_pool.cpp:306

• [P2] Validate an existing footer during preview. When info.pool_metadata is present, this branch is skipped and the function never compares its config_hash with compute_config_hash_(info). A database header at offset 0 combined with a footer from another pool therefore reaches the confirmation prompt, then the later adopt_device_() aborts; that contradicts preview_rescan's guarantee that it applies the same refusals. Add the config-hash check for the current-footer case while preserving the existing zero-hash bootstrap behavior.

    }
    uint32_t const chunk_capacity = grown.has_value()

category/async/storage_pool.cpp:145

• [P2] Validate the footer capacity before deriving its chunk count. A footer with the new MND0 magic but chunk_capacity == 0 reaches metadata_t::chunks() here, where the division by chunk_capacity + sizeof(uint32_t) is a division by zero; --rescan-devices then aborts instead of rejecting the malformed pool with a diagnostic. Apply the same nonzero/power-of-two footer validation used for the stranded footer before calling chunks() on the current footer.

    if (memcmp(footer->magic, "MND0", 4) == 0) {
        ret.pool_metadata = device_pool_metadata_{
            .chunk_capacity = footer->chunk_capacity,
            .num_cnv_chunks = footer->num_cnv_chunks == 0
                                  ? legacy_default_num_cnv_chunks
                                  : footer->num_cnv_chunks,
            .config_hash = footer->config_hash,
            .chunks = footer->chunks(ret.size)};

category/mpt/db_metadata_context.cpp:592

• [P1] Make the recorded-size update recoverable across the two metadata copies. Each hold_dirty() ends at the end of one loop iteration, so a crash before sync_metadata_to_disk_() can leave two clean copies with different recorded_device_size values; because the CLI reads copy 0 directly, it can then search for a later stranded footer at the stale size and force an archive/restore. This needs a durable intent/commit or equivalent recovery protocol rather than relying on the per-copy dirty scopes.

    for (auto const &copy : copies_) {
        auto *const m = copy.main;
        auto const g = m->hold_dirty();
        m->recorded_device_size = size;
    }

• Files reviewed: 17/17 changed files
• Comments generated: 2
• Review effort level: Lite

---

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