Repository navigation
fix(reporting-storage): refuse a ledger upgraded to storage contract 2 with the real remedy - #1857
Conversation
…2 with the real remedy
adcp-reporting-storage 0.2.0b1+ upgrades a 0.1.x ledger in place, widening
the configuration, obligation-period and status-checkpoint keys with
offering_id. A 0.1.x process that keeps running through a rolling deploy,
or restarts after it, used to fail in ways that hid the cause:
- create_schema() raised "Reporting caller ownership requires a
stopped-worker maintenance migration", with a hint to run
migrate_legacy_reporting. That migration doesn't apply here and would
move reporting state into quarantine.
- commit_obligation raised LedgerConflictError LEDGER_WRITE_FAILED
("integrity rule"), and put_configuration and the status checkpoint
write raised a raw InvalidColumnReference (42P10): their ON CONFLICT
targets no longer match a unique index.
Refuse an upgraded ledger (reporting_configurations has offering_id)
before any DDL with ReportingSchemaAheadError, an operator-action error
that startup re-raises unwrapped. Map 42P10 on those writes to the same
remedy: obligation writes raise LedgerConflictError code
REPORTING_SCHEMA_AHEAD, and configuration and checkpoint writes raise
ReportingSchemaAheadError. The remedy is to run adcp>=10.0.0b7 with
adcp-reporting-storage>=0.2.0b1 everywhere; downgrading means restoring
the pre-upgrade backup; do not run migrate_legacy_reporting. The packaged
SQL's own check now raises the same message, for operators who apply the
DDL themselves.
Refs #1831
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| f" ON CONFLICT ({_KEY}) DO NOTHING", # nosec B608 | ||
| (*scope.checkpoint_key, json.dumps(asdict(scope)), "0" * 64), | ||
| ) | ||
| except Exception as error: |
There was a problem hiding this comment.
Medium: This status-checkpoint refusal arm ships untested. _WIDEN in the new conformance test widens reporting_configurations, reporting_obligations, and adds a bare offering_id column to reporting_consumer_statuses — it never touches reporting_status_scope_checkpoints, so the ON CONFLICT ({_KEY}) here never hits 42P10 in any test. The PR body lists the status-checkpoint write as one of the three sites that fail with a raw InvalidColumnReference on a contract-2 ledger, yet the only path this branch handles is the one with no coverage. If the sqlstate detection regresses here, an old pod degrades to a raw 42P10 on a durable async path with no test to catch it. Extend _WIDEN to widen the checkpoint key and add a case exercising _lock_scopes_on.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean, well-scoped contract-2 refusal change.
What I checked:
- 42P10 (
schema_ahead_error) detection keys on index name via sqlstate, not prose — correct shape._translate_integrity_errorchecks it first so obligation writes preserve their.codecontract (REPORTING_SCHEMA_AHEAD). require_owned_schema_or_emptyfails closed before any DDL.ReportingSchemaAheadErroris additive — no semver break, no public-surface break.- No credentials in ctx_metadata, no generated-code hand-edits, no type-layering violations, CI gates untouched.
Medium findings (1 — non-blocking):
packages/adcp-reporting-storage/src/adcp_reporting_storage/outbox/status_pg.py:301— Status-checkpoint contract-2 refusal arm is untested;_WIDENwidens configurations/obligations but never the checkpoint key, so that branch is never exercised.
Decision trace: no critical/high findings (row 1 no). gated_paths is false (row 2 no). high_risk false (rows 3,5 no). The single medium finding is not in {data-loss, schema, infra} (row 4 no). No prior decision (row 6 no). No no-auto-approve team match (row 7 no). Only 1 medium finding, fewer than 3 (row 8 no). Falls through to row 9 → approve.
Medium findings
- packages/adcp-reporting-storage/src/adcp_reporting_storage/outbox/status_pg.py:301 — Status-checkpoint contract-2 refusal arm is untested
…snapshot Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — additive public-API snapshot update, no blocking or medium findings.
This is a subsequent push after a prior approve. The delta since the prior run is a single line adding ReportingSchemaAheadError to tests/fixtures/public_api_snapshot.json, mapped to adcp_reporting_storage.migration. This is the correct additive companion to the new public export (added to migration.py __all__, re-exported via __init__.py), and the schema_ahead_error helper is correctly kept out of __all__. Additive/non-breaking — no semver concern.
The prior medium finding (status_pg.py:301 — untested contract-2 refusal arm) is outside this delta and was not rechecked; note the diff also includes a new conformance test (test_reporting_storage_contract2_refusal_pg.py) consistent with addressing that area.
Decision table: no critical/high (row 1 n/a); gated_paths false (row 2 n/a); high_risk false (rows 3,5 n/a); no medium findings (rows 4,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. (review_decision REVIEW_REQUIRED does not block since gated_paths is false.)
…s absent Import psycopg only after isolated_reporting_pool has checked it is installed, as the native suite runs without it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
This PR is a one-line test-robustness fix: from psycopg import errors moves from function entry into the isolated_reporting_pool(...) context-manager block in tests/conformance/reporting/test_reporting_storage_contract2_refusal_pg.py. The context manager runs pytest.skip/importorskip first, so a runner without psycopg now skips cleanly rather than erroring on the bare import. The change matches the three sibling tests and touches no production code.
No critical/high/medium findings were reported. No author-team gate (no no-auto-approve match). gated_paths is false, so the REVIEW_REQUIRED status does not force escalation. high_risk is false. Prior decision was approve and this delta is a net-neutral/positive refinement.
Decision table: rows 1–8 do not fire (no blocking findings, no gated path, no deletions, no medium findings, no team gate) → row 9 → approve.
…is contracted #1857 refused any ledger whose reporting_configurations has offering_id. The rolling-safe upgrade on next (#1856) adds that column in an expand step but keeps the four-column generation key that 0.1.x writes, until the operator contracts. So 0.1.0b5 would refuse to restart mid-rollout, though it otherwise runs on the expand state unchanged. Refuse only when the configuration primary key includes offering_id, i.e. once the ledger is contracted. That is the same test the packaged SQL's key check makes. Refs #1856 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…is contracted (#1867) #1857 refused any ledger whose reporting_configurations has offering_id. The rolling-safe upgrade on next (#1856) adds that column in an expand step but keeps the four-column generation key that 0.1.x writes, until the operator contracts. So 0.1.0b5 would refuse to restart mid-rollout, though it otherwise runs on the expand state unchanged. Refuse only when the configuration primary key includes offering_id, i.e. once the ledger is contracted. That is the same test the packaged SQL's key check makes. Refs #1856 Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Option 2 of #1831, for the 0.1.x line, so it can ship in 0.1.0b5 (#1834). The docs half went into
nextin #1855. The rolling-safe upgrade is tracked in #1856.Problem
adcp-reporting-storage 0.2.0b1+ upgrades a 0.1.x ledger in place, widening the configuration, obligation-period and status-checkpoint keys with
offering_id. A 0.1.x process still running through a rolling deploy, or restarted after it, failed in ways that hid the cause:create_schema()(restart)RaiseException: Reporting caller ownership requires a stopped-worker maintenance migration, HINT: Use migrate_legacy_reporting (the 8 → 9 ownership migration: wrong, and it quarantines state)commit_obligationLedgerConflictError LEDGER_WRITE_FAILED: the ledger write violated an integrity rule(a wrapped 42P10)put_configuration, status checkpoint writeInvalidColumnReference(42P10: no unique index matches theON CONFLICTtarget)Change
migration.require_owned_schema_or_empty, which every store's bootstrap runs before any DDL, now refuses a ledger whosereporting_configurationshasoffering_id. It raises the newReportingSchemaAheadError, aReportingOperatorActionError, so service startup re-raises it unwrapped. It's exported fromadcp_reporting_storage.schema_ahead_error()maps 42P10 on the writes whose keys the upgrade widens:_translate_integrity_error, raiseLedgerConflictErrorwith codeREPORTING_SCHEMA_AHEAD, so callers that branch on.codekeep working;put_configurationand the status checkpoint placeholder insert raiseReportingSchemaAheadError.reporting_ledger_account_generations.sql: its key check names contract 2 with the same remedy before falling through to the caller-ownership error, for operators who apply the packaged SQL themselves.All three carry the same remedy:
adcp>=10.0.0b7withadcp-reporting-storage>=0.2.0b1on every reporting process;migrate_legacy_reporting.Evidence
tests/conformance/reporting/test_reporting_storage_contract2_refusal_pg.pyhas four tests:create_schemarefuses and leaves the key unchanged;put_configurationandcommit_obligationname the upgrade.The tests apply the same key changes as
next's 0002. All four fail onmainand pass here.End to end with the published wheels:
adcp9.6.1 + storage from source);adcp==10.0.0b8+adcp-reporting-storage[pg]==0.2.0b1;create_schema→ReportingSchemaAheadError,commit_obligation→LedgerConflictError[REPORTING_SCHEMA_AHEAD],put_configuration→ReportingSchemaAheadError. Reads still work, and the new pair reads everything back.0.1.0b5 → 0.2.x upgrade path, checked because
mainhas storage schema changes not yet onnext(feat(reporting-storage): order the account change feed by xid and seq #1827, feat(reporting-storage): accept 255 character row binding IDs and credential refs #1828, feat(reporting-storage): collect orphaned row objects by listing with a clone fence #1835, perf(reporting-storage): drop the per-object advisory lock where nothing pairs on it #1836): a ledger built by this branch with the change feed on upgrades cleanly through 0.2.0b1'screate_schema()andpython -m adcp migrate --dsn …(0004–0006 applied, "up to date"). feat(reporting-storage): order the account change feed by xid and seq #1827's extra horizon columns and index are left in place. So releasing 0.1.0b5 doesn't strand anyone, but those features still need porting tonext.Local CI-equivalent
tests/conformance/reporting/run (CI's ignore list, PostgreSQL 15.18 locally,-n 4): 2669 passed, 20 failed. Rerunning the 20 in fresh databases: 3 fail with this change and 6 on unmodifiedmain. The one that failed only here (test_reporting_materializer_history…[postgres]) passes 3/3 when run alone.make lintandmake typecheck-allpass.Refs #1831
🤖 Generated with Claude Code