sync: scope durability pull to the volume's accepted destinations#123
Conversation
The durability pull merged peer-asserted components and freshness coordinates for any destination name a peer sent, so a buggy or compromised peer could pollute the local destination_run_ids with rows for destinations this node neither requires for offload nor syncs to. Scope the merge to the volume's offload_requires ∪ sync_to set: an entry for any other destination is dropped (not an error that aborts the pull, so one junk entry cannot deny a legitimate sync its durability exchange). Drops are counted and listed on the report, and surfaced as sync warnings, so the filtering is observable rather than silent. The monotonic merge and rewind refusal for accepted components are unchanged. Addresses #104 (pull-scoping hygiene; the exploitable gating concern was closed by #120).
Surface the new dropped count: the standalone peer-sync pull-durability summary adds dropped=N with a per-drop detail line, and the sync output's durability line notes drops for unconfigured destinations. The CLI pull fixture's volume declares offload_requires so its seeded component is accepted under the scoped merge.
There was a problem hiding this comment.
Pull request overview
This PR tightens durability metadata pulls so peer-provided durability components and push-freshness coordinates are only merged for destinations that the local volume actually references, reducing the chance of a peer polluting local durability state for irrelevant destinations.
Changes:
- Scope
pullDurabilitymerges toacceptedDestinations(volume)(union ofoffload_requiresandsync_to) and record dropped out-of-scope entries in the pull report. - Surface drop information in node-sync warnings and CLI output.
- Update and add tests to ensure configured destinations are accepted and unconfigured destinations are dropped without aborting the pull.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
sync/node.go |
Pass accepted destination set into durability pull; surface dropped-entry warnings. |
sync/node_origin_test.go |
Adjust fixture volume config so the tested destination is accepted by the new scope filter. |
sync/durability.go |
Add accepted-destination scoping, drop reporting, and a helper to build the accepted set. |
sync/durability_test.go |
Update existing tests to declare destinations in config; add coverage for dropping unconfigured destinations. |
cmd/squirrel/sync.go |
Show dropped count in sync report durability summary line. |
cmd/squirrel/peer_sync_pull_durability.go |
Include dropped count and per-drop details in the standalone pull output. |
cmd/squirrel/peer_sync_pull_durability_test.go |
Ensure the CLI fixture volume declares the destinations it exercises. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, c := range resp.Components { | ||
| if err := validateComponent(c); err != nil { | ||
| return rep, fmt.Errorf("component %+v: %w", c, err) | ||
| } | ||
| if _, ok := accepted[c.Destination]; !ok { |
There was a problem hiding this comment.
Fixed in 244369b. The scope check now runs before validateComponent, so an out-of-scope destination is dropped without full validation — a junk component with a bad origin can no longer abort the pull. Accepted-destination components are still fully validated (a malformed entry we'd actually store should fail loudly). Covered by TestPullDurabilityDropsUnconfiguredDestinations (mixed valid+junk, junk dropped, legit components still applied).
| @@ -109,6 +161,15 @@ | |||
| if err := validateFreshness(f); err != nil { | |||
| return rep, fmt.Errorf("freshness %+v: %w", f, err) | |||
| } | |||
| if _, ok := accepted[f.Destination]; !ok { | |||
| rep.Dropped++ | |||
There was a problem hiding this comment.
| // DurabilityPullReport summarises one durability metadata pull from a | ||
| // peer: how many vector components were fetched, how many landed in | ||
| // the local destination_run_ids (advanced or re-confirmed), and which | ||
| // were refused as rewinds. Every fetched component lands in exactly | ||
| // one of the two buckets. | ||
| // the local destination_run_ids (advanced or re-confirmed), how many | ||
| // were refused as rewinds, and how many were dropped because the peer | ||
| // named a destination outside this volume's accepted target set. Every | ||
| // fetched component lands in exactly one of the applied / rewind / | ||
| // dropped buckets. |
There was a problem hiding this comment.
Fixed in 244369b. Fetched now counts components + freshness, and a merged freshness coordinate counts as Applied, so every fetched entry lands in exactly one of applied/rewind/dropped and Dropped can never exceed Fetched. Reworded the CLI/sync output from 'peer components' to 'peer entries' to match. TestPullDurabilityCapsDropSamples asserts fetched==dropped==50 on an all-junk pull.
| for _, dr := range rep.Drops { | ||
| d.report.Warnings = append(d.report.Warnings, | ||
| fmt.Sprintf("durability pull from %s dropped %s", d.node.Name, dr)) | ||
| } |
There was a problem hiding this comment.
Fixed in 244369b. The per-drop warning loop is gone; pullPeerDurability now emits a single summary warning ('dropped N entries for unconfigured destinations (e.g. )'). The sampled Drops slice is capped at maxDurabilityDropSamples (16) while Dropped stays exact, so the warning list is bounded regardless of how many junk destinations a peer sends.
| for _, dr := range rep.Drops { | ||
| fmt.Fprintf(w, " dropped %s\n", dr) | ||
| } |
There was a problem hiding this comment.
Fixed in 244369b. printDurabilityPull prints at most the capped sample then a '… N more dropped' tail, so stdout stays bounded under an adversarial/buggy peer while the exact dropped count is still shown on the summary line.
Address Copilot review on #123: - Scope-check before full validation. An out-of-scope destination is dropped before validateComponent/validateFreshness runs, so a junk entry for an unconfigured destination with a malformed origin can no longer abort the whole pull — restoring the intended fail-safe. - Reconcile the counts. Fetched now counts components and freshness together and a merged freshness coordinate counts as applied, so every fetched entry lands in exactly one of applied/rewind/dropped and Dropped can never exceed Fetched. - Bound the drop detail. Dropped stays exact, but the sampled Drops slice is capped (maxDurabilityDropSamples) and the sync warning is a single summary line, so a peer flooding out-of-scope destinations can't blow up the report or the output that renders it. The CLI printer prints the sample then a '… N more' tail. Tests: freshness-drop path, the sample cap under a 50-destination flood, and the count reconciliation.
Addresses #104 (pull-scoping hygiene; the exploitable gating concern was closed by #120).
What remained of #104
#120 de-fanged the exploitable part of #104: the offload gate now needs a freshness coordinate, and peer-relayed-target trust is the design's accepted trust boundary. What remained is hygiene — the durability pull merged peer-asserted vector components (and #120's freshness coordinates) for any destination name a peer sent. A buggy or compromised peer could therefore pollute the local
destination_run_ids/destination_push_freshnesswith rows for destinations this node neither requires for offload nor syncs to.The filter
pullDurabilitynow scopes the merge to the volume's accepted target set — the union ofoffload_requiresandsync_to(acceptedDestinations(vol)). Both call sites (the standalonepeer-sync pull-durabilitycommand and the automatic post-close pull insync/node.go) build the set from the same*config.Volume, the same place the offload path readsOffloadRequires.A pulled component or freshness entry whose destination is outside that set is dropped, not stored. The monotonic merge + rewind refusal for accepted components is unchanged.
Fail-safe / backward-tolerant
A drop is not an error that aborts the pull. One junk entry cannot deny a legitimate sync its durability exchange — the loop drops the unknown destination, counts it, and keeps merging the accepted components. Drops are recorded on the report (
Droppedcount +Dropsdetail), surfaced as syncWarningsand on the CLI summary line, so the filtering is observable rather than silent.Tests
TestPullDurabilityDropsUnconfiguredDestinations: a pull carrying a mix ofoffload_requirestarget — merged;sync_totarget — merged;junk— dropped, not stored, counted (Dropped=1) and reported (Drops);Applied=2).Existing pull tests (
TestPullDurabilityCopiesComponents,TestPullDurabilityMergesFreshness,TestPullDurabilityRefusesRewind, the close-timeTestNodeSyncAdvancesVectorAndPullsDurabilityAtClose, and the CLI end-to-end pull) now declare the destinations they exercise inoffload_requires/sync_toso their evidence is accepted under the scoped merge — exercising both union members.go vet ./...,go test ./..., andgolangci-lint runare clean.Judgment calls
destination_run_ids, and it's audit-niceness, not exploitable — recording which peer asserted a row doesn't change which rows are accepted. So this does not fully close [critical] Durability pull merges unscoped, unprovenanced peer-asserted components #104; the provenance-tagging residual remains for a separate change.