feat(compat): harden continuation ledger lifecycle - #1061
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| async def get_operation_by_idempotency_key( | ||
| self, idempotency_key: str, *, principal_id: str | ||
| ) -> CompatibilityPurchaseOperation | None: ... |
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
| ), 0) FROM adcp_compat_operations | ||
| """ | ||
| ).fetchone()[0] | ||
| if records > self.max_records or continuation_bytes + operation_bytes > self.max_bytes: |
There was a problem hiding this comment.
Medium: The global-byte ceiling runs on every write path, including complete/_transition for an already-executed seller mutation. If principal A saturates max_bytes, principal B completing an in_flight operation whose seller mutation already succeeded trips this check first (before the per-principal check at L919), rolls back, and forces B into ambiguous/reconcile — money spent, terminal result unrecorded. The per-principal quotas isolate new work but not completion of executed work: one noisy tenant can starve another's terminal writes. Recoverable via reconciliation, but worth deciding deliberately whether the global ceiling should gate completion of an already-committed mutation, or only new issuance/claims.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — hardening of the unreleased legacy purchase-continuation coordinator.
What I checked:
- No critical/high findings (row 1 clear). The reviewer confirmed the signature changes touch an unreleased coordinator (added post-beta.5, no tag contains f2f1dc0) — no published contract breaks, so no semver signal is required.
_derive_token(purchase_continuation.py:1540): HMAC-SHA256 over (fingerprint, binding_hash) with 256-bit key — deterministic/idempotent for identical inputs, unforgeable without the key. Safe.- Refresh CAS binds to original
task_idbefore mutating (purchase_continuation.py:843) — task-substitution rejected. Safe. - Credential/signed-URL persistence firewall is heavily and adversarially tested; no credentials landing in echoed metadata.
- gated_paths: false; author not on a no-auto-approve team; no prior decision.
- high_risk is true but all reasons are
(modified)files, and only ONE medium finding exists — row 5 requires a modified high-risk file AND a medium finding, but the single medium is onsqlite_continuation_store.py, which IS a modified high-risk file.
Re-check of row 5: The medium finding at sqlite_continuation_store.py:886 lands on a file listed in high_risk_reasons as (modified). That satisfies row 5 (high_risk true + modified entry + a medium finding) → escalate.
Correcting: this is an escalate, not approve.
Medium findings
- src/adcp/compat/sqlite_continuation_store.py:886 — Global byte-quota gates completion of already-executed mutations across tenants
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — prior finding resolved, no blocking or medium findings in the delta.
This is a subsequent review following an approve prior decision. The one prior medium finding (sqlite_continuation_store.py:886 — global byte-quota gating completion of already-executed mutations across tenants) has been addressed: the delta introduces a durable reserved_result_bytes reservation taken at CLAIMED→{IN_FLIGHT,AMBIGUOUS}, with ledger quota enforced only on that reserve transition. Later pending/terminal writes check only against the stored reservation, so a saturated global quota can no longer strand an already-executed mutation. Reservation accounting is atomic (rolled back on failure), the migration UPDATE is idempotent, and the reservation is released/re-taken across resume. Three new tests cover fail-closed pre-execution, survival of mid-flight quota tightening, and cross-worker reservation durability.
Decision-table walk: no critical/high findings (row 1 n/a); gated_paths false (row 2 n/a); no (deleted) reasons (row 3 n/a); no medium findings at all (rows 4/5/8 n/a); no author team gate (row 7 n/a). high_risk is true but all reasons are (modified) with no medium-or-higher finding, so the modification is presumed safe. Falls through to row 9 → approve.
1 participant
Summary
Why
The first continuation-ledger hardening left retry issuance, pending task progression, secret-bearing payloads, and unbounded storage insufficiently constrained. This change closes those lifecycle and durability gaps while retaining CAS fencing for reconciliations.
Validation
uv run pytest -q tests/test_purchase_continuation.py— 115 passeduv run pytest -q— 6,871 passed, 40 skipped, 9 deselected, 1 xfailed