mirror of
https://github.com/kuhyx/screen-locker.git
synced 2026-11-10 19:07:16 +01:00
Apply sync tombstones across the device union, not per device log
The previous commit dropped tombstoned records while reading each device log, which was not enough: _merge_device_records unions across devices keeping the highest HLC, so a record tombstoned on one device and still live on another was simply absent from one dict and won the merge from the other -- ingested exactly as before. Collect tombstoned ids across every device log, then suppress by id after the union. crdt-sync makes deletion monotonic inside its own merge for this reason; this reader rolls its own merge, so it has to honour the same rule itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018M5gKyYzxWG6QkCaj1wWfk
This commit is contained in:
parent
590f606856
commit
cdee896d43
@ -55,6 +55,27 @@ def _session_records(log_json: str) -> dict[str, tuple[dict, Hlc]]:
|
||||
return _records_matching(log_json, _is_session_payload)
|
||||
|
||||
|
||||
def _tombstoned_ids(log_json: str) -> set[str]:
|
||||
"""Return the ids this device log marks deleted.
|
||||
|
||||
Collected separately from :func:`_records_matching` because suppression has
|
||||
to be applied across the whole device union, not per file: one device
|
||||
tombstoning a record while another still holds it live must delete it, and
|
||||
a per-file skip would just let the live copy win the merge.
|
||||
|
||||
Raises the same decode errors as :func:`_records_matching`.
|
||||
"""
|
||||
raw = json.loads(log_json)
|
||||
if not isinstance(raw, dict):
|
||||
msg = f"top-level sync payload is not a JSON object: {raw!r}"
|
||||
raise TypeError(msg)
|
||||
return {
|
||||
record.id
|
||||
for record in (Record.from_dict(data) for data in raw.values())
|
||||
if record.deleted
|
||||
}
|
||||
|
||||
|
||||
def _records_matching(
|
||||
log_json: str, predicate: Callable[[object], bool]
|
||||
) -> dict[str, tuple[dict, Hlc]]:
|
||||
|
||||
@ -36,6 +36,7 @@ from screen_locker._sync_records import (
|
||||
_manual_records,
|
||||
_records_matching,
|
||||
_session_records,
|
||||
_tombstoned_ids,
|
||||
)
|
||||
from screen_locker._sync_retry import with_sync_retry
|
||||
|
||||
@ -115,6 +116,7 @@ def _merge_device_records(
|
||||
return {}
|
||||
|
||||
merged: dict[str, tuple[dict, Hlc]] = {}
|
||||
tombstoned: set[str] = set()
|
||||
for device in devices:
|
||||
path = f"{_DEVICES_PREFIX}/{device}/log.json"
|
||||
try:
|
||||
@ -132,6 +134,7 @@ def _merge_device_records(
|
||||
continue
|
||||
try:
|
||||
records = extract(text)
|
||||
tombstoned |= _tombstoned_ids(text)
|
||||
except (ValueError, KeyError, TypeError) as exc:
|
||||
_logger.warning("Corrupt sync data at %s: %s", path, exc)
|
||||
continue
|
||||
@ -140,6 +143,19 @@ def _merge_device_records(
|
||||
if existing is None or existing[1] < hlc:
|
||||
merged[rid] = (payload, hlc)
|
||||
|
||||
# Applied after the union, not per device: one device deleting a record
|
||||
# while another still holds it live must delete it. crdt-sync's own merge
|
||||
# makes deletion monotonic for exactly this reason, and this reader rolls
|
||||
# its own merge, so it has to honour the same rule itself.
|
||||
for rid in tombstoned & merged.keys():
|
||||
_logger.info(
|
||||
"Synced record %s is tombstoned on another device — dropping it, "
|
||||
"so a deleted %s stops counting here too",
|
||||
rid,
|
||||
what,
|
||||
)
|
||||
del merged[rid]
|
||||
|
||||
return merged
|
||||
|
||||
|
||||
|
||||
@ -85,6 +85,36 @@ class TestPullAllManualRecords:
|
||||
result = _workout_sync.pull_all_manual_records()
|
||||
assert [rid for rid, _ in result] == ["manual:kept"]
|
||||
|
||||
def test_a_tombstone_on_one_device_beats_a_live_copy_on_another(self) -> None:
|
||||
"""Suppression must apply across the union, not per device log.
|
||||
|
||||
The per-file skip alone was not enough: the tombstone was merely
|
||||
absent from one device's dict while the live copy won the merge from
|
||||
the other, so the record was ingested exactly as before.
|
||||
"""
|
||||
_workout_sync.SYNC_TOKEN_FILE.write_text("tok")
|
||||
hlc = Hlc(wall_time_ms=1000, counter=0, node_id="phone")
|
||||
tombstoned = Record(
|
||||
id="manual:dup",
|
||||
fields={"payload": (_manual_payload(), hlc)},
|
||||
deleted=True,
|
||||
deleted_hlc=hlc,
|
||||
).to_dict()
|
||||
client = _multi_device_client(
|
||||
{
|
||||
"phone": json.dumps({"x": tombstoned}),
|
||||
"pc": json.dumps(
|
||||
{
|
||||
"y": _manual_record_dict("manual:dup", _manual_payload()),
|
||||
"z": _manual_record_dict("manual:keep", _manual_payload()),
|
||||
}
|
||||
),
|
||||
}
|
||||
)
|
||||
with patch.object(_sync_client, "GitHubSyncClient", return_value=client):
|
||||
result = _workout_sync.pull_all_manual_records()
|
||||
assert [rid for rid, _ in result] == ["manual:keep"]
|
||||
|
||||
def test_skips_missing_and_corrupt_device_logs(self) -> None:
|
||||
"""Skips missing and corrupt device logs."""
|
||||
_workout_sync.SYNC_TOKEN_FILE.write_text("tok")
|
||||
|
||||
Loading…
Reference in New Issue
Block a user