fix(compat): harden legacy purchase continuation recovery - #1060
Merged
Conversation
|
|
||
| async def get_operation( | ||
| self, operation_id: str, *, principal_id: str | ||
| ) -> CompatibilityPurchaseOperation | None: ... |
| self, operation: CompatibilityPurchaseOperation | ||
| ) -> CompatibilityPurchaseOperation: | ||
| """CAS ``IN_FLIGHT`` to ``AMBIGUOUS`` using the operation revision.""" | ||
| ... |
| except asyncio.CancelledError: | ||
| # Let the durable CAS settle so cancellation cannot strand an unknown | ||
| # transition between seller completion and local persistence. | ||
| await task |
| task.cancel() | ||
| store.release.set() | ||
| with pytest.raises(asyncio.CancelledError): | ||
| await task |
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — hardening follow-up to #1059 on the legacy purchase continuation coordinator; no blocking findings.
Checked against the decision table top-down:
- No no-auto-approve team match (HARD RULE clear).
- gated_paths: false, so row 2 does not apply despite review_decision REVIEW_REQUIRED.
- high_risk: true, but both reasons are
(modified)files with no medium-or-higher finding — no escalation (rows 3–5 do not fire). - No prior decision; no sticky escalation.
- Zero medium findings, so row 8 does not fire.
- Falls through to row 9 → approve.
What the reviewer verified: revision-fenced principal-scoped recovery API, cancellation-safe reservation/completion, off-loop sync callbacks, exact source-version result validation before durable replay, buyer-visible pricing projection binding, and SQLite parent/path/ownership/purge hardening. CAS/migration/recovery flows consistent against base and covered by 79 continuation tests. The one semver-prefix note is low and defused (module unreleased).
Nits (non-blocking):
- src/adcp/compat/purchase_continuation.py:476 — new required params added to public exports without
!marker (module unreleased, no live break). - Migration fail-closed gate and lexical-path symlink rejection confirmed correct.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Root cause
The initial continuation ledger could durably reserve a seller mutation without leaving a public, fenced recovery path. It also treated arbitrary executor mappings as success, retained only seller-observed pricing rather than the buyer-visible projection, inferred replay safety too broadly, and reopened SQLite through a pathname whose parent was not validated.
Migration behavior
Pre-hardening ledgers migrate in place. Missing execution snapshots can be adopted only by an exact retry, which increments the operation revision. Because old ledgers did not retain the buyer-visible pricing subset or a verified 3.x replay guarantee, unresolved rows remain non-executable; already-terminal results still replay and authoritative reconciliation can still recover an applied result.
Validation
uv run pytest tests/ -q— 6,835 passed, 40 skipped, 9 deselected, 1 xfailedFollow-up to #1059.