fix(add-party): harden offline ACS import (crash-safe + DAR preflight) - #249
Open
schronck wants to merge 19 commits into
Open
fix(add-party): harden offline ACS import (crash-safe + DAR preflight)#249schronck wants to merge 19 commits into
schronck wants to merge 19 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Improves the new-member-side AddParty ACS import workflow to be resilient to crashes during the required “participant disconnected from synchronizers” window, reducing the risk of leaving the participant in a broken/disconnected state after ImportPartyAcs.
Changes:
- Adds a durable workflow-artifact marker to detect re-entry into the ACS import disconnect window and trigger recovery before retrying.
- Replaces bulk disconnect with per-synchronizer disconnect and adds reconnect + post-reconnect health verification via polling
ListConnectedSynchronizers. - Adds a unit test for the health predicate used by the reconnect verification.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/decman/src/workflow/storage.rs | Adds a new workflow artifact kind used as a durable crash-safety marker for ACS import. |
| crates/decman/src/workflow/mod.rs | Threads db + instance_name into the peer step call so ACS import can persist/read the crash-safety marker. |
| crates/decman/src/workflow/add_party/steps/acs_sync.rs | Implements crash-safe disconnect/import/reconnect bracketing, reconnect health verification, and adds unit test coverage for the verification predicate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Author
|
Copilot review loop — converged after 5 rounds. 6 findings, all fixed:
3 functionality, 3 doc/naming, 0 security. Round 5 clean. CI still running. |
schronck
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/decman/src/workflow/add_party/steps/acs_sync.rs:486
PackageServiceClient::connect(config.admin_api_url())bypassesNodeConfig::admin_channel()and therefore skips the configured Admin API TLS settings (client certs / root CA, etc.). In TLS-enabled deployments this will fail (or behave inconsistently) even though the rest of the workflow usesadmin_channel()for Admin API calls. Construct the client fromconfig.admin_channel().await?instead to ensure consistent connectivity and TLS configuration.
let mut client = PackageServiceClient::connect(config.admin_api_url()).await?;
schronck
force-pushed
the
fix/add-participant
branch
from
August 4, 2026 12:30
82335dc to
3b0d062
Compare
scolear
reviewed
Aug 4, 2026
Comment on lines
+425
to
+430
| use std::collections::BTreeSet; | ||
|
|
||
| use canton_proto_rs::com::daml::ledger::api::v2::{ | ||
| CumulativeFilter, EventFormat, Filters, GetActiveContractsRequest, GetLedgerEndRequest, | ||
| WildcardFilter, cumulative_filter, get_active_contracts_response::ContractEntry, | ||
| }; |
scolear
reviewed
Aug 4, 2026
Comment on lines
+462
to
+465
| let mut stream = state | ||
| .get_active_contracts(tonic::Request::new(request)) | ||
| .await? | ||
| .into_inner(); |
Member
There was a problem hiding this comment.
Just noting that for large parties, this can be hundreds of Gigabytes of data.
The SyncAcs step disconnects the new member's participant from its synchronizers for the duration of ImportPartyAcs. If the participant shuts down uncleanly inside that window it can be left with orphan ACS rows (archived, NULL reassignment_counter, no activation) and then FATAL-crash on every reconnect — the observed devnet failure. Canton's import isn't atomic and we can't safely auto-repair via the admin API, but the window is now crash-safe on DecMan's side: - durable in-flight marker written before disconnecting; on any re-entry (DecMan or participant crash) the step recovers the participant first - never left disconnected: reconnect always runs, and success is reported only after the synchronizer is verified healthy (not merely that ReconnectSynchronizers was accepted) - a participant that can't be brought back yields an actionable error instead of a cryptic 6-strike abort - drops DisconnectAllSynchronizers for per-synchronizer DisconnectSynchronizer Unit test covers the health-verify predicate.
A DisconnectSynchronizer failure partway through the loop returned early via ? before the reconnect bracket, leaving the participant partially disconnected. Fold disconnect + import into one unit so the reconnect/health-verify always runs. Also correct the connected-list comment (verification checks the configured synchronizer, not that list).
When both the import and the reconnect/health-verify fail, report the unhealthy-participant error (operator-actionable, needs manual repair) instead of masking it behind the retryable import error; fold the import result in as context. Also correct the marker doc to describe its durable, conservative-recovery semantics (never cleared).
…mantics Rename interrupted -> disconnect_window_opened and reword the log/error so they don't imply the prior attempt was necessarily interrupted; the marker is durable and just means the disconnect window was entered at least once.
reconnect_and_verify_healthy only logged the underlying reason (admin API unreachable / RPC error / connected-but-unhealthy) as warnings; the bail said only 'did not become healthy'. Thread the last-seen reason into the error so the operator-facing message names the actual root cause.
Refuse to disconnect the participant for the import unless it's currently connected and healthy on the synchronizer. Opening the disconnect/import window on an already-degraded participant is what risks turning a recoverable state into orphan-ACS-row corruption; fail fast and leave it untouched instead.
… DAR New IT phase (localnet-only, runs last): kicks P3, bulk-creates GovernanceRules for the party, unvets governance-core on P3 via its Canton admin API, then re-adds P3 and asserts P3's AddParty peer run FAILS at SyncAcs — the import can't validate contracts whose package the target lacks. The add-party workflow has no DAR/vetting precheck before import, so nothing catches this today. RED half of reproduce-then-fix.
…ntracts' DAR" This reverts commit f8a3b75.
The add-party workflow had no package check before SyncAcs, so onboarding a member whose participant lacks a package the party's contracts use failed mid-import (after disconnecting) — the devnet 'onboarded a node without the DARs' incident. Now the coordinator enumerates the party's active-contract package ids and ships them with the ImportAcs command; the new member verifies it has them all (PackageService.ListPackages) BEFORE disconnecting, and fails fast with an actionable 'missing package(s)' error otherwise.
Adds a tiny leaf package (orphan-marker, dec-party-signatory) and an IT phase that uploads it to P1/P2 only, seeds OrphanMarker contracts into the party, then re-adds P3 (which never got the DAR) and asserts P3's AddParty peer fails fast with a missing-package error — i.e. the preflight catches it before the disconnect/import window.
Tighten the reproduction assertion to require the preflight's 'missing … package … before onboarding' wording, not just any package error — so green strictly proves add-party refused BEFORE the disconnect/import window, not that the import merely died inside it.
…ion peers The DAR-preflight added a 3rd payload item (package ids). Decode 3, but fall back to the legacy 2-item payload (empty package set → preflight skipped) so a newer peer talking to an older coordinator during a rolling upgrade isn't wedged decoding forever.
Match the codebase convention (limit: 0 = no limit); a finite cap could omit a required package on a participant with many packages and wrongly fail the preflight.
…ds' marker Tighter than the generic word-match: this marker is emitted only by the preflight and implies the concrete missing-package list, so green proves the preflight (not a raw mid-import Canton failure) produced the error.
- bind the party id to a local before collect_party_package_ids (clearer than a temporary in the call args; it already compiled) - capture and debug-log the 3-item ImportAcs decode error on the legacy 2-item fallback, so real corruption is distinguishable from an intentional old payload
…skip empty-ACS scan - health preflight: if the participant isn't connected+healthy, try to reconnect+verify and re-list rather than refusing outright (a disconnected participant is recoverable, and ImportPartyAcs needs it disconnected anyway); bail only if it truly can't be recovered - coordinator: skip the package-id ledger scan when the snapshot is empty (no contracts means no packages to check)
schronck
force-pushed
the
fix/add-participant
branch
from
August 4, 2026 20:26
3b0d062 to
aa24dd0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hardens the add-party offline ACS import against the two devnet failure modes we hit.
Crash-safe disconnect/import window:
DAR-vetting preflight (the 'onboarded a node without the DARs' failure):
Reproduction / regression guard:
Not solved here: OnPR (AddPartyAsync) would remove the disconnect entirely but is alpha/dev-PV-only in Canton 3.5.8; tracked separately.
Validated green in CI including the integration tests.