fix(node): repair source-less legacy pins by bounded additive discovery - #321
fix(node): repair source-less legacy pins by bounded additive discovery#321beardthelion wants to merge 2 commits into
Conversation
The boot sweep resolved each legacy row's bytes only through pin_sources_for_oid. A row predating provenance has repo_id NULL and no pin_repo_sources entry, so the source set is empty, the `for repo_id in sources` body never runs, and the cursor advances past it. list_pinned_cids also filters those rows out on is_raw_cidv1, so they were both unadvertised and unrepairable: nothing on the node would ever fix them. The sweep exists precisely for pre-provenance rows, and that was the shape it could not handle. For a source-less row, discovery now probes a bounded set of warm local repos for the object and, on a hit, repairs the key through the existing repair_legacy_provider_cid. What it records is the part worth reading closely. It calls record_pin_source additively and then mark_pin_sources_incomplete. It does NOT call backfill_pin_provenance, and repo_id stays NULL. Reading identical bytes out of a repo proves that repo HOLDS the object, not that it is the source: forks, a vendored file, a shared LICENSE blob and the empty tree all collide across repos. Since backfill_pin_provenance guards on AND repo_id IS NULL, the first claim written is permanent, so an exclusive claim would let probe order decide which repo's visibility rules gate a public read surface, with no correction path. It would also disable the resolver's own fallback: needs_scan is sources.is_empty() || at_cap || incomplete, so a non-empty below-cap unmarked set reads as complete and stops the scan that would have found a servable copy. The additive record plus the incomplete marker keeps that fallback alive and makes nothing permanent. Candidates are quarantined-filtered. list_all_repos is a bare SELECT over repos with no quarantine or visibility filter, while the resolver's own legacy scan loads list_quarantined_repos and gates on it, so discovery loads the same set and drops those ids. A quarantined repo is hidden from every reader, so it must not become a recorded source. Private non-quarantined repos stay in the list: an additive source record does not widen what the resolver will serve, since every source is gated independently. The candidate list and the warm filter load once per pass, lazily, on the first source-less row, and the is_dir sweep runs under spawn_blocking rather than inline on a worker. Nothing goes through repo_store.acquire, so a cold repo is never pulled back from remote storage; a repair pass must not become a bulk restore. Two classification rules matter more than they look. A cold or quarantined candidate does NOT mark the row retryable. The candidate list is every repo on the node rather than the row's holders, and sweep_legacy_provider_cids rewinds the cursor whenever any retryable skip occurred, so counting a cold candidate would make every source-less row retryable on every run on a cold-storage node: the cursor would rewind forever, the sweep would never drain, and each pass would pay full discovery cost. Only a read error on a warm candidate is evidence about the row. Conversely, cap exhaustion IS retryable rather than terminal, and candidates are ordered oldest-first by (created_at, id) rather than by id: repo_id derives from the owner DID, which anyone can grind, so a first-N-wins cap over an id-sorted list would let someone bury the true holder permanently behind cheap registrations. MAX_LEGACY_DISCOVERY_PROBES bounds the expensive unit, a bounded object read from a warm repo. A candidate dropped cold, quarantined, or unsafe costs no probe. Its doc states the contract it encodes rather than its resemblance to the resolver's own cap, which happens to share the value under a different contract. The source record is best-effort and says so at the site rather than claiming a later pass heals it. Once the key is rewritten the row is raw CIDv1, so the cost gate skips it free and nothing revisits it; the healing path is the resolver's needs_scan fallback, which the incomplete marker keeps available. The no-remote-fetch property is asserted from both sides. On the effect side, a cold candidate's disk path still does not exist after two full runs, with a control proving the sweep declined an available copy rather than finding nothing to take. On the call side, a source scan of this module (comments and string literals stripped, four anti-vacuity assertions) rejects the fetch-capable call shapes, and a mutation planting a real acquire in the probe loop turns it red. What that pair does not cover, and the test says so: a fetch reached transitively through git::store or db, and git's own promisor lazy-fetch on a partial clone. Neither shape exists in the tree today. Concurrency around the marker is executed rather than assumed. Discovery records then marks, in that order, because a successful record_pin_source clears the marker. Under a proven lock-wait interleaving, a concurrent record committing before discovery's mark leaves the row marked, and two concurrent passes over one row converge on a single source with the marker set. Both the swap and the drop of that pair are red-checked. One residual is pinned by a test rather than papered over, and it is not introduced here. pin_sources_incomplete is one boolean per object, not per (object, repo), so a genuine pusher whose record_pin_source commits strictly AFTER discovery's mark clears it, leaving a non-empty below-cap unmarked set that tells needs_scan to stop scanning. No ordering inside this sweep can prevent that, and record_pin_source's own doc already states the same residual for its existing call sites; closing it needs a per-(oid, repo) marker, which is a change to the marker's shape. It is bounded in practice: the clearing writer really holds the bytes, so every source in the set is servable, and for these rows it cannot be a regression, since before discovery ran they were keyed on a provider CID and were neither served nor advertised. Tests: fifteen regressions driving the sweep entry points. The repair, cheap second pass, probe cap and multi-holder cases were observed RED first. The must-not cases carry mutations instead, since a guard against something happening has no honest pre-fix red: quarantined holder never serves, a cold candidate is neither fetched nor materialized and does not rewind the cursor, an unsafe candidate name is dropped without aborting the pass while a later safe candidate still repairs, a raw-CIDv1 row never enters discovery, and a provenanced row still resolves through the source loop alone. Every guard carries a red-check mutation with an attributed message, including three on the design itself: one adds backfill_pin_provenance beside the additive record and must redden the no-exclusive-claim assertion, one neutralizes the incomplete marker, and one swaps the record and mark order.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
beardthelion
added
crate:node
record_pin_source's doc already names this residual and accepts it: the pin_sources_incomplete marker is one boolean per object rather than per (object, repo), so a genuine record from a third repo clears a marker that a different repo's failed record set. Its two neighbouring cases are tested, the same repo re-recording and a no-op insert that must not clear, and this one sat between them described in prose and guarded by nothing. The test drives the real path rather than writing rows. Repo A first-pins the object. Repo B genuinely holds it and pushes, its skip-branch record fails on the definite-error arm, and the code marks the set incomplete. An anonymous read is then SERVED from B, which is the assertion that matters: it proves the unrecorded holder is reachable while the marker stands. Repo C pushes, its record succeeds, and the clear fires inside C's transaction. The identical read now 404s, with the preload counter proving the fallback scan never ran. Nothing here changes behavior. It converts an accepted tradeoff from a comment into a guard, so widening it takes a deliberate edit rather than going unnoticed. The doc comment says the residual fails closed, and that holds: the provenance loop and the fallback scan both serve through the same gate_and_serve, so clearing the marker only ever removes a gated search and can never serve an object the caller could not already read. The window is exactly one to fifteen sources, because at cap the count-guarded insert is a no-op and the marker survives. If someone implements the per-(oid, repo) marker the doc proposes, this test is expected to go red. It should be updated to the new behavior, not deleted. Proven load-bearing: removing the clear from record_pin_source leaves the second read at 200 and turns the test red on the marker assertion.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Make capped discovery eventually examine every eligible holder
crates/gitlawb-node/src/ipfs_pin.rs:442
When a node has more than 16 warm, non-quarantined repositories, this always probes the same oldest 16. TheRetryableresult only rewinds the pinned-CID cursor, so the next run rebuilds the same ordering and repeats the same prefix. A source-less legacy object held only by repository 17 (or later) is therefore never repaired or advertised, while every boot repeats the work indefinitely. Persist/derive a bounded rotating candidate continuation so retries can reach later candidates without losing the per-run work cap. -
[P1] Do not let one timed-out row exhaust discovery for the whole pass
crates/gitlawb-node/src/ipfs_pin.rs:379
DiscoveryCtxgives every source-less row in a pass one shared deadline. If the first row spends that budget on a slow or hung candidate, all later rows callrepair_legacy_provider_cidwith an expired deadline, become retryable without a meaningful probe, and trigger a rewind. Stable CID ordering then lets the same first row consume the budget on every boot, permanently starving later, otherwise repairable rows. Allocate a bounded deadline per row (or persist fair resume state) instead of sharing it across the pass. -
[P1] Keep the incomplete marker durable when discovery adds its source
crates/gitlawb-node/src/ipfs_pin.rs:445
record_pin_sourceandmark_pin_sources_incompleteare separate best-effort writes. If the source insert succeeds but the marker update fails, the repaired row is raw CIDv1 with a nonempty, below-cap, unmarked source set; later sweeps skip it, and/ipfstreats that set as complete and suppresses its fallback scan. Thus, after discovery selected a private holder, an unrecorded public duplicate can remain permanently unavailable once the transient DB error clears. Commit the source and scan-required state atomically, or retain a durable retry/scan trigger when the marker cannot be written.
#173 removed Db::list_all_repos in favour of a keyset-paged list_repos_page_for_scan, so this branch stopped compiling on rebase. One call site, load_discovery_ctx, ported with its reviewed semantics intact. The quarantine drop gets simpler rather than different: each page row carries its own quarantined flag, so the separate list_quarantined_repos query is gone and the filter runs over rows the pass already read. Private non-quarantined repos still stay in the list. Warm-only via validated_repo_disk_path inside spawn_blocking is untouched, so nothing here reaches repo_store.acquire and a repair pass still cannot become a bulk restore, and an unsafe path is still dropped with the same warn and stays terminal. Oldest-first by (created_at, id) now comes from the query's ORDER BY (index backed by migration v25) instead of a client-side sort, and pages concatenate in that order, so the explicit sort_by is redundant and removed. The anti-grinding reason for that order is unchanged and the doc comment keeps it: repo_id derives from a grindable owner DID, so an id sort would let someone bury the true holder past the probe cap. The probe cap still bounds expensive reads rather than candidates considered, and no cap on candidates was added. This pager runs to exhaustion, unlike the resolver's, which drives the same query and stops when its probe or visit budget is spent. The comment says why: the resolver is an anonymously reachable route holding scarce walk admission, while this is background maintenance on a timer with no caller and no permit contention that needs the whole warm candidate set to settle a row. Paging is per pass, matching load_discovery_ctx's existing once-per-pass lifetime. Refs #321.
|
Closing this in favour of #173, which now carries the work. Stacking it separately was my call and it was the wrong one. The argument I made was that #173 had to land first and this immediately after with no release between them, which only holds if nothing ever ships in that gap. That is not a property I can guarantee, and jatmn's point stands: the resolver change in #173 withholds the provider CID for an unrepaired row, so shipping it without the repair leaves an upgraded node's existing pins unavailable. They belong in one change. What landed is ahead of this branch's pushed head. This PR points at The three findings from the review round here are fixed in #173 rather than carried over: The capped discovery that always probed the same oldest sixteen is now a persisted keyset continuation advanced once per completed table traversal, over probes that started with live budget only, persisted from inside the pass so a shutdown cannot swallow it. The sweep also re-arms on a timer instead of running once per boot, which is what makes the reach bound wall-clock rather than a count of restarts. The shared per-pass deadline that let one hung candidate starve every later row is now a per-row slice of the pass budget, and a row reached after the budget is spent is skipped free rather than charged for a probe it cannot make. The source insert and the fallback-arming marker now commit in one transaction, so the nonempty-below-cap-unmarked state that suppressed the resolver's fallback is unreachable from discovery. Each was verified by reverting the exact line it depends on and watching the named assertion go red, including present-but-wrong variants where the fix adds a condition rather than removing one. Discussion continues on #173. |
2 participants
Answers the third P1 from the #173 review round: the legacy CID sweep could not repair pre-provenance pins.
Stacked on #173, deliberately. The base is
fix/issue-135-ipfs-cid-tree-gate, notmain. Everything this touches (sweep_pass,repair_legacy_provider_cid,pin_repair_cursor,record_pin_source,pin_sources_for_oid) is introduced by #173 and does not exist on main, so this cannot branch independently. It is split out rather than folded in because it adds a new mechanism to startup behavior, where the other two findings were clamps on existing awaits, and reviewing it properly turned up enough to deserve its own round.The defect
The sweep resolved each legacy row's bytes only through
pin_sources_for_oid. A row predating provenance hasrepo_idNULL and nopin_repo_sourcesentry, so the source set is empty, thefor repo_id in sourcesbody never runs, and the cursor advances past it.list_pinned_cidsalso filters those rows out onis_raw_cidv1, so they were both unadvertised and unrepairable. The sweep exists precisely for pre-provenance rows, and that was the one shape it could not handle.The fix, and the design decision worth reviewing
Discovery probes a bounded set of warm local repos for the object and, on a hit, repairs the key through the existing
repair_legacy_provider_cid.What it records is the part to look at. It calls
record_pin_sourceadditively and thenmark_pin_sources_incomplete. It does not callbackfill_pin_provenance, andrepo_idstays NULL.The tempting version writes the discovered repo as the first-pinner. That is unsafe. Reading identical bytes proves a repo holds the object, not that it is the source, and objects collide across repos routinely: forks, a vendored file, a shared LICENSE blob, the empty tree. Because
backfill_pin_provenanceguards onAND repo_id IS NULL, the first claim written is permanent, so probe order would decide which repo's visibility rules gate a public read surface with no correction path. It would also switch off the resolver's own fallback, sinceneeds_scanissources.is_empty() || at_cap || incompleteand a non-empty below-cap unmarked set reads as complete. The additive record plus the marker keeps that fallback alive and makes nothing permanent. There is a mutation that adds the exclusive claim back and must redden the no-claim assertion, so the decision is guarded rather than just described.Properties worth checking in review
list_all_reposhas no quarantine or visibility filter, while the resolver's own legacy scan loadslist_quarantined_reposand gates on it, so discovery loads the same set and drops those ids. Private non-quarantined repos stay: an additive source record does not widen what the resolver serves, since every source is gated independently.repo_store.acquire; a repair pass must not become a bulk restore. Asserted from both sides: the cold candidate's path still does not exist after two runs, with a control proving the sweep declined an available copy, plus a source scan that a plantedacquireturns red.(created_at, id).repo_idderives from the owner DID, which anyone can grind, so a first-N-wins cap over an id-sorted list would let someone bury the true holder permanently behind cheap registrations.Known residual, not introduced here
pin_sources_incompleteis one boolean per object rather than per(object, repo), so a genuine pusher whoserecord_pin_sourcecommits strictly after discovery's mark clears it, leaving a non-empty below-cap unmarked set that tellsneeds_scanto stop scanning. No ordering inside this sweep prevents it, andrecord_pin_source's own doc already states the same residual for its existing call sites. It is bounded in practice, since the clearing writer really holds the bytes so every source stays servable, and for these rows it cannot be a regression, because before discovery ran they were keyed on a provider CID and were neither served nor advertised. A test pins it as known. Closing it properly needs a per-(oid, repo)marker, which is a change to the marker's shape and belongs on its own.Verification
Fifteen regressions driving the sweep entry points. The repair, cheap second pass, probe cap and multi-holder cases were observed failing first. The must-not cases carry mutations instead, since a guard against something happening has no honest pre-fix red: quarantined holder never serves, a cold candidate is neither fetched nor materialized and does not rewind the cursor, an unsafe candidate name is dropped without aborting the pass while a later safe candidate still repairs, a raw-CIDv1 row never enters discovery, and a provenanced row still resolves through the source loop alone.
Every guard carries a mutation that reverts it and requires the failure to match a named message, so a red is attributed to the property rather than to removing the seam the test injects at. Three of those target the design itself: adding
backfill_pin_provenancebeside the additive record, neutralizing the incomplete marker, and swapping the record and mark order. The suite passes with fmt, clippy, andmetadata --lockedclean; CI on this head is the check to read.Reasoned-not-run and stated rather than folded in: a fetch reached transitively through
git::storeordb, and git's promisor lazy-fetch on a partial clone. Neither shape exists in the tree today.