fix(reconciliation): stop reporting replicated files as orphan storage on per-node clusters - #960
Merged
Merged
Conversation
…e on per-node clusters On a per-node-storage cluster the reconciler scoped the manifest to the entries this node had originated before both the storage walk and the diff, on the assumption that other nodes' files could not be on this disk. With file replication on (or a local backend over a shared mount) every node holds every file, so each run reported the replicas of every other node as orphan storage — nearly the whole manifest on a writer, all of it on a reader. Nothing was deleted, because the storage sweep re-checks each candidate against the manifest, but that re-check is a race guard doing a job it was never meant to do, the dry-run audit was unusable, and each run spent a manifest-sized FSM lookup pass rejecting replicas. The same scoping fed prefix derivation, and the root walk skips every database this node has an entry in, so a measurement only other nodes write was never walked here and a genuine orphan under it was invisible. The membership set is now every manifest entry — a tracked path is never an orphan-storage candidate, whatever its origin — and the origin scoping moves into computeDiff, applied to the orphan-manifest direction only: an entry another node originated that is missing from this disk is left to file replication. manifestToKeys is gone; the walk derives its prefixes from the full manifest. In cmd/arc/main.go the reconciler gets the coordinator's node id instead of the raw cluster.node_id: the coordinator generates one when the key is unset and stamps it into every manifest entry, while the raw value was empty there, refused by the reconciler at startup with an error and the feature disabled. Regression test: the #957 probe, now also asserting the sweep re-check is never exercised for a replica; plus the orphan-manifest direction pinned in both senses and the foreign-prefix walk at the default root-walk cap. Fixes #957
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
1 participant
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.
Summary
Fixes #957. On a per-node-storage cluster the reconciler scoped the manifest to this node's own-origin entries before both the storage walk and the diff, on the assumption that other nodes' files could not be on this disk. With file replication on (or
storage.backend=localover a shared mount) every node holds every file, so each run reported the replicas of every other node as orphan storage: nearly the whole manifest on a writer, all of it on a reader. Nothing was deleted — the storage sweep re-checks each candidate against the manifest — but that re-check is a race guard doing a job it was never meant to do, the dry-run audit an operator reviews before turning the dry run off was unusable, and each run spent a manifest-sized FSM lookup pass rejecting replicas. The same scoping fed prefix derivation; the root walk skips every database this node has an entry in, so a measurement only other nodes write was never walked here and a genuine orphan under it was invisible (at the default root-walk setting, not only at zero — measured by the adversarial pass).computeDiffbuilds its map from the full key set; a tracked path is never an orphan-storage candidate, whatever its origin.computeDiff(localNodeID,perNodeStorage): in per-node mode an entry another node originated that is missing from this disk is skipped — the puller's business with replication, another disk without it. Origin-less (pre-Phase-1) entries stay candidates on every node, as before. The invalidperNodeStorage=true, localNodeID=""fails safe (reports nothing);NewReconcilerrejects it anyway.WalkPartial, and foreign databases make the root walk cheaper.manifestToKeysremoved; the two comments that encoded the wrong assumption rewritten; the sweep re-check stays as defence in depth.cmd/arc/main.go):LocalNodeID: clusterCoordinator.LocalNodeID()instead ofcfg.Cluster.NodeID. The coordinator generates an id when the key is unset and every producer ofOriginNodeIDusesLocalNodeID(); with the raw value a bare-metal node without an explicit id got "Failed to create reconciler — feature disabled" and the fix would be unreachable there. Helm always sets the id.## Bug fixes.Pre-existing, named and not solved here: an entry whose origin node left or was renamed is foreign to every survivor, so a file lost everywhere is reported by nobody (the documented contract: the manifest sweep runs on the origin writer); origin-less entries may be proposed for deletion by several nodes (FSM no-op on the second, audit double-count); the snapshot-install gap (
FSM.Restorenever fires the delete callback); and, found by the review, a node restored from an empty disk under a stablecluster.node_idnever re-acquires its own-origin files (the puller and catch-up skip self-origin), so an act-mode run there proposes a manifest delete for each — identical under the old filter, kept a report by the default dry run, filed as #959. Plan + matrix:docs/progress/2026-09-28-reconciler-replica-orphans-957.md(untracked).Configuration matrix
reconciliation.enabled=false(default)clusterCoordinator == nilat the same gateBackendStandalonereconciliationBackendKindnever returns it;perNodeStorage=false→ unchangedperNodeStorage=false— membership set was already the full set; orphan-manifest unchangedLocalNodeID != ""viaNewReconciler, now the coordinator's idSkippedRecheckno longer inflated; foreign-only measurements walkedcluster.node_idunset (non-default)clusterCoordinator != nilat the gatereconciliation.max_root_walk_databases=0(non-default)reconciliation.grace_window_seconds=1,clock_skew_allowance_seconds=0(non-default, live-run values)reconciliation.manifest_only_dry_run=false(non-default)MaxDeletesPerRununaffectedmax_manifest_sizeexceededcluster.node_idTest plan
go build ./cmd/... ./internal/...,gofmt -lempty,go vet ./internal/reconciliation/ ./cmd/arc/go test -race ./internal/reconciliation/green (whole package)TestReconcile_LocalModeReplicaOfForeignFileIsNotOrphanStorage(the reconciliation: local-mode origin filter reports every replicated foreign-origin file as orphan storage; only the sweep re-check prevents deletion #957 probe, dry-run and act mode, plusSkippedRecheck == 0),TestReconcile_LocalModeOwnEntryMissingIsOrphanManifest,TestReconcile_LocalModeWalksForeignPrefixes(default root-walk cap; same database, measurement only node-b writes, true orphan under it),TestComputeDiff_PerNodeStorage(scoped, unscoped, fail-safe). ExistingTestReconcile_LocalModeFiltersForeignOriginFileskeeps passing.OrphanStorageCount=1,SkippedRecheck=1) and the foreign-prefix test fails (StorageFileCount=1, want 3); with only the orphan-manifest scoping removed, the existing local-mode test and the own-entry test report the foreign entry (count=2,got 1 (sample: [db/m/foreign.parquet])) and the diff unit test failsLocalNodeIDfield doc and theBackendLocal/main.gocomments described origin scoping of orphan-storage candidates, the opposite of the code; the note's "silently" was wrong (the empty id was refused with a startup Error and the feature disabled); and the never-walked claim now carries its qualifier (a database this node already had an entry in). Also from the review, stated in the note: the generated node id is host name plus PID, so on bare metal it changes per restart and entries from before the last restart count as another node's; setcluster.node_idexplicitly.cmd/arc/main.gochanged): Pattern 1 rig built from this branch (enterprise-local, writer1-3 + reader1, replication on, tiering on every node) withreconciliation.enabled=true,grace_window_seconds=1andclock_skew_allowance_seconds=1(all non-default; dry run at its default). Every node built its reconciler (no "Failed to create reconciler" line;backend_kind=local). Six flushes on the primary → six replicas on reader1 and on writer2 within 2 s → dry-run reconciliation triggered on reader1, writer2 and the primary: each reportsmanifest_file_count=6 storage_file_count=6 orphan_storage_count=0 orphan_manifest_count=0 skipped_recheck=0 skipped_grace=0; re-run minutes later with the two-second window (grace 1 s + skew 1 s) over the same replicas: the same zeros on reader1 and writer2, where all six files are foreign-origin and would each have been an orphan-storage candidate before the fix. Aside found on the way, not changed here:clock_skew_allowance_seconds=0is replaced by the 5-minute default (applyDefaultstreats<= 0as unset), so zero is not a settable value for that key; the run used 1.