fix(adoption): re-check donor growth at retire time, not just at export
Closes the TOCTOU window flagged in review on #93369 (merged via #93430): the divergence guard compared EXPORT-TIME message counts, but another backend can append donor messages between the export snapshot and the retire loop — that growth would be stamped behind the non-recoverable adopted_by_profile archive, the exact H2 class the guard exists to prevent, just via a narrower race. The retire loop now re-reads live donor vs local counts immediately before end_session and leaves the donor unretired (donor_retired=False, warn-logged) on any donor-ahead signal; the next resume's export-time guard then handles the divergence normally. Equal-count CONTENT divergence (donor rewind+rewrite) remains invisible to count comparison — documented as accepted: bytes stay in the donor store either way. New red-first-verified regression simulates the exact race by appending to the donor from inside an export_session_lineage wrapper. adoption+ownership suites: 25 passed; ruff clean.
This commit is contained in:
@@ -394,6 +394,26 @@ class SessionPortabilityMixin:
|
||||
if not seg_id:
|
||||
continue
|
||||
try:
|
||||
# TOCTOU close-out: the guard above compared EXPORT-TIME
|
||||
# counts, but another backend can append donor messages
|
||||
# between export and this loop. Re-read both stores right
|
||||
# before stamping; a donor-ahead signal here skips the
|
||||
# stamp so growth never lands behind a non-recoverable
|
||||
# archive. (Count comparison cannot see equal-count
|
||||
# CONTENT divergence — e.g. a donor rewind+rewrite; that
|
||||
# residual case is accepted: bytes stay in the donor
|
||||
# store either way, only reachability differs.)
|
||||
donor_now = len(donor_db.get_messages(seg_id))
|
||||
local_now = len(self.get_messages(seg_id))
|
||||
if donor_now > local_now:
|
||||
retire_ok = False
|
||||
logger.warning(
|
||||
"adoption divergence at retire time: donor "
|
||||
"segment %s grew to %d messages (local %d) — "
|
||||
"leaving donor unretired",
|
||||
seg_id, donor_now, local_now,
|
||||
)
|
||||
continue
|
||||
# First end_reason wins in end_session(); reopen first so
|
||||
# the adoption boundary is stamped even on ended segments
|
||||
# (e.g. 'compression' parents).
|
||||
|
||||
@@ -389,3 +389,35 @@ def test_donor_retired_reports_false_on_retirement_failure(stores, monkeypatch):
|
||||
assert result["adopted"] is True
|
||||
assert result["donor_retired"] is False
|
||||
assert not default_db.get_session(STRANDED_ID)["archived"]
|
||||
|
||||
|
||||
def test_donor_growth_between_export_and_retire_blocks_retirement(stores, monkeypatch):
|
||||
"""TOCTOU close-out (review on #93369): messages appended to the donor
|
||||
AFTER export but BEFORE retirement must block the non-recoverable
|
||||
stamp — the retire loop re-reads live counts, not export-time ones."""
|
||||
default_db, profile_db = stores
|
||||
_seed_stranded(default_db)
|
||||
|
||||
real_export = default_db.export_session_lineage
|
||||
|
||||
def _export_then_append(session_id):
|
||||
payload = real_export(session_id)
|
||||
# Another backend appends AFTER the export snapshot is taken.
|
||||
default_db.append_message(STRANDED_ID, "user", "raced question")
|
||||
default_db.append_message(STRANDED_ID, "assistant", "raced answer")
|
||||
return payload
|
||||
|
||||
monkeypatch.setattr(default_db, "export_session_lineage", _export_then_append)
|
||||
result = profile_db.adopt_session_lineage_from(default_db, STRANDED_ID)
|
||||
|
||||
# Adoption itself still serves (profile copy has the snapshot)...
|
||||
assert result["adopted"] is True
|
||||
# ...but the grown donor is NOT stamped behind a non-recoverable archive.
|
||||
assert result["donor_retired"] is False
|
||||
donor = default_db.get_session(STRANDED_ID)
|
||||
assert not donor["archived"], "raced donor growth must stay reachable"
|
||||
assert len(default_db.get_messages(STRANDED_ID)) == 8
|
||||
# The next resume retries: donor now ahead → export-time guard catches it.
|
||||
second = profile_db.adopt_session_lineage_from(default_db, STRANDED_ID)
|
||||
assert second["donor_retired"] is False
|
||||
assert not default_db.get_session(STRANDED_ID)["archived"]
|
||||
|
||||
Reference in New Issue
Block a user