fix(compat): preserve continuation replay fences - #1062
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
| if legacy is not None: | ||
| retired_legacy = conn.execute( | ||
| "SELECT 1 FROM adcp_compat_issuance_tombstones " | ||
| "WHERE issuance_fingerprint IS NULL AND principal_id = ? " |
There was a problem hiding this comment.
Medium: This guard never fires. Every tombstone is inserted in _purge_candidate_batch only for rows where issuance_fingerprint is not None, so issuance_fingerprint is always non-NULL on tombstones. This query filters issuance_fingerprint IS NULL, which therefore matches zero rows — the or retired_legacy is not None arm is dead, and the legacy_equivalence_hash written onto every tombstone is write-only. If the intent is to fence a mixed-version pre-fingerprint reissue against a purged fingerprinted authorization (the exact rolling-upgrade case this PR claims to harden), it does not engage — drop the issuance_fingerprint IS NULL predicate. If the intent is only to fence purged legacy rows — which never occur, since legacy rows are deliberately never purged — then the query and the stored hash are dead weight. Either way the code and intent diverge.
| END | ||
| """, | ||
| """ | ||
| CREATE TRIGGER IF NOT EXISTS adcp_compat_issuance_tombstones_delete_guard |
There was a problem hiding this comment.
Medium: Tombstones are permanent (this guard) and count against both global and per-principal record/byte quotas (_enforce_ledger_quota), and purge replaces each removed continuation with one — never fewer. A long-lived, high-throughput principal accumulates tombstones monotonically with no in-code pruning path, so max_records_per_principal / max_bytes_per_principal is eventually exhausted, after which every new continuation for that principal fails closed with STORE_QUOTA_EXCEEDED. Fail-closed is the right posture for a replay fence, but the availability cliff is real and the docs' "archive them in an application-owned durable idempotency store" is the only escape — no hook is provided. Worth a follow-up: expose an operator-facing archival/prune path, or document the sizing math so deployments can bound the horizon.
There was a problem hiding this comment.
Ladon verdict: Comment (human reviewer recommended)
Comment — 2 medium findings surfaced for human review.
This is a follow-up hardening of the SQLite legacy-continuation ledger. Core replay-fence logic (atomic-migration race, fail-closed triggers, claim races, expiry-safe purge, issuance tombstones, per-principal byte quotas) is sound and well-tested. No critical/high findings.
Medium findings
- src/adcp/compat/sqlite_continuation_store.py:555 — retired_legacy tombstone guard never fires; equivalence hash is write-only (dead guard)
- src/adcp/compat/sqlite_continuation_store.py:157 — Permanent tombstones grow unbounded and eventually exhaust ledger quota
Why not approve: The high_risk flag is set because src/adcp/compat/sqlite_continuation_store.py (modified) matches src/adcp/compat/**, and this run has medium findings. Decision-table row 5 (high_risk + (modified) + a medium finding) → escalate; however both mediums are quota-management/dead-code hardening concerns on durable state, not schema/data-loss/infra categories. Re-checking the table: row 1 no (no critical/high), row 2 no (gated_paths false), row 3 no (no deletion), row 4 no (neither medium is data-loss/schema/infra category), row 5 DOES fire (high_risk true + (modified) + medium finding present).
Correcting: row 5 fires → escalate.
Medium findings
- src/adcp/compat/sqlite_continuation_store.py:555 — retired_legacy tombstone guard never fires; equivalence hash is write-only
- src/adcp/compat/sqlite_continuation_store.py:157 — Permanent tombstones grow unbounded and eventually exhaust ledger quota
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — hardening follow-up resolves both prior Medium findings with no new blocking issues.
What I checked:
- Both prior Medium findings are resolved: (1) the write-only
legacy_equivalence_hashcolumn is removed via an atomic in-migration rename/copy/drop (sqlite_continuation_store.py L179-L209) with the fence now keying on token_hash + (principal_id, issuance_fingerprint) directly; (2) permanent tombstones now count against global and per-principal record/byte quotas (L910-L911, L1071-L1081, L1136-L1147) with exhaustion failing closed as a documented, intentional availability boundary. - New purge logic (insert-before-delete, expires_at<=now gating, pre-fingerprint retention) is correct and well-tested; full suite reported green.
- Change is confined to the compat continuation store, docs, and tests — no public-API/wire-shape surface change, no generated-code hand-edit, no type-layering violation.
Decision path: No critical/high findings (row 1 n/a). gated_paths is false (row 2 n/a). high_risk is true but only via a (modified) compat file, and the current run surfaced zero findings — so rows 3, 4, and 5 do not fire. Prior decision was comment, not escalate, and no new medium+ findings exist (row 6 n/a). No no-auto-approve team match (row 7 n/a). Zero medium findings (row 8 n/a). Falls through to row 9 → approve. Net improvement over the prior review.
1 participant
Summary
target_bindingWhy
The continuation ledger could delete a terminal row while its token was still valid, then deterministically reissue the same token and execute another purchase under a new idempotency key. Per-principal quotas also omitted routing bindings, allowing one tenant to consume global storage beyond its configured share. This follow-up preserves replay fences across payload cleanup and rolling upgrades while making tenant accounting symmetric.
Validation
uv run pytest -q tests/test_purchase_continuation.py— 120 passeduv run pytest -q— 6,875 passed, 40 skipped, 9 deselected, 1 xfailed