Skip to content

fix(sync): self-heal out-of-band rewrites instead of latching self-conflicts (TIN-3277) - #576

Open
Jesssullivan wants to merge 4 commits into
mainfrom
fix/tin-3277-oob-vclock-retick-20260729
Open

fix(sync): self-heal out-of-band rewrites instead of latching self-conflicts (TIN-3277)#576
Jesssullivan wants to merge 4 commits into
mainfrom
fix/tin-3277-oob-vclock-retick-20260729

Conversation

@Jesssullivan

@Jesssullivan Jesssullivan commented Jul 30, 2026

Copy link
Copy Markdown
Owner

v2 — rewritten after both adversarial refuters rejected v1. Every must-fix is addressed below. Still DRAFT; the re-verify pass decides readiness.

The defect

An out-of-band writer (home-manager secrets/dotfile materialization on neo, an editor bypassing the tcfs write hook) rewrites an enrolled file without ticking the vclock. The clock freezes at its last-synced value while content diverges, so conflict.rs::compare_clocks sees EQUAL clocks + differing blake3 and returns Conflict. The reconcile Conflict arm is record-only — it bumps times_recorded and flips status, it never pushes — so the path is structurally unpushable. Live on neo: 7 permanent conflicts (secrets/**, dotfiles/tcfs/devices.json) stuck 24+ days with times_recorded in the thousands.

The fix

When the pair is provably a local-only rewrite, tick a comparison-only clone of the vclock before compare_clocks, so it classifies LocalNewerPush and converges. conflict.rs is untouched (compare_clocks byte-identical), so keep-both / loser-guard / FF-veto surfaces are unchanged.

v2 predicate (self_rewrite_retick_applies)

  !device_id.is_empty() && remote_device == device_id   // corroboration ONLY, not the safety boundary
  && local_hash != remote_hash                          // identical content is UpToDate anyway
  && !is_git_internal_path(rel_path)                    // .git keeps the fail-closed FF-veto / keep-both path (G5-git-13)
  && tracked.is_some()
  && tracked.blake3 != local_hash                       // evidence the LOCAL file moved since last sync
  && tracked.blake3 == remote_hash                      // ★ NEW, LOAD-BEARING: the remote is our own last-synced bytes
  && stored_ordering == Some(Ordering::Equal)            // ★ computed against the STORED clock, before any re-tick

The safety proof is CONTENT identity, not device identity

This is the central v2 change. tracked.blake3 == remote_hash means the remote copy is byte-identical to what this device last synced. Overwriting it therefore provably cannot lose another writer's work — there is nothing on the remote that is not already in this device's own tracked baseline.

Any foreign write — from any device, by any mechanism — changes remote_hash, fails the clause, and falls through to the ordinary compare_clocksConflict path exactly as before this PR. Concretely this closes:

  • A peer publishing a spoofed written_by: "neo". written_by (manifest.rs) is an unauthenticated client-set JSON string; validate_indexed_manifest_binding binds manifest bytes to the index's manifest_hash, it does not sign the device claim, and the registry-signing key is HKDF-derived from the shared master key. v1 made that string an authorization input. v2 does not: to pass the predicate an attacker must publish content equal to our tracked baseline, i.e. leave the remote exactly as this device last left it, which is a no-op.
  • A second isolated state cache under one device_id (the macOS FileProvider, a tcfs push against a different state dir — memory records this as live: "0.12.17 resolve blind to isolated caches"). Its push changes remote_hash; predicate fails; conflict recorded.
  • A colliding / cloned device_id (re-imaged host reusing a hostname-derived id). Same mechanism — content, not identity, decides.
  • dotfiles/tcfs/devices.json pushed stale. If this device's local registry copy missed a legitimate revocation or enrollment that landed remotely, the remote content is not our baseline, so no heal — a recorded conflict, as today.

written_by == device_id is retained as corroboration, and because it preserves the pre-existing behavior that a distinct-device equal-clock pair records a conflict (and makes legacy v1 manifests, empty written_by, fail closed).

No silent loss / why no parked copy

KeepBoth and the .git undo-bundle machinery exist to preserve a second writer's bytes. Under the content-identity clause the displaced remote content is tracked.blake3 — this device's own last-synced bytes, still described by its own state entry and, until GC, still addressable by the manifest key. A .conflict-{device_id} sidecar would duplicate our own history, and on secrets/** would multiply ciphertext copies of sensitive material for zero recovery benefit.

In place of a sidecar, every self-heal push now logs at info: path, device, displaced_manifest (the remote manifest key being replaced), displaced_hash, local_hash. That is sufficient to reconstruct what was replaced.

Refuter must-fixes → disposition

# Finding Disposition
C-MF1 Double-tick promotion. v1's predicate read a clock the TIN-2584 block had already ticked, so a strictly-dominated (Less) self-pair became Equal → ticked again → GreaterPush. Fixed structurally. stored_ordering is computed once, against the stored clock, before any tick; the two rules are joined by else if; the predicate takes stored_ordering: Option<Ordering> as a parameter so a caller cannot hand it a mutated clock. Regression test tin3277_self_pair_dominated_clock_is_not_promoted_to_push pins main's behavior (recorded Conflict). Independently, the refuter's exact trace also now fails the content clause (tracked.blake3 != remote_hash there).
C-MF2 Missing tracked.blake3 == remote_hash — the justification's own unstated premise; isolated-second-cache clobber with no ConflictInfo, no keep-both, GC-eligible chunks. Fixed. Added as the load-bearing clause; the safety argument is rewritten around it. Negative test tin3277_remote_moved_since_baseline_still_records_conflict.
C-MF3 Vetoed-push path may rewrite the tracked baseline and loop forever; upload.skipped invisible. Investigated + pinned end-to-end. For the healed shape the veto is unreachable: the engine re-derives its clock from the tracked state with one local_edit_inferred tick (engine.rs:2205-2215), reaching {neo:2} vs remote {neo:1}Greater/LocalNewer. tin3277_full_cycle_self_heal_advances_clock_and_clears_latch proves it with a real plan → execute → state cycle asserting pushed == 1. If the remote does move between plan and execute, tin3277_veto_between_plan_and_execute_records_conflict_and_settles pins the whole chain: push skipped, pushed == 0, a visible ConflictInfo recorded (status = Conflict, surfaced by tcfs conflicts / D-Bus), baseline moved to the live local hash — which disarms the predicate, so the next cycle settles into the pre-fix recorded-conflict behavior rather than replanning forever. Fail-closed, not silent. Plus: execute_plan now logs skipped pushes at info (they were in neither pushed nor errors).
C-MF4 Tests mirror rather than pin; (c) vacuous, (d) can't mutate state, (f) hand-builds the post-push world. Reworked. Vacuous (c) replaced by the content-clause negative case; hand-built (f) replaced by the real full-cycle test, which also asserts the latched ConflictInfo is cleared and status leaves Conflict (so the latch setup is load-bearing). Added the dominated-clock regression and the veto cycle test. Mutation-tested — see below.
S-MF1 Spoofed/colliding device identity → silent clobber. Closed by the content clause. written_by demoted to corroboration; documented in code and above.
S-MF2 devices.json self-heal-pushable, bypassing the registry's Ed25519 verification. Narrowed, not closed (corrected at the v3 gate). The content clause closes the specific property *"no clobber of remote bytes this device has not seen at dotfiles/tcfs/devices.json"*. It does **not** demonstrate the different, undemonstrated property that the canonical Ed25519-signed registry (tcfs-secrets/src/device.rs`, untouched by this diff) stays correctly reflected. No hard carve-out added, deliberately — see below.
S-MF3 KeepBoth / undo-bundle safety net bypassed; no parked copy. Documented as redundant under content identity (the displaced bytes are our own baseline), + info audit log of the displaced manifest key — now honestly labelled best-effort plan-time provenance (fields renamed displaced_manifest_at_plan / displaced_hash_at_plan), because the commit happens later after the engine's own fresh remote read. A remote that moves in between is vetoed and logged at execute time instead. Corrected at the v3 gate.
S-MF4 Secrets SSOT now unconditionally local-wins with a TIN-3278-unreliable evidence gate and age non-determinism. Acknowledged, narrowed, and gated — see below.

Explicit decisions the refuters asked for

No secrets/** or devices.json carve-out. With content identity, the dangerous scenarios are already excluded: a stale local registry can only heal when the remote is byte-identical to our own baseline, i.e. when the remote holds no revocation or enrollment we are missing. A carve-out would instead leave exactly the 7 live stuck paths permanently unhealable, which is the ticket.

Age non-determinism is real and intended here. Every home-manager re-materialization produces new ciphertext for identical plaintext, so post-fix each activation will push. That is the heal, not a bug — but it does mean secrets churn becomes visible push traffic.

LANDING GATE (not a followup): run a real ciphertext parity check neo↔honey for the 7 stuck paths before this reaches a host. Size parity is a weak proxy; decrypt-and-compare or recipient-set comparison is the real check. Promoted from a silent followup at the refuters' insistence.

TIN-3278 interaction. State-cache key-namespace duplication (absolute vs relative keys) affects 2 of the 9 entries in this triage and is a separate PR. It can make tracked resolve to the wrong entry, which would make the predicate fail (no heal) rather than fire wrongly — the content clause fails closed on a mismatched baseline. Not fixed here.

Multi-host honesty. This heals the self-pair only. The out-of-band writer runs on every host, so the equal-clock divergence class is inherently multi-host: peers keep accumulating record-only conflicts on the same paths, and the actual structural defect — the Conflict arm that records but never acts — is untouched. Expect some conflict count to migrate to honey rather than drop fleet-wide. Remaining TIN-3277 scope for a later PR: route HM writes through tcfs, or make the Conflict arm actionable (AutoResolver already returns KeepLocal for equal device names and has no caller on this path).

Symlink blind spot. compare_both_exist_symlink has the same equal-clock hole and is untouched (neo's dotfiles are a nix/HM symlink farm). Deliberately out of scope to keep this narrow.

Gate round (v3) — second-pass must-fixes

Two independent adversarial reviews of v2 both returned approve-with-must-fix. All five must-fixes are applied on this head.

# v2 must-fix Disposition
A-MF1 Prove the fix is not a no-op on its own target population — the re-tick lives only in compare_both_exist; compare_both_exist_symlink has no re-tick at all, and compare_both_exist routes to it before any new code runs. neo's dotfiles are a nix/HM symlink farm and the headline test uses a hand-created regular file, which cannot disambiguate. Answered with live read-only evidence: all 9 conflicted entries in neo's state cache are REGULAR files — zero symlinks, so this comparator is the right one and the fix is not a no-op. Probed via lstat on the entry keys in ~/.local/share/tcfsd/state.json (no writes, no daemon interaction, freeze respected). The 7 times_recorded = 4696 paths: secrets/api/{github_token,anthropic,gitlab_token,crates_io_token}.age, secrets/infrastructure/tailscale_auth_key.age, secrets/.manifest.toml, dotfiles/tcfs/devices.json — plus secrets/.audit.log, which appears twice under two key namespaces (/Users/jess/tcfs/secrets/.audit.log and /secrets/.audit.log, different blake3) — the live TIN-3278 duplication, in the flesh. Symlink parity stays a follow-up, correctly scoped.
A-MF2 No degenerate-local-content guard on the one push no tcfs-aware actor initiated: tracked.blake3 != local_hash cannot tell "HM re-materialized the secret" from "HM/agenix failed and left a 0-byte or truncated file". Fixed in code, not prose. Two fail-closed clauses: refuse when local_size == 0 while tracked.size > 0; and for *.age paths refuse bytes that no longer carry an age container header (binary or armored). Keyed off the .age extension, not a secrets/ prefix — because secrets/.manifest.toml is plaintext TOML by design and is one of the 7 live stuck paths, so a prefix rule would have made this fix a permanent no-op there (exactly the A-MF1 failure mode). Verified against the live corpus: the 5 stuck .age files all carry the binary header, so the guard admits them; a binary-only guard would have been fine but the armored form is accepted too, proven by test. Residual limit stated in the doc comment: a truncation that preserves the header is not detectable from content alone.
A-MF3 The new skipped-push log asserts "remote moved since plan", but upload.skipped is also true for benign UpToDate dedup, RemoteNewer, and the fast path. Fixed. The message is now keyed off the engine's own upload.outcome (Conflict → "execute-time conflict veto: remote moved since plan"; RemoteNewer; UpToDate → "no-op: remote already holds this content"; else "skipped without a conflict verdict") under a neutral event name, with the verdict still logged as a field.
B-MF1 Plan/execute tracked-state divergence. Planning resolves tracked via the fuzzy StateCache::get_by_rel_path (suffix match over the whole map, HashMap-order tie-break); the push resolves its own baseline via the exact canonical key (StateCache::get), and only the exact entry is ever written back by state.set. Under TIN-3278 the safety proof can be evaluated against a different record than execute mutates, with no detection. "Fails closed" was asserted, not proven. Fixed by proving it instead of asserting it. reconcile() now also resolves the exact-key entry and proves the two lookups return the same record (pointer equality into the one entry map); self_rewrite_retick_applies takes tracked_is_exact and refuses when it does not hold, with a warn! naming TIN-3278. No other classification decision reads the flag, so nothing else changes behavior. The full-cycle test — which drives the real reconcile → execute_plan → state path — still heals, which is the proof that pointer equality holds on the real code path and this guard is not itself a no-op.
B-MF2 Audit-log integrity under race: the displaced_* fields are read at plan time but the overwrite happens later, so they are not provably what was displaced — and this log is S-MF3's sole replacement for a parked copy. Fixed by correcting the claim (the reviewer's own second option). Fields renamed displaced_manifest_at_plan / displaced_hash_at_plan; the comment and the S-MF3 row now say best-effort plan-time provenance, and point at the execute-time skip log for the moved-remote case. Emitting from the commit path would mean plumbing this through engine::upload_planned_push_with_device's own remote read — a cross-cutting change with more blast radius than the honesty fix it buys; called out as a follow-up rather than smuggled in.

Also taken from the reviewers' non-blocking notes: the doc comment no longer claims the content clause provably prevents loss — it now states the precise strength (validate_indexed_manifest_binding binds manifest bytes to manifest_hash, not the declared file_hash to the chunk payload, so the guarantee is "no honest remote writer's content is displaced"), and the noted coverage gap — that no test pinned the else if independently of the content clause — is closed by ..._same_content_republish_dominated_clock_is_not_double_ticked (mutation E below).

Mutation testing — the tests pin, they do not mirror

Run on sting against this exact tree:

  • Mutation A — delete the tracked.blake3 != remote_hash clause → tin3277_remote_moved_since_baseline_still_records_conflict FAILS with got Ok(Push { reason: LocalNewer }). The content clause is load-bearing and the test catches its removal.
  • Mutation B — A, plus restore v1's sequential structure and pass the live (already-ticked) clock ordering → two tests fail, including tin3277_self_pair_dominated_clock_is_not_promoted_to_push with got Ok(Push { reason: LocalNewer }). This is a verbatim reproduction of C-MF1's trace, caught by the new regression test.
  • Mutation C — delete the tracked_is_exact clause → tin3277_inexact_tracked_lookup_declines_self_heal FAILS with got Ok(Push { reason: LocalNewer }).
  • Mutation D — delete both degenerate-content clauses → two tests fail: ..._emptied_local_rewrite_is_not_pushed and ..._non_ciphertext_local_age_rewrite_is_not_pushed, both with got Ok(Push { reason: LocalNewer }).
  • Mutation E — restore v1's sequential structure and live-clock ordering while leaving the content clause intact..._same_content_republish_dominated_clock_is_not_double_ticked FAILS with got Ok(Push { reason: LocalNewer }). This is the test that pins the else if on its own; mutation B needed two edits to reach the same class of defect.
  • Unmutated: all 13 pass.

Tests (13)

Test Pins
..._self_rewrite_equal_clock_pushes_instead_of_conflicting the heal itself
..._distinct_device_equal_clock_still_records_conflict two-honest-device narrowing
..._remote_moved_since_baseline_still_records_conflict ★ content-identity negative case
..._self_pair_dominated_clock_is_not_promoted_to_push ★ double-tick regression (C-MF1)
..._deferred_push_does_not_accumulate_ticks no per-cycle clock growth
..._git_internal_self_pair_stays_conflict .git carve-out (G5-git-13)
..._full_cycle_self_heal_advances_clock_and_clears_latch ★ real plan→execute→state: pushed == 1, clock {neo:1}{neo:2}, latch cleared, next cycle no-op
..._veto_between_plan_and_execute_records_conflict_and_settles ★ real cycle: veto → visible conflict → predicate disarmed, no loop
..._same_content_republish_dominated_clock_is_not_double_ticked ★ v3: the else if independently of the content clause (mutation E)
..._emptied_local_rewrite_is_not_pushed ★ v3: an emptied out-of-band rewrite never publishes
..._non_ciphertext_local_age_rewrite_is_not_pushed ★ v3: plaintext over intact *.age never publishes — plus a secrets/.manifest.toml control proving the guard is an extension rule, not a prefix rule
..._inexact_tracked_lookup_declines_self_heal ★ v3: baseline that may not be the record the push overwrites → fail closed
..._age_header_guard_accepts_both_container_forms ★ v3: binary and armored age headers admitted; missing file / short read fail closed

Every *.age test now uses real age-container bytes, so each verdict still turns on the clause it is testing rather than on an incidental header failure.

sting results (verbatim)

All on sting (never on neo), scratch worktree /tmp/tin3277-wt3, removed afterwards.

$ cargo test -p tcfs-sync --lib tin3277
running 13 tests
test result: ok. 13 passed; 0 failed; 0 ignored; 0 measured; 436 filtered out; finished in 0.05s

$ cargo test -p tcfs-sync
test result: ok. 449 passed; 0 failed        # lib
... 13 integration suites, every one `test result: ok`, 0 failed anywhere

$ cargo fmt --all -- --check
FMT_CLEAN

$ cargo test -p tcfs-sync --lib tin3277
running 8 tests
test reconcile::tests::tin3277_self_rewrite_equal_clock_pushes_instead_of_conflicting ... ok
test reconcile::tests::tin3277_distinct_device_equal_clock_still_records_conflict ... ok
test reconcile::tests::tin3277_self_pair_dominated_clock_is_not_promoted_to_push ... ok
test reconcile::tests::tin3277_remote_moved_since_baseline_still_records_conflict ... ok
test reconcile::tests::tin3277_git_internal_self_pair_stays_conflict ... ok
test reconcile::tests::tin3277_deferred_push_does_not_accumulate_ticks ... ok
test reconcile::tests::tin3277_veto_between_plan_and_execute_records_conflict_and_settles ... ok
test reconcile::tests::tin3277_full_cycle_self_heal_advances_clock_and_clears_latch ... ok
test result: ok. 8 passed; 0 failed; 0 ignored; 0 measured; 436 filtered out; finished in 0.05s

$ cargo test -p tcfs-sync          # full crate
lib:                            444 passed; 0 failed   (13.09s)
auto_pull_logic_test:             6 passed; 0 failed
e2e_conflict_resolution:         11 passed; 0 failed
e2e_delete_scenarios:             6 passed; 0 failed
e2e_directory_ops:                6 passed; 0 failed
e2e_edge_cases:                   8 passed; 0 failed
e2e_file_state_transitions:       7 passed; 0 failed
e2e_placeholder_lifecycle:        4 passed; 0 failed
e2e_state_transition_chains:      6 passed; 0 failed
e2e_two_device_sync:              4 passed; 0 failed
state_cache_reload_test:          3 passed; 0 failed
symlink_handling_test:            8 passed; 0 failed
two_device_sync_test:             3 passed; 0 failed
Doc-tests:                        0 passed; 0 failed

$ cargo clippy -p tcfs-sync --all-targets -- -D warnings
Finished `dev` profile [unoptimized + debuginfo] target(s) in 52.84s     # no warnings

Scratch worktrees /tmp/tin3277-wt2 (v2) and /tmp/tin3277-wt3 (v3) on sting removed afterwards.

Scope / collisions

One file: crates/tcfs-sync/src/reconcile.rs. conflict.rs, engine.rs, state.rs unchanged. Source-only under the deploy freeze — no config, no CHANGELOG, no deploys.

…IN-3277)

An out-of-band writer (home-manager secrets/dotfile materialization, an
editor bypassing the tcfs write hook) rewrites an enrolled file without
ticking the vclock. The clock freezes at its last-synced value while the
content diverges, so compare_clocks sees EQUAL clocks + differing blake3
and returns Conflict. The reconcile Conflict arm is record-only, so the
path becomes structurally unpushable -- observed live on neo as 7
permanent conflicts stuck 24+ days with times_recorded in the thousands.

Heal it: when the pair is provably a local-only rewrite, tick a
comparison-only CLONE of the vclock before compare_clocks so it
classifies LocalNewer -> Push. conflict.rs is untouched (compare_clocks
byte-identical), so keep-both / loser-guard / FF-veto are unchanged.

The safety proof is CONTENT identity, not device identity. The
load-bearing clause is tracked.blake3 == remote_hash: the remote copy is
byte-identical to what this device last synced, so overwriting it
provably cannot lose another writer's work. Any foreign write -- from any
device, including one publishing a spoofed written_by, or a second state
cache under one device_id -- changes remote_hash, fails the clause, and
records a Conflict exactly as before. written_by == device_id is kept as
corroboration only; it is an unauthenticated client-set string and is
deliberately not the safety boundary.

Both out-of-band re-tick rules now key off a single stored_ordering
computed against the STORED clock before any tick, joined by else-if.
Previously the new predicate read a clock the TIN-2584 block had already
ticked, so a strictly-dominated (Ordering::Less) self-pair could be
double-ticked into Greater/LocalNewer and push over a remote the local
side had never seen.

Also: log the displaced remote manifest key at info on every self-heal
push (audit trail in place of a parked copy -- the displaced bytes are
this device's own tracked baseline), and log execute-time push vetoes,
which previously left no trace in either `pushed` or `errors`.

Scope: heals the SELF-pair only. Peer-side accumulation and the
record-only Conflict arm remain open TIN-3277 scope.
@Jesssullivan
Jesssullivan force-pushed the fix/tin-3277-oob-vclock-retick-20260729 branch from 7932d2c to fe69adb Compare July 30, 2026 04:06
@Jesssullivan

Copy link
Copy Markdown
Owner Author

v1 → v2 (7932d2cfe69adb)

Both refuters rejected v1. Rework below; PR body rewritten in full. Still DRAFT.

The one change that collapses most of it: the predicate now requires tracked.blake3 == remote_hash. The safety proof moves from device identity (an unauthenticated written_by string) to content identity: the remote copy is byte-identical to what this device last synced, so overwriting it provably cannot lose a second writer's work. Any foreign write — any device, any mechanism, including a spoofed written_by — changes remote_hash, fails the predicate, and records a Conflict exactly as before. written_by == device_id survives as corroboration only.

Per finding

Finding Change
C-MF1 double-tick promotes dominated self-pair to Push stored_ordering computed once against the STORED clock before any tick; the two rules joined by else if; the predicate takes stored_ordering: Option<Ordering> so a caller cannot pass a mutated clock. Regression test tin3277_self_pair_dominated_clock_is_not_promoted_to_push pins main's Conflict.
C-MF2 missing tracked.blake3 == remote_hash → isolated-second-cache clobber Clause added; it is now the load-bearing one. Negative test tin3277_remote_moved_since_baseline_still_records_conflict.
C-MF3 vetoed push rewrites baseline / loops / invisible Veto proven unreachable for the healed shape via a real plan → execute → state cycle (pushed == 1; the engine's own local_edit_inferred tick reaches Greater independently). If the remote does move mid-cycle, a second cycle test pins the outcome: pushed == 0, visible ConflictInfo + status = Conflict, baseline moves to the live local hash which disarms the predicate → settles into pre-fix recorded-conflict behavior, no loop. execute_plan now logs skipped pushes at info (previously in neither pushed nor errors).
C-MF4 tests mirror, don't pin Vacuous latched test replaced; hand-built post-push world replaced by the real cycle test (which now also asserts the latch is cleared). Two new negative/regression tests. Mutation-tested, below.
S-MF1 spoofed/colliding device identity → silent clobber Closed by the content clause; written_by explicitly demoted to corroboration in code and body.
S-MF2 devices.json self-heal-pushable past its Ed25519 gate Closed by the content clause: a stale local registry can only heal when the remote holds no revocation/enrollment we are missing. No hard carve-out — stated and argued explicitly in the body, since a carve-out would leave the 7 live stuck paths permanently unhealable.
S-MF3 KeepBoth / undo-bundle bypassed, no parked copy Documented as redundant under content identity (displaced bytes are our own tracked baseline; a sidecar on secrets/** would multiply ciphertext for zero recovery benefit) + every self-heal push now logs displaced_manifest at info for auditability.
S-MF4 secrets SSOT local-wins, unreliable gate, age non-determinism Acknowledged head-on in the body: every HM re-materialization will push (intended). The neo↔honey ciphertext parity check is promoted from a followup to a LANDING GATE. TIN-3278 interaction documented — a mis-keyed tracked makes the predicate fail closed (no heal), not fire wrongly.
Multi-host note Body now states plainly that this heals the self-pair only; peer-side accumulation and the record-only Conflict arm remain open TIN-3277 scope. No overclaim.

Mutation testing (the tests pin, they do not mirror)

  • Drop the content clause → tin3277_remote_moved_since_baseline_still_records_conflict FAILS: got Ok(Push { reason: LocalNewer }).
  • Drop it and restore v1's sequential structure / live-clock ordering → two failures, including tin3277_self_pair_dominated_clock_is_not_promoted_to_push: got Ok(Push { reason: LocalNewer }) — a verbatim reproduction of C-MF1's trace.
  • Unmutated: 8/8 pass.

sting gate

cargo fmt --all -- --check clean · cargo test -p tcfs-sync = 444 lib + 72 integration passed, 0 failed · cargo clippy -p tcfs-sync --all-targets -- -D warnings clean. Verbatim output in the PR body.

…content

Gate-round fixes for the TIN-3277 self-heal, all in compare_both_exist:

* tracked_is_exact: planning resolves `tracked` with the fuzzy suffix matcher
  StateCache::get_by_rel_path, while the push path re-resolves its baseline with
  the exact canonical key (StateCache::get) and only that exact entry is ever
  written back. Under TIN-3278's live key duplication those can be different
  records, so reconcile() now proves the two lookups return the SAME entry
  (pointer equality) and the self-heal predicate fails closed when they do not,
  with a warn! naming TIN-3278. No other classification decision reads the flag.

* Degenerate local content: refuse to publish an out-of-band rewrite that
  emptied the file (local size 0 while the baseline was non-empty), and for
  *.age paths refuse bytes that no longer carry an age container header (binary
  or armored). Keyed off the .age extension rather than a secrets/ prefix
  because secrets/.manifest.toml is plaintext TOML by design and is one of the
  live stuck paths. Residual limit documented: header-preserving truncation is
  not detectable from content alone.

* Skipped-push log: key the message off the engine's own verdict instead of
  asserting "remote moved since plan" — upload.skipped is also set for benign
  content dedup (UpToDate) and RemoteNewer.

* Plan-time audit fields renamed displaced_*_at_plan and documented as
  best-effort provenance: they record the remote as observed during
  classification, not what the commit actually displaced.

* Softened the doc comment's "provably": the clause compares our baseline
  against the manifest's self-declared file_hash, which validate_indexed_-
  manifest_binding does not bind to the chunk payload.

Tests 8 -> 13: emptied rewrite, non-ciphertext .age rewrite (with a
secrets/.manifest.toml control proving the guard is not a prefix rule), inexact
baseline decline, age-header helper across both container forms, and a second
structural double-tick regression that pins the else-if with the
content-identity clause satisfied (the shape mutation B needed two edits to
reach). *.age tests now use valid age bytes so each verdict still turns on the
clause it is testing.
The veto test drives a *.age path, so the new degenerate-content guard applies
to its local bytes too. Give both the baseline and the out-of-band rewrite a
real age header so the test exercises the execute-time veto rather than being
short-circuited by the container check.
@Jesssullivan

Copy link
Copy Markdown
Owner Author

Adversarial gate: v1 reject → v2 approve-with-must-fix ×2 → fixed at the gate

Round 1: reject. Two independent refutations found a real correctness defect (the predicate read a clock the TIN-2584 block had already ticked, so a strictly-dominated self-pair could be double-ticked into a push) and a real safety hole (the "self-pair" proof rested on the unauthenticated, client-set written_by string).

Round 2 (v2, fe69adb): both reviewers returned approve-with-must-fix, and both confirmed the two round-1 defects are genuinely closed — C-MF1 structurally (stored_ordering computed once pre-tick, else if exclusivity, ordering passed as a parameter) and C-MF2 by the content-identity clause tracked.blake3 == remote_hash, re-derived by hand against the code.

Round 3 (this head): all five v2 must-fixes applied. Two commits on top of fe69adb, still one file.

What changed at the gate

  1. "Prove the fix is not a no-op on its own target population" (the sharpest finding — the re-tick lives only in the regular-file comparator, and compare_both_exist routes symlinks away before any of the new code runs). Answered with live read-only evidence rather than argument: all 9 conflicted entries in neo's state cache are regular files — zero symlinks. lstat on the entry keys in ~/.local/share/tcfsd/state.json; no writes, no daemon interaction, deploy freeze respected. The 7 stuck-at-times_recorded = 4696 paths are the four secrets/api/*.age, secrets/infrastructure/tailscale_auth_key.age, secrets/.manifest.toml, and dotfiles/tcfs/devices.json. Bonus: secrets/.audit.log shows up twice under two key namespaces with different blake3 — the live TIN-3278 duplication, confirmed in the wild. Symlink parity remains a correctly-scoped follow-up.

  2. Degenerate-local-content guard — the residual half of S-MF4, previously answered with prose and a human landing gate. Now code: refuse when local_size == 0 while tracked.size > 0, and for *.age paths refuse bytes that no longer carry an age container header (binary or armored). Keyed off the .age extension, not a secrets/ prefix, because secrets/.manifest.toml is plaintext TOML by design and is one of the 7 live stuck paths — a prefix rule would have made this fix a permanent no-op there, which is the very failure mode finding 1 is about. Checked against the live corpus: the 5 stuck .age files carry the binary header, so the guard admits them. Residual limit documented, not hidden: header-preserving truncation is not detectable from content alone.

  3. Plan/execute tracked-state divergence — planning resolved tracked with the fuzzy suffix matcher while the push re-resolves its baseline with the exact canonical key, and only that exact entry is ever written back, so under TIN-3278 the safety proof could be evaluated against a record execute never touches. The claim that this "fails closed" was asserted, not proven. Now proven instead: reconcile() resolves the exact-key entry too and proves the two lookups return the same record (pointer equality into the one entry map); the predicate takes tracked_is_exact and refuses when it does not hold, with a warn! naming TIN-3278. Nothing else consumes the flag. The full-cycle test — which drives the real reconcile → execute_plan → state path — still heals, which is what proves this guard is not itself a no-op.

  4. The new skipped-push log stops asserting a veto. upload.skipped is also set for benign content dedup and RemoteNewer, so the message is now keyed off the engine's own upload.outcome.

  5. Audit-log honesty. The displaced_* fields are read at plan time while the commit happens after the engine's own fresh remote read, so they were never provably what was displaced. Renamed displaced_manifest_at_plan / displaced_hash_at_plan and labelled best-effort plan-time provenance in the code, the doc comment, and the S-MF3 row. Emitting from the commit path would mean plumbing through the engine's upload — noted as a follow-up rather than smuggled in.

Also from the non-blocking notes: the doc comment no longer claims the content clause provably prevents loss (validate_indexed_manifest_binding binds manifest bytes to manifest_hash, not the declared file_hash to the chunk payload), and the S-MF2 disposition is corrected from "closed" to narrowed, not closed — the content clause closes the clobber property, not the different claim that the Ed25519-signed device registry stays correctly reflected.

Evidence the new guards are load-bearing (mutation testing on sting)

Mutation Result
C — delete the tracked_is_exact clause ..._inexact_tracked_lookup_declines_self_heal FAILS: got Ok(Push { reason: LocalNewer })
D — delete both degenerate-content clauses two FAIL: ..._emptied_local_rewrite_is_not_pushed, ..._non_ciphertext_local_age_rewrite_is_not_pushed
E — restore v1's sequential structure with the content clause intact ..._same_content_republish_dominated_clock_is_not_double_ticked FAILS: got Ok(Push { reason: LocalNewer })

Mutation E closes a coverage gap the reviewers named: v2 had no test that pinned the else if independently of the content clause (mutation B needed both edits). It does now.

Verification (sting only — never neo)

cargo test -p tcfs-sync --lib tin327713 passed, 0 failed. Full crate: 449 lib tests + 13 integration suites, every one test result: ok, 0 failed anywhere. cargo fmt --all -- --check clean, cargo clippy -p tcfs-sync --all-targets -- -D warnings clean. Scratch worktree removed.

Still blocking, by design

The neo↔honey ciphertext parity check for the 7 stuck paths stays a landing gate, not a follow-up: age encryption is non-deterministic, so size parity is a weak proxy — decrypt-and-compare or recipient-set comparison is the real check. And the honest limitation is unchanged: this heals the self-pair only. The out-of-band writer runs on every host, so expect some conflict volume to migrate to peers rather than drop fleet-wide.

@Jesssullivan
Jesssullivan marked this pull request as ready for review July 30, 2026 04:43
Jesssullivan added a commit that referenced this pull request Jul 30, 2026
…(TIN-3278)

The primary state cache holds duplicate entries for the same logical file
under two key namespaces: an absolute canonicalized local path (the form
path_key() always produces on live get/set/mark_conflict today) and a bare
prefix-relative key left behind by an older keying scheme. Live evidence
on neo: secrets/.audit.log tracked at both
/Users/jess/tcfs/secrets/.audit.log (live, conflict record refreshes each
cycle) and /secrets/.audit.log (orphaned, times_recorded frozen at 1881
since 2026-07-07). Because get/set/remove/mark_conflict all re-derive the
key via path_key(), the orphan is never visited by any live read/write
path -- it just sits there forever, double-counted by raw-entry scans like
StateCache::conflicts() ("tcfs conflicts" reporting 9 vs the daemon's
per-cycle plan line reporting conflicts=8).

Root cause: path_key()'s identity fallback (return the input unchanged
when canonicalize fails) makes an unresolvable key indistinguishable from
an already-canonical one. A bare "/secrets/.audit.log" key's parent
("/secrets") does not exist at the filesystem root, so it canonicalizes to
itself and looks canonical even though it never independently resolves.

Fix: keep path_key() as the sole choke point for live writes (unchanged),
and add a load-time migration pass (migrate_duplicate_keys, wired into both
StateCache::open and StateCache::reload_from_disk):

  - resolve_key_on_disk() mirrors path_key() but returns None instead of
    falling back to identity, so migration can tell "canonical" apart from
    "unresolvable".
  - Pass 1 re-keys/merges any key whose independently-resolved form differs
    from its stored key.
  - Pass 2 handles orphans that cannot independently resolve at all: match
    against the other loaded keys using the same suffix convention
    get_by_rel_path() already uses for cross-host lookups. Only an
    unambiguous single match is merged; zero or 2+ candidates leave the
    orphan untouched rather than guess at a target.
  - merge_duplicate_sync_states() joins causal history and never drops a
    conflict record: vector clocks are merged pointwise-max via the
    existing VectorClock::merge (entry vclock, plus same-side joins of a
    merged conflict payload's local_vclock/remote_vclock), because
    partial_cmp_vc reads a dropped component as 0 and would let a peer
    falsely dominate -- classifying RemoteNewer and silently overwriting
    the dropped side's divergence. Scalar fields (blake3/size/mtime/
    chunk_count/remote_path/device_id) have no join, so they still come
    from the higher-last_synced side; if either side carries a conflict the
    merged entry keeps one (later detected_at as payload, times_recorded
    as the max of the two, remote_manifest_key backfilled from the other
    side when missing -- mirroring mark_conflict's preserve-on-missing).
  - Load-time migration is IN-MEMORY ONLY and never marks the cache dirty.
    StateCache::open is reached from read-only CLI paths that hold no
    cross-process StateFileLock (lock_explicit_state_cache only locks when
    a --state override is supplied), and Drop flushes on dirty -- so
    dirtying here would turn "tcfs conflicts" into an unlocked writer whose
    atomic-rename flush can clobber the daemon's concurrent locked update.
    The fold is free for readers; it becomes durable on the next flush by a
    legitimate lock-holding writer (the daemon dirties on any set /
    mark_conflict and flushes the whole map under its own lock). A
    debug_assert pins the invariant at construction.
  - Each merge is logged via tracing::warn! and recorded in a new
    StateCache::key_migration_log() accessor. Reload suppresses records
    already seen in-process, so an orphan that disk keeps reintroducing
    does not re-emit the same warn line every reconcile cycle.
  - One canonicalize attempt per key per call, computed up front and shared
    by both passes, so an unresolvable orphan no longer re-stats every
    candidate on each reload.

Known boundary (documented on migrate_duplicate_keys): the len() != 1
ambiguity guard does not cover a multi-root host where two registered roots
share a relative suffix but only one materializes the file; closing that
needs root-scoped matching, which state-cache keys carry no attribution for
today.

Also adds StateCache::conflicts_naming_unknown_devices(), a read-only scan
for TIN-3278 defect 2 (ghost device ids, e.g. a stale "yoga" string, named
inside a recorded conflict but absent from the caller-supplied known-device
set). Reports only -- never mutates the entry, never touches devices.json
(that migration is TIN-3277 / TIN-1417 territory).

No file-level overlap with the TIN-3277 fix (#576, reconcile.rs only);
trial merge between the two branches is clean. PR #565 (TIN-2864,
Codex-owned) also touches state.rs but in an unrelated region.

Tests (9 TIN-3278 tests, all run on sting):
  - tin3278_dedup_merge_on_load_collapses_duplicate_key_namespaces
  - tin3278_dedup_merge_never_drops_a_conflict_record
  - tin3278_merge_duplicate_sync_states_keeps_richer_conflict_and_max_times_recorded
  - tin3278_merge_joins_vector_clocks_instead_of_dropping_one_side
  - tin3278_merge_joins_conflict_side_clocks_same_side_only
  - tin3278_migration_does_not_dirty_or_rewrite_for_unlocked_readers
  - tin3278_migration_is_idempotent_on_second_load
  - tin3278_ambiguous_orphan_suffix_match_is_left_untouched
  - tin3278_conflicts_naming_unknown_devices_reports_without_mutating

sting: cargo fmt --all --check PASS, cargo test -p tcfs-sync PASS
(571 passed / 0 failed), cargo clippy -p tcfs-sync --all-targets
-D warnings PASS (0 warnings).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant