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. |
📝 WalkthroughWalkthroughAdds an hourly, bounded reconciliation worker with cooperative shutdown, startup wiring, backend-specific pin-state checks, and gap metrics. It scans eligible repository objects, fills missing pins, and optionally encrypts path-scoped data and anchors manifests to IRYS. ChangesReconciliation sweep
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReconciliationWorker
participant Database
participant LocalRepository
participant IPFS
participant IRYS
ReconciliationWorker->>Database: Load repository batch and backend pin state
ReconciliationWorker->>LocalRepository: Scan eligible objects
ReconciliationWorker->>IPFS: Pin missing objects and encrypted output
ReconciliationWorker->>IRYS: Anchor merged encrypted manifest when configured
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
5c771ec to
b31cf88
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: 3
🧹 Nitpick comments (1)
crates/gitlawb-node/src/reconciliation.rs (1)
25-32: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFirst reconciliation pass waits a full hour after startup.
The sweep exists as a crash/restart backstop, but the loop always sleeps
SWEEP_INTERVAL_SECS(1h) before the first pass. A crash/restart is precisely when unfilled gaps are most likely, so the backstop is least effective right when it's needed most.♻️ Run one pass immediately, then fall into the interval
tokio::spawn(async move { let node_seed = *node_keypair.to_seed(); let mut cursor = 0usize; + + // Run one pass shortly after startup so a crash/restart doesn't + // wait a full interval before the backstop kicks in. + run_startup_pass(&db, &config, &http_client, &node_seed, &node_did, &mut cursor).await; loop {🤖 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` around lines 25 - 32, Update the reconciliation loop inside the tokio::spawn block so one sweep executes immediately on startup before waiting for SWEEP_INTERVAL_SECS. Preserve the existing periodic interval behavior for all subsequent passes, including the current cursor and node_seed handling.
🤖 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 151-157: Replace the per-object awaited db.is_pinned calls in the
repo_gaps loop with one batched query for all object_list SHAs, using an array
membership check against pinned_cids, then compute repo_gaps from the returned
pinned set while preserving the existing treatment of lookup errors as gaps.
- Around line 147-259: Decouple the encrypted reseal and Arweave-anchor flow
from the public-object gap checks in the reconciliation loop. Update the early
exits around repo_gaps so zero public gaps skips only public pinning, metrics,
and related reporting, while the has_path_scoped block still runs to repair
withheld copies and anchor manifests. Preserve the existing behavior for
repositories with public gaps and avoid invoking encrypted resealing when its
existing path/configuration guards are not satisfied.
- Around line 68-77: Update the reconciliation sweep around list_all_repos so
quarantined repositories are excluded before batching and cursor advancement.
Reuse the repository’s established quarantined filtering behavior or predicate,
ensuring batch, pagination, and empty-result handling operate only on eligible
repositories.
---
Nitpick comments:
In `@crates/gitlawb-node/src/reconciliation.rs`:
- Around line 25-32: Update the reconciliation loop inside the tokio::spawn
block so one sweep executes immediately on startup before waiting for
SWEEP_INTERVAL_SECS. Preserve the existing periodic interval behavior for all
subsequent passes, including the current cursor and node_seed handling.
🪄 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
Run ID: 53b027bc-082b-4c03-98fb-79aedf72c8d0
📒 Files selected for processing (3)
crates/gitlawb-node/src/main.rscrates/gitlawb-node/src/metrics.rscrates/gitlawb-node/src/reconciliation.rs
b31cf88 to
9352318
Compare
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 reachable blobs for ordinary public repositories
crates/gitlawb-node/src/reconciliation.rs:137
When a repository has no path-scoped rule (the normal public-repo case), this assigns an emptyallowedset and then passes it toreplicable_objects_fail_closed. That helper drops every blob not inallowed, so the sweep only re-pins commits and trees; a dropped file blob is never recovered.replicable_blob_setalready computes the reachable public-blob set for the no-rule case, so use it here as well (or take the equivalent no-withheld path) and add a regression test with a public repo containing a blob. -
[P1] Do not derive replication visibility from raw mirror rows
crates/gitlawb-node/src/reconciliation.rs:81
list_all_reposreturns physical mirror rows. Those rows are deliberately stored as public with no visibility rules, while the canonical row can be private or path-restricted;Db::get_repoexplicitly prefers the canonical row to avoid this bypass. The canonical row is skipped here, but its mirror passes the anonymous root gate and the sweep publishes its commits and trees, disclosing private commit metadata and tree paths. Resolve visibility from the canonical/deduplicated record before doing any external pinning (and retain the physical path only after that decision). -
[P2] Recheck quarantine at the replication write boundary
crates/gitlawb-node/src/reconciliation.rs:78
The worker reads the quarantine set and repository rows in separate queries, then does no per-repo quarantine check before calling the external pinning backends. If an operator quarantines a repo after the first query, that pass still publishes its objects even though quarantined rows are meant to be ineligible for replication. Fetch only non-quarantined rows atomically or recheck immediately before the external writes. -
[P1] Clamp the batch cursor when the eligible set changes
crates/gitlawb-node/src/reconciliation.rs:93
The cursor survives between passes butallis rebuilt after filtering a mutable repository/quarantine set. For example, a 101-repo pass leavescursor = 100; if deletion or quarantine reduces the next eligible list to 99, this slicesall[100..99]and panics. Because this is the only spawned worker, reconciliation then stops permanently until a process restart. Reset or clamp the cursor before slicing and cover a shrinking eligible set. -
[P1] Reconcile IPFS and Pinata state independently
crates/gitlawb-node/src/reconciliation.rs:169
The sole gap test asks only whether apinned_cidsrow exists, then uses that result to skip both backends. IPFS success records that row even if the subsequent Pinata upload fails; conversely,record_pinata_cidcan create the row without an IPFS pin. On later passesrepo_gapsis zero, so the missing backend is never retried, despite Pinata tracking its ownpinata_cidcompletion state. Compute each backend's missing set independently (or call each helper with the eligible objects) so partial replication failures are actually repaired. -
[P2] Bound one repository's reconciliation work
crates/gitlawb-node/src/reconciliation.rs:132
REPOS_PER_PASSbounds only the number of repositories. For each selected repo the worker materializes all object IDs and all blob IDs, then reads and uploads every missing object sequentially, with no object/byte/time budget or shutdown observation until the pass completes. A large repository can therefore monopolize the blocking pool and keep the hourly worker running indefinitely, defeating the stated bounded-work and graceful-shutdown behavior. Add per-repo work limits and cooperative cancellation/progress checks.
9352318 to
e27e41f
Compare
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] Continue reconciliation past the per-repository object cap
crates/gitlawb-node/src/reconciliation.rs:164
list_all_objectsdeterministically returns the whole object set, but the worker permanently truncates it to the first 50,000 entries before the pin helpers discard already-pinned entries. A repository with an eligible missing object after that prefix will revisit and skip the same pinned prefix every hour, so that object is never repaired. Keep resumable/rotating per-repository progress (or select missing objects before applying a continuation-aware cap) and cover an object beyond the cap. -
[P1] Track local-IPFS completion independently of Pinata completion
crates/gitlawb-node/src/reconciliation.rs:182
Although the new code says the two backends are reconciled independently,ipfs_pin::pin_new_objectsusesDb::is_pinned, which returns true for anypinned_cidsrow. If IPFS fails and the subsequent Pinata upload succeeds,record_pinata_cidcreates that row; all later IPFS passes skip the object permanently. Represent and query IPFS completion separately, then add the IPFS-failure/Pinata-success recovery case. -
[P1] Retry failed encrypted-manifest anchors
crates/gitlawb-node/src/reconciliation.rs:235
The Irys anchor is attempted only whenencrypt_and_pinreturns newly sealed blobs. If sealing and recording succeeds but the manifest anchor fails (or the process stops after recording), subsequent sweeps see unchanged recipient tags, return an emptysealedlist, and never enter the anchor path again. Persist/reconcile anchor completion independently, or re-anchor the existing encrypted rows, so a transient anchor failure does not leave recovery copies undiscoverable forever. -
[P2] Recheck quarantine immediately before publishing reconciliation output
crates/gitlawb-node/src/reconciliation.rs:84
The deduplicated repository list filters quarantine only at the start of a pass. An operator can quarantine a selected repository while the worker scans it; the pass will still submit public objects, encrypted copies, and possibly an Irys manifest at lines 182-270. Recheck eligibility at the external-write boundary (or select and lock/check it atomically) so quarantine takes effect before replication is published. -
[P2] Make the claimed work and shutdown bounds apply to the whole sweep
crates/gitlawb-node/src/reconciliation.rs:129
The 50,000-object cap is imposed only afterlist_all_objects,replicable_blob_set, andall_blob_oidshave already materialized/walked the entire repository; phase 2 then walks and encrypts every withheld blob without any cap. The worker also observesshutdown_rxonly after the full pass, while the IPFS helper has no request timeout. A large repository or a stalled Kubo request can therefore monopolize the worker indefinitely and prevent shutdown. Bound the scans and both phases with continuable progress, request timeouts, and cooperative cancellation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/db/mod.rs`:
- Around line 2263-2271: Separate local-IPFS completion from Pinata completion:
in crates/gitlawb-node/src/db/mod.rs lines 2263-2271, update has_ipfs_cid to
check explicit local-IPFS state and mark existing Pinata rows with local state
when repaired; in lines 2301-2314, use the same backend-specific predicate for
bulk reconciliation filtering; and in crates/gitlawb-node/src/ipfs_pin.rs lines
110-117, allow local pinning when only a Pinata CID exists.
🪄 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: 7aa80ce7-4a55-4389-bedb-c159517a8faf
⛔ 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/ipfs_pin.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/metrics.rscrates/gitlawb-node/src/reconciliation.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/gitlawb-node/src/metrics.rs
- crates/gitlawb-node/src/reconciliation.rs
- Remove (conflated IPFS/Pinata); add + batch filter methods for per-backend missing-set computation - Apply per-repo cap AFTER filtering already-pinned objects so the cap reflects actual work, not raw object list size - Recheck quarantine before Phase 1 and Phase 2 pinning - Anchor ALL existing encrypted blobs (not just newly-sealed) so failed manifest anchors retry on subsequent passes - Pass shutdown_rx through run_pass for cooperative mid-pass shutdown
de0a40a to
9897aaa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/db/mod.rs`:
- Around line 2162-2171: Separate legacy Pinata CIDs from local-IPFS state
before the predicates and upsert logic use pinned_cids.cid: add a
migration/backfill or explicit local-IPFS indicator that identifies existing
rows created by record_pinata_cid and clears or excludes their stale cid values.
Update record_pinned_cid, has_ipfs_cid, and filter_ipfs_pinned_oids to use that
corrected state, ensuring legacy Pinata-only rows can be replaced by a real
local CID.
🪄 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: bbbf32f1-3c85-447e-9339-63e133d2dbe1
📒 Files selected for processing (3)
crates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/ipfs_pin.rscrates/gitlawb-node/src/reconciliation.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/gitlawb-node/src/ipfs_pin.rs
- crates/gitlawb-node/src/reconciliation.rs
| /// Record the local IPFS CID for a git object. | ||
| /// If a Pinata-only row already exists (cid IS NULL), this updates it with | ||
| /// the real IPFS CID so that `has_ipfs_cid` correctly reflects IPFS state. | ||
| pub async fn record_pinned_cid(&self, sha256_hex: &str, cid: &str) -> Result<()> { | ||
| sqlx::query( | ||
| "INSERT INTO pinned_cids (sha256_hex, cid, pinned_at) | ||
| VALUES ($1, $2, $3) | ||
| ON CONFLICT(sha256_hex) DO NOTHING", | ||
| ON CONFLICT(sha256_hex) DO UPDATE SET | ||
| cid = COALESCE(pinned_cids.cid, EXCLUDED.cid), | ||
| pinned_at = EXCLUDED.pinned_at", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Backfill legacy Pinata rows before using cid as local-IPFS state.
This only fixes new rows: record_pinata_cid binds cid = NULL on insert, but existing rows from the old Pinata path can still have Pinata’s CID in cid. Those rows are treated as locally pinned by has_ipfs_cid and filter_ipfs_pinned_oids, while record_pinned_cid’s COALESCE refuses to replace the stale value. Add a migration/backfill or explicit local-IPFS state before relying on these predicates. This is the same unresolved issue noted in the previous review.
#!/usr/bin/env bash
rg -n 'ALTER TABLE.*pinned_cids|UPDATE pinned_cids|pinata_cid|record_pinata_cid' \
crates --glob '*.sql' --glob '*.rs' || trueAlso applies to: 2268-2278, 2306-2319, 2322-2333
🤖 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/db/mod.rs` around lines 2162 - 2171, Separate legacy
Pinata CIDs from local-IPFS state before the predicates and upsert logic use
pinned_cids.cid: add a migration/backfill or explicit local-IPFS indicator that
identifies existing rows created by record_pinata_cid and clears or excludes
their stale cid values. Update record_pinned_cid, has_ipfs_cid, and
filter_ipfs_pinned_oids to use that corrected state, ensuring legacy Pinata-only
rows can be replaced by a real local CID.
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
New Features
Monitoring
Bug Fixes