Skip to content

Commit 41c2be7

Browse files
authored
fix(release): tighten V9 reporting and CI readiness checks (#1550)
Read current PR messages on CI reruns without changing the validated head. Pin existing reporting exports and give row-storage operators executable, quoted foreign-key remedies with accurate staging-lock documentation. Fixes #1383 Fixes #1493 Refs #1490, #1495, #1510
1 parent 3db6694 commit 41c2be7

9 files changed

Lines changed: 341 additions & 19 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1101,6 +1101,7 @@ jobs:
11011101
if: github.event_name == 'pull_request' || github.ref == 'refs/heads/main'
11021102
permissions:
11031103
contents: read
1104+
pull-requests: read # Read the current title/body when a check is rerun after an edit.
11041105

11051106
steps:
11061107
- uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0
@@ -1110,6 +1111,8 @@ jobs:
11101111
persist-credentials: false
11111112
- name: Validate PR source or every integrated main commit and breaking footer
11121113
run: python3 -m scripts.check_main_policies conventional
1114+
env:
1115+
GH_TOKEN: ${{ github.token }}
11131116

11141117
downstream-imports:
11151118
needs: changes

‎docs/reporting-row-storage.md‎

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,22 @@ To install everything this guide uses while developing: `pip install -e '.[dev,p
5656

5757
## Layout notes
5858

59-
`adcp_reporting_row_sets` is the side table that records where a revision's rows live until the shared Core layout
60-
(persistence Stage 3). It matches the JavaScript table column for column except that `obligation_id` has no
59+
`adcp_reporting_row_sets` is the side table that records where a revision's rows live.
60+
It matches the JavaScript table column for column except that `obligation_id` has no
6161
foreign key: Python obligations live in `reporting_obligations`, not in the JavaScript
6262
`adcp_reporting_obligations` table. If the JavaScript migration created the table first, `ready_row_storage`
63-
refuses with the constraint name; apply this migration first, or drop that constraint.
63+
refuses with the constraint name and a schema-qualified, quoted statement to remove it.
64+
Apply the Python migration first, or, for an existing database with Python as its writer,
65+
remove the JavaScript foreign key before running readiness again. The default constraint name is:
66+
67+
```sql
68+
ALTER TABLE adcp_reporting_row_sets
69+
DROP CONSTRAINT adcp_reporting_row_sets_obligation_id_fkey;
70+
```
71+
72+
Run this in the ledger schema. Use the exact statement from the refusal if the schema or
73+
constraint has a different name. This removes the cross-SDK foreign key; row integrity
74+
continues to be verified against the stored digests.
6475

6576
A revision written before protocol-shape control totals (see the release notes) has a binding over `{name, value}`
6677
pairs. Its row set records the rows-only digest profile `rows_v1`, and its pair binding is verified against the
@@ -302,7 +313,9 @@ A period qualifies once the later of its period end and its newest revision or a
302313
`ledger_record_retention_days` is the generation's `status_retention_days`; a shorter value is ignored, because a row
303314
retention shorter than the advertised window is non-conformant.
304315

305-
Python keeps revision headers and their records until the shared Core layout (persistence Stage 3). A pruned row set
316+
Python keeps revision headers and their records. Record retirement refuses periods with protected reconciliation
317+
or audit evidence, so production periods carrying those records remain retained; row-byte pruning still runs.
318+
A pruned row set
306319
stays as a small marker that makes reads answer as expired; the records are already outside the status projection,
307320
whose horizon excludes periods past retention. Versioned buckets, soft delete and retention locks are allowed as
308321
backstops, but a `pruned` period is not "erased" while they keep the old bytes.
@@ -346,12 +359,13 @@ either verify in the other. Three things must match between the two deployments:
346359
- **The same binding definitions.** A binding's identity digest is `sha256(JCS([kind, provider, identity_config]))`
347360
over the names the JavaScript SDK uses (`location`, `prefix`, `keyTemplate`), so an object binding that differs in
348361
location, prefix or key template is a different binding and is refused rather than silently redirected.
349-
- **The row-storage migration applied by this SDK first**, because the JavaScript `adcp_reporting_row_sets` has a
350-
foreign key to its own obligations table that Python obligations can never satisfy. See "Layout notes".
362+
- **The row-storage migration applied by this SDK first**, or the JavaScript foreign key removed with
363+
`ALTER TABLE adcp_reporting_row_sets DROP CONSTRAINT adcp_reporting_row_sets_obligation_id_fkey;`
364+
in the ledger schema. Python obligations cannot satisfy that foreign key. See "Layout notes" for renamed constraints.
351365

352-
Until the shared Core layout (persistence Stage 3) each SDK keeps its own revision headers, so what is shared today
366+
Each SDK keeps its own revision headers, so what is shared today
353367
is the row sets: JavaScript-written row sets verify through `ReportingRowStorageV1.find` and `read_all` here, and the
354-
other way around.
368+
other way around. A shared Core layout is no longer planned; keep one writer SDK per ledger database.
355369

356370
The acceptance test runs both SDKs against one PostgreSQL schema and one filesystem bucket root, in both directions:
357371

‎scripts/check_main_policies.py‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,12 +176,21 @@ def event_context() -> tuple[str, str, dict[str, Any]]:
176176
return kind, target, event
177177

178178

179-
def conventional(kind: str, target: str, event: dict[str, Any]) -> dict[str, Any]:
179+
def conventional(
180+
kind: str, target: str, event: dict[str, Any], *, api: GitHub | None = None
181+
) -> dict[str, Any]:
180182
if kind == "push":
181183
commits = main_commits(target)
182184
else:
183185
pr = event["pull_request"]
184-
validate_message(pr["title"] + "\n\n" + (pr["body"] or ""))
186+
# Reruns retain the original webhook payload, including an obsolete PR
187+
# description. Read the editable message again without changing which
188+
# checkout and commits this run is allowed to validate.
189+
if api is None:
190+
api = GitHub(os.environ["GH_TOKEN"])
191+
current = api.repo(f"pulls/{pr['number']}")
192+
require(current["head"]["sha"] == pr["head"]["sha"], "PR head changed; run current CI")
193+
validate_message(current["title"] + "\n\n" + (current["body"] or ""))
185194
head, base = pr["head"]["sha"], pr["base"]["sha"]
186195
require(SHA.fullmatch(head) and SHA.fullmatch(base), "invalid PR source identities")
187196
fork = git("merge-base", base, head)

‎scripts/export_resolution.py‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@
2323

2424
#: Namespaces the snapshot covers: the two public surfaces an adopter imports
2525
#: from, the consolidated generated namespace behind them, and every generated
26-
#: domain module. Built on first use so this module imports nothing from
26+
#: domain module, plus the reporting and ledger public surfaces. Built on first
27+
#: use so this module imports nothing from
2728
#: ``adcp`` until asked.
2829
#:
2930
#: ``error_details`` is deliberately absent. Every name it carries is a class
@@ -39,6 +40,8 @@ def snapshot_modules() -> tuple[tuple[str, str], ...]:
3940
("adcp", "adcp"),
4041
("adcp.types", "adcp.types"),
4142
("adcp.types._generated", "adcp.types._generated"),
43+
("adcp.reporting", "adcp.reporting"),
44+
("adcp.reporting.ledger", "adcp.reporting.ledger"),
4245
*(
4346
(f"adcp.types.domains.{domain}", f"adcp.types.domains.{domain}")
4447
for domain in adcp.types.domains.DOMAINS

‎src/adcp/reporting/ledger/row_storage.py‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
against digests committed in the database before a row is released.
1010
1111
Row location lives in the ``adcp_reporting_row_sets`` side table, keyed by revision
12-
ID, until the shared Core layout. A revision without a row set still carries its
12+
ID. A revision without a row set still carries its
1313
rows in the legacy ``reporting_revision_rows`` table.
1414
1515
Two binding kinds exist. ``postgres`` stores chunk bodies as ``BYTEA`` in the same
@@ -583,13 +583,13 @@ async def _refuse_foreign_core_keys(connection: Any) -> None:
583583
"""Refuse a row-set table that another SDK created with a core-table foreign key.
584584
585585
The JavaScript migration references ``adcp_reporting_obligations``, which
586-
Python obligations never populate (they live in ``reporting_obligations``
587-
until the shared Core layout). Writing here would fail at the first row
588-
set, so refuse up front with the remedy.
586+
Python obligations never populate (they live in ``reporting_obligations``).
587+
Writing here would fail at the first row set, so refuse up front with the remedy.
589588
"""
590589
row = await (
591590
await connection.execute(
592-
"SELECT conname FROM pg_constraint WHERE conrelid ="
591+
"SELECT conname, quote_ident(conname), quote_ident(current_schema())"
592+
" FROM pg_constraint WHERE conrelid ="
593593
" to_regclass(quote_ident(current_schema()) || '.adcp_reporting_row_sets')"
594594
" AND contype = 'f' AND (SELECT relname FROM pg_class WHERE oid = confrelid)"
595595
" <> ALL(%s::text[]) LIMIT 1",
@@ -601,7 +601,8 @@ async def _refuse_foreign_core_keys(connection: Any) -> None:
601601
"STATE_UNAVAILABLE",
602602
"adcp_reporting_row_sets has a foreign key to another SDK's core table "
603603
f"({row[0]}); apply reporting_row_storage.sql before that SDK's migration, "
604-
"or drop the constraint",
604+
"or remove that foreign key with "
605+
f"ALTER TABLE {row[2]}.adcp_reporting_row_sets DROP CONSTRAINT {row[1]};",
605606
)
606607

607608
# -- providers --------------------------------------------------------
@@ -970,8 +971,10 @@ async def find_shareable(
970971
Identical rows are those with the same ``row_manifest_sha256``. Sharing
971972
never crosses an obligation, so it never crosses a consumer. A header-only
972973
revision references the existing chunks instead of storing them again.
973-
Takes ``FOR SHARE`` on the row set so retention cannot retire it before the
974-
caller's transaction commits.
974+
Takes ``FOR SHARE`` for the lifetime of the caller's transaction. Object
975+
staging releases that transaction before the ledger commit, so
976+
``persist_revision_row_set`` re-checks liveness and takes its own
977+
``FOR SHARE`` lock inside the commit transaction before sharing chunks.
975978
"""
976979
if not prepared.encoded.chunks:
977980
return None
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
"""Readiness failures supply a usable remedy for cross-SDK foreign keys."""
2+
3+
from __future__ import annotations
4+
5+
import pytest
6+
7+
from adcp.reporting.ledger import PgReportingLedgerStore
8+
from adcp.reporting.ledger.row_storage import ReportingRowStorageV1, ReportingRowStoreError
9+
10+
from ._generation_support import isolated_reporting_pool
11+
12+
13+
async def test_foreign_core_key_refusal_supplies_an_executable_quoted_remedy() -> None:
14+
async with isolated_reporting_pool() as pool:
15+
from psycopg import sql
16+
17+
store = PgReportingLedgerStore(pool=pool, row_storage=True)
18+
await store.create_schema()
19+
async with pool.connection() as connection:
20+
await connection.execute("CREATE TABLE other_sdk_obligations (id TEXT PRIMARY KEY)")
21+
await connection.execute(
22+
sql.SQL(
23+
"ALTER TABLE adcp_reporting_row_sets ADD CONSTRAINT {} "
24+
"FOREIGN KEY (obligation_id) REFERENCES other_sdk_obligations (id)"
25+
).format(sql.Identifier('other SDK "obligation" key'))
26+
)
27+
with pytest.raises(ReportingRowStoreError) as caught:
28+
await ReportingRowStorageV1._refuse_foreign_core_keys(connection)
29+
assert caught.value.code == "STATE_UNAVAILABLE"
30+
assert caught.value.detail is not None
31+
remedy = caught.value.detail.split("with ", 1)[1]
32+
assert 'DROP CONSTRAINT "other SDK ""obligation"" key";' in remedy
33+
await connection.execute(remedy)
34+
await ReportingRowStorageV1._refuse_foreign_core_keys(connection)

0 commit comments

Comments
 (0)