[SashankBhamidi]

Closes #9.

Sessions can push now (to_push(), claim(), settle()), and conflicts get resolved with resolve() or resolve_conflicts().

One thing different from the issue: I didn't mark in-flight pushes unknown when the store opens, since another process opening the same file would do that to pushes that are still live. The next complete ingest settles them instead. Catch is, two processes syncing the same scope at once can send an edit twice. A version guard rejects a second change or delete, but a create whose id the server picks can end up twice, unless keys are unique.

Also:

• _scope_records() takes a states argument now, so a custom store needs a small change.
• An object's data fields are in one place now; they were copied by hand in seven.
• Fixed the design page: a discarded create gets deleted, not synced. My mistake from Local edits and conflict detection #8.

After Tobi's review:

• UpdateHolder refuses with PendingEditError if the holder has its own edit.
• A create in flight is only taken as ours if the server's copy holds what we sent.
• Taking over an id keeps the removed record till it's purged.
• Claims are numbered, and settle() only takes its own session's latest claim.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[read-the-docs-community]

Documentation build overview

📚 calendaring-sync | 🛠️ Build #35293594 | 📁 Comparing 3166a94 against latest (c8cf7c1)

  🔍 Preview build  

22 files changed · + 2 added · ± 20 modified

+ Added

• _modules/calendaring_sync/objects/push.html
• _modules/calendaring_sync/objects/resolution.html

± Modified

• contribute.html
• genindex.html
• py-modindex.html
• _modules/index.html
• explanation/design.html
• reference/errors.html
• reference/glossary.html
• reference/index.html
• reference/objects.html
• reference/replica.html
• and 10 more...

[tobixen]

Disclaimer: AI-generated review below the line, generated by Claude Opus 5.5 (Claude Code), from the prompts:

Sashank asks for a thorough review of #29
please post it as is, but with disclaimer

Findings 1-3 were reproduced against the PR head; the rest are from reading the code.

---

Review: #29 "Pushes and conflict resolution"

#29 (head 73cfb13, 2 commits, +1974/-267)

How I checked it: CI is green. I checked it out locally.
pytest src/calendaring_sync gives 692 passed, doctests included, both stores.
I read the full non-test diff and the test list. Findings 1-3 were reproduced with the script below.

Repro script (output: holder edit now b'N'; synced/other/b'THEIRS'/pushed; tombstone None)

from calendaring_sync import *
S = dict(fingerprint_scheme="s", media_type="text/plain")

def fresh():
    r = Replica(MemoryStore())
    with r.session("w", **S) as s:
        s.ingest(ChangeSet("w", None, "t1", {"b": Changed("v1", "fb", b"B", logical_key="u")}, full_listing=True))
    return r

# 1. UpdateHolder clobbers holder's pending edit
r = fresh()
b = r.find_record("w", "b").local_key
r.change(b, b"B local", fingerprint="fbl", logical_key="u")
k = r.create("w", b"N", fingerprint="fn", logical_key="u", object_id="n")
with r.session("w", **S) as s:
    s.ingest(ChangeSet("w", "t1", "t2", {}))
    c = s.claim(k); s.settle(c, LogicalKeyTaken("b"))
r.resolve(r.record(k), UpdateHolder())
print("1. holder edit now:", r.record(b).edit.content, "(was b'B local')")

# 2. lost answer + unique key: holder taken as our create, local content dropped
r = Replica(MemoryStore())
with r.session("w", **S) as s:
    s.ingest(ChangeSet("w", None, "t1", {}, full_listing=True))
k = r.create("w", b"MINE", fingerprint="fm", logical_key="u")
with r.session("w", unique_logical_key=True, **S) as s:
    s.ingest(ChangeSet("w", "t1", "t2", {})); s.settle(s.claim(k), Unknown())
with r.session("w", unique_logical_key=True, **S) as s:
    s.ingest(ChangeSet("w", "t2", "t3", {"other": Changed("v9", "fo", b"THEIRS", logical_key="u")}))
rec = r.record(k)
print("2.", rec.state, rec.object_id, rec.content, [e.kind.value for e in r.read_feed()][-1:])

# 3. claim(object_id=) deletes the unpurged tombstone instead of reusing it
r = fresh()
b = r.find_record("w", "b").local_key
with r.session("w", **S) as s:
    s.ingest(ChangeSet("w", "t1", "t2", {"b": Removed()}))
k = r.create("w", b"N", fingerprint="fn")
with r.session("w", **S) as s:
    s.ingest(ChangeSet("w", "t2", "t3", {})); s.claim(k, object_id="b")
try:
    print("3. tombstone:", r.record(b))
except Exception as e:
    print("3. tombstone gone before purge:", type(e).__name__, e)

Overall: solid, carefully layered work. The tests are thorough, the _data.py dedup is a real
cleanup, and the design doc changes go with the code. Going against the issue
on "unknown at store open" is reasoned and disclosed. Findings follow, most severe first.

1. UpdateHolder silently overwrites the holder's own pending edit (data loss) — CONFIRMED

replica.py:1387: _changed_to(holder, ...) replaces holder.edit without looking at it.
_editable() refuses only conflict, in-flight and removed holders. A holder with a
pending local change passes, and that change is gone without a trace.

Repro: synced b (key u) gets a local change. A create with key u is refused with
LogicalKeyTaken("b"), then resolved with resolve(..., UpdateHolder()). Now
b.edit.content == b"N" and the local change to b is lost.

test_a_holder_that_takes_no_edit covers conflict and in-flight holders only.
Suggest: refuse a holder that holds an edit (new error, or reuse EditRefusedError), or
document that it wins. Refusing is safer.

Related: _holder() falls back to find_by_logical_key, which also matches records whose
edit carries the key. Another local, unpushed create with the same key can therefore be
picked as "the object holding the key", although the server never refused against it.
It should probably consider synced keys only (record.logical_key), not edit.logical_key.

2. Lost answer + unique_logical_key: the refusing holder is taken for our create — CONFIRMED

_classify.py:135 with replica.py:668. A server-id create, with a key, is claimed and
the answer is lost (Unknown). The server had in fact refused it with LogicalKeyTaken.
The next ingest brings in the existing holder, which has the same key and is new to the
store. The holder is matched to our in-flight create, and in_flight turns the result
into UNCHANGED "whatever the server made of its content". The record becomes synced to
the other object's content, the feed says pushed, and the local create's content is
gone without a conflict.

The design does say the key only counts "if the adapter says that key is unique", but
it says nothing about this case. Suggest: treat it as our write only when the fingerprint
matches the edit's, and otherwise open BOTH_CREATED (or LOGICAL_KEY_TAKEN). The cost
is spurious conflicts on servers that rewrite content, which seems the right side to err on.
The same reasoning applies, more weakly, to a create at its own client-chosen id.

3. claim(object_id=) / Applied(object_id) delete an unpurged removed record — CONFIRMED

replica.py:1128, :1161 via _free_object_id. create() reuses a removed record that
holds the id (it keeps the local key and moves last_entry over). The new paths
delete it instead, before purge(). That breaks the purge contract ("removed records
whose last feed entry is at or below through"). A feed reader still behind the REMOVED
entry then finds record(key) is None.
Suggest: share one way of taking over an id. Either reuse the tombstone as create()
does, or keep the tombstone and let the id move.

The same holds for resolve() returning None (replica.py:1395): it writes
CONFLICT_RESOLVED for a local key and then deletes the record at once. FeedEntry.local_key's
docstring (objects/feed.py:47) only allows for purged records. Update the doc, or keep a
tombstone.

4. The design doc understates the double-send risk

design.rst:140: "the second write is rejected where the adapter guards it with the
object's version, and nothing is lost either way". Creates have no version. Two processes
syncing one scope can each send a create whose server picks the id, giving two server
objects. The guard is a precondition (If-None-Match: *), and it exists only for
client-chosen ids. This is no data loss, but it does duplicate, and the PR text claims the
opposite. Please spell out the create case.

5. A stale settle can override another process's live claim

There's no claim identity (token or generation). In _settle, record.edit == claimed.edit
is the only check. Say process A's push was reset by B's ingest, and B has re-claimed it,
so it's IN_FLIGHT again. A's late Rejected (replica.py:1151) then sets it PENDING while
B's push is in flight, so a third claim can go out. A's late Gone or LogicalKeyTaken
opens a conflict under B. The next ingest heals it, but it's avoidable: store a claim
counter or claimed_at and compare it. A narrow window, so this is a suggestion, not a
blocker.

6. Smaller things

• Session.settle (replica.py:434) doesn't check that this session claimed the
  record. Any IN_FLIGHT record of the scope is accepted once the session has claimed
  anything. Tracking claimed local keys in the session would be cheap.
• settle() runs _check_outcome before the closed and state checks, so a closed
  session raises ValueError rather than SessionError for a bad outcome. That's a nit,
  but it's inconsistent with claim().
• Schema: conflict_holder and the new index were added, but SCHEMA_VERSION is still 1
  (constants.py:12). A store created by main fails on the missing column. That's fine
  if nothing is released (no tags yet), but worth a sentence in the PR, or a bump if dev
  stores matter.
• resolve()/resolve_conflicts() don't refuse while a session for the scope is
  open in this process. The issue says "runs outside any sync". It seems harmless given
  per-call transactions, but either enforce it or drop the wording.
• A Merge with a new UID after a LOGICAL_KEY_TAKEN on a record whose id was set at
  claim(object_id=) keeps the old id. Then claim(object_id=new) raises
  "already has object id". Is that intended for adapters that derive the URL from the UID?
• The issue's checklist boxes are all unticked. Most look covered by tests
  (test_each_push_is_claimed_and_settled_once, _SETTLED_LATE, TestResolutions, ...),
  so tick them or say what's left.

Verdict

Request changes for 1 and 3, which lose user data or break a documented contract. 2 needs
at least a design decision recorded. The rest is optional.

[SashankBhamidi]

Thanks for running this @tobixen.

Fixed 1 to 5, and from 6 settle() only takes its own session's claims now.
For 2 I dropped the in-flight shortcut, so a create is ours only if it holds what we sent.
For 3 the removed record stays till it's purged.
The rest I've kept as is.

The schema is edited in place till the first release, and I'll tick the boxes after merge.

[SashankBhamidi]

Oops, forgot to hit enter on the push :)

[tobixen]

Disclaimer: AI-generated text below the line, generated by Claude Opus 5.5 (Claude Code), from the prompts:

Plese (sic) review the latest activity on PR#29 [https://github.com//pull/29, the follow-up commit 3166a94 to my earlier review]
Make a draft for a comment

---

Thanks @SashankBhamidi, I went through 3166a94 and it addresses findings 1–5 and the settle() part of 6 well:

1. UpdateHolder now refuses a holder with a pending edit (PendingEditError), and _holder() only matches on a synced key, so an unpushed create sharing the key can't be picked.
2. Dropping the in-flight shortcut and requiring same_data against the sent edit is the safer side. The cost is a conflict after a lost answer when the server rewrites content on create, which seems acceptable, and the design doc now says what happens.
3. Keeping the removed record and only clearing its object_id preserves the purge contract, and the FeedEntry.local_key docstring now covers the deleted-create case.
4. The double-send paragraph is accurate now.
5. The claims counter is a clean fix for stale settles.

The test suite passes locally at 3166a94.

Remaining things, none blocking:

• settle() still runs _check_outcome before the closed check, so an ended session raises ValueError instead of SessionError when the outcome is also wrong. Just a nit.
• My question on Merge with a new UID after a LOGICAL_KEY_TAKEN, on a record that got its id from claim(object_id=): the record keeps the old id, so a later claim(object_id=new) raises "already has object id". Intended, or something for a follow-up issue?
• Editing the schema in place until the first release is fine with me.

Looks good to merge from my side once the boxes are ticked.

Addressed in 3166a94, see #29 (comment)

[SashankBhamidi]

Thanks, Tobi!

Both are intended: claim() and ingest() also check their arguments first, and the href comes from a UUID, so a new UID keeps it.