Add copy.on_dest_newer conflict resolution (default: skip) - #74
Open
srhoods wants to merge 3 commits into
Open
Conversation
drsync always overwrote the destination when source and destination
mtimes disagreed, regardless of direction (times_equal is an unsigned
|diff| check) — correct for a one-directional migration, but a
destination edited out-of-band during a dataset merge would be
silently clobbered on the next pass with no way to protect it short of
excluding the path entirely.
copy.on_dest_newer (proto CopyOptions.ConflictPolicy) adds a
direction-aware check ahead of the normal diff:
- skip (new default): a destination file strictly newer than the
source (beyond mtime_slop_ns) is left completely untouched - no
copy, no owner/mode/xattr fixup either - and recorded as
JR_SKIPPED_NEWER (own counter, not sampled: every occurrence is a
real conflict an operator needs to audit).
- overwrite: the pre-existing behavior, source always wins
regardless of direction. The right choice for a strict
one-directional mirror; opt in explicitly.
A skipped file emits neither JR_COPIED nor JR_META_FIXED, so it's
naturally invisible to VERIFY (passctrl.seedVerify seeds only from
those two record types) - no change needed there, and no risk of
VERIFY's own recopy-on-mismatch silently undoing the skip.
Threaded end to end: proto (new ConflictPolicy enum, JR_SKIPPED_NEWER
journal type, skipped_newer counter) -> spec.go (default/validate/
encode) -> SQLite schema+migration+accumulation -> API/CLI/WebUI
surfacing -> agent decode -> walker.c's diff predicate.
Caught a real mixed-fleet safety bug while testing the agent decode:
pb_put_msg omits an entirely zero-valued CopyOptions submessage from
the wire rather than sending an empty one, so an old coordinator's
JobOptions (or any JobOptions whose CopyOptions all happens to be at
proto zero values) never reaches dec_copy_opts's field loop at all -
setting the skip default only there left exactly that case silently
resolving to overwrite. Fixed by pre-setting the default in
dec_job_options itself, before any field parsing.
Tests: Go spec tests (default/explicit-overwrite/invalid-value), a new
C unit test driving the real wire-decode path (which caught the bug
above before it shipped), and a full e2e script covering dest-newer
skip, source-newer still copying, an unchanged file staying clean, and
explicit overwrite - added to the CI matrix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsdNZLfmAFrMX2VUtkLtmm
scale_e2e.sh seeds $DST/bigdir/f0001.txt with unbackdated (now) content to test that a stale destination file gets replaced. With copy.on_dest_newer now defaulting to skip, that destination write landing after the source's own write (moments earlier in the same setup loop) made the destination read as newer — so the file was correctly left alone by the new default, not overwritten, and the test's content-match assertion failed. Backdate the destination fixture to 2000-01-01, matching the existing precedent in direct_write_e2e.sh's own stale-destination fixture. This is a real interaction with the new default, not a bug in it: caught by CI on the copy.on_dest_newer PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsdNZLfmAFrMX2VUtkLtmm
JOB_TEMPLATE (webui/console.html) is a separately-maintained copy of template.yaml for the WebUI's "new job" editor and had drifted: it was missing copy.on_dest_newer entirely, so a job created from the WebUI template would fall back to the coordinator's own default (correct, since it's unset) but without the operator ever seeing the option or its explanation the way template.yaml readers do. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsdNZLfmAFrMX2VUtkLtmm
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
copy.on_dest_newer: skip (default) | overwrite, resolving a gap where drsync always overwrote the destination on any mtime disagreement, regardless of direction — correct for a one-directional migration, but silently destructive for a dataset-merge scenario where the destination was edited out-of-band and is now newer.skip(new default): a destination file strictly newer than the source is left completely untouched (no copy, no metadata fixup) and recorded asJR_SKIPPED_NEWER— every occurrence, not sampled, since it's a real conflict an operator needs to audit.overwrite: restores the exact pre-existing behavior (source always wins) for a strict one-directional mirror.JR_COPIEDnorJR_META_FIXED, so it's naturally invisible to VERIFY (seedVerifyonly seeds from those two record types) — no risk of VERIFY's own recopy-on-mismatch undoing the skip.ConflictPolicyenum,JR_SKIPPED_NEWER,skipped_newercounter) →spec.go(default/validate/encode) → SQLite schema+migration+accumulation → API/CLI/WebUI surfacing → agent decode →walker.c's diff predicate.Bug caught along the way
Testing the agent-side decode surfaced a real mixed-fleet safety hole before it shipped:
pb_put_msgomits an entirely zero-valuedCopyOptionssubmessage from the wire rather than sending an empty one, so an old coordinator'sJobOptionsnever reachesdec_copy_opts's field loop at all. Setting theskipdefault only insidedec_copy_optsleft that case silently resolving tooverwrite— exactly backwards from the intended mixed-fleet safety. Fixed by pre-setting the default indec_job_optionsitself, before any field parsing.Test plan
gofmt -l ./go vet ./...cleango test -count=1 ./...— all green, including new spec tests (default, explicit overwrite, invalid-value rejection)make -C agent test— all green, including a new C unit test (on_dest_newer_test.c) driving the realdec_work_grant -> dec_job_options -> dec_copy_optsdecode path across absent-field /CONFLICT_UNSPECIFIED/_SKIP/_OVERWRITE— this test caught the mixed-fleet bug above before it was fixedtest/on_dest_newer_e2e.sh, added to the CI matrix) covering dest-newer-skip, source-newer-still-copies, an unchanged file staying clean, and explicit overwrite — run locally end to end, and falsified (temporarily disabled the walker-side check, confirmed the e2e fails for the expected reason, restored)🤖 Generated with Claude Code
https://claude.ai/code/session_01PsdNZLfmAFrMX2VUtkLtmm