[bokelley]

Closes #1829

What

An opt-in collector for row objects the write-intent sweep cannot see, with the ownership fence that stops a cloned or restored ledger database from deleting the live database's objects. It mirrors JS adcp-client#3291 (collector) and #3593 (fence). The design record (specs/shared-sdk-persistence-proposal.md §4.2) tracks the listing collector; the fence post-dates it and follows the JS design.

The sweep deletes only what an intent lists, so it misses an upload whose PUT succeeded before the intent recorded it, intents for a binding no running host configures, and intents lost to a restore.

Collector

PgReportingLedgerStore.collect_orphaned_row_objects lists each configured object binding under {prefix}/{namespace_key}/ and deletes an object only when all hold:

1. Its key is a well-formed row object (.jsonl or .jsonl.gz, no ./.. segment) under that prefix. Anything else, or anything a provider returns outside it, is dropped and counted as foreign.
2. It is older than min_age_seconds (default 7 days, enforced minimum 1 day). An unreadable or non-RFC 3339 write time counts as young.
3. A version- and binding-blind pre-check finds no chunk of a not-yet-pruned row set and no write intent (any state, old or new JSON keys) naming the key.
4. Under the per-object advisory lock of every binding (configured plus registered, sorted by binding ID), no live or unavailable row set under any binding references that exact version and no open intent created it; then the provider deletes that exact version.

read_only bindings are skipped unless include_read_only. dry_run reports up to 1000 candidates. Deleting requires acknowledge_exclusive_namespace=True. limit bounds objects per binding per pass, a page cap bounds empty pages, a repeated token or oversized page fails the binding, a failed first page of a resumed cursor drops the cursor.

Providers gain an optional list (filesystem, S3, GCS, Azure) and the conformance suite checks it when present. A provider without list keeps working and is reported unsupported.

Fence

A create-only epoch chain at {prefix}/{namespace_key}/_collector-fence/{epoch:016d}.json, recorded as (epoch, token, previous_token) in adcp_reporting_orphan_collector_fences. A real pass takes a session advisory lock, proceeds only if the bucket's highest epoch is exactly the recorded claim, commits the intent for epoch + 1 under a fresh token, writes the object create-only, verifies the token and prunes all but the predecessor, all before its first delete. It re-reads the fence before the first delete of every page and every 25 deletes. A finished claim always mints a fresh token. Refusals are reported as status == "fenced" with a cause (claimed_elsewhere, fence_advanced, token_mismatch, fence_missing, fence_regressed, fence_unreadable, claim_lost, migration_required); a provider or database error is failed, never fenced. dry_run checks without claiming. take_over_orphan_collector_fence(acknowledge_previous_owner_gone=True) is the deliberate step after a restore. The record bytes and the scope hash are the JS SDK's.

Production maintenance (opt-in)

PgReportingLedgerStore(row_storage=..., orphan_collection=ReportingOrphanCollectionOptionsV1(...)) adds an orphan_collection operation to maintenance_operations(): interval_seconds (default one day) spaces full listings, an unfinished listing continues each pass without relisting finished bindings, and a fenced namespace looks again within the hour. The worker reports fenced as stalled work and unsupported as pending work, as integer counters only. That needed a small change in the adcp package (_diagnostics.py, service.py); it is additive and an older adcp simply drops the unknown counters.

Python-specific decisions

• The fence table is in the idempotent reporting_row_storage.sql (no versioned migration runner) and deliberately outside the required-schema manifest and REPORTING_ROW_STORAGE_TABLES, so a deployment that never enables the collector needs nothing from it. Until it exists a pass reports every binding fenced / migration_required (a dry run too) and deletes nothing. The empty-ledger replacement treats its rows as startup rows.
• The claim holds a pooled psycopg connection with autocommit toggled and the session lock; any failed cleanup closes the connection rather than returning it to the pool. The lock flag is set before the lock call so a cancellation cannot leak it.
• Hardening beyond JS: a fence listing in which the provider skipped an entry fails closed (fence_unreadable), the filesystem list never lists or follows symlinks, strings are rejected for bindings/skip_bindings (they would act as substring tests), and listing validates with fullmatch.
• A mid-pass recheck deliberately treats an unfinished claim as healthy, as JS does: it is a sibling host of the same database mid-claim.
• No !: opt-in, additive. The one visible change is that the conformance result now ends with a list entry for providers that implement it, noted in the release notes.

Rollout, migration and rollback

1. Back up the ledger database. create_schema adds one table; nothing else changes unless you opt in.
2. Claim from the live database first: one real pass, or a takeover. First claim wins: a clone that collects before the original has run a pass since the copy claims first and the original is fenced. The clone still cannot delete an object the original wrote after the copy until it is min_age_seconds old. Never enable the collector on a copy of the deployment; start with dry_run=True.
3. Rollback: remove the orphan_collection option (nothing runs). The table and fence objects are inert and can stay. A lifecycle rule must not expire _collector-fence/.

Tests

Ported from the JS suites (reporting-row-orphan-collector-pg, reporting-orphan-collector-fence-pg, reporting-orphan-fence, provider list tests, maintenance-warnings):

• Collector on PostgreSQL and the filesystem provider: orphan vs committed, intent-named and young; foreign keys; read_only and unsupported; budget and cursors; completed/skip; delete and listing failures; other-version and row-set-state matrix; old intent keys; shared bucket and prefix; hostile provider output; repeated tokens; input validation; lock waits proven with pg_locks (an intent committed while it waits, an adoption through another binding).
• Fence: clone before the original collected, clone collecting first, restore, takeover, shared fences, every refusal cause, malformed/oversized/foreign fence objects, an unfinished claim and a copy taken during one, two hosts at once, a claim appearing mid-pass (stops at exactly 25 deletes), missing table, provider failures, cancellation leaving no session lock, the pure decision table and the byte format.
• Providers: filesystem list (pages, stability, symlinks, resume after delete), S3 (moto: noncurrent versions, no delete markers, bad tokens, truncation without markers), GCS and Azure fakes, a negative conformance table, and scheduling state.
• Regression: these fail on the previous code (no list, no collector or fence).

make lint and make typecheck-all pass; the Postgres conformance tests above pass locally, run serially.

#1492 (the object-lock contract from adcp-client#3606) follows as its own PR once this lands.

🤖 Generated with Claude Code

[aao-secretariat]

Ladon verdict: Approve

Approve — clean additive PR, no blocking or medium findings.

This PR adds an opt-in orphan collector and ownership fence to adcp-reporting-storage, ported from reviewed JS adcp-client #3291/#3593. Verified:

• Additive change, correctly tagged feat(reporting-storage) with no breaking ! — introduces new V1 types and an optional provider capability detected via getattr/callable rather than being added to the runtime_checkable Protocol, so existing providers stay valid and report unsupported. No public-surface break, so semver signal is correct.
• Delete path is fail-safe: delete_unreferenced(any_binding=True) takes per-object locks in sorted order and re-checks exact (key,version) non-reference under lock; mid-pass fence recheck bounds post-takeover deletes to ≤25, each version-exact guarded — the takeover-mid-pass window cannot remove a referenced object.
• Operational readiness: provider I/O bounded by deadlines; fence claim uses lock_timeout=120s and closes the pooled connection on cleanup failure so the session lock cannot leak.
• No credentials in ctx_metadata, no generated-code edits, no import-layering breaks, no CI gate weakening.

Decision table: no critical/high/medium findings; gated_paths false; high_risk false; no author team gate. Despite review_decision: REVIEW_REQUIRED, row 2 only fires when gated_paths is true — it is false here, so the PR falls through to row 9.

Non-blocking follow-up (Low, not surfaced as blocking): GCS/Azure list on the fence-read safety path is validated only against in-repo fakes; S3 is moto-tested. Real-provider pagination/consistency there is unexercised — worth a future integration test.

[aao-secretariat]

Ladon verdict: Approve

Approve — additive, opt-in orphan-object collector with clone/restore ownership fence; no blocking or medium findings.

What I checked:

• No no_auto_approve_team match; gated_paths: false (review_decision REVIEW_REQUIRED is irrelevant when gated_paths is false); high_risk: false; no prior decision.
• Reviewer found no critical/high/medium findings — rows 1–8 of the decision table do not fire, so this falls through to row 9 (approve).
• Semver: exports are additive and delete_unreferenced gains a defaulted kw-only param; feat: is correct, no wire-shape change, DR-0032/0033 N/A.
• Fence recheck cadence, lock ordering (no cycle vs. adoption path), clone/crash safety (fail-closed token mismatch), operability (bounded I/O, lock_timeout, delete retry/counting), and migration gating all verified by the reviewer.
• Tests cover every fence cause, clone/takeover, mid-pass stop-at-25, missing table, cursor resume, hostile provider output, and list conformance.

No concerns requiring a human gate.

[aao-secretariat]

Ladon verdict: Approve

Approve — subsequent pass, clean.

The delta since the prior approve is a single test-only change in tests/conformance/reporting/test_reporting_orphan_fence_pg.py: the cancelled-claim test now captures the task outcome via asyncio.gather(..., return_exceptions=True) and asserts isinstance(outcome, CancelledError), replacing with pytest.raises(CancelledError): await task. This is the correct idiom — awaiting a cancelled task re-raises into the test coroutine itself, so capturing the outcome as a value is the robust form. Load-bearing assertions (no leaked session lock, clean re-collect) are unchanged.

No production code in the delta. No critical/high/medium findings surfaced. Prior approve had no blockers to re-surface. high_risk is false, gated_paths is false, and no no-auto-approve team match.

Decision table: rows 1–8 do not fire (no findings, no gates, no team match, review_decision REVIEW_REQUIRED but gated_paths false so row 2 inapplicable). Falls through to row 9 → approve.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean subsequent review, no blocking or medium findings.

This is a subsequent push following a prior approve (head c4b44ca). The delta since that pass is a single file: tests/fixtures/public_api_snapshot.json, which gains 7 orphan-collector/fence exports under a test: commit. The reviewer verified each class→module mapping against its definition site (e.g. ReportingOrphanFenceTakeoverV1 → orphan_collector.py, ReportingOrphanFenceRefusalV1 → orphan_fence.py) — all correct. These are pure additions with no removal, rename, or type-signature change, so no semver break on the public surface and no feat!:/BREAKING CHANGE: requirement.

Decision walk: no critical/high findings (row 1 n/a); gated_paths is false (row 2 n/a); no deleted high-risk files (row 3 n/a); no medium findings at all (rows 4/5/8 n/a); prior decision was approve, not escalate (row 6 n/a); no no-auto-approve team match (row 7 n/a). Falls through to row 9 → approve. Note review_decision is REVIEW_REQUIRED, but with gated_paths: false that does not gate the outcome.

[aao-secretariat]

Ladon verdict: Approve

Approve — test-robustness fix with no blocking or medium findings.

This is a subsequent review (prior outcome: approve). The delta is a one-line test-robustness fix adding pytest.importorskip("psycopg_pool") to guard the sole test in test_reporting_orphan_maintenance.py that constructs PgReportingLedgerStore, whose __init__ raises ImportError when PG_AVAILABLE is false. The guard converts a pre-condition error to a skip on machines without the pg extra while still running fully in CI where the extra is installed — a legitimate conditional skip, not a CI-gate disable.

Decision trace: no critical/high/medium findings (rows 1, 4, 5, 8 don't fire); gated_paths is false (row 2 doesn't fire); high_risk is false (rows 3, 5 don't fire); prior decision was approve, not escalate (row 6 doesn't fire); no no-auto-approve team match (row 7 doesn't fire). Falls through to row 9 → approve.

Note: review_decision is REVIEW_REQUIRED, but since gated_paths is false, the hard gate in row 2 does not apply — a clean diff with no triggers approves normally.

[aao-secretariat]

Ladon verdict: Approve

Approve — test-only delta, no blocking or medium findings.

This is a subsequent review following a prior clean approve. The delta is a single test-only change in tests/conformance/reporting/test_reporting_change_feed_pg.py: it replaces the function-level from psycopg.errors import CheckViolation with pytest.importorskip("psycopg") + psycopg.errors.CheckViolation, so the Postgres conformance test skips cleanly when psycopg is absent instead of raising ImportError. psycopg's __init__ imports the errors submodule, so the attribute access resolves correctly. The change is correct and well-scoped; it reopens nothing from the prior pass.

Checked: no critical/high/medium findings; no public-API surface change, no generated-code hand-edits, no CI-gate disabling (the change improves skip behavior rather than disabling a test). high_risk is false, gated_paths is false, and no no-auto-approve team match.

Decision table: rows 1–8 do not fire (no findings, no gate triggers), so this falls through to row 9 → approve.