Key the capture-path vault by location, so a Run can hold more than one - #699
Merged
Conversation
run_capture_path had run_id as its PRIMARY KEY: one observed path per Run, upserted with ON CONFLICT (run_id) DO UPDATE. The next slice registers a second Distribution once a scan's bytes reach APS Data Management, and that is a second real path for the same Run (/gdata/dm/2BM/<yyyy-mm>/<exp>/data/<file>.h5 alongside /local1/2BM/<exp>/<file>.h5) carrying the same PI surname, so it needs the same vaulting. Writing it would have overwritten the acquisition path in place, and every cora-capture-path:// locator minted against that path would have stopped resolving, silently, with no event recording that it happened. Nothing writes a second path yet, so this closes a trap laid in the next slice's path rather than a live defect, and it is cheapest now: the sweep that mints those locators is still switched off. host and root are NULLABLE because rows written before this migration genuinely have no recorded location, and NULL says so where a sentinel string would assert a location nobody measured. That pulls in UNIQUE NULLS NOT DISTINCT (PG15+, deployed on PG18): under the default NULLS DISTINCT every legacy row would be unique against itself and ON CONFLICT would insert duplicates instead of re-observing. It also forces the surrogate capture_path_id primary key rather than merely recommending it, since a PRIMARY KEY cannot span nullable columns. That key DEFAULTs to gen_random_uuid(): a key carrying no meaning a writer could get right or wrong should not be ceremony every INSERT has to remember. The locator is minted from the location the ROW RECORDED, not from a fresh match against current settings. resolve() keys the vault lookup on host and root, so correctness depends on the minted pair equalling the stored pair; deriving them independently at two different times (the Run's terminal, and the sweep tick arbitrarily later) meant any settings change between them produced a locator that resolved to nothing. Current settings still get a say, as an explicit refusal rather than a re-match: a row whose recorded root has left the allowlist is skipped, because the probe would decline to read it anyway. Rows with no recorded location are excluded from candidacy outright, so they cannot become a stuck head the sweep re-selects every tick forever. Root matching and normalization moved to cora.shared.storage_root. They cannot live in either BC: cora.run.aggregates depends only on infrastructure and shared, so the aggregate that persists and keys on root could not import the function that defines it, and that is exactly how the normalization drifted. The recorder stored a configured root verbatim while the locator embedded an rstripped copy, so a single trailing slash in settings meant the two held different strings for one directory and every lookup missed, permanently, for every run. get_latest() replaces latest() (every sibling store method is a verb) and orders by observed_at, updated_at, then capture_path_id, making the tie total and identical in both adapters. Preferring the acquisition tier instead would point the display read at exactly the copy that gets capacity-purged first. The ordering is deliberately lossy and documented as such: it answers where this was last seen, not everywhere CORA has seen it. Reviewed by a four-lens panel. The migration was applied to a real PG18 container and measured; no P0. Both P1s were the mint/record split above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
run_capture_pathheld one observed path per Run (run_idas PRIMARY KEY,ON CONFLICT (run_id) DO UPDATE). It now holds one per storage location.Why now
The next slice registers a second Distribution once a scan's bytes reach APS Data Management. That is a second real path for the same Run, carrying the same PI surname, so it needs the same vaulting. Under the old key, writing it would have overwritten the acquisition path in place and every
cora-capture-path://locator minted against it would have stopped resolving, silently, with no event recording that it happened.Nothing writes a second path yet, so this closes a trap laid in the next slice's path rather than a live defect. It is cheapest now: the sweep that mints those locators is still switched off.
Schema
hostandrootare nullable, because rows written before this migration genuinely have no recorded location andNULLsays so where a sentinel would assert a location nobody measured. That pulls inUNIQUE NULLS NOT DISTINCT(PG15+, deployed on PG18): under the default, every legacy row would be unique against itself andON CONFLICTwould insert duplicates instead of re-observing. It also forces the surrogatecapture_path_idPK rather than merely recommending it, since aPRIMARY KEYcannot span nullable columns.Mint from the recorded location, not from settings
resolvekeys the vault lookup on host and root, so correctness depends on the minted pair equalling the stored pair. Deriving them independently at two different times (the Run's terminal, and the sweep tick arbitrarily later) meant any settings change between them produced a locator that resolved to nothing. The candidate now carries its recorded location andmintconsumes it. Current settings still get a say, as an explicit refusal rather than a re-match.Rows with no recorded location are excluded from candidacy outright, so they cannot become a stuck head the sweep re-selects every tick forever.
Root matching moved to
cora.sharedIt cannot live in either BC:
cora.run.aggregatesdepends only on infrastructure and shared, so the aggregate that persists and keys onrootcould not import the function defining it. That is exactly how the normalization drifted, and the bug it produced was real: the recorder stored a configured root verbatim while the locator embedded anrstripped copy, so one trailing slash in settings meant the two held different strings for one directory and every lookup missed permanently.Review
Four-lens gate panel (security, data/migration, architecture, test coverage). The migration was applied to a real PG18 container and measured statement by statement; no P0. Both P1s were the mint/record split above, found independently by three reviewers.
Also fixed from the panel:
latestrenamedget_latestwith a total ordering (capture_path_id DESC) so ties are identical in both adapters; a docstring that described a mechanism the code did not contain; a stale comment contradicting the docstring it pointed at; an inaccurate precedent citation; and the erasure guidance redirected torun_id(deleting one location and leaving a sibling keeps the surname on disk).Coverage the panel was right about: the recorder's location write had never executed in its matched branch, and the host axis of the new key was decorative (deleting it from the lookup left the whole suite green). Both now tested, with positive controls.
Verification
architecture 31,288 · integration + architecture re-run 32,556 · unit + integration + contract 18,582 · ruff, pyright, tach clean. No P0 findings.
🤖 Generated with Claude Code