bug(syncengine): additional-root conflict resolution can overwrite saves or discard conflict state #83

Summary

ResolveRootConflict, the explicit conflict-resolution API for an additional named save location, logs failure of its attempted pre-resolution safety snapshot but continues executing the requested resolution.

• keep-remote can overwrite locally unique content without a successful local recovery copy.
• keep-local (secondary policy/consistency finding) can report a resolved conflict and advance reconciliation metadata after that safety-capture failure. The test did not demonstrate immediate loss of local bytes in this mode.

A separate reproduced failure occurs when keep-remote clears agreed-base/lineage/conflict state before attempting to fetch the peer manifest. A cancelled peer fetch returns an error but leaves the conflict removed and the agreed hash and recorded lineage cleared.

Code path analysis and current status

• Source reviewed: dev-public at 0b8d8bfec803, 16 October 2026.
• Common safety step: ResolveRootConflict calls CreateBeforeReplacing, but converts any error into a warning and continues into the selected resolution.
• keep-local path: the local-resolution branch stores the local root hash as agreed before fetching the peer manifest. A failed fetch is ignored, SetSyncStateForRoot is called with whatever lineage was obtained and its error is ignored, then the conflict is cleared and a peer pull is triggered.
• keep-remote path: the remote-resolution branch empties the agreed hash, resets root lineage and clears the conflict before FetchManifest verifies that the peer and named location are available.
• Downstream apply: only after fetch and location lookup succeed does keep-remote call syncOneRoot. That additional-root path has no independent CreateBeforeReplacing admission, so it cannot recover the protection lost when the common safety step continued after failure.
• Related work: the open snapshot/watcher PR #54 does not change this conflict-resolution path.

The backup-failure reproduction follows the common safety step into a completed resolution. The cancelled-fetch reproduction returns before file mutation, but only after the prior agreed hash, lineage and conflict record have already been discarded.

Reproduction A — backup failure during two-sided conflict resolution

The issue was reproduced with two synthetic Go testutil.TestDaemon instances over loopback HTTP. Temporary directories and synthetic data were used; no real game saves or credentials were involved.

1. Pair two peers with one synthetic game and a named additional location config. Establish a common settings.ini = BASE state and agreed lineage.
2. Pause automatic syncing and create two different edits: LOCAL UNIQUE on the resolving peer and PEER UNIQUE on its counterpart.
3. Trigger a comparison and assert that ActiveRootConflicts() contains a genuine conflict for config; do not proceed without that precondition.
4. Make the resolving peer's BackupsDir unusable by pointing it at an existing regular file.
5. Invoke ResolveRootConflict(ctx, gameID, peerID, "config", "keep-remote"); inspect the return error, local bytes, conflict status and lineage.
6. Independently recreate the two-sided conflict and repeat with "keep-local".

Reproduction B — peer fetch failure after premature state mutation

1. Recreate a genuine two-sided config conflict with settings.ini = LOCAL UNIQUE on the resolving peer and a different version on its peer.
2. Record the active conflict, existing agreed root hash, settings.ini lineage and local bytes.
3. Call ResolveRootConflict using keep-remote with a cancelled context so the peer-manifest fetch fails.
4. Inspect the error and compare conflict, lineage, agreed-hash state and file bytes with the recorded values.

In keep-remote, SetAgreedHashForRoot("", ...), SetSyncStateForRoot(..., nil, nil), and clearRootConflict happen before FetchManifest. If the peer becomes unavailable or its named location disappears, the method can return an error after discarding the conflict record and prior lineage information.

Confirmed with a synthetic paired-daemon test on current dev-public: a cancelled transport context makes the remote manifest fetch fail. The method returns an error, the local LOCAL UNIQUE file remains intact, but the conflict is cleared, the agreed root hash changes from its prior value to an empty string, and the stored settings.ini lineage is erased. This is reconciliation-state loss, not immediate file-byte loss in this case.

The keep-local path also treats failed peer-manifest and lineage-persistence operations as nonfatal. Audit whether the resulting agreed-base/conflict state remains truthful when those steps fail.

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): TestAuditLive_KeepRemoteConflictMustStopWhenBackupFails, TestAuditLive_KeepLocalConflictRequiresSafetySnapshot, and TestAuditLive_KeepRemoteFetchFailureMustRetainConflictAndLineage all reproduce on the pinned upstream revision.
• GREEN upstream control: TestMultiRootScenario_ResolveALocationConflictKeepingMine passes.
• This shows unsafe loss of one local conflict version (keep-remote) and false completion/state advancement (keep-local), not proof of universal irreversible loss or an observed production incident.

Root-cause analysis

The conflict-resolution operation is not fail-closed as a single state transition. Its safety capture is advisory in executable behavior even though the function contract says a snapshot is taken first, and consequential metadata mutations are committed before later fallible prerequisites have succeeded.

For keep-remote, clearing the base, lineage and conflict is used to prevent the subsequent pull from immediately rediscovering the same divergence. That sequencing solves the successful-path conflict loop, but it exposes partially committed state when peer fetch, named-location validation or later application fails. There is no rollback to the previously recorded conflict state.

For keep-local, local bytes are not replaced, so whether a snapshot must be mandatory is a policy decision. The consistency problem remains: fetch and lineage-persistence failures are treated as nonfatal while the conflict is cleared and success is returned, so the recorded agreed state can claim a completed resolution without all supporting state being established.

The narrow correction is to complete all required preflight and safety work before clearing conflict or lineage, propagate consequential persistence failures, and either commit the resolution state only after successful application or restore the coherent prior state on failure.

Suggested minimal fix and acceptance criteria

• Define a clear safety requirement for each resolution. Where a recoverable pre-resolution copy is promised or required, return on capture error before mutating bytes or reconciliation state; verify that endangered files are actually present in the safety archive, not merely that a ZIP was created.
• If keep-local is intentionally allowed without a safety copy because it does not itself modify local files, make that policy explicit and ensure the method does not report a safety guarantee it could not satisfy.
• For keep-remote, fetch and validate the required peer manifest/location before clearing conflict or lineage; retain the conflict on fetch, validation, snapshot or subsequent apply failure, or restore coherent prior state.
• Check and propagate consequential metadata-persistence errors; do not report success when the recorded resolution state is inconsistent.
• Make these regressions GREEN for backup failure in both resolution modes and peer fetch failure before applying keep-remote; retain GREEN ordinary successful conflict resolution/convergence controls.
• Keep the correction local to additional-root conflict admission/state transitions; avoid unrelated protocol or UI changes.

Related: #82 covers routine additional-root pulls and deletions that bypass primary-root-style safety admission without a conflict. The two call sites may share a narrow safety helper but should retain separate regression coverage.

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.