fix(node): implement reconciliation sweep as durability backstop (#218) - #244
fix(node): implement reconciliation sweep as durability backstop (#218)#244Gravirei wants to merge 9 commits into
Conversation
github-actions
Bot
added
the
needs-tests
|
Thanks for the contribution. A couple of things will help us review this faster:
See CONTRIBUTING.md. Update the PR and these notes will clear automatically. |
|
Warning Review limit reached
Next review available in: 58 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe node adds a bounded periodic reconciliation worker that restores missing public pins and encrypted recovery copies. Database APIs distinguish local IPFS pins from Pinata-only records, while Git subprocess tracking, startup wiring, and Prometheus counters support the worker. ChangesDurability reconciliation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NodeStartup
participant ReconciliationWorker
participant Database
participant GitScan
participant Pinning
participant Metrics
NodeStartup->>ReconciliationWorker: start periodic sweep
ReconciliationWorker->>Database: list repository batch and pinned OIDs
ReconciliationWorker->>GitScan: scan visible repository objects
ReconciliationWorker->>Pinning: pin missing objects and reseal withheld blobs
Pinning->>Database: record pin results
ReconciliationWorker->>Metrics: record gaps found and filled
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
beardthelion
added
crate:node
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The security design here is genuinely careful, and I want to lead with that: I could not construct any input (rule shape, is_public value, quarantine timing, or DID form) that makes the sweep pin, announce, seal-in-plaintext, or anchor content a private repo must withhold. The announceable gate evaluates the anonymous perspective (listable_at_root(..., None), so the owner short-circuit never fires), the object filter is the anon-perspective fail-closed set, all four sinks run only on that filtered set, the encrypted phase seals ciphertext, and quarantine is rechecked before pinning and fails closed on error. That is the hard part and it is done well.
The durability mechanics are where the problems are: one hard break plus several coverage/cost holes that undercut the guarantee the PR is written to provide. Findings highest first.
Findings
-
[P1] Drop the
pinned_cids.cidNOT NULL constraint before writing NULL Pinata-only rows
crates/gitlawb-node/src/db/mod.rs:2342
record_pinata_cidnow bindscid = NULLfor new rows, but the column iscid TEXT NOT NULLand no migration relaxes it. Every first-time Pinata pin fails the INSERT with a NOT NULL violation — this is not sweep-only, it globally breaks the Pinata write path (the push-time pin calls the same function), so Pinata-only state never records and the caller retries into the same error. Main bindscid = pinata_cid, so this is a regression introduced here. Ship a new migration that doesALTER TABLE pinned_cids ALTER COLUMN cid DROP NOT NULL(and reconcile it with main's existing pinata_cid work, see the stale-base note below). Reproduce with a fresh object, Pinata configured, IPFS unconfigured: the insert errors andhas_pinata_cidstays false. -
[P2] Subtract already-pinned objects before the per-repo cap, or page within the repo
crates/gitlawb-node/src/reconciliation.rs:181
object_listis truncated toMAX_OBJECTS_PER_REPO(50k) before the IPFS/Pinata missing-set is computed. On a stablelist_all_objectsorder, a repo with more than 50k replicable objects always presents the same prefix; if that prefix is already pinned and the dropped object sits past it, the gap is never a candidate and the sweep reports success while the hole persists — exactly the large-history case the backstop exists for. Compute the missing set first (or page the scan) so coverage does not stop at the cap. -
[P2] Order the sweep cursor by a stable key so idle repos are not starved
crates/gitlawb-node/src/reconciliation.rs:99
The cursor is a positional index intolist_all_repos_deduped(), which isORDER BY updated_at DESC. Every push reshuffles that order, so hot repos cluster at low indices while cold/idle repos drift around the cursor and can be skipped indefinitely — and idle repos are precisely the ones with only the sweep as a safety net. Order the eligible set by a stable key (id or created_at) so the positional cursor deterministically covers everyone. -
[P2] Bound the object walk itself, not only the post-walk pin batch
crates/gitlawb-node/src/reconciliation.rs:142
list_all_objectsrunsgit cat-file --batch-all-objectsand materializes one String per object with no streaming, beforeMAX_OBJECTS_PER_REPOapplies. A repo with millions of loose objects spikes ~1GB transient on one blocking thread per pass; since repos are sequential, one pathological repo stalls the rest of that pass. The comment at the top of the file claims the cap prevents monopolizing the blocking pool, but the cap bounds pin work, not scan cost. -
[P2] Do not re-anchor the full encrypted manifest to Arweave every pass
crates/gitlawb-node/src/reconciliation.rs:323
Phase 2 anchors the whole merged manifest for any path-scoped repo that has anyencrypted_blobsrow, on every hourly pass, even whenencrypt_and_pinsealed nothing new. That is a paid permanent-ledger write on a timer; a caller who creates public path-scoped repos with withheld blobs turns one-time sealing into unbounded anchor spend. Gate the anchor on "something new was sealed this pass, or the last anchor is known to have failed." -
[P2] Rebase off the 36-commit-stale base and re-review the merged state
crates/gitlawb-node/src/db/mod.rs:2159
The base is 36 commits behind main and both touchdb/mod.rs. This PR removesis_pinned, changesrecord_pinned_cid's ON CONFLICT from DO NOTHING to DO UPDATE, and introduces acid = NULLPinata convention, while main independently evolved the samepinned_cids/pinata_cidarea (it keptis_pinnedwith a live caller and addedhas_pinata_cidrather than this PR'shas_ipfs_cid). The shipped behavior is the rebase resolution, not what the diff shows, so this needs a rebase and a re-review on merged state before it can land. -
[P2] Add tests for the leak-class and coverage-critical behavior
crates/gitlawb-node/src/reconciliation.rs:1
The diff ships no tests. For a feature that emits repo content to public networks under a visibility filter, the fail-closed properties and the coverage guarantee need guards: a private repo produces zero pins, a quarantined repo is skipped across both phases, a path-scoped-withheld blob never reaches a sink, and the cursor eventually covers every repo. Each should go red if the corresponding gate is removed. -
[P3] Smaller items
crates/gitlawb-node/src/main.rs:504
The sweep is spawned unconditionally (unlike auto-sync atmain.rs:492, gated onif config.auto_sync), so it full-scans up to 100 repos hourly and runs the missing-set DB queries even when neither IPFS nor Pinata is configured — gate the spawn on a configured backend. There is no deadline on eitherspawn_blocking; a stalledgitchild leaks the blocking thread and delays shutdown, which only checks the signal between repos. And three DB calls use?(reconciliation.rs:205,:225,:322), aborting the entire pass on a transient error, where every sibling checkcontinues and skips just the one repo — make them consistent.
Net: the confidentiality core is solid and I verified it does not leak; the blocker is the Pinata NOT NULL regression, and the durability guarantee has real coverage holes (large-repo tails, idle repos) plus the stale base. All fixable without touching the visibility design.
…lawb#218) Implements periodic reconciliation sweep as durability backstop for dropped replication work (closes Gitlawb#218). Changes: - reconciliation.rs: hourly sweep with cursor-based pagination, per-backend missing-set computation, quarantine rechecks, cooperative shutdown, deadline-bound git scans, stable cursor ordering - db/mod.rs: has_ipfs_cid, filter_ipfs_pinned_oids, filter_pinata_pinned_oids, record_pinned_cid DO UPDATE with WHERE clause, record_pinata_cid NULL cid, migration v12 (DROP NOT NULL), list_all_repos_deduped_stable - ipfs_pin.rs: use has_ipfs_cid instead of is_pinned - main.rs: gate sweep spawn on configured backend - metrics.rs: reconciliation gap counters
900164d to
6186749
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/gitlawb-node/src/reconciliation.rs (1)
205-205: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInconsistent per-repo error handling aborts the entire pass.
filter_ipfs_pinned_oids(Line 205),filter_pinata_pinned_oids(Line 225), andlist_all_encrypted_blobs(Line 322) use?, so a transient DB error on a single repo propagates out ofrun_passand terminates the whole batch. Every other DB call in this loop logs andcontinues to the next repo. Since the cursor was already advanced past this batch, the un-processed repos won't be retried until the cursor wraps. Prefer the samematch … { Err(e) => { warn!; continue } }pattern for consistency and resilience.Also applies to: 225-225, 322-322
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/reconciliation.rs` at line 205, The per-repository DB calls currently propagate errors and abort run_pass, unlike the surrounding resilient loop. In the repository-processing flow, replace the ? handling for filter_ipfs_pinned_oids, filter_pinata_pinned_oids, and list_all_encrypted_blobs with match-based handling that logs a warning and continues to the next repository on error, while preserving successful results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/reconciliation.rs`:
- Around line 90-101: Replace the numeric offset cursor logic in the repository
sweep around list_all_repos_deduped with stable ordering and keyset pagination:
order repositories by an immutable deterministic key, filter after the
previously scanned repository id, and persist the last scanned id as the cursor.
Update the cursor type and reset behavior for empty or completed sweeps while
preserving the REPOS_PER_PASS limit and avoiding skipped repositories when
updated_at changes.
- Around line 257-267: Update the reconciliation flow to capture the lengths of
ipfs_candidates and pinata_candidates before they are moved into pin calls, then
record their sum as gaps found. Keep gaps found recording independent of the
repo_filled > 0 guard so failed pins still count detected gaps, while continue
recording gaps filled from pinned_ipfs and pinned_pinata.
---
Nitpick comments:
In `@crates/gitlawb-node/src/reconciliation.rs`:
- Line 205: The per-repository DB calls currently propagate errors and abort
run_pass, unlike the surrounding resilient loop. In the repository-processing
flow, replace the ? handling for filter_ipfs_pinned_oids,
filter_pinata_pinned_oids, and list_all_encrypted_blobs with match-based
handling that logs a warning and continues to the next repository on error,
while preserving successful results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cb8a7e4f-87b9-4ff0-b10a-c3da4c3c170d
📒 Files selected for processing (5)
crates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/ipfs_pin.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/metrics.rscrates/gitlawb-node/src/reconciliation.rs
- [P1] Migration v12: DROP NOT NULL on pinned_cids.cid - [P2] Subtract already-pinned before per-repo cap (no pre-cap) - [P2] Stable sweep cursor via list_all_repos_deduped_stable - [P2] Deadlines on blocking git scans (REPO_SCAN_DEADLINE) - [P2] Gate manifest anchor on newly-sealed content - [P3] Gate sweep spawn on configured backend - [P3] DB errors skip repo, not abort pass
github-actions
Bot
removed
the
needs-tests
- Replace numeric offset cursor with keyset pagination using repo.id - Record gaps_found before pin calls (from missing-set size) so detection is recorded even when all pin calls fail - Remove redundant deduped-gaps computation from filled-only path
The prior test used Db::new_in_memory and Config::default which don't exist — broke cargo clippy --workspace --all-targets.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Traced the new reconciliation module against the base-branch code it calls into (push_delta.rs, visibility_pack.rs, smart_http.rs) rather than reviewing the diff in isolation. The durability idea and the quarantine/visibility reuse are sound; one finding should block merge.
Findings
-
[P1] Make REPO_SCAN_DEADLINE actually kill the git subprocess it wraps
crates/gitlawb-node/src/reconciliation.rs:530
tokio::time::timeoutracing aspawn_blockinghandle only stops awaiting it on elapse, it doesn't abort the blocking task. Inside that closure,list_all_objectsandblob_paths(viareplicable_blob_set) shell out togit cat-file/git rev-list/git ls-treewith plainCommand::output(), noprocess_group, no timeout of their own —blob_pathsrunsgit ls-treeonce per reachable commit. On a slow or pathological repo, "deadline exceeded, skip" fires while the blocking thread and however many git children were mid-walk keep running unbounded, and the cursor revisits the same repo every pass.smart_http.rsalready has the fix for this exact class (process_group(0)+ a kill-on-drop guard that reaps the whole process group, built for the #174 watchdog gap) — reuse it here instead of the bare timeout. -
[P2] Recheck visibility rules, not just quarantine, before pinning
crates/gitlawb-node/src/reconciliation.rs:512
Rules andis_publicare fetched once per repo before the full scan and reused unchanged through both pin phases; only quarantine gets rechecked immediately before pinning. If an owner narrows visibility mid-scan, the sweep pins/reseals against the stale, more-permissive snapshot. For content-addressed public pins that's effectively irreversible. Recheck visibility the same way quarantine is already rechecked, right before each pin phase. -
[P3] Fix the vacuous spawn-gate test
crates/gitlawb-node/src/reconciliation.rs:790
test_spawn_gate_is_not_broken_by_constant_typosassertsSWEEP_INTERVAL_SECS != 0and never touchesconfigor callsspawn(). It would pass unchanged if the actual empty-config short-circuit were deleted or inverted. Either delete it or test the real gate against a minimalConfig.
… git subprocesses during scans
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/gitlawb-node/src/git/visibility_pack.rs (2)
24-34: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDownstream impact of the
GitCommand::output()stdio bug (seecrates/gitlawb-node/src/git/mod.rs).
for-each-refhere has no explicit.stdout()config before.output(). WithGitCommand::output()not forcing piped stdio,refnameswill always come back empty, soassert_all_refs_are_commitssilently no-ops (Ok(())) instead of validating refs. Fix belongs inGitCommand::output().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/git/visibility_pack.rs` around lines 24 - 34, Update GitCommand::output() in the git module to force command stdout to be piped before executing, while preserving existing stderr and status handling. This ensures callers such as assert_all_refs_are_commits receive refname output when no explicit stdout configuration is provided.
160-181: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDownstream impact of the
GitCommand::output()stdio bug (seecrates/gitlawb-node/src/git/mod.rs).Both
rev-list --allandls-tree -rzhere rely on.output()without explicit stdio config, socommits_stdout/listing_stdoutwill always be empty, makingblob_paths(and everything built on it — visibility filtering for both the push path and the new reconciliation sweep) see zero blobs. Fix belongs inGitCommand::output().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/git/visibility_pack.rs` around lines 160 - 181, Update GitCommand::output() in git/mod.rs to capture and return the child process stdout and stderr when no explicit stdio configuration is provided. Preserve the existing command execution and status handling so callers such as the rev-list and ls-tree flows in visibility_pack.rs receive their output for blob-path and visibility processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/git/mod.rs`:
- Around line 117-129: Update GitCommand::output() to configure both stdout and
stderr as Stdio::piped() before calling spawn_registered(), so
wait_with_output() captures command output. Leave GitCommand::spawn() unchanged
for callers that manage stdio themselves.
In `@crates/gitlawb-node/src/git/push_delta.rs`:
- Around line 179-187: Update GitCommand::output in the git command
implementation to explicitly configure stdout and stderr as piped before
invoking the underlying command output operation. Preserve the existing output
and error propagation behavior so list_all_objects and
list_all_objects_with_type receive the subprocess streams without requiring
call-site changes.
---
Outside diff comments:
In `@crates/gitlawb-node/src/git/visibility_pack.rs`:
- Around line 24-34: Update GitCommand::output() in the git module to force
command stdout to be piped before executing, while preserving existing stderr
and status handling. This ensures callers such as assert_all_refs_are_commits
receive refname output when no explicit stdout configuration is provided.
- Around line 160-181: Update GitCommand::output() in git/mod.rs to capture and
return the child process stdout and stderr when no explicit stdio configuration
is provided. Preserve the existing command execution and status handling so
callers such as the rev-list and ls-tree flows in visibility_pack.rs receive
their output for blob-path and visibility processing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 76e5c342-75fd-48e0-a364-0c5cf8e9bab5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/git/mod.rscrates/gitlawb-node/src/git/push_delta.rscrates/gitlawb-node/src/git/visibility_pack.rscrates/gitlawb-node/src/reconciliation.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/gitlawb-node/src/reconciliation.rs
Also fix clippy warnings: thread_local const initializer, unused arg method suppression, single_component_path_imports lint.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gitlawb-node/src/git/mod.rs (1)
135-160: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake timeout cancellation and PID registration atomic.
spawn_registeredspawns the child before registering its pgid, so the timeout handler inreconciliation::run_passcan inspect the registry and SIGTERM only processes already present in the set. Also,timeoutreturningErrdoes not cancel the runningspawn_blockingtask; the task can continue issuing laterGitCommand::output()calls while the timeout path has already skipped the repo. Move spawn/registration behind shared cancel/registry state, include canceled process groups during the scan, and reject or terminate children when cancellation is already signaled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/git/mod.rs` around lines 135 - 160, Make process creation and PID registration coordinated with the shared cancellation state used by reconciliation::run_pass. Update spawn_registered and its callers so cancellation is checked before and immediately after spawning, the child is terminated and not registered when cancellation is already signaled, and registration cannot occur after the timeout scan has passed; ensure the timeout cleanup scans canceled process groups as well as registered ones so running spawn_blocking GitCommand::output calls cannot continue issuing work after timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/gitlawb-node/src/git/mod.rs`:
- Around line 135-160: Make process creation and PID registration coordinated
with the shared cancellation state used by reconciliation::run_pass. Update
spawn_registered and its callers so cancellation is checked before and
immediately after spawning, the child is terminated and not registered when
cancellation is already signaled, and registration cannot occur after the
timeout scan has passed; ensure the timeout cleanup scans canceled process
groups as well as registered ones so running spawn_blocking GitCommand::output
calls cannot continue issuing work after timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f599befb-4cf1-497a-b77c-1ab787e3ba86
📒 Files selected for processing (2)
crates/gitlawb-node/src/git/mod.rscrates/gitlawb-node/src/reconciliation.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/gitlawb-node/src/reconciliation.rs
Introduce ScanContext bundling the pgid registry and an AtomicBool canceled flag. spawn_registered now checks canceled before and after spawn: - Before: refuse to spawn if the deadline already fired. - After: if the timeout fired mid-spawn, SIGTERM + wait the child and return an error (no zombie, no registration). The blocking closure checks canceled between each git command (list_all_objects, replicable_blob_set) and bails out early. On timeout, the async side sets canceled=true before the SIGTERM sweep, closing the window where a concurrent spawn_registered could register a new pgid after the sweep passes.
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] Recompute the object exposure set after a visibility change
crates/gitlawb-node/src/reconciliation.rs:172
The blocking scan derivesobject_listfrom the rules captured at the start of the pass, but the pre-upload recheck at lines 247-276 only asks whether/remains anonymously listable. If an owner adds a path rule such as/secret/**while the scan is running, root access still passes and the old list still contains the newly-withheld blob, so lines 338-349 publish it to IPFS/Pinata in plaintext. Re-derive the replicable set from the fresh rules and repo state (or otherwise synchronize the permission decision with the upload) before any irreversible public write; Phase 2 should likewise use the fresh identity/state when deriving recipients. -
[P1] Keep the pin listing compatible with Pinata-only rows
crates/gitlawb-node/src/db/mod.rs:2321
This change deliberately permits and insertscid = NULLfor a Pinata-only pin, butPinnedCidRecord.cidremains aStringand this query decodes it as one. The first successful Pinata-only upload therefore makeslist_pinned_cidsfail with SQLx's unexpected-NULL error;/api/v1/ipfs/pinsmaps that error to a 500, which also breaks the CLI consumers of that endpoint. Make the response field nullable or explicitly filter/represent non-local rows, and add coverage for the supported Pinata-only configuration. -
[P1] Make timeout cancellation atomic with process registration
crates/gitlawb-node/src/git/mod.rs:179
A timeout can setcanceledand drain the registry after the post-spawn load at line 180 but before line 208 inserts the new process group. That group then misses the only kill sweep and the detachedspawn_blockingtask continues inwait_with_output()pastREPO_SCAN_DEADLINE. Coordinate the cancellation check and registration with the timeout's sweep (and kill the entire-pgidin the immediate-cancel branch, rather than only the child PID) so no child can be registered after cancellation has already won. -
[P2] Bound the encrypted recovery phase too
crates/gitlawb-node/src/reconciliation.rs:413
withheld_blob_recipientsperforms a full history walk and onegit ls-treeper reachable commit, then the result is encrypted and uploaded without a deadline or work cap. Unlike the preceding scan it has neitherREPO_SCAN_DEADLINEnor aScanContext, so a large or stalled path-scoped repository can hold the sweep and a blocking worker indefinitely, leave its Git children outside the timeout cleanup, and then trigger an unbounded recovery upload. Run this phase under the same cancellation/process tracking and a restartable per-pass budget. -
[P2] Apply the repository cursor and limit in SQL
crates/gitlawb-node/src/db/mod.rs:1262
list_all_repos_deduped_stabledoes afetch_allof every deduped repository;run_passonly finds the cursor and slices 100 after that allocation. Consequently the advertised 100-repository cap does not bound the hourly query, transfer, dedup work, or memory use, and deleting the cursor row resets the scan to the first page. Make this a real keyset query (id > cursor, ordered byid, withLIMIT) and explicitly wrap only when the bounded query is exhausted. -
[P2] Do not count disabled backends as reconciliation gaps
crates/gitlawb-node/src/reconciliation.rs:278
The worker intentionally starts when either backend is configured, but it always computes and counts both missing sets. On a valid Pinata-only node, every object is added toipfs_missingandgaps_foundeven thoughipfs_pin::pin_new_objectsimmediately no-ops for an empty IPFS URL; the converse happens for an IPFS-only node. That makes the new counters permanently report unfillable gaps and can drive false durability alerts. Only compute and count a backend's missing set when that backend is enabled.
P1 — Recompute object exposure after visibility change: Re-derive allowed set from fresh rules mid-pass so newly-withheld blobs are excluded from missing sets and never published to a public backend. P1 — Pinata-only compatible list_pinned_cids: PinnedCidRecord.cid -> Option<String> so NULL rows don't 500 the endpoint. P1 — Atomic timeout cancellation with process registration: Hold the registry lock across the canceled check + pgid insert, preventing the sweep from interleaving. Kill -pgid (process group) in the immediate-cancel branch. P2 — Bound encrypted recovery phase: Wrap withheld_blob_recipients in REPO_SCAN_DEADLINE timeout + ScanContext. P2 — Keyset pagination in SQL: list_all_repos_deduped_stable takes cursor + limit, pushing the LIMIT into SQL so the hourly pass doesn't O(repos) allocate and transfer every sweep. P2 — Don't count disabled backends as gaps: Gate ipfs_missing / pinata_missing computation behind backend-enabled checks so a Pinata-only node doesn't report permanent unfillable gaps.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/gitlawb-node/src/git/mod.rs (2)
156-217: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCancellation race is correctly closed by serializing on
registry's lock.The pre-spawn check (unlocked, best-effort) plus the post-spawn check-and-insert under
ctx.registry.lock()(Lines 193-213) properly serializes againstrun_pass's cancellation kill-loop (which also takesregistry.lock()), so a pgid is either killed by the sweep-side loop or self-terminated here — no leaked/untracked child in either interleaving.One gap: after sending
SIGTERMto the process group (Line 201),child.wait_with_output()(Line 204) blocks indefinitely if the group ignores the signal. Since this runs on aspawn_blockingthread, a stuck git process (or a grandchild that detached from signal handling) would pin that thread forever, and this is the exact "backstop for dropped/delayed work" path — it should itself not have unbounded blocking. Consider a bounded wait with aSIGKILLescalation after a short grace period.♻️ Sketch of a bounded escalation
if let Some(pgid) = pgid { #[cfg(unix)] unsafe { let _ = libc::kill(-pgid, libc::SIGTERM); } } - let _ = child.wait_with_output(); + // Give the group a brief grace period, then escalate. + let mut child = child; + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(2); + loop { + match child.try_wait() { + Ok(Some(_)) => break, + Ok(None) if std::time::Instant::now() < deadline => { + std::thread::sleep(std::time::Duration::from_millis(50)); + } + _ => { + #[cfg(unix)] + if let Some(pgid) = pgid { + unsafe { let _ = libc::kill(-pgid, libc::SIGKILL); } + } + let _ = child.wait(); + break; + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/git/mod.rs` around lines 156 - 217, Bound the post-spawn cancellation cleanup in the spawn flow around the `ctx.canceled` branch and `PgidGuard`: after sending `SIGTERM`, wait only for a short grace period, then send `SIGKILL` to the process group if the child has not exited, and reap it before returning the timeout error. Replace the unbounded `child.wait_with_output()` path while preserving process-group cleanup and the existing `TimedOut` result.
220-232: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTie the spawn guard lifetime to the child.
spawn()currently returns(Child, impl Drop), so discard it as(child, _)andPgidGuard::dropremoves the pgid beforewait/wait_with_outputcompletes. Current.spawn()sites keep_guardalive, but the API still allows that mistake. Return an owned wrapper over bothChildandPgidGuardso the guard cannot outlive or be separated from the process it protects.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/git/mod.rs` around lines 220 - 232, Update the spawn API and its callers so the returned process value owns both the Child and its PgidGuard, rather than returning them separately. Introduce an owned wrapper with the required Child operations, ensure waiting/output methods retain the guard until completion, and update existing spawn sites to use the wrapper while preserving pgid deregistration in PgidGuard::drop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/gitlawb-node/src/git/mod.rs`:
- Around line 156-217: Bound the post-spawn cancellation cleanup in the spawn
flow around the `ctx.canceled` branch and `PgidGuard`: after sending `SIGTERM`,
wait only for a short grace period, then send `SIGKILL` to the process group if
the child has not exited, and reap it before returning the timeout error.
Replace the unbounded `child.wait_with_output()` path while preserving
process-group cleanup and the existing `TimedOut` result.
- Around line 220-232: Update the spawn API and its callers so the returned
process value owns both the Child and its PgidGuard, rather than returning them
separately. Introduce an owned wrapper with the required Child operations,
ensure waiting/output methods retain the guard until completion, and update
existing spawn sites to use the wrapper while preserving pgid deregistration in
PgidGuard::drop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a1b4cc64-18ad-4609-a155-355009cd8d0c
📒 Files selected for processing (3)
crates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/git/mod.rscrates/gitlawb-node/src/reconciliation.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/gitlawb-node/src/reconciliation.rs
- crates/gitlawb-node/src/db/mod.rs
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] Preserve structural objects when refreshing visibility
crates/gitlawb-node/src/reconciliation.rs:279
The initial scan correctly usesreplicable_objects_fail_closed, which preserves commits and trees while applying the allow set only to blobs. The subsequent refresh instead intersects every OID withreplicable_blob_set, whose contract explicitly contains blobs only. Consequently a missed push-time pin for a commit or tree is never repaired by either backend, and the resulting off-node object set cannot reconstruct the repository. Reapply the type-aware fail-closed filter with the fresh blob set (or otherwise retain non-blobs). -
[P2] Keep the IPFS-pins response compatible with Pinata-only rows
crates/gitlawb-node/src/db/mod.rs:159
New Pinata-only records intentionally havecid = NULL, but/api/v1/ipfs/pinsserializes those records unchanged whilegl ipfs listreads onlycid. A successful Pinata-only pin therefore renders as?, despite the response containing a usablepinata_cid; this changes the documented local-pin response contract and breaks its CLI consumer. Return a usable backend-aware CID or update the endpoint and consumer together. -
[P2] Do not run the refreshed visibility walk on a Tokio worker
crates/gitlawb-node/src/reconciliation.rs:273
replicable_blob_setperforms synchronous Git history traversal (rev-listand anls-treeper reachable commit), yet this second invocation is made directly fromrun_pass, outside bothspawn_blockingandREPO_SCAN_DEADLINE. A large or stalled repository can therefore block a Tokio worker indefinitely after the initial bounded scan and delay shutdown or unrelated async work. Fold this recomputation into the bounded scan, or give it equivalent cancellation-aware blocking execution. -
[P2] Register every Git subprocess in the timed scan
crates/gitlawb-node/src/git/store.rs:69
The new timeout only terminates process groups registered throughGitCommand, butblob_pathscalls this rawCommand::new("git")viahead_commitduring both reconciliation scans. If thatrev-parsestalls, the timeout stops awaiting the blocking task without being able to signal or reap its child, leaving a blocking worker behind despite the advertised per-repo deadline. Route scan-path subprocesses through the registered wrapper (and audit the helpers reached by the scan). -
[P2] Bound the pin phase as well as the Git scan
crates/gitlawb-node/src/reconciliation.rs:351
Each backend is allowed to process 50,000 missing objects serially, and the new deadline covers only the earlier Git walk. With an unavailable backend, this loop awaits one upload at a time until the client timeout for every object, so a single repository can hold the sole sweep task for days and prevent the cursor from reaching other repositories. Apply a per-repository wall-clock budget/cancellation to pinning (with bounded batching or concurrency).
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Confirmed jatmn's structural-objects P1 by execution rather than restating it: on a public repo with no rules, the fail-closed scan yields 4 structural objects and the intersect at reconciliation.rs:279-282 yields 0. It is unconditional, not narrowing-only. The rest below is what this head still needs, and the first item is a scope call I am settling as lead.
Findings
-
[P1] Split this into three PRs before the next round
crates/gitlawb-node/src/reconciliation.rs:1
Three of the five findings on this head were introduced by the fixes for the previous round's findings, and the diff has grown from 607 to 1032 lines, mostly in a subprocess-registry layer bolted onto shared serving-path helpers. That is a loop that costs more each turn. Land (1) thepinned_cidsnullable-cid semantics plus migration 12 and thegl ipfs listconsumer, (2) theGitCommandprocess-group registry on its own with tests on the serving path it now changes, and (3) the sweep on top. The durability need is real and I want it in; the current shape is not reviewable one round at a time. -
[P1] Delete or rewrite the spawn-gate test, it passes with the gate removed
crates/gitlawb-node/src/reconciliation.rs:573
I removed theif config.ipfs_api.is_empty() && config.pinata_jwt.is_empty() { return; }block at:41-44and re-ran the module: both tests still pass.tokio::spawnonly enqueues the task, and the test has no await after the call, so it is never polled. The doc comment at:566-572asserts the opposite. Extractshould_spawn(&Config) -> booland assert both directions. -
[P2] Match the loop's own convention at the fresh-visibility recompute
crates/gitlawb-node/src/reconciliation.rs:278
This is the only?insidefor repo in &batch; every sibling failure warns and continues. The cursor is advanced past the whole batch at:118before the loop starts, so one repo's git error abandons up to 99 already-selected repos, and they wait for a full cursor wrap before anything looks at them again. -
[P2] Do not hold the scan registry lock across the child wait
crates/gitlawb-node/src/git/mod.rs:194
The cancel-after-spawn branch takesctx.registry.lock()and then callschild.wait_with_output()under it, while the deadline handler atreconciliation.rs:202acquires that samestd::sync::Mutexfrom async context. A process group that ignores SIGTERM blocks a tokio worker on the lock. Snapshot the pgids under a short lock and kill outside it, and useunwrap_or_else(|e| e.into_inner())at both sites so a poisoned lock cannot end the sweep task permanently. -
[P2] Ship the v12 upgrade-path test with the migration
crates/gitlawb-node/src/db/mod.rs:889
A fresh-DB suite runs the migration array from scratch and cannot see an upgrade-path bug;migration_v11_creates_owner_did_columnatdb/mod.rs:3665is the pattern to mirror. Seed the legacycid = pinata_cidrow shape the migration comment sayshas_ipfs_cidhandles, and assert the classification. I could not determine whether a Kubo add and a Pinata upload return the same CID for the same bytes; if they ever do,cid IS DISTINCT FROM pinata_cidmarks a genuinely pinned object as a permanent gap and re-uploads it every pass. That test should settle it either way. -
[P3] Give the sweep an operator switch and document it
crates/gitlawb-node/src/main.rs:502
Any node with IPFS or Pinata configured now runs hourly full-object scans over up to 100 repos, with no way to turn it off and no mention in the operator docs. Auto-sync is the precedent:config.auto_sync,README.md:344,.env.example:152. -
[P3] Anchor the pass delta, not the merged manifest
crates/gitlawb-node/src/reconciliation.rs:523
The push path anchors only what it sealed (api/repos.rs:1175); the sweep mergeslist_all_encrypted_blobsinto every anchor, so each pass republishes entries already on the ledger. Not a new disclosure, since past deltas cover the same OIDs, but it is a paid permanent write and it diverges from the established pattern.
Superseded by my review on 88e49b5; dismissing so the state reflects the current head.
0db5551 to
beae7cd
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Rechecked head beae7cd against my prior review on 88e49b5 and re-verified each finding against the checkout (not just blind-search candidates). The latest round fixes a lot of the earlier durability and API-contract work (structural-object refresh, keyset repo pagination, nullable cid migration + test, Pinata-only /api/v1/ipfs/pins synthesis, spawn-gate tests, GITLAWB_RECONCILIATION_SWEEP, bounded git scans, and pin-phase timeouts). The confidentiality core still looks careful. I still see PR-owned issues that need to be addressed before this is ready.
Findings
-
[P1] Re-validate quarantine and visibility immediately before each irreversible public pin
crates/gitlawb-node/src/reconciliation.rs:418
Phase 1 re-fetches quarantine,is_public, and rules, re-runs the fail-closed refilter, and only then builds the missing sets. Neither the up-to-300s refilter (~302–351) nor the subsequent pin phases (~418–449, up to 600s total) re-check quarantine or visibility. If the owner quarantines the repo or narrows visibility during either window, the sweep can still publish content to IPFS/Pinata — and the code itself notes that stale public pins are effectively irreversible (~248). Add the same pre-upload gate used at ~250–290 immediately before each backend pin (or inside the pin loops), not only before the git scan. -
[P2] Phase 2 still uses stale repo identity for encrypted recovery
crates/gitlawb-node/src/reconciliation.rs:512
Phase 2 re-fetchesfresh_repoand passesfresh_repo.is_publictolistable_at_root, butwithheld_blob_recipientsis called with batch-snapshotrepo.is_publicandrepo.owner_did. Phase 1 already usesfresh_repofor the refilter (~297–298). If ownership oris_publicchanges mid-pass, recovery copies can be sealed for the wrong owner/recipient set and the Arweave manifest can carry a staleowner_did(~592). Passfresh_repofields into the phase-2 blocking call the same way phase 1 does. -
[P2] Legacy
record_pinata_cidupdates can falsely mark objects as locally IPFS-pinned
crates/gitlawb-node/src/db/mod.rs:2410
Migration v12 andhas_ipfs_cidcorrectly treat legacy rows wherecid = pinata_cidas Pinata-only, butrecord_pinata_cid'sON CONFLICTpath updates onlypinata_cidand leaves the oldciduntouched. When Pinata returns a new CID for such a row,has_ipfs_cid/filter_ipfs_pinned_oidsseecid IS NOT NULL AND cid IS DISTINCT FROM pinata_cidand classify the object as locally IPFS-complete even thoughcidis still the old Pinata fallback. Both the push path (ipfs_pin::pin_new_objects) and the sweep then skip local IPFS repair permanently. Clear or NULLcidwhen updatingpinata_cidon legacy equal-cid rows (or when the storedcidequals the previouspinata_cid), and add a test that re-pins a legacy row with a different Pinata CID. -
[P2]
record_pinned_cidcannot repair a stale wrong local CID
crates/gitlawb-node/src/db/mod.rs:2240
The new v12ON CONFLICTupsert only updatescidwhencid IS NULL OR cid = pinata_cid. If a row already has a wrong localcidthat differs frompinata_cid, a later successful IPFS pin is ignored,has_ipfs_cid/filter_ipfs_pinned_oidstreat the object as complete, and both the sweep and push path skip repair permanently. Allow overwrite when the stored CID is known-bad or add an explicit repair path for reconciliation. -
[P2] Pinata-only nodes still inflate IPFS gap metrics
crates/gitlawb-node/src/reconciliation.rs:354
This was in my prior review and is still open on this head._ipfs_enabledis computed but unused;ipfs_missingandgaps_ipfsare always built and counted even whenconfig.ipfs_apiis empty, whilepin_new_objects("", …)no-ops. Pinata-only deployments permanently report unfillable IPFS gaps ingitlawb_reconciliation_gaps_found_total. Gate IPFS missing-set computation, gap counting, and the IPFS pin call behind!config.ipfs_api.is_empty()the same way Pinata is gated at ~383. -
[P2] Bound the encrypted recovery upload phase
crates/gitlawb-node/src/reconciliation.rs:571
The git walk forwithheld_blob_recipientsis now deadline-bounded, butencrypt_and_pinis awaited with no timeout. A repo with many withheld blobs or a slow IPFS backend can hold the sole sweep task indefinitely and delay shutdown (only checked at the top of the per-repo loop). Wrap phase 2 sealing in the samePIN_PHASE_DEADLINE(or a dedicated budget) used for public pinning. -
[P2] Do not hold the scan registry lock across child reap
crates/gitlawb-node/src/git/mod.rs:194
In the post-spawn cancellation branch,spawn_registeredholdsctx.registry.lock()while callingchild.wait_with_output(). The timeout handler inrun_pass(~219) needs that same lock to snapshot pgids for SIGTERM. A git child that ignores SIGTERM blocks the async timeout path from cleaning up other registered processes in the same scan. Snapshot pgids under a short lock, release, then wait/kill outside the lock (mirrorsmart_http.rs's bounded SIGTERM→SIGKILL escalation). -
[P2] Add guards for the leak-class and coverage-critical sweep behavior
crates/gitlawb-node/src/reconciliation.rs:1
The new spawn-gate and migration v12 tests are useful, but this head still has no tests that a private repo produces zero pins, a quarantined repo is skipped across both phases, a path-scoped withheld blob never reaches a sink, or the stable cursor eventually covers every repo.metrics::testsalso does not assert registration or increment behavior forgitlawb_reconciliation_gaps_found_total/gitlawb_reconciliation_gaps_filled_total. Each guard should go red if the corresponding gate is removed. -
[P3] Per-repo missing-set cap can starve the same objects every pass
crates/gitlawb-node/src/reconciliation.rs:368
Missing sets are built fromHashSet::difference(arbitrary order), thentruncate(MAX_OBJECTS_PER_REPO). Repos with more than 50k unpinned objects per backend can leave the same tail subset unselected on every hourly pass. Use deterministic ordering (OID sort) and rotate the cap window, or page within the repo. -
[P3] Filter queries still send the full uncapped object list to Postgres
crates/gitlawb-node/src/reconciliation.rs:358
list_all_objectsmaterializes every OID before the per-backend cap applies.filter_ipfs_pinned_oids/filter_pinata_pinned_oidsthen pass the entireobject_listthroughANY($1). Very large repos can spike memory and produce slow or failing filter queries even though pin work is capped. Batch the filter queries or cap before hitting SQL. -
[P3] Document the new operator switch
crates/gitlawb-node/src/config.rs:89
GITLAWB_RECONCILIATION_SWEEPdefaults to on and is absent fromREADME.mdand.env.example(unlikeGITLAWB_AUTO_SYNC, which is documented in both). Operators cannot discover how to disable the hourly full-object scan. -
[P3] Do not log "worker started" when the sweep is gated off
crates/gitlawb-node/src/main.rs:512
reconciliation::spawnreturns immediately when neither backend is configured orreconciliation_sweepis false, butmainalways logsreconciliation sweep worker started. That makes runtime logs contradict the gate the new tests exercise. -
[P3] A filter DB error on one backend skips the other backend's gap-fill
crates/gitlawb-node/src/reconciliation.rs:358
filter_ipfs_pinned_oidsandfilter_pinata_pinned_oidseach usecontinueon error, aborting the whole repo iteration. A transient failure in the Pinata filter (~384–388) skips already-computed IPFS pinning; a failure in the IPFS filter (~358–362) skips Pinata work entirely. Treat filter errors per-backend (empty missing set + warn) so independent backends do not block each other. -
[P3] Mid-pass shutdown advances the cursor past unprocessed repos
crates/gitlawb-node/src/reconciliation.rs:135
The cursor is set tobatch.last().idbefore the per-repo loop. A shutdownbreakmid-batch leaves the cursor at the batch end, so the next pass queriesid > cursorand skips every unprocessed repo in the interrupted batch until the cursor wraps. Defer cursor advancement until the batch finishes, or persist per-batch progress. -
[P3] Pin-phase timeout drops the future but not in-flight uploads
crates/gitlawb-node/src/reconciliation.rs:418
tokio::time::timeout(PIN_PHASE_DEADLINE, pin_new_objects(...))returns an empty pinned list on expiry while per-objectreqwestPOSTs started inside the loop keep running (ipfs_pin.rs/pinata.rs). The timeout arms also discard partial pin progress, sogaps_foundcan rise whilegaps_filledundercounts objects pinned before the deadline. Use a cancellation token or shared client with abort, and count partial fills before returning. -
[P3] Successful external pins count as filled even when DB persistence fails
crates/gitlawb-node/src/ipfs_pin.rs:134
Pre-existing in the push pin path; reconciliation now amplifies it viagaps_filled(~451–454).pin_new_objects/pinata::pin_new_objectspush(sha, cid)into their return vec after a successful upload even whenrecord_pinned_cid/record_pinata_cidfails (warn-only), so metrics overstate durable progress while the next pass retries the upload. -
[P3] Scan timeout does not fully reclaim blocking work
crates/gitlawb-node/src/reconciliation.rs:226
WhenREPO_SCAN_DEADLINEfires, the async side SIGTERMs registered pgids once and moves on without a grace period, SIGKILL escalation, or reap.tokio::time::timeoutalso does not cancel thespawn_blockingtask, so timed-out scans can keep running in the pool. On non-Unix targets the kill path andprocess_group(0)registration are compiled out (git/mod.rs:181–185,reconciliation.rs:217–231), leaving orphangitchildren with no termination hook. -
[P3] Quarantine recheck is deferred until after the full git scan
crates/gitlawb-node/src/reconciliation.rs:166
is_repo_quarantinedis not checked until after the scan completes (~250). A repo quarantined during the up-to-300s walk still pays the full git I/O cost every pass before being skipped. This is wasted work, not a pin leak (quarantine is rechecked before pinning), but it matters on pathological or repeatedly quarantined repos. -
[P3] Pin reads still bypass
GitCommandcancellation wiring
crates/gitlawb-node/src/git/store.rs:294
This PR routes scan/refilter git throughGitCommand, butipfs_pin::pin_new_objects,pinata::pin_new_objects, andencrypt_and_pinstill read bytes viastore::read_object, which uses plainCommand::new("git")(pre-existing). Pin-phase timeouts therefore cannot terminate stalledcat-filechildren the way scan timeouts can. Finish routing read paths through the registered wrapper or an equivalent cancellation hook. -
[P3]
gaps_founddouble-counts objects missing on both backends
crates/gitlawb-node/src/reconciliation.rs:410
repo_gaps = gaps_ipfs + gaps_pinataadds the per-backend missing-set sizes. One OID absent from both backends incrementsgitlawb_reconciliation_gaps_found_totaltwice even though the metric description says "objects that should be pinned but are not." Count unique OIDs or record per-backend metrics separately. -
[P3] Mid-pass shutdown overreports repos scanned
crates/gitlawb-node/src/reconciliation.rs:615
A shutdownbreakcan exit the per-repo loop early, butrun_passstill returns(batch.len(), …). The pass-complete log therefore reports the full batch size even when only a prefix was processed. -
[P3] PR widens exposure on the already-unsigned pins route
crates/gitlawb-node/src/api/ipfs.rs:238
/api/v1/ipfs/pinswas already on the unsignedipfs_routesmerge before this PR (server.rs:220, tracked in #121). This change addspinata_cidto every entry, so anonymous callers can now enumerate node-wide Pinata CIDs without signing. If the index is meant to stay authenticated (#134 on the CLI side), omit backend-specific fields for anonymous reads or gate the route. -
[P3]
list_pinscan emit"cid": null
crates/gitlawb-node/src/api/ipfs.rs:236
Migration v12 allowscidto be NULL, anddisplay_cidisp.cid.or_else(|| p.pinata_cid). A row with both columns NULL serializes"cid": null, breaking the prior always-string contract. Filter incomplete rows or guarantee both backends write at least one CID before listing. -
[P3] Durability-backstop wording overstates behavior when sweep is gated off
crates/gitlawb-node/src/git/push_delta.rs:265
Push-time pin failures log that the reconciliation sweep backstops them, andmaindescribes the sweep as filling gaps so dropped replication never means data loss.should_spawnis a no-op when neither IPFS nor Pinata is configured or whenreconciliation_sweep=false, so those nodes have no backstop. Tighten the comments/logs to match the gate, or document the dependency on a configured backend.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed head beae7cd by execution rather than by reading the diff. The confidentiality core on a canonical repo still holds up: I could not construct a rule shape, is_public value, or DID form that gets a withheld blob into a sink through the normal path. Two things changed my read this round, and both came from looking at merged behavior instead of the diff.
Findings
-
[P1] Skip mirror rows in the sweep, or resolve them to a canonical row first
crates/gitlawb-node/src/reconciliation.rs:166
Mirror rows are written byupsert_mirror_repowithis_public = truehardcoded (db/mod.rs:1032), and nothing replicates visibility rules to a mirror:sync.rshas zero references to rules. The sweep loads rules withlist_visibility_rules(&repo.id), so for a mirror with no canonical twin it gets an empty rule set and a public flag, and the gate here allows unconditionally. I ran the conjunction against a real DB: the mirror is returned bylist_all_repos_deduped_stable, its rules are empty, andlistable_at_rootreturns true, while the same gate still denies a private canonical repo. That makes the gate vacuous for exactly the repos whose rules this node does not have. Promisor mode usually keeps withheld blobs off disk, but a repo that was public when first mirrored is cloned Plain (sync.rs:76), and git does not delete those objects when the origin later narrows visibility. The result is an irreversible publish to IPFS and Pinata of content the origin now withholds. Pinning previously only ran on the authenticated push path against a repo whose rules this node owns, so this PR is what makes that reachable. The slash-form id test is already the established way to spot a mirror (api/repos.rs:1765,db/mod.rs:2560). -
[P1] Make sweep coverage survive a restart
crates/gitlawb-node/src/reconciliation.rs:65
The cursor is a localOption<String>inside the spawned task, so every process start resets the sweep to the first page. WithREPOS_PER_PASSat 100 and an hourly interval, a node with more than 100 repos that restarts more often than a full cycle never reaches the tail, and idle repos are the ones with only this backstop. That is the coverage guarantee the PR is written to provide, so it needs to hold across a deploy. Note there is no node-state or key-value table in the schema today, so persisting it means new DDL, which is one more reason to land the storage change separately from the worker. -
[P1] Split this into three PRs, as asked last round
crates/gitlawb-node/src/reconciliation.rs:1
This is the second time, so I am settling it rather than restating it. The diff has gone 607 to 1032 to 1311 lines across the rounds where I asked for the split. Findings continue to trace to previous rounds' fixes rather than to the original defect: thefresh_repore-fetch added for a prior finding is used for the phase 1 gate but not for the phase 2 seal two lines later, the nullable-cid work introduced the classification state machine below, and the process-group registry introduced the lock-across-wait problem jatmn has now filed twice. A wrong answer here publishes content permanently, which is the wrong risk profile for a change this shape. Land (1) thepinned_cidsnullable-cid semantics with migration v12 and the/api/v1/ipfs/pinsconsumer, (2) theGitCommandprocess-group registry with tests on the serving path it changes, then (3) the sweep on top. Each is reviewable in one round; this is not. -
[P2] Test the behavior this PR exists to change
crates/gitlawb-node/src/reconciliation.rs:418
I emptied both missing sets right before the pin phases, so the sweep detects gaps and repairs nothing, and ran the full suite: 517 passed, 0 failed. Gap repair is the entire premise and nothing holds it. The same is true of the pieces underneath it. Replacingrecord_pinned_cid's conditional upsert withDO NOTHING, which removes the only path by which a Pinata-only row ever becomes IPFS-pinned, leaves all 63 db tests green, and revertingrecord_pinata_cid's NULL bind to the legacycid = pinata_cidfallback also leaves them green, including the new v12 test. The v12 test is genuinely load-bearing for the DDL and the classification predicate, so this is about the writers, not that test. -
[P2] Stop inferring IPFS provenance from CID inequality
crates/gitlawb-node/src/db/mod.rs:2348
has_ipfs_cidandfilter_ipfs_pinned_oidsdecide "locally pinned" withcid IS NOT NULL AND cid IS DISTINCT FROM pinata_cid, which treats a value comparison as a provenance record. A CID is a function of the bytes, so this is correct only while the two backends happen to disagree. Today they likely do, since Kubo is called withcid-version=1&raw-leaves=true(ipfs_pin.rs:30) and the Pinata v3 upload sends plain multipart with no codec parameters, but that is a third party's chunking default, not an invariant this repo controls or tests. If they ever agree, a successful Pinata write downgrades a correctly pinned row to not-pinned and the object is re-read, re-uploaded and re-counted as a gap every hour. This is the question I raised last round and it is still open; the fix is to record provenance rather than infer it, for example backfilling legacy equal rows to NULL in the migration and reducing the predicate tocid IS NOT NULL. Worth compiling before you commit to the exact shape. -
[P2] Delete or rewrite the spawn-gate test, it still passes with the gate removed
crates/gitlawb-node/src/reconciliation.rs:679
Re-ran my check from last round on this head: I replaced the early return at:56-61withlet _ = should_spawn(&config);and all 6 reconciliation tests stayed green, includingtest_spawn_gate_skips_when_no_pin_backends_configured. The fourshould_spawncases you added are real and do test the predicate, so keep those. It is the test that callsspawn()and asserts nothing that should go, or return something fromspawn()it can assert on.
jatmn's round on this head is otherwise still open as written, and I am not going to re-litigate it here. I confirmed one of theirs directly: _ipfs_enabled at reconciliation.rs:354 is declared and never read, while pinata_enabled does gate at :383, so a Pinata-only node counts every object as an unfillable IPFS gap forever.
One scoping note on their phase 2 finding, so the fix stays a one-liner. Passing fresh_repo.owner_did and fresh_repo.is_public at :512-514 is right and worth doing, but the only columns any code updates on repos are updated_at and quarantined (db/mod.rs:1327, :1469). There is no public/private toggle and no ownership transfer, so the stale values are identical to the fresh ones today and this is about not leaving the trap armed. The mid-pass narrowing that actually can happen comes through the visibility rules table and quarantine, so that is where a recheck earns its keep.
Net: the visibility design on canonical repos is still the strong part of this work and I want the durability backstop in. The mirror path is a genuine gap that only appears in merged behavior, the coverage guarantee does not survive a restart, and the premise has no test. Those are three different subsystems, which is the argument for the split rather than a seventh round on one branch.
Summary
Implements the periodic reconciliation sweep the replication path already assumes as a durability backstop. Previously, every path that drops a pin or recovery copy (mid-drain panic, node crash/seal, client disconnect at the receive-pack tail) resulted in data loss with no safety net.
Motivation & context
Closes #218
The codebase justified tolerating dropped post-push replication work by pointing at a reconciliation sweep that did not exist. This made "lost forever" literal rather than conservative phrasing, violating the project's stated promise that "once code is pushed to the network, it should not disappear because one server went down."
Kind of change
What changed
crates/gitlawb-node/src/reconciliation.rs (new): Periodic sweep that re-derives the set of objects a repo should have pinned/sealed under current visibility rules
crates/gitlawb-node/src/metrics.rs: Added gitlawb_reconciliation_gaps_found_total and gitlawb_reconciliation_gaps_filled_total counters
crates/gitlawb-node/src/main.rs: Registered reconciliation module and spawned the background sweep task
How a reviewer can verify
cargo check -p gitlawb-node cargo clippy -p gitlawb-node -- -D warnings cargo test -p gitlawb-node -- metrics::testsBefore you request review
cargo test --workspacepasses locally (DB-dependent tests require a running Postgres)cargo clippy --workspace --all-targets -- -D warningsis cleanfix(...))Notes for reviewers
The sweep is intentionally conservative per pass (100 repos, hourly) to avoid competing with the push path for resources. The cursor wraps around so every repo is eventually covered. Encrypted pin re-sealing and Arweave manifest anchoring are best-effort (failures are logged and skipped).
Summary by CodeRabbit