Skip to content

feat(reporting): compose provisional source lifecycle - #1225

Merged
bokelley merged 14 commits into
mainfrom
feat/reporting-a2-source-lifecycle
Sep 26, 2026
Merged

bokelley merged 14 commits into
mainfrom
feat/reporting-a2-source-lifecycle

Conversation

@bokelley

@bokelley bokelley commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Repeated source observations need atomic publication and durable replay across the service, ledger, and PostgreSQL adapters. This PR composes provisional acquisition, production source registration, and inline staging so a configured service can publish an immutable observation and recover it after restart.

Replaying an acquisition and revision ID revalidates the supplied rows and digest in both ledger stores. Changed rows, an incorrect row count, or an incorrect managed digest are rejected without changing the original observation, checkpoint, or publication. Shared memory/PostgreSQL vectors cover conflicts, identical replay, producer acquisition, and the prepared-publication adapter.

D5 decision: Option A. The adopter's existing ReportingProductionSource.configuration_binding(configuration) is the source authority; None denies work. The SDK checks immediately before dispatch and again under the account lock before inline sealing and ledger publication. A denied result is discarded without a successful checkpoint, remaining source work for that account stops for the current turn, and a restored binding resumes on the next turn. Other accounts continue. Every dispatch and locked publication also compares the live media-buy/product mapping with the immutable admitted generation, refusing remapping even when both products are advertised. Restoring the original mapping permits retry; a result refused before sealing is fetched again. Restart and replay use fresh checks. In-flight fetches finish; revocation takes effect at the next dispatch or publish. Adopters own callback caching and latency. The unused private grant/epoch/CAS model has been removed; no authorization adapter or persisted grant is introduced, and F/L/P are dropped. Existing feed and destination authorization remain in place.

The PostgreSQL inline seal uses the same account-locked transaction when it shares the ledger pool, including a size-one pool. Failure and cancellation settle the transaction before retry; driver exception details are discarded at the source-task boundary.

Validation on 7c907196: make lint, make typecheck-all, and full make test pass (11,902 passed, 2,357 skipped, 9 deselected, 1 xfailed; 81.43% coverage against the 80% requirement; harness exit 0). All 91 focused D5/R1 memory/PostgreSQL regressions and packaging deadline controls also pass. Parsed workflow comparison confirms that only the two production/status timeout values changed, with all selectors preserved. Current-head CI is green. c56’s technical review of this head found no blocking findings; all 14 review threads are resolved. The PR is ready for review, and aao’s formal review is running on this head.

PostgreSQL installed production verification retains main's 2,400-second aggregate budget (1,800 seconds for the driver-absent case), with a 45-minute CI step and 50-minute job limit.

The production/status conformance step now has a 40-minute limit and its job has a 45-minute limit. The prior run passed all 1,234 tests in 1,783.94 seconds, then completed the harness with exit status 0 at 1,803.564 seconds, crossing the old 30-minute step limit during cleanup. This change preserves five minutes for job setup and cleanup and keeps the complete test selection.

Comment thread tests/conformance/reporting/test_reporting_inline_storage.py
Comment thread tests/conformance/reporting/test_reporting_inline_storage.py
Comment thread src/adcp/reporting/source_work/operation_contracts.py Fixed
Comment thread tests/type_checks/reporting_source_operation_contracts.py Fixed
Comment thread tests/type_checks/reporting_source_operation_contracts.py Fixed
Comment thread tests/type_checks/reporting_source_operation_contracts.py Fixed
Comment thread src/adcp/reporting/ledger/provisional.py Fixed
Comment thread src/adcp/reporting/ledger/provisional.py Fixed
Comment thread src/adcp/reporting/ledger/provisional.py Fixed
Comment thread tests/type_checks/reliable_reporting_lifecycle.py Fixed

Copy link
Copy Markdown
Contributor Author

The combined-contract R1 replay correction is pushed in both ledger stores, with shared memory/PostgreSQL conflict and identical-replay vectors plus producer/prepared-adapter coverage. Please review this PR head when ready; fresh CI is running after the core PostgreSQL job-budget fix.

Copy link
Copy Markdown
Contributor Author

Main now includes #1218 and #1219 and this draft reports a merge conflict. Please rebase your D5 implementation branch onto current main once the implementation is ready, and retain the A2 R1 replay fix and vectors.

Copy link
Copy Markdown
Contributor Author

D5 implementation note from source tracing: InlineReportingSource.execute calls _publish, whose _seals.put at inline_source.py:915 durably seals before the producer receives the result. A post-execute ledger check protects revision/observation/checkpoint, but cannot satisfy Brian’s explicit pre-seal recheck for this source. Please account for this boundary in the implementation and its memory/PG vectors.

Copy link
Copy Markdown
Contributor Author

Read-only merge-tree against current main identifies five conflict paths inherited from #1218: src/adcp/reporting/service.py, the service lifecycle conformance test, tests/test_reliable_reporting_lifecycle.py, tests/test_reliable_reporting_service.py, and the lifecycle type-check fixture. ledger/producer.py auto-merges. Please reconcile the #1218 service behavior with D5 checks during your rebase; I am leaving your separate worktree untouched.

@bokelley
bokelley force-pushed the feat/reporting-a2-source-lifecycle branch from 12d72b2 to 1f1033e Compare September 26, 2026 17:27
Comment thread tests/conformance/reporting/test_reporting_inline_storage.py
Comment thread tests/conformance/reporting/test_reporting_inline_storage.py Fixed

Copy link
Copy Markdown
Contributor Author

The base-sdist Python 3.10 installed job is red on the current merge head. I downloaded its artifact: all 11 failures are in tests/conformance/reporting/test_reporting_publication_time.py. Eight memory cases now skip dispatch/publication (DelayedReader.calls == 0, no revision, or expected clock conflict not raised); three direct-commit/temporal-negative cases raise _SourceAuthorizationRevokedError: BINDING_MISMATCH. The installed child built merge commit 2d30c1f containing this PR, so these are current-head failures rather than a stale base. Artifact: pg-reporting-production-installed-base-sdist-1 in run 36261558949. Please check the D5 binding fixture/dispatch setup in that test module and rerun the installed gate.

@bokelley bokelley left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review of the current draft: one blocking P2 finding below.

187 focused tests passed on Python 3.12/PostgreSQL 16 across observation replay, the prepared adapter, source authorization, inline storage, publication clocks, and source registration. The separate in-flight mapping probe failed on both memory and PostgreSQL.

R1 is addressed by the replay validation and tests. Please fix the source-binding publication gap before readiness; final approval is held for the ready/current head after CI.

Comment thread src/adcp/reporting/ledger/producer.py

bokelley commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor Author

Technical re-review of 58ea16bd175739b9d8a07f6984db4090cf9408d9: P2 fixed; no remaining blocking findings from this review.

The source-binding thread is resolved. Dispatch and both account-locked publication checks compare the live mapping with the immutable admitted scope. The blocked-fetch and restoration vectors cover refusing changed mappings, fetching fresh rows after seal refusal, and reauthorizing replay of a seal retained before ledger refusal.

116 focused tests passed on this head using Python 3.12/PostgreSQL 16: authorization (including all eight new cases), producer lock ordering/settling, publication clocks, prepared observations, provisional observations, source registration, and my original two P2 reproductions. R1 remains covered.

The PR is draft and current-head CI is still running, with no failures observed. The separate aao-secretariat identity must post formal APPROVE on the final ready/current head after green CI; this shared-login comment is the technical verdict.

Comment thread src/adcp/reporting/ledger/producer.py

Copy link
Copy Markdown
Contributor Author

Review — D5 revocation / restored scheduling / PG size-one pool, at 58ea16bd175739b9d8a07f6984db4090cf9408d9

Covers both 4b35cc6 and the new head 58ea16b. No blocking findings in the three focus areas. One flaky-test note and two observations below.

I stood up PostgreSQL 15.18 (socket-only) locally so the PG-gated tests actually ran instead of skipping — that matters here, because the size-one pool behaviour is the part that can't be judged by reading.

1. D5 dispatch/publish revocation — sound

The model is right: _REVOKED_ACCOUNTS remembers only denials, turn-scoped, and every dispatch/publish re-asks the adopter with nothing cached.

source_turn() is deliberately re-entrant, so I checked the scoping at both real call sites rather than trusting the docstring: production/service.py:645 opens a fresh set per loop iteration and service.py:747 per _run_worker. Denials are shared across configurations within a turn and never leak across turns — which is exactly what makes restoration possible.

3db5a5066 (discovery probes) is correct and complete:

  • _SourceAuthorizationRevokedError is imported at offerings.py:26, so the new raise doesn't NameError.
  • require_account_work sits outside the try, so a genuine account-wide stop isn't swallowed and re-labelled BINDING_MISMATCH.
  • The except _SourceAuthorizationRevokedError: raise clause is load-bearing — without it the broad except Exception below would mask every revocation as a binding mismatch.

The asymmetry is intentional and coherent: discovery raises without poisoning, while the producer's dispatch/publish checks call source_revoked() and own the account-wide stop.

Observation (non-blocking). After the fix, "this candidate has no binding" and "this account is already revoked" raise the same type out of source_binding(). Today that's benign — the only catchers are the three producer sites and both cases end the turn retryably — but the signal is lossy if a future caller ever wants to keep probing on one and stop on the other.

2. Restored scheduling — correct, and the lease is safe

Revocation is explicitly non-terminal: _run_leased_turn catches and returns the turn, and critically the finally: await self._store.release_period_close(...) means a revoked turn always releases its lease rather than stranding it until expiry. Pending acquisitions stay retryable.

memory.py re-queues claimed-but-unfinished work behind a monotonically increasing per-generation counter (turn + offset, counter advanced by len(candidates)) — round-robin, distinct values, no starvation of siblings by a repeatedly-denied item.

3. PG size-one shared pool — correct; I exercised it

seals._pool is self._pool → share one transaction instead of taking a second checkout that would self-deadlock a max_size=1 pool. Right fix.

I traced the owner = asyncio.current_task() guard through the real task boundary, since bind_inline_publication's docstring ("carry into its inline executor task") reads like it might cross tasks: it doesn't. bind_inline_publication only stores a factory; the transaction is opened at inline_source.py:897 inside the executor, and publish_seal is awaited in that same async with block. Owner always matches — the check is a defensive assertion against a future refactor, not a live path.

Observation (non-blocking). Pool sharing keys on identity (is), not equality. An adopter wiring a different AsyncConnectionPool object to the same database takes the fallback path and two checkouts. Not a deadlock (separate pools), but it silently opts out of the shared publication boundary.

4. New head 58ea16b — constituent pin

Closes a real admission→publication TOCTOU: the mapping is read before the lock and re-checked inside it, "including on replay". Checked the two ways this could go wrong and both are fine — producer_constituents and binding.constituents() both return tuple[ReportingConstituent, ...] (no list/tuple mismatch that would make != always true), and producer_constituents never returns None, so the admitted_constituents is not None skip is only the no-progress-store case. source_revoked is NoReturn, so _admitted_source_constituents has no implicit fall-through.

The comparison is order-sensitive — fine as long as both sides derive from the same ordering, worth a glance if the mapping source ever changes.

Flaky test (worth a look, not a blocker)

test_postgres_inline_publication_shares_size_one_pool[True-storage_error] timed out twice for me. I chased it rather than reporting it raw, and both failures occurred while a second pytest process was concurrently loading the same PostgreSQL instance. With the machine otherwise idle:

isolated whole-file
58ea16b 6/6 pass 3/3 pass (26 passed)
4b35cc6 — 3/3 pass (18 passed)

So it reads as a contention-sensitive wait budget in the test, not a product deadlock — but a size-one pool is precisely where a tight timeout will flake on a shared CI runner. Worth either a more generous wait or an assertion that distinguishes "deadlocked" from "slow".

What I ran

  • make lint — clean on both heads
  • test_reporting_source_authorization.py — 26/26 pass on 58ea16b against real PostgreSQL (0 skipped)
  • 18/18 on 4b35cc6 against real PostgreSQL
  • Module-deletion check for dfd55fe62 (−1,768 lines): no surviving Python references to source_work / operation_contracts (remaining grep hits are the unrelated reporting_production_source_work SQL table), and service, producer, offerings, _source_authorization, inline_source all import cleanly.

A wider sweep of provisional/lifecycle tests on my ad-hoc PG 15 showed some failures/errors concentrated in test_reporting_core_lifecycle.py, outside these focus areas. I could not attribute them to this PR and my environment differs from CI's PG 16 — not raising them as findings; CI is authoritative there.

CI at time of writing: 34 SUCCESS / 1 skipped / 15 pending, no failures.

@bokelley
bokelley force-pushed the feat/reporting-a2-source-lifecycle branch from 58ea16b to 7d75d75 Compare September 26, 2026 19:52

Copy link
Copy Markdown
Contributor Author

Please renew review and approval for exact head 7d75d755032a19e52c827f3269ece26b2ec40cbd, rebased onto main 2a45bafddd28afe9e6013a715289c2e1d9b208db.

The conflict resolutions preserve D5's exact admitted mapping checks at dispatch and locked publication, R1 replay revalidation, and main's order-independent scheduled refresh assertions. Main's newer PG budget remains 2,400 seconds with 45-minute step / 50-minute job headroom. GitHub now reports this PR mergeable; all 14 existing threads are resolved.

Validation on this head: make lint, make typecheck-all, 222 affected memory/PostgreSQL tests, and 12 packaging deadline/cleanup controls pass. After push, all 79 authorization and replay regression tests pass again. Fresh CI is queued.

The PR stays draft as requested. The automated review workflow skips drafts, so renewed technical review is requested here and through the coordinator; earlier approval is not being treated as approval of this rebased head.

Copy link
Copy Markdown
Contributor Author

Re-reviewed 7d75d755032a19e52c827f3269ece26b2ec40cbd against the P2-cleared 58ea16bd175739b9d8a07f6984db4090cf9408d9: no blocking findings.

  • D5 preserves the exact admitted mapping checks at dispatch and under both publication locks, including blocked-fetch refusal, restoration and seal replay. The implementation and regression vectors are unchanged by the rebase.
  • R1 remains intact in memory and PostgreSQL: recorded-observation replay validates the resubmitted revision and rows through commit_revision within the existing atomic boundary. The scheduling assertions retain supersession/history checks without depending on list order.
  • Main's PG packaging budgets are preserved: 2,400-second child, 45-minute step and 50-minute job; driverless children retain 1,800 seconds. The workflow and packaging helper match the rebased main versions.

Independently ran 115 passing checks on Python 3.12/PostgreSQL 16: 79 authorization/replay cases, 12 packaging deadline/diagnostic/cleanup controls, 18 PG core-lifecycle cases, four late-account cases and both original P2 reproductions. No failures or skips.

All review threads remain resolved. Current-head CI is running without failures observed; the PR is still draft. Formal aao-secretariat review should follow ready status and green CI on the final current head.

Copy link
Copy Markdown
Contributor Author

Renewed technical review requested for 7c90719649ee2d3d8d238e38c580b08d16ea1d6d.

The only change since the reviewed 7d75d755 is .github/workflows/ci.yml: the production/status step is 40m and its job is 45m, with a comment explaining cleanup headroom. A parsed workflow comparison confirms every test selector and all other configuration are unchanged. The previous job passed all 1,234 tests but harness cleanup reached 1,803.564s, beyond its old 30m step limit.

Fresh make lint, make typecheck-all, and all 91 D5/R1 memory/PostgreSQL regressions and packaging deadline controls pass. Full local make test and new-head CI are running. The PR remains draft until remaining gates and this renewed technical review clear; marking ready will request the formal aao review on the same head.

Copy link
Copy Markdown
Contributor Author

Reviewed 7c90719649ee2d3d8d238e38c580b08d16ea1d6d against 7d75d755032a19e52c827f3269ece26b2ec40cbd: no blocking findings.

The diff changes only pg-reporting-production: the conformance step increases from 30 to 40 minutes and the job from 35 to 45 minutes, plus an explanatory comment. An independent parsed-workflow comparison confirms every selector and all other configuration are identical. Source and tests are unchanged, preserving D5's exact admitted mapping and R1 replay validation. The separate installed-PG budgets remain 2,400 seconds / 45-minute step / 50-minute job.

I checked the previous job logs: 1,234 tests passed in 1,783.94 seconds, and the harness reported exit 0 at 1,803.564 seconds, followed by Actions' 30-minute timeout error. The new limits provide room for the observed run and cleanup while retaining five minutes between the step and job limits for setup and finalization.

Current-head CI is running without failures observed; the PR remains draft. Formal aao-secretariat review should follow ready status and green CI on the final current head.

@bokelley
bokelley marked this pull request as ready for review September 26, 2026 21:27

@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 — clean review, no blocking or medium findings.

Checked: provisional source lifecycle composition, D5 Option A live source authorization/revocation, borrowed-pool inline storage, fixed-profile source registry, and application handler delegation across 45 files (+5,221/-158). Public-surface changes are strictly additive with type-check contracts and appropriate feat/fix conventional-commit prefixes (no undeclared breaking changes to the adcp.* surface). Inline storage is account-scoped, immutable, and fail-closed; authorization is rechecked at dispatch under the account lock; every new branch is covered by tests. No disabled tests, blanket # type: ignore, generated-code hand edits, or import-layering violations. CI timeout bump is justified.

Decision-table walk: no critical/high/medium findings (row 1, 4, 5, 8 don't fire); gated_paths is false (row 2 n/a); high_risk is false (rows 3/5 n/a); no prior decision (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 that only gates approval when gated_paths is true, which it is not here.

@bokelley
bokelley merged commit 2bfbca4 into main Sep 26, 2026
55 checks passed
@bokelley
bokelley deleted the feat/reporting-a2-source-lifecycle branch September 26, 2026 21:34
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