Repository navigation
feat(reporting): persist reporting-subscription engine state through a CAS store - #1722
Conversation
…a CAS store The reporting-subscription engine kept its state in a process-local bag, so a restart or a second worker lost grants, losses, capacity slots, exceptions and version snapshots, and with them what the seller owes. ReportingSubscriptionStateStore persists one JSON document per principal, written by compare-and-swap. The engine loads the document, applies a change and writes it back; on a conflict it reloads and reapplies the change, and it raises ReportingSubscriptionStateConflictError when the document keeps changing. An unchanged document is not rewritten. InMemoryReportingSubscriptionStore implements the protocol for tests and single workers. encode_principal_state and decode_principal_state define the versioned document; version configurations and destination snapshots are never pruned, and exceptions are dropped status_retention_days after they close. A PostgreSQL implementation follows in adcp-reporting-storage. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| "unauthorized_at": iso_z(lost), | ||
| "count": count, | ||
| } | ||
| for (sid, account_id, lost), count in sorted(principal.withheld.items()) |
There was a problem hiding this comment.
Medium: withheld is serialized but never pruned, unlike exclusions (which prune_closed_before drops after status_retention_days). Every grant/revoke cycle adds one (subscription_id, account_id, unauthorized_at) entry that lives forever, and _mutate rewrites the entire document on every CAS. On the durable path this PR builds for, a long-lived principal's document grows unbounded and the full row is re-serialized and re-written on each mutation. Losses past retention stop appearing in evaluation updates but their withheld entries are never reclaimed. Consider pruning aged withheld entries alongside exceptions, or key them so stale losses fall out.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean, well-tested refactor of the reporting-subscription engine to a CAS-backed JSON document store.
Decision trace (top-down):
- Rows 1 (critical/high): no Critical/High findings. ✓
- Row 2 (gated_paths): false. ✓
- Row 3 (deletions in high-risk): high_risk is true but reasons are both
(modified), no(deleted). ✓ - Row 4 (medium + data-loss/schema/infra category): the single medium is an unbounded-growth concern on the engine code, not a schema/infra/data-loss-category finding. ✓
- Row 5 (high_risk modified + any medium): the high-risk-path matches are
src/adcp/server/__init__.pyandsrc/adcp/server/principal.py(both modified); the one medium finding is onsrc/adcp/reporting/subscriptions/engine.py:440, NOT on a high-risk-path file. No medium exists on the modified high-risk files, so row 5 does not fire. ✓ - Row 6 (sticky escalate): no prior decision. ✓
- Row 7 (no-auto-approve team): no match. ✓
- Row 8 (≥3 medium): only 1 medium finding. ✓
- Row 9: approve.
What was checked: CAS _mutate loop fails closed (reload+reapply ×8 then raises conflict rather than silent drop; unchanged-doc short-circuit tested); destinations stored as JSON dicts still resolve via SubscriptionDestination.coerce (end-to-end tested); InMemory store internal break is not a semver issue (adcp.reporting not in public re-export tree, ships in no release); semver signal correct (feat:, additive exports); test plan complete (87 passed).
Medium findings:
- src/adcp/reporting/subscriptions/engine.py:440 —
withheldentries persisted but never pruned; whole-document rewrite per CAS can grow unbounded on the durable path. Non-blocking (Postgres impl is a separate PR); worth a human glance but within the 1–2-medium approve threshold.
Medium findings
- src/adcp/reporting/subscriptions/engine.py:440 — withheld entries persisted but never pruned; whole-document rewrite per CAS grows unbounded on durable path
Part of #1695, item 6 (durable storage), SDK side.
The reporting-subscription engine kept its state in a process-local bag. A restart or a second worker lost grants and losses, timezone changes,
max_accountsslots, exceptions and per-version destination snapshots, and with them what the seller owes.ReportingSubscriptionStateStoreprotocol: one JSON document per principal, written by compare-and-swap (load/compare_and_swap). The engine loads the document, applies a change and writes it back. On a conflict it reloads and reapplies the change, up to 8 attempts, then raisesReportingSubscriptionStateConflictError. An unchanged document isn't rewritten.InMemoryReportingSubscriptionStore: now implements the protocol, for tests and single workers.encode_principal_state/decode_principal_statedefine it. Version configurations and destination snapshots are never pruned, soprior_versionsstay answerable while obligations cite them. Exceptions are droppedstatus_retention_daysafter they close, as the spec allows (TS parity, adcp-client#3509).principal_record_to_document/principal_record_from_document: a JSON codec forPrincipalRecord, so a durablePrincipalRecordStorecan persist the subscription readback.docs/reliable-reporting-service.md§Engine state.The subscription module isn't in any release yet, so changing
InMemoryReportingSubscriptionStore's internals breaks nothing shipped.Follow-ups: the PostgreSQL implementations of both stores ship in a separate
adcp-reporting-storagePR, so a storage-only commit releases only storage. A test-only PR then adds the PostgreSQL conformance test.Tests: new
tests/test_reporting_subscription_state_store.pycovers:With the engine, principal and consumer suites: 87 passed.
Refs #1695
🤖 Generated with Claude Code