bug(syncengine): additional-root pulls and deletions bypass pre-replacement safety snapshots #82

Summary

Ordinary P2P synchronization of an additional named save location can overwrite or delete local files without first establishing a usable pre-replacement snapshot. The primary save location already refuses to replace at-risk files when its safety capture fails; the additional-location path does not apply the same guard.

This is a missing protection boundary for existing local data, not a request to block harmless downloads of new files or all legitimate peer updates.

Code path analysis and current status

• Source reviewed: dev-public at 0b8d8bfec803, 16 October 2026.
• Primary-root safety boundary: the primary-root path computes filesAtRisk(localManifest, decision). If existing local files would be replaced or removed, it calls CreateBeforeReplacing and returns before entering the local-write section when capture fails.
• Additional-root apply path: syncOneRoot computes its decision and conflict state, then enters the mutation sequence: Writing → applyLocalDeletions → propagateDeletions → createPulledDirsIn → pullFiles.
• Missing admission: there is no corresponding filesAtRisk calculation or CreateBeforeReplacing check between the additional-root decision and that mutation sequence.
• Post-apply state: after application, the function proceeds into its normal lineage/agreed-hash update path. The bug(syncengine): additional-root pulls and deletions bypass pre-replacement safety snapshots #82 tests directly assert the byte-preservation failure; they do not independently assert every possible metadata result.
• Related work: the open snapshot/watcher PR #54 addresses incomplete automatic captures and watcher behaviour, but does not add this admission boundary to the additional-root apply path.

Both reproduced cases reach the same unguarded boundary: replacement reaches pullFiles, while deletion reaches applyLocalDeletions, without first proving that the endangered local version is recoverable.

Reproduction — two synthetic loopback peers

The issue was reproduced with two isolated Go testutil.TestDaemon peers, temporary file trees and loopback HTTP (no real game files or external services).

1. Track one synthetic game on peers A and B, each with a named additional location config. Pair the peers and let settings.ini = BASE converge, establishing shared lineage.
2. Disable automatic syncing for the test and wait for prior activity to settle.
3. Change only A's settings.ini to REMOTE NEW. B still holds BASE.
4. On B, configure BackupsDir to refer to an existing regular file rather than a directory; creating a new local safety ZIP must now fail deterministically.
5. Trigger B's synchronization with A. Inspect B's bytes, the result, and locally available safety snapshots.
6. Repeat from a fresh paired fixture for a deletion: both peers start with critical.ini = BASE; A deliberately deletes it; B still has it; B's snapshot destination is blocked before sync.

The remote update and deletion are ordinary valid sync decisions when adequate recovery protection exists. The top-level sync call returned status="in_sync", direction="none" in both reproductions. These fields may summarize the primary root, but no location-specific failure was surfaced for the additional-root mutation. The defect is applying the additional-root changes without the protection required for the corresponding primary-root operation.

Evidence and limits

• Test baseline: dev-public@0b8d8bfec803, 16 October 2026. Synthetic paired-daemon regressions were executed using Go 1.26.4 in an isolated sandbox and Go 1.27.2 in a separate Linux amd64 environment. All files were created in temporary test directories; no real save data was used. The tests were overlaid onto the pinned source without production-code changes.
• Regression results (RED) on this revision: TestAuditLive_ExtraRootPullRefusesToOverwriteWithoutSafetyBackup and TestAuditLive_ExtraRootDeleteRefusesWithoutSafetyBackup. The former replaces BASE with REMOTE NEW and the latter removes critical.ini despite the deliberately unusable local backup destination.
• Differential control (GREEN): TestAuditControl_PrimaryRootRefusesOverwriteWithoutSafetyBackup passes in that same sandbox, preserving the primary save's existing BASE bytes when its backup destination is unusable. The normal existing upstream conflict-resolution control also passes.
• These tests demonstrate loss of the receiving device's prior working version without a successful local safety copy. They do not establish loss of every peer/archive copy or a real-world incident.

Root-cause analysis

The safety invariant was implemented in the primary-root orchestration path rather than in a shared pre-mutation boundary used by every root. Additional roots reuse the destructive helpers, but those helpers assume their caller has already performed any required safety admission; syncOneRoot does not do so.

This is not a bad sync decision: the remote replacement and propagated deletion are legitimate convergence actions when recovery protection exists. The defect is asymmetric admission policy across two paths that can perform the same classes of local mutation. As a result, an unusable backup destination stops the primary-root path but is never consulted by the additional-root path.

The narrow correction is to apply the same at-risk-file rule before mutating each additional location, while retaining location scope: a failure protecting one named location should stop that location without unnecessarily blocking safe new-file pulls or unrelated roots.

Suggested minimal fix and acceptance criteria

• Extend the existing primary-root at-risk-file admission rule to all additional named save locations before local overwrites or propagated deletions. Preserve ordinary sync of new/unchanged files that does not endanger existing content.
• If protection is required, obtain a successful snapshot that actually includes all endangered files; a ZIP with skipped required entries must not count as proof of recoverability.
• On failed admission, preserve local bytes and report a location-specific failure. Preventive acceptance criterion (not independently observed in these tests): do not advance lineage/agreed hashes for changes that were not safely applied or acknowledge successful additional-root application.
• Add negative tests for overwrite and deletion with unavailable backup storage, plus positive controls with writable storage, non-destructive new-file pulls, and normal deletion propagation.
• Keep a failure in one extra location scoped to that location; do not regress primary-root sync or unrelated named locations.

Related: #83 covers explicit user-directed additional-root conflict resolution, a separate entry point with a shared snapshot-safety concern. This report covers routine additional-root sync. A narrow shared safety-admission helper may be appropriate, without redesigning the sync protocol.

We would welcome the maintainer's preferred next step. If helpful, we can submit a draft PR with the regression coverage and a narrowly scoped fix, or help review an implementation.