feat(reporting): compose provisional source lifecycle - #1225
Conversation
|
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. |
|
D5 implementation note from source tracing: |
|
Read-only merge-tree against current main identifies five conflict paths inherited from #1218: |
12d72b2 to
1f1033e
Compare
|
The base-sdist Python 3.10 installed job is red on the current merge head. I downloaded its artifact: all 11 failures are in |
bokelley
left a comment
There was a problem hiding this comment.
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.
|
Technical re-review of 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 |
Review — D5 revocation / restored scheduling / PG size-one pool, at
|
| 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 headstest_reporting_source_authorization.py— 26/26 pass on58ea16bagainst real PostgreSQL (0 skipped)18/18on4b35cc6against real PostgreSQL- Module-deletion check for
dfd55fe62(−1,768 lines): no surviving Python references tosource_work/operation_contracts(remaining grep hits are the unrelatedreporting_production_source_workSQL table), andservice,producer,offerings,_source_authorization,inline_sourceall 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.
58ea16b to
7d75d75
Compare
|
Please renew review and approval for exact head 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: 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. |
|
Re-reviewed
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 |
|
Renewed technical review requested for The only change since the reviewed Fresh |
|
Reviewed The diff changes only 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 |
There was a problem hiding this comment.
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.
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;Nonedenies 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 fullmake testpass (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.