fix(reporting): reuse staged payloads across source observations

[bokelley]

Repeated acquisitions with identical row bytes previously created separate staged objects because the object reference included the acquisition key. The memory and filesystem staging stores now use an account-scoped content reference, allowing observations to reuse verified payload bytes while retaining distinct manifests and seals.

Filesystem publication writes and fsyncs an owned temporary file, atomically links it without replacing a concurrent winner, and fsyncs the final directory chain. Reuse verifies retained bytes. The initial root durability check syncs only directories created for that root and the nearest existing parent, so staging also works beneath a traversable but unreadable ancestor. A fresh instance also best-effort syncs an existing root's parent to repair a root name left unsynced by another writer; permission denial on that parent does not prevent reuse. The restricted-ancestor regression enforces its own access denial even on privileged runners. Existing NDJSON and legacy opaque-reference reads remain compatible.

The installed PostgreSQL timeout correction is now provided by merged #1221 (2,400-second PG aggregate bound with 50/45-minute CI job/step limits); the redundant smaller correction was dropped during rebase. No staging behavior or test selection changed in that integration.

Validation on the current head: make lint, make typecheck-all, commit hooks, and 76 focused staging/inline-source/lifecycle tests pass. The POSIX restricted-ancestor regression passes. make test passed on this head (11,884 passed, 2,255 skipped, 9 deselected, 1 xfailed; 81.68% coverage). Fresh CI is running.

[bokelley]

Rebased the four staging commits onto current main after #1218 merged; the source delta is unchanged. Current-head lint/typecheck and 48 focused staging/evidence tests pass (28 skipped). Fresh CI is running. @aao-secretariat please review the current head when checks finish.

[bokelley]

@aao-secretariat #1224 is ready for independent review on e354b32. All current-head CI checks passed before ready status; the ready-triggered code_review is running. No review threads remain open.

[aao-secretariat]

Ladon verdict: Approve

Approve — account-scoped content-addressed staging refactor with correct fail-closed filesystem publish semantics.

What I checked:

• Filesystem publish path: fsync temp → hard-link without replacing a winner → verify-before-reuse, fail-closed on corruption. Correct and exhaustively tested.
• Account isolation, legacy-ref read compatibility, and the fix: semver signal all check out — no public-surface break, no discriminated-union change, no generated-code edits, no import-layering violations.
• Diff is limited to src/adcp/reporting/inline_source.py plus two test files (conformance + staging-reuse), consistent with a non-breaking bug fix.

Medium findings:

• src/adcp/reporting/inline_source.py — _sync_directory fsyncs every ancestor up to filesystem root on each stage, including the hot reuse path, buying no durability while contending on shared parent inodes. Worth tightening but not blocking.

Decision path: no critical/high findings; gated_paths false; high_risk false; only one medium finding (not data-loss/schema/infra category); no author-team gate; fewer than three mediums. Falls through to row 9 → approve.

Note: review_decision is REVIEW_REQUIRED but gated_paths is false, so row 2 does not fire.

[bokelley]

@aao-secretariat The Medium fsync finding is fixed on 0596246 and its thread is resolved. Please review this current head when available; fresh CI is running.

[aao-secretariat]

Ladon verdict: Approve

Approve — durability-barrier optimization in FileSystemStagingStore, clean pass.

The delta narrows per-write directory fsync to the store's own root (walking target.parent up through self._root) with a one-time root/ancestor commit via _ensure_root_durable, guarded by a threading.Lock. The fsync chain preserves crash consistency, every failure path stays fail-closed (retained bytes, no escaped seal), and no public API / wire-shape / semver / import-layering surface is touched. New and updated tests cover the behavior.

No critical/high/medium findings. Consistent with the prior approve on the earlier head; no prior blocker to re-surface.

Decision path: 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 (row 6 n/a); no no-auto-approve team match (row 7 n/a) → row 9 approve.

[bokelley]

The installed PostgreSQL sdist lane hit its 1,800-second aggregate deadline after 1,443 tests, during typing, without an assertion failure. I cherry-picked the already-reviewed narrow timeout correction from #1217 to head 6f4cc7b. Local lint, typecheck, and 20 focused tests pass; fresh CI is running. @aao-secretariat, please review and post an APPROVE on this exact head once satisfied. All existing threads are resolved.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean pass, no findings.

Delta since the prior approve (head 0596246) is three coordinated timeout bumps: CI job timeout-minutes 40→45, step timeout-minutes 35→40, and the _production_packaging.py subprocess timeout 1800→2100s. The timeout hierarchy holds (subprocess 35m < step 40m < job 45m, ~5-min headroom per layer), so the inner run_step SIGTERM→SIGKILL evidence-capture path still fires first — fail-closed ordering preserved.

No public surface, wire shape, credential handling, or spec touched; no semver signal required. No CI gate disabled. high_risk=false, gated_paths=false, no author team gate. No blocking or medium findings.

[bokelley]

Exact-head technical verdict — 6f4cc7b38ab820c8209dde29993e601e16e168d5

No blocking findings. Technically good to merge once CI is green.
(Posting as a comment, not a formal review — the formal exact-head APPROVE will come from the separate aao-secretariat identity.)

The delta is exactly what was described

Tip commit touches 2 files, +3/−3, all numeric:

	before	after	margin
aggregate test timeout (_production_packaging.py)	1800 s (30 m)	2100 s (35 m)	—
step timeout-minutes	35 m	40 m	+5 m over aggregate
job timeout-minutes	40 m	45 m	+5 m over step

The ladder is the substantive thing to check here, and it holds: aggregate < step < job, with the 5 m / 5 m headroom preserved exactly as before. The bump is uniform, so the change can't invert the ordering or let the job deadline fire before the aggregate one — which is what would turn a slow run into an uninformative kill instead of a readable test timeout. Matches the stated cause (1,443 tests completed, typing cut off by the old deadline).

One thing worth flagging, since my earlier approval was dismissed

The staging durability code on this head is not the same implementation I approved previously — the branch was rebased and carries 05962460c ("bound staging fsync to store root") rather than the variant I'd reviewed. I re-reviewed it rather than assuming continuity. It is equivalent on the property that matters and better on two counts:

• _ensure_root_durable() does the full, unconditional ancestor walk once per instance under a lock, including when the root already exists — the property that matters, since existence is not durability evidence (a peer may have created an ancestor without syncing its parent).
• It's called at the top of _write, and _write is now an instance method — so the barrier is structurally enforced rather than being a caller contract. That closes a residual I'd raised against the earlier variant.
• _sync_directory derives its stop point from self._root instead of a hardcoded depth, so it stays correct if _path ever gains a level. That closes a second residual.

Nits (optional, non-blocking, not regressions — main doesn't have these either)

1. _write uses target.parent.mkdir(parents=True, exist_ok=True) while _root_synced latches True. If the store root is removed externally after the first stage, a later stage silently recreates the chain and _sync_directory stops at self._root, so the root's own entry in its parent is never re-synced. Narrow (requires external removal mid-process); a non-recursive mkdir would fail closed instead.
2. __init__ keeps Path(root) unresolved, so a relative root re-binds if the process chdirs; and payload opens don't use O_NOFOLLOW. Both pre-existing on main.

What I ran at this exact head

• make lint — clean
• pytest -k staging — 50 passed, 32 skipped (skips are PG-gated; no local PG)
• pytest tests/test_reporting_inline_source.py tests/test_reporting_staging_reuse.py — 50 passed

CI

35 SUCCESS / 14 running / 0 failures. base-vcs and base-sdist are already green. pg-sdist is the decisive one — it's the job the headroom change targets and it's still in flight, so treat this verdict as conditional on it landing green.

[bokelley]

@aao-secretariat #1224 is rebased on merged #1221 at ad5c6dc. The redundant timeout cherry-pick was dropped because main already has the larger PG-specific bound; staging source behavior is unchanged. Current-head make lint, make typecheck-all, and 44 focused staging/lifecycle tests pass. Fresh CI is running; please renew formal approval on this exact head when reviewed.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean re-review, one non-blocking medium noted inline.

This PR touches only src/adcp/reporting/inline_source.py and three test files. The core change swaps os.replace for os.link + FileExistsError/verify, correctly preventing a concurrent winner from being clobbered, with fail-closed corruption handling and thorough fault-injection tests.

Medium findings

• src/adcp/reporting/inline_source.py — _ensure_root_durable fsyncs every ancestor up to / rather than just store-owned directories, raising PermissionError on the first stage under a traversable-but-unreadable ancestor (mode 711); contradicts the head commit's "bound fsync to store root" intent. Non-blocking.

Decision trace: no critical/high findings (row 1 n/a). gated_paths is false (row 2 n/a). Not high-risk / no deletions (rows 3, 5 n/a). The single medium is not in data-loss/schema/infra category (row 4 n/a). Prior decision was approve, not escalate (row 6 n/a). No no-auto-approve team match (row 7 n/a). Only 1 medium finding, fewer than 3 (row 8 n/a). Falls through to row 9 → approve.

[aao-secretariat]

Ladon verdict: Approve

Approve — no blocking or medium findings.

This is a subsequent pass (prior outcome: approve). The delta narrows _ensure_root_durable in inline_source.py from fsyncing the root plus every ancestor up to / down to only the newly-created directories plus the first existing ancestor — a correct durability superset that avoids opening traversable-but-unreadable ancestors. Loop termination, concurrency (mkdir exist_ok), and fail-closed semantics all hold.

The only noted item is Low: the new test exercises the boundary path but, because the process owns the 0o711 ancestor, it isn't a true fail-without-fix guard. Low findings are omitted and do not block.

Decision-table walk: no critical/high (row 1 no); gated_paths false (row 2 no); high_risk false, no deletions/modifications triggers (rows 3–5 no); prior decision was approve, not escalate (row 6 no); no no-auto-approve team match (row 7 no); zero medium findings (row 8 no). Falls through to row 9 → approve.

Note: review_decision is REVIEW_REQUIRED, but gated_paths is false, so no hard gate applies.

[bokelley]

Technical verdict — ab469df1 (permission fix + rebase)

The permission fix is correct and I'd merge it. No blocking findings. Two things worth your attention below — one about the test, one about a narrowed durability property.

The fix does what it claims — traced, not just read

I instrumented os.open and watched which directories actually get fsynced:

Fresh chain (<base>/a/b/c/staging, three missing levels):

fsync: <base>/a/b/c/staging   <- missing
fsync: <base>/a/b/c           <- missing
fsync: <base>/a/b             <- missing
fsync: <base>/a               <- missing
fsync: <base>                 <- boundary (first pre-existing)

Every created directory's parent is covered, and the walk stops at the first pre-existing ancestor — nothing above it is ever opened. That's the Medium closed: 0711 ancestors are no longer touched.

Edge cases hold up too: a relative root terminates the boundary walk at .; a root path that's an existing file fails closed with FileExistsError; and the if parent == boundary: raise FileNotFoundError guard means the walk can't spin at /.

⚠️ The regression test passes on the unfixed code (in some environments)

I ran the new test_staging_under_traversable_unreadable_ancestor against the pre-fix inline_source.py:

[1] new test on FIXED code    -> 1 passed
[2] same test on PRE-FIX code -> 1 passed   <- guards nothing here

The guard is os.geteuid() == 0, which doesn't catch a non-root process holding CAP_DAC_OVERRIDE — common in containers and some CI images. My runner is uid 1000 and O_RDONLY on a 0o711 directory still succeeds, so the chmod never bites and the test is vacuous.

Worth making it self-verifying: after the chmod, try opening restricted and skip only if it raises — otherwise the environment can't substantiate the regression and the test should say so rather than passing quietly. I therefore cannot confirm the fix from this test locally; CI's runner is authoritative for it. The trace above is what convinces me the fix is right.

Observation: the pre-existing-root case no longer syncs the root's parent

Same trace, second instance against an already-created root:

CASE 2 (root pre-exists, fresh instance)
fsync: <base>/a/b/c/staging          <- root only
dirs synced at/above the root's parent: NONE

The comment says "the first pre-existing ancestor is sufficient." That holds for ancestors created outside this process's lifetime, but not for the one case previously called out on this PR: if instance A creates the root and dies between mkdir(parents=True) and its fsync loop, the root's own name isn't durable — and instance B now finds root.is_dir() true, takes the missing-empty branch, and never repairs it. The prior full walk did repair that; this narrowing gives it up.

The window is tiny (microseconds, and journals flush on their own within seconds), and the prior behaviour was outright broken under ordinary permissions, so this is a reasonable trade — I'd just rather it be a stated decision than a silent one. If you want both properties cheaply:

with contextlib.suppress(OSError):
    self._sync_one_directory(boundary.parent)   # best-effort repair

Readable parent → durability restored; traverse-only parent → skipped, permission fix intact.

Rebase (6f4cc7b → ad5c6dce) — no new surface

Checked separately, since the raw head-to-head diff is 58 files and dominated by main's own progress. Comparing each head's net contribution against its own base, the PR contributes the same four files and all four are byte-identical to what I reviewed at 6f4cc7b.

The headroom commit is gone from the branch because it was superseded, not lost — main is now strictly more generous (aggregate 2400 s, step 45 m, job 50 m, ladder margins intact), and the PR no longer touches either file (both byte-identical to main).

What I ran at ab469df1

• make lint — clean
• All four PR-touched test files against real PostgreSQL — 134 passed, 0 skipped (without PG it's 105 passed / 28 skipped)
• The test-the-test run above, and the os.open trace for both branches of the new logic

CI at time of writing: 10 SUCCESS / 37 pending, no failures.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean subsequent pass, no blocking or medium findings.

This is a follow-up to a prior approve. The delta touches src/adcp/reporting/inline_source.py and its tests. The reviewer verified:

• The inline_source.py change adds a best-effort parent fsync when the staging root pre-existed, guarded against the filesystem root and fail-open on PermissionError — a correct durability trade-off, not a trust-boundary regression.
• The rewritten test_reporting_staging_reuse.py monkeypatches os.open to deterministically enforce the restriction, with assertions matching the new control flow.

No credential-in-metadata concerns, no generated-code hand edits, no type-layering or discriminated-union changes, no public-surface semver signal implicated. No high-risk paths, no gated paths, no author-team gate.

Decision table: rows 1–8 do not fire (no critical/high/medium findings, gated_paths false, high_risk false, no team gate, fewer than three mediums). Falls through to row 9 → approve.

[bokelley]

Technical verdict — 7183783c

Both of my previous observations are resolved, and I verified each resolution by proving the guarding test fails without its fix. No findings. Good to merge once CI is green.

Delta from ad5c6dce is just the two expected files (+68/−1).

1. The regression test now actually guards the regression

This was my main concern last round — the test passed on the unfixed module here, because this runner holds CAP_DAC_OVERRIDE and geteuid() != 0, so the chmod 0o711 never bit. Forcing PermissionError through a monkeypatched os.open removes the environment dependency entirely. Re-ran the same experiment:

[1] hardened test on FIXED code        -> 1 passed
[2] hardened test on PRE-FIX module    -> 1 failed
        E  PermissionError: unreadable ancestor

It now fails where it must, on the same machine where it previously passed vacuously. The pytest.raises(...) self-check on the forced restriction is the right touch — the test proves its own premise before relying on it.

Also worth noting the assertion is on attempts, not successes: opened.append(path) happens before the raise, so assert restricted not in opened catches a probe even if the code would have swallowed the error. That's the stronger form.

2. The repair works, and the assertion guarding it is real

Traced the fsync set at this head — a fresh instance against a pre-existing root now syncs the root's parent:

CASE 2 (root pre-exists, fresh instance)
fsync: <base>/a/b/staging
fsync: <base>/a/b            <- repair, previously absent
root's parent synced: True

And the new assert owned in opened isn't decorative — I ran the hardened test against ab469df1 (the head without the repair) and it fails exactly there:

>  assert owned in opened  # repair a possible unsynced root name
E  AssertionError

So the cross-instance repair for "creator died between mkdir and its fsync loop" is restored and regression-guarded.

3. The tolerance branch the test doesn't reach

In the test, root.parent is owned, which is readable — so the except PermissionError path never executes there. I exercised it separately with the parent itself forced unreadable:

root's parent UNREADABLE, root pre-exists -> stage SUCCEEDED
attempted to open parent: True (swallowed, not fatal)

Best-effort as intended: it tries, swallows, and staging still succeeds. The permission fix holds in the worst case.

On the narrower catch

You used except PermissionError rather than the blanket suppress(OSError) I suggested — that's better than what I proposed. A parent that vanished (FileNotFoundError) or isn't a directory still fails closed instead of being silently ignored; only the one condition that's legitimately tolerable is tolerated. Worth keeping as-is.

What I ran at 7183783c

• make lint — clean
• All four PR-touched test files against real PostgreSQL — 134 passed, 0 skipped
• Test-the-test against both prior heads (ad5c6dce, ab469df1), plus an os.open trace and a forced-unreadable-parent probe

CI at time of writing: 12 SUCCESS / 36 pending, no failures.