Skip to content

feat(reporting): sweep abandoned uploads and prune expired row sets - #1469

Merged
bokelley merged 2 commits into
mainfrom
claude/issue-1457-row-retention
Oct 11, 2026
Merged

bokelley merged 2 commits into
mainfrom
claude/issue-1457-row-retention

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Part of #1457 (Stage 2 of adcontextprotocol/adcp#7996, sections 4.2 and 4.3). Builds on the merged object-write path (#1467) and providers (#1468).

Change

  • sweep_row_write_intents resolves abandoned object uploads. An expired open intent moves to sweeping under the account lock so no writer can commit against it; then only the objects that upload created are deleted, at their recorded versions. An interrupted sweep resumes. Only intents of bindings this host configures are considered, so an unconfigured binding cannot starve the rest.
  • prune_expired_row_sets retires a period's row bytes in an ordered, resumable sequence: rows stop being served first (pruning), external object versions are deleted next, and manifests and bodies go last, so a failed delete leaves every locator intact. Expired rows read like an unknown revision, never as an integrity failure. A period qualifies once max(status_retention_days, ledger_record_retention_days) has passed since the latest of its period end, newest revision, newest adjustment and newest row set recorded (feat(reporting): define superseded snapshot content retention adcp#8100), measured on the database clock, and no materializer work is pending. Eligibility is re-checked under the account lock in phase 1, so a late revision cannot lose its rows to a stale candidate list; phases 2 and 3 touch only the row sets phase 1 marked.
  • Object deletes use the lifetime lock from feat(reporting): write row chunks to object storage with fenced uploads #1467 (delete_unreferenced): the lock, the reference check and the provider delete happen in one transaction per object, so a concurrent commit adopting an object either already references it (kept) or finds it gone and fails closed. An object a live row set of another obligation still references is never deleted.

Python keeps revision headers and their records until the shared Core layout (persistence Stage 3); full period retirement follows in a later PR.

Locks

The sweep claim and pruning phases 1 and 3 take the Python account lock (hashtext('adcp.reporting:' || account_id)), which guards Python-only Core tables; the object lock (identical to the JS SDK's key text) guards the delete. The full audit table is in #1467.

Tests

tests/conformance/reporting/test_reporting_row_retention_pg.py (PostgreSQL): sweeps (created vs adopted vs committed objects, resumption, unconfigured bindings), pruning in every binding kind, longer record retention, pending materializer holds, and resumption after a failed delete.

Merged by the repo owner's watcher without waiting for code-owner review; @rachitm022 is requested for visibility and will test the finished stack.

🤖 Generated with Claude Code

Adds row retention (adcontextprotocol/adcp#7996, sections 4.2 and 4.3).

- sweep_row_write_intents resolves abandoned object uploads. An expired open
  intent moves to sweeping under the account lock so no writer can commit
  against it, then only the objects that upload created are deleted at their
  recorded versions. Adopted objects and objects a committed chunk references
  are never deleted, and an interrupted sweep resumes.
- prune_expired_row_sets retires a period's row bytes in an ordered, resumable
  sequence: rows stop being served first, external object versions are deleted
  next, and manifests and bodies go last, so a failed delete leaves every
  locator intact. Expired rows read like an unknown revision, never as an
  integrity failure. A period qualifies once max(status_retention_days,
  ledger_record_retention_days) has passed since its newest record and no
  materializer work is pending.

Python keeps revision headers and their records until the shared Core layout
(persistence Stage 3).

Part of #1457.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Comment thread src/adcp/reporting/ledger/row_retention.py
aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Oct 11, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — additive reporting row-retention feature, no blocking findings.

What I checked:

  • row_retention.py (374 new lines): three-phase prune ordering is correct (mark pruning under account lock → delete external objects outside txn → delete manifests/bodies + mark pruned), so a failed delete leaves locators intact and resumes safely. Eligibility is re-checked under lock to prevent stale-candidate loss; verified against immutable BEFORE UPDATE triggers.
  • Candidate INNER JOIN to reporting_configurations is FK-safe; delete_unreferenced's per-object lock+check+delete preserves cross-obligation objects.
  • Additive feature, correct feat: semver signal, no public-surface break, new exports layered correctly.

Medium findings (1):

  • src/adcp/reporting/ledger/row_retention.py:128 — sweep_row_write_intents discards the failure cause; silent object-leak accumulation on a scheduled path.

Decision table: no critical/high findings (row 1 n/a). gated_paths is false (row 2 n/a). high_risk false, no deletions (rows 3,5 n/a). The single medium is a warn-only error-handling concern, not data-loss/schema/infra category (row 4 n/a). No prior escalation, no no-auto-approve team match (rows 6,7 n/a). Only 1 medium finding, below the 3-medium threshold (row 8 n/a). Falls through to row 9 → approve. review_decision: REVIEW_REQUIRED does not block since gated_paths is false.

Worth addressing the swallowed failure cause at line 128 in a follow-up, but it does not block merge.

Medium findings

  • src/adcp/reporting/ledger/row_retention.py:128 — Sweep discards failure cause; silent object-leak accumulation

Adds per-intent failure causes and a skipped-unconfigured count to the sweep result.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — prior Medium resolved, no new findings.

This is a subsequent review following a prior approve. The developer's delta resolves the only prior finding (row_retention.py:128 — sweep discarded the failure cause). The sweep now returns causes as (row_set_id, code) using ReportingRowStoreError.code (verified stable, secret-free) or type(error).__name__, surfacing failures to the caller rather than silently discarding them. A new test covers the failed-cause branch. New fields carry defaults (additive — no semver signal needed).

Gate checks:

  • No no-auto-approve team match.
  • gated_paths: false — row 2 does not apply.
  • high_risk: false.
  • No critical/high/medium findings in this run (row 1, 4, 5, 8 do not fire).
  • Prior decision was approve, so sticky-escalation row 6 does not apply.

None of rows 1–8 fire; falls through to row 9 → approve. Note: review_decision is REVIEW_REQUIRED, but with gated_paths false this does not block (the deprecated protected-branch rule no longer applies).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant