feat(state_tool): assemble the merged state of a subnet merge

[mraszyk]

Adds a merge command to state-tool, performing the state side of a subnet
merge: the counterpart of the existing split command, which performs the state
side of a subnet split.

state-tool merge --base <destination checkpoint> \
                 --source <source checkpoint> \
                 --output <merged checkpoint>

The command assembles the checkpoint at --output from the ones at --base
(the subnet the canisters are merged into) and --source (the subnet being
merged away): the result holds everything of base, with the canisters and
canister snapshots of source added to those of base, and is marked as the
product of a subnet merge.

Only the canisters and their snapshots are taken over from source. Everything
else — system metadata, subnet queues, ingress history — is base's.

File contents are hard linked rather than copied, so the assembly is cheap no
matter how large the two states are, and sound because checkpoints are
immutable: the links are only ever read afterwards. The two input checkpoints
may stay read-only, as the linking creates the destination directories itself
rather than inheriting their permissions from the tree it copies.

The marker is written through CheckpointLayout::subnet_merged_marker() as a
SubnetMerged protobuf rather than by hand, and canister IDs that collide
between the two checkpoints are refused rather than silently resolved: a
collision means the two checkpoints do not belong to the same merge.

The output is expected to be outside both inputs, which is not checked, and
what an output inside one of them does depends on where. Under base, or under
source's canister or snapshot directory, the linking creates the output before
listing the input it reads, so it finds the output and links it into itself
until the merge fails; the cleanup then leaves the input as it was. Anywhere
else under source the linking never reaches the output and the merge
succeeds, leaving the merged checkpoint inside the source checkpoint. The caller
picks all three paths, so this is left to it rather than guarded against.

The files of the result are marked read-only and synced, as the state manager
does before a directory it assembled becomes a checkpoint.

The merge links straight into the output, which it creates itself. Creating it
is what claims it, rather than a check that it is free: two merges of the same
output cannot both proceed into it, and an output that already exists is
reported by that same step rather than by a check before it. The output is
removed if any step fails — the sync of the directory it was created in
included — so that a merge that reports a failure leaves no checkpoint behind,
not even a complete one whose durability is all that could not be established.
If that removal itself fails, the path it could not remove is reported, appended
to the error that made the merge fail: the caller would otherwise be told the
merge failed, find the output still sitting there, and get already exists on
the retry. A merge interrupted outright cannot clean up after itself at all, so
an incomplete checkpoint is what it leaves at the output path.

The change is purely additive — a new command module plus its registration and
CLI wiring. No existing behaviour changes, and there are no new dependencies:
ic-protobuf, ic-state-layout and ic-types are already dependencies of
state-tool.

The rest of a subnet merge — halting both subnets, downloading the two states,
computing the batch time the merged state starts from, and proposing and
uploading it — is deliberately out of scope here. A subnet merge system test,
not yet on master, drives that workflow and calls this command for the assembly
step.

That test passes with the assembly performed by this command, as it stands: the
destination subnet adopts the recovery CUP at the merged state, which means the
manifest of the assembled checkpoint matches the hash the recovery proposal was
made with, and a canister that the source subnet hosted is found on the
destination subnet still holding the blob it had in its stable memory and its
canister snapshot, with a cycles balance down by no more than an idle canister
is expected to burn.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Nested output paths can recurse indefinitely, and state-sync markers can leave merged checkpoints unverified.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds checkpoint assembly support for subnet merges to state-tool.

Changes:

• Adds the merge CLI command.
• Hard-links base and source canister state into a merged checkpoint.
• Adds merge markers, collision checks, and tests.

File summaries

File	Description
rs/state_tool/src/main.rs	Wires merge CLI arguments and execution.
rs/state_tool/src/commands/merge.rs	Implements and tests checkpoint merging.
rs/state_tool/src/commands.rs	Registers the merge module.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Balanced

---

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

[Copilot]

🟡 Changes recommended

Relative output paths fail, and unsuccessful merges can leave incomplete checkpoints at the final output path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

rs/state_tool/src/commands/merge.rs:54

• The final output directory is created before validation and all later fallible work. For example, the colliding-canister path returns Err at line 67 after this has already linked the entire base checkpoint, leaving an incomplete output that blocks retries via the output.exists() check and could be mistaken for a checkpoint. Assemble under a temporary sibling and rename it to output only after marking/syncing succeeds, with cleanup on every error (or at least perform all collision checks before creating output).

    link_tree(&base, &output)?;

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Failure cleanup, dangling outputs, and symlink handling can produce unsafe or contract-violating behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Production checkpoint assembly and recovery adoption warrant final human validation, particularly because the end-to-end system test is not included here.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Staging-directory ownership is racy between concurrent merge invocations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Checkpoint assembly, hard-linking, durability, and failure cleanup are operationally sensitive and merit final human review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[zeropath-ai]

✅ No security or compliance issues detected. Reviewed everything up to 0460989.

Security Overview

• 🔎 Scanned files: 3 changed file(s)
• 🔗 Scan Link: https://zeropath.com/app/repositories/f5e9d2bb-29a4-4b64-902a-391d65fd8b2d?scanId=c189663b-809b-43aa-bb95-23661ca2929b&codeScanTypes=PrScan&tab=issues

Detected Code Changes

Change Type	Relevant files
Enhancement	► rs/state_tool/src/commands.rs        Add merge module to commands (new pub mod merge)
Enhancement	► rs/state_tool/src/commands/merge.rs        Implement merge functionality to assemble merged subnet state (new file)
Enhancement	► rs/state_tool/src/main.rs        Introduce CLI merge command with base, source, and output arguments and dispatch to merge functionality

[Copilot]

🟡 Changes recommended

Nested outputs can recurse indefinitely, and final publication can replace a concurrently created output.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

rs/state_tool/src/commands/merge.rs:43

• This only makes the output absolute; it does not reject an output nested under either input. For example, --output <base>/merged makes the staging directory a child of base; link_tree(base, staging) then discovers that staging directory and recursively copies it into itself until failure. Resolve the non-existent output through its deepest existing ancestor, canonicalize both inputs, and reject output descendants before creating the staging directory.

    let absolute_output = std::path::absolute(&output)
        .map_err(|err| format!("failed to resolve {}: {err}", output.display()))?;

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[mraszyk]

On the suppressed comment about merge.rs:43, and the summary line about nested outputs recursing indefinitely: that check was removed deliberately in 471f184, so this is a decision rather than an oversight, and it seems worth answering in the open since the point is in the review summary.

The mechanism is exactly as described, and it is the one that was fixed at your prompting in the first place: link_tree creates its destination before listing its source, so a staging directory under an input is discovered by that very listing and linked into itself, one level deeper each time, with nothing bounding the recursion.

The judgement is that the caller picks all three paths, and this is a tool run by hand on states no replica is serving, so an output inside one of the inputs is a mistake at the call site rather than something the command has to defend against — the same reasoning as on the symlink comments earlier in this review.

It is now stated as a precondition where the caller looks, in e0979a8: the doc comment of do_merge, the help text of --output, and the PR description.

[Copilot]

🔵 Needs a closer look

Nested output paths can trigger recursive staging-directory traversal.

Review details

Suppressed comments (1)

rs/state_tool/src/commands/merge.rs:43

• Reject output paths that resolve beneath either input before creating the staging directory. With --output <base>/merged, <base>/merged.merging is created first; link_tree(base, staging) then discovers that staging directory while traversing base and recursively links it into itself. The same failure occurs beneath the source's canister/snapshot trees, and .. or symlink aliases bypass a lexical check, so resolve each input and the output's deepest existing ancestor before testing ancestry.

    if output.exists() {
        return Err(format!("{} already exists", output.display()));
    }

• Files reviewed: 5/6 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[schneiderstefan]

What is the plan with validating that the subnet is properly cooled down with no streams, non-trivial ingress statuses etc? Skip, add here, or do somewhere else in the pipeline?

[schneiderstefan]

This feels unnecessarily complicated for a silly edge case where we somehow run multiple scripts in parallel.

[mraszyk]

Agreed, and it is gone. The staging directory it was guarding went away in 5201fd1: the merge links straight into the output, and creating the output with create_dir is both the claim and the existence check, so what used to be a check plus a separate atomic claim is now one call. The paragraph arguing about concurrent invocations is gone with it, in 671772e.

What remains of the edge case costs nothing: create_dir fails with AlreadyExists, which is reported as <output> already exists. That is the same error the check it replaced produced, so nothing was added for the parallel case — a step was removed.

[schneiderstefan]

This is an example of a comment that doesn't add much and is unnecessary. In fact most comments in this file are.

[mraszyk]

Taken, and taken as being about the file rather than only that line: 671772e cuts the comments to a third of what they were, 50 lines removed.

What I kept is the non-obvious part of a step, one or two lines each: why the height handed to the layout does not matter, why a canister collision is an error rather than something to resolve, why the output's parent is synced, and that the linked-in files are already read-only while the marker is not. What went: the five-line argument about claiming the output, the durability commentary on the cleanup, the read-only paragraph this comment is on, and the restatements in link_tree's doc comment.

Say the word if it is still too much and I will go further — the doc comment on do_merge is the one place I deliberately left long, since it is what a caller reads.

[schneiderstefan]

Keep in mind that as long as the staging path exists, the replica would crash on startup.

[mraszyk]

Good point, and it is what settles the question rather than a caveat on it: I had argued the unparseable .merging name was the safer of two evils, on the grounds that a directory carrying a valid height and no unverified marker is more dangerous. If the replica crashes on startup for as long as the staging path exists, then a merge that is interrupted outright leaves the node unable to start, which is worse than either.

Moot now: the staging directory is gone as of 5201fd1 and the merge assembles at the output directly. Nothing unparseable is created any more.

The cost is that an interrupted merge now leaves an incomplete checkpoint at the output path instead — no crash on startup, but nothing marks it as incomplete either, since the inputs carry no unverified marker to inherit. Worth knowing when running this.

[schneiderstefan]

Nit: This is only used at one place, plus one test, I'd inline it.

[mraszyk]

Done in 671772e — inlined as Height::new(0) at the one call site and in the test, with the reason it does not matter as a two-line comment where it is passed.

[Copilot]

🟡 Changes recommended

Nested output paths can recursively copy themselves and corrupt an input checkpoint.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[mraszyk]

@schneiderstefan on the cool-down question: it is validated out of band, from the metrics visualized at a new subnet merging dashboard, before the states are downloaded — so neither skipped nor added here.

That keeps this command mechanical: it links two checkpoints together and does not deserialize either state, so it has nothing to say about streams or ingress statuses even in principle. The registry side is the same shape — merge_subnets records both subnets being halted with all streams to and from the source empty as a precondition of the mutation rather than something it verifies, and only checks what it can see in the registry, namely that no canister migration is in flight for the ranges being rerouted.

[Copilot]

🔵 Needs a closer look

Cleanup failures are ignored, and the snapshot test fixture does not represent the actual checkpoint layout.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

rs/state_tool/src/commands/merge.rs:78

• The cleanup result is discarded, so an I/O or permission error from remove_dir_all makes the command return only the assembly error while leaving output behind. That contradicts the failure-cleanup contract and also causes a retry to fail with “already exists”; return an error that reports the cleanup failure and its path.
  rs/state_tool/src/commands/merge.rs:190
• This fixture is not checkpoint-shaped for snapshots: the real layout is snapshots/<canister-id>/<snapshot-id>/snapshot.pbuf (rs/state_layout/src/state_layout.rs:338-344), whereas this writes snapshots/<canister>/snapshot.pbuf. As a result, the snapshot tests never exercise the nested snapshot-ID level, and CompleteCheckpointLayout::snapshot_ids() would reject their merged output. Build snapshots with real CanisterId/SnapshotId paths and assert the merged checkpoint can enumerate or load them.

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[mraszyk]

Both suppressed comments of the last review are taken, in 0460989. They came without threads, so answering here.

The discarded cleanup result. Correct, and the consequence you name is the one that matters: the caller was told why the merge failed, found the output still sitting there, and got already exists on the retry. The original error is still the one it needs, so the cleanup failure and the path are appended to it rather than replacing it. No test — reaching that path needs remove_dir_all to fail on a directory this command created and made writable, which nothing in a unit test can arrange, the same as the sync path.

Worth recording that the first version of this fix was wrong in a way the existing tests caught: I wrote the guard as if let (Err(err), Err(cleanup)) = (&result, fs::remove_dir_all(&output)), and both elements of a tuple are evaluated before it is matched, so the cleanup ran on the success path too and deleted every merged output. Six tests failed. It is if let Err(err) = &result && let Err(cleanup) = ... now.

The snapshot fixture. Right, and the reference is right: the layout is snapshots/<hex(canister_id)>/<hex(snapshot_id)>/snapshot.pbuf, so the fixture was a level short and nothing exercised the deepest thing the merge links. It has that level now, and merge_unions_canisters_and_snapshots reads a snapshot of a canister from either input through it.

Two things this does not change, which are worth stating since the comment reads as though it might. The collision check is right as it stands: the top level of snapshots is the canister ID, so comparing the entries of the two directories compares canisters, which is what a merge needs to be disjoint. And the merge itself never depended on the depth — link_tree recurses, so a snapshot nested one level deeper was always linked correctly. This was a gap in what the tests demonstrated, not in what the code did.

I did not switch the fixture to real CanisterId/SnapshotId values, so snapshot_ids() still cannot be called on it. The names are readable placeholders throughout these tests, canisters included, and the merge treats them as opaque directory names; making them real hex ids would buy the enumeration assertion at the cost of every path in the tests becoming unreadable. Happy to do it if a reviewer wants that assertion.

[Copilot]

🔵 Needs a closer look

Checkpoint integrity and recovery semantics warrant final human validation despite comprehensive unit coverage.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced