feat(reporting): materialize revisions with durable fenced work - #1187
Conversation
B2.1 of 4 within B2 of B1/B2. Preserve the coherent reserve, verified I/O, captured finish and logical enqueue transaction unit; keep production activation gated on the remaining seller slices. Refs #1167
Guard writer metadata and custom-store reservation failures, keep component reprs private, and retain bounded artifact setup and cleanup headroom. Refs #1167.
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Reviewed PR #1187 (+7103/−109), an additive, isolated durable reporting-materializer unit adding materializer/{pg,memory,service,work,capture,schema,_errors}.py, a 420-line isolated SQL migration, and additive delivery/store refactors. The reviewer read the largest files (pg.py 955L, memory.py 670L, the SQL, service/verification/capture) and verified fenced ACK atomicity, DB-level work-guard invariants, deadline-bounded I/O, notification quarantine, the process-local verification seal, and the correct additive feat(reporting) semver signal.
No Critical/High/Medium findings; no gate-weakening; no public-surface breaks. high_risk is false, gated_paths is false, no no-auto-approve team match, and no prior decision. Decision table rows 1–8 do not fire → row 9 approve.
The materializer stores suppress `materialization_event` for every `ReportingMaterializationRecord` so only a fenced verified finish can enqueue a readiness intent. That single `notify` flag also gated `delivery_dirty`, so an ordinary public `commit_materialization` through `InMemoryReportingMaterializerStore` or `PgReportingMaterializerStore` stopped marking the status scope dirty. The projector then never saw the outcome and the projected status stayed stale indefinitely, losing the public persistence compatibility the parent kept. Split the connection-bound and memory commit primitives into independent `notify` and `dirty` flags. Reservation still writes its delivery record and attempt without either, and the finish transaction still owns its own captured dirty work, so the atomic finish boundary is unchanged. `test_public_terminal_outcome_keeps_projection_dirty_work_without_readiness` asserts the restored `materialization` dirty evidence with no readiness in the materializer or the ordinary queue, and `test_reserved_attempt_keeps_the_finish_transaction_as_the_only_dirty_work` pins reservation as the only path that stays clean. Both run on memory and real PostgreSQL and fail on the previous behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ects
`materializer_errors` closes every non-writer exception into
`ReportingWriterError("RESOURCE_UNAVAILABLE", "same_identity", "unknown")` so a
driver, hook or provider message can never become a diagnostic. It also caught
the SDK's own argument validators, so `claim_materialization(lease_seconds=1)`
and `read_materializer_boundaries(after=-1)` reported an unknown external
effect: the adopter is told a destination write may have started, and loses the
message naming the bound they violated.
Raise those two fixed-message validations as `ReportingMaterializerUsageError`,
a `ValueError` subclass, and re-raise it from the guard. Existing `except
ValueError` handling is unchanged, arbitrary internal failures stay closed, and
`_LeaseHeartbeat` still treats any renewal failure as a lost lease.
`test_invalid_caller_arguments_stay_actionable_and_claim_no_external_effect`
covers both bounds on memory and real PostgreSQL, asserting the actionable
message, the absence of a writer failure record, an unchanged store image and
no write effect.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Clean pass on the delta. No critical/high/medium findings reported. The change correctly decouples projection-dirty work from the readiness-notify event so ordinary public materialization outcomes keep marking the status scope dirty (fixing a silent projector stall), while claim/finish paths pass dirty=False to avoid double-dirty. The new ReportingMaterializerUsageError keeps caller-argument rejections as plain ValueErrors rather than phantom external-effect failures. Both changes land with conformance tests on both backends.
Gate checks:
- No no-auto-approve team match.
- gated_paths: false; review_decision: APPROVED (gate satisfied regardless).
- high_risk: false.
- Prior decision was
approvewith no findings — this run remains clean.
No rows 1–8 fire → row 9 approve.
`advertised_notifications` matches the ledger by exact `type(...)`, so a materializer store is refused outright rather than only being denied the Managed/Reconciled claims. A deployment advertising Core `reporting.ledger_changed` today through `PgReportingReconciliationStore` gets a closed `notification_chain_unready` after switching to `PgReportingMaterializerStore`. That is correct and fail closed — B2.4 admits these stores only once it proves projection, component and mount readiness — but the rollout doc said only that the helpers "retain C's Managed/Reconciled veto", which reads as if Core advertisement survives. Say what actually happens, give adopters the two supported options, and note that polling is unaffected because this helper gates notification capabilities only. No production identity set is widened. `test_materializer_store_closes_core_advertisement_until_b24_admits_it` pins both halves on memory and real PostgreSQL: the reviewed store still advertises `reporting.ledger_changed`, and the materializer store raises. B2.4 has to change this deliberately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or medium findings.
Checked: no critical/high/medium findings surfaced. gated_paths is false, review_decision is APPROVED, high_risk is false, and no no-auto-approve team match. Prior decision was also approve, so this is a net-neutral iteration.
Delta since prior approve is two defensive additions: a docs clarification (adopting a materializer store closes notification advertisement entirely, not only Managed/Reconciled) and a new conformance test pinning that both materializer stores lose Core reporting.ledger_changed. Reviewer verified the mechanism against _capabilities.py exact-type gating and confirmed the test raises correctly on both backends — fail-closed and accurate.
Decision table: rows 1–8 do not fire (no critical/high, gate satisfied, no deletions, no medium findings, no team gate, prior was approve not escalate). Falls through to row 9 → approve.
The base branch was changed.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass on PR #1187 (prior outcome: clean approve at 8e18ca1).
The delta since the prior approve is three commits (compose with integrated main, qualify against integrated baselines, order accounts by native timestamps). The reviewer verified: the ORDER BY served_at::text alias fix in pg.py is correct, the CI required-gate/lane matrix in ci.yml is fail-closed and partitions cleanly, and the C-collation pin removal converges to already-reviewed main.
No Critical/High/Medium findings surfaced. Decision-table walkthrough: no critical/high (row 1 no); gated_paths false (row 2 no); high_risk false so no deleted/modified sensitive-path triggers (rows 3/5 no); no medium findings at all (rows 4/8 no); prior decision was approve, not escalate (row 6 no); no no-auto-approve team match (row 7 no). Falls through to row 9 → approve.
Note: review_decision is REVIEW_REQUIRED, but with gated_paths false this does not force escalation — the row 2 gate only fires when gated_paths is true.
1 participant
Durable materializer workers reserve one persistent external identity, resume it after process loss, and commit verified outcomes with captured status inputs and fenced acknowledgment. This is B2.1 of the reporting rollout. It does not activate Managed/Reconciled or delivery-ready advertisement.
Composition and preserved boundaries
8e18ca12b9a0c3750f80aa822058c02982ab3e52with actual main5487f2bdef23c5102118b305be9e868228f6ce61through ordinary two-parent compositionaf549d76eac9f7b2f548d4c8bbb3b4b51868b91e, followed by sole-parent corrections. No rebase, squash, reset, or remote rewrite.Integration corrections
The required
Postgres conformance tests (Postgres 16)rollup now depends on the existing PG matrix, status job, and materializer job. It always runs and accepts only three exactsuccessresults; failure, cancellation, skipped, or empty results fail. The new materializer job hascontents: read; the gate retainspermissions: {}. There are 20 expanded CI jobs. Core/process/status remain at 15 minutes, the inherited materializer job/pytest budgets remain 35/30 minutes, and Python retains its inherited 60/45-minute budgets. No ceiling was raised during this integration.Core excludes exactly the four complete materializer files selected by the materializer job, in addition to main's existing process/status exclusions. URL-present collection accounts for 2274 cases as 1969 core + 7 process + 251 status + 47 materializer, disjoint with an exact union. Whole-file dispatch maintains the partition when cases are added.
Frozen A/B/C/B1 controls now use integrated commits
17ee407a,0f34c666,967b6e28, and5487f2bd; the three earlier foundation pins are unchanged. Removed obsolete C-only fixture imports and service initialization flags. The frozen helper still requires PostgreSQL 16 and UTF8 and records its actual locale. Full installed binary provenance, readiness classification, old worker behavior, and immutable-object comparisons remain required.A real scheduler defect surfaced under libc
en_US.utf8: selectingserved_at::textcreates an output alias namedserved_at, so unqualifiedORDER BY served_atsorted text instead of timestamps. Under that locale, a just-served account sorted ahead of-infinityand repeatedly displaced a never-served account. A same-server PostgreSQL 16.14 C/libc discriminator reproduces the difference. Both sampling queries now explicitly order the underlying timestamp column, preserving serialized cursor values and the timestamp seek predicate. No DDL, index, deadline, or fingerprint changes.The populated migration fixture now requires the exact prior B464 catalog plus the ten rc.6 waiver-binding objects, and still compares every original object before/after installation. It does not permit arbitrary extra objects. Separately, 59 operational awaits were moved immediately before their assertions, including one embedded subprocess-source site. Complete inverse AST reconstruction preserves calls, arguments, expectations, and normal execution order; no conditional operand was promoted. Two SQL literal groupings and four explanatory handler comments preserve AST/constants.
Validation and limits
Exact final head
8ea0dbe46aafa356a40bb256dbb97316487c2468:en_US.utf8: 543 passed, 0 skipped in 545.28s pytest, under the unchanged 900s local ceiling. This includes native materializer migration, transaction, process-crash, service, and ledger controls.en-US: 21/21 migration/catalog controls each (27.34s /28.15s), with 910/910 frozen entries matching. Final libc catalog read also matches 910/910; all strict validators pass, with no fingerprint regeneration.2fed6422was 427 passed/116 intended PostgreSQL skips. That is retained as its own result; the final PostgreSQL selection includes the memory cases.The original composition's two obsolete-helper collection errors, the first libc run's 3 failed/540 passed result, the C discriminator's 2 failed/1 passed result, and the isolated failing backfill probe are retained. They are not relabeled or replaced by later passes.
Normal commit hooks passed. Complete installed VCS/sdist and frozen-wheel controls were not run locally: the available filesystem cannot accommodate the full historical source archives and isolated builds. Fresh exact-head CI must provide those controls and whole-job lifecycle evidence. No earlier PR's local tests, CI, source approval, or scanner acceptance transfers here.
Secret scanning is separate from source approval. A fresh exact-head full-content/history Gitleaks scan is retained with its complete coverage and individual match adjudications; it is not an accepted GitGuardian substitute for this PR. Fresh GitGuardian and genuine CodeQL output must be assessed independently of check conclusions.
Rolling qualification excludes pre-
17ee407aA, pre-0f34c666B, pre-967b6e28C, and pre-5487f2bdB1 binaries. These limits must accompany release notes. A's aggregate readiness after C remains intentionally closed while supported ordinary operations remain covered. Historical per-capability catalog cost (9rLB, no #1191 credit), inherited fail-closed snapshot invalidation, the #1199 execution hold, and the TS diagnostic hold remain unchanged. Guard #1201 stays draft/LAST and the legacy publishing workflow remains disabled. Source integration does not qualify artifacts, release publication, later rollout stages, or cross-runtime behavior.