fix(node): implement reconciliation sweep as durability backstop (#218) - #244
fix(node): implement reconciliation sweep as durability backstop (#218)#244Gravirei wants to merge 29 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. |
|
Caution Review failedAn error occurred during the review process. Please try again later. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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. Git subprocess tracking, startup wiring, configuration, 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 GitCommand
participant PinningBackends
participant Metrics
NodeStartup->>ReconciliationWorker: start periodic sweep
ReconciliationWorker->>Database: load cursor and list repository batch
ReconciliationWorker->>GitCommand: scan repository objects
ReconciliationWorker->>Database: filter existing pins
ReconciliationWorker->>PinningBackends: pin missing objects and reseal withheld blobs
PinningBackends->>Database: record pin results
ReconciliationWorker->>Metrics: record gaps found and filled
ReconciliationWorker->>Database: persist completed cursor
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.
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
github-actions
Bot
removed
the
needs-tests
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.
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
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
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.
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.
Superseded by the re-review at c868820.
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
-
[P0] Keep strict Ed25519 verification for identity-bearing signatures
crates/gitlawb-core/src/identity.rs:80
This replacesverify_strictwithVerifier::verifyand removes the regression test for the identity-point forgery. In the resolveded25519-dalekversion, ordinary verification does not reject small-order public keys orR, so adid:keycontaining the identity point plusR = identity, S = 0verifies for arbitrary messages.identity::verifyis the shared primitive for HTTP request authentication, UCANs, and certificates, so this lets an attacker satisfy signed write authentication without a private key. The identical change incrates/gitlawb-attest/src/attestation.rs:114also makes cert-bound attestations forgeable. Restore strict verification (or explicitly reject weak keys) in both paths and retain the deleted tests. -
[P1] Do not return raw database and internal failures to HTTP clients
crates/gitlawb-node/src/error.rs:171
The newe.to_string()response bodies undo the opaque 500 boundary and the deleted tests exercised precisely this behavior. Public handlers can now return SQLx schema/query diagnostics, filesystem paths, backend response details, and context-wrapped operational data to unauthenticated callers whenever a query or internal operation fails. Log the detailed error server-side and restore fixed client-facing messages forDbandInternal. -
[P1] Fence visibility changes across the whole public pin batch
crates/gitlawb-node/src/reconciliation.rs:514
The sweep rechecks and refilters once, then gives up to 50,000 objects to a sequential IPFS/Pinata upload loop that can run for minutes. If the owner quarantines the repo or adds a root/path deny after that preflight, every remaining object into_pinis still published to the public backend. The same race exists in the Pinata block at line 563. Couple the visibility/quarantine version to each irreversible upload (or hold an appropriate policy fence) instead of treating one batch-start check as the pin boundary. -
[P1] Re-derive encrypted recipients at the sealing boundary
crates/gitlawb-node/src/reconciliation.rs:635
This derives the withheld-blob recipient map, then performs a potentially long walk and seal/upload phase using that stale map. Removing a reader after the derivation can therefore cause the sweep to create its first recovery envelope for an already-revoked DID; the ciphertext remains decryptable after the policy change. Re-fetch and re-derive the recipient set immediately before sealing, with the same policy fence needed for a multi-object batch. -
[P2] Make the encrypted-pin deadline actually bound Git reads
crates/gitlawb-node/src/reconciliation.rs:689
The outertokio::time::timeoutcannot preemptencrypt_and_pin: that async function directly calls synchronousgit::store::read_objectfor each blob, which uses unboundedCommand::output(). While that call blocks, the timeout is not polled and the Git child is neither deadline-limited nor reaped, so a stuck repository can exceed the advertised 300-second cap and occupy a Tokio worker. Use the bounded read API underspawn_blockingand pass the remaining deadline through the sealing loop. -
[P3] Keep the reconciliation gap metrics on one unit of account
crates/gitlawb-node/src/reconciliation.rs:495
gaps_foundis the unique OID union across IPFS and Pinata, butgaps_filledlater sums successful backend writes. One object absent from both backends consequently reports one gap found and two gaps filled, contradicting the metric help text and allowing dashboards to show more repaired gaps than detected. Either count the filled OID union too, or deliberately redefine and rename the metrics as per-backend repairs. -
[P2] Avoid the extra no-op cycle for exact-size repository pages
crates/gitlawb-node/src/reconciliation.rs:760
A page containing exactlyREPOS_PER_PASSrepositories persists its final cursor even when it is the final page. The next hourly pass only observes an empty batch and clears the cursor, so exactly 100 repositories are swept every two hours rather than hourly (and each exact full page adds another empty hour to larger cycles). Fetch a look-ahead row or otherwise determine whether a successor exists before persisting the cursor.
The shared verify primitives (identity::verify and the attestation verifier) accepted the identity-point forgery: public key A = identity, R = identity, S = 0 satisfies [S]B = R + [k]A for any message under ordinary Ed25519 verification. Since identity::verify backs HTTP request authentication, UCANs, and certificates, that is an authentication bypass. Use verify_strict, which rejects small-order R and public keys.
…wb#226) AppError::Db and AppError::Internal serialized the raw error string into the HTTP body, exposing query text and schema details on open routes like GET /api/v1/repos. Log the real error server-side (chain via {e:#}) and return opaque generic messages; connection-level failures still map to 503 db_unavailable.
…ciliation-sweep-v2 # Conflicts: # crates/gitlawb-node/src/error.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] Fix the failing formatting gate
crates/gitlawb-core/src/identity.rs:228
The added identity-point test is not rustfmt-formatted; the same is true of the new attestation test atcrates/gitlawb-attest/src/attestation.rs:498,502.cargo fmt --all -- --checkfails on exactly these hunks, which is why the requiredfmt + clippycheck is red despite the other test/build jobs passing. This is a mechanical failure rather than a problem with the strict-verification change itself: run rustfmt over the touched Rust files and rerun the required check before merging. -
[P1] Fence a visibility change against an already-built public pin batch
crates/gitlawb-node/src/reconciliation.rs:514
The worker fetches current rules and refilters the candidate set once, then passes up to 50,000 OIDs to a sequential IPFS/Pinata loop. Those loops consult onlyhas_*_cid; they never observe quarantine or visibility again. Meanwhile, an owner can commit a rule update throughset_visibility_ruleindependently, with no shared lock, revision, or cancellation channel. If/secret/**is made private (or the repository is quarantined) after this preflight while an earlier object is uploading, the remaining pre-authorized objects are still uploaded in cleartext to a public content-addressed backend. The root cause is treating a batch-level snapshot as authorization for an irreversible multi-object operation. Introduce a policy epoch/version or synchronization boundary that invalidates queued work when visibility narrows, and have the pin loop stop/revalidate before dispatching further uploads. Add a delayed-upload race test that changes a path rule mid-batch and proves the later object is never sent. -
[P1] Re-authorize encrypted recipients at the seal boundary
crates/gitlawb-node/src/reconciliation.rs:641
withheld_blob_recipients_boundedsnapshots the recipient map before a potentially long Git walk, thenencrypt_and_pinseals that map without another visibility or quarantine check. A reader removed after the snapshot can therefore receive a newly created, publicly stored recovery envelope that they can still decrypt; if Irys is configured, the new envelope is also anchored. The comment inencrypted_pinthat reader removal is non-retroactive only justifies preserving an envelope created before revocation—it does not justify creating one after revocation from stale policy. The root cause is the same stale authorization snapshot crossing an irreversible batch boundary. Re-fetch/rederive the recipients after the walk and protect the sealing loop with the same policy-version/cancellation mechanism as public pinning; add a race test for a reader removal during recipient derivation. -
[P1] Do not reuse the expired scan deadline for pin-boundary refilters
crates/gitlawb-node/src/reconciliation.rs:518
scan_deadlineis deliberately shared by the initial object scan and its first visibility refilter, but it is then reused for the IPFS and Pinata pin-boundary refilters. A slow yet successful initial scan can consume most or all of the 300-second budget. The nextrefilter_public_objectscomputes a zero remaining duration, returnsNoneimmediately, and the caller converts that to an emptyto_pinlist; the Pinata check comes even later, after the IPFS phase. The result is a fail-closed but permanent hourly skip for large/slow repositories, which defeats the durability backstop rather than retrying work under a usable bound. Separate the scan budget from the authorization-at-dispatch budget, or define one explicit aggregate per-repo budget that reserves time for each mandatory revalidation. Add a deterministic test with an exhausted initial scan budget and verify that a later pin-boundary check still has an allocated budget or yields an explicit retry state. -
[P2] Make the encrypted pin deadline able to stop Git reads
crates/gitlawb-node/src/encrypted_pin.rs:155
The sweep wrapsencrypt_and_pinintokio::time::timeout(PIN_PHASE_DEADLINE), but the function directly calls synchronousread_objectfor each OID. That path ultimately uses unboundedCommand::output()on the Tokio worker. Whengit cat-filehangs, the worker cannot poll the timeout future, so the claimed 300-second cap never fires and the child is not reaped. The root cause is applying an async timeout outside blocking work instead of making the blocking operation deadline-aware. Move the read tospawn_blockingand use the existing bounded/reaped Git read API with the remaining phase deadline; include a fake hung Git executable test that asserts both timely return and child cleanup. -
[P2] Reset the cursor when a full page is the terminal page
crates/gitlawb-node/src/reconciliation.rs:760
The code usesbatch.len() < REPOS_PER_PASSas a proxy for “this is the final page.” That is false when the final page contains exactly 100 rows: it persists the last ID, and the next hourly run performs only an empty query that clears the cursor. A node with exactly 100 repositories therefore performs a real sweep every other hour; exact multiples introduce an unnecessary empty-hour cycle between full rounds. This is an off-by-one pagination termination design rather than a transient condition. FetchREPOS_PER_PASS + 1rows (or perform a successor lookahead), process only the first page, and clear the cursor once the absence of a successor is known. Add coverage for exactly 100 and 200 repositories. -
[P3] Use the same unit for gaps found and gaps filled
crates/gitlawb-node/src/reconciliation.rs:495
gaps_foundis intentionally the union of missing OIDs across IPFS and Pinata, butgaps_filledsums successful writes from both backends at:617. For one object absent from both and successfully repaired on both, operators see one gap found and two gaps filled. That contradicts the metric HELP strings, which describe objects/gaps rather than backend operations, and lets dashboards report repairs exceeding detections. Decide whether these counters represent unique object gaps or backend repair operations, then make both code and HELP text follow that definition. For object-level progress, deduplicate successful OIDs before incrementing; for backend-level progress, use separate backend-labelled metrics.
Superseded by a re-review at 810d71c.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed at 810d71cc. The confidentiality core still holds: I could not construct a rule shape, DID form, or mirror row that gets a withheld blob to a public backend, and the quarantine/visibility recheck at each pin boundary is the right structure. What blocks this round is one defect that makes the backstop stop working on exactly the repos it exists for, plus the migration collision from my 08-09 round that is still open.
Findings
-
[P1] Fix the formatting gate
crates/gitlawb-core/src/identity.rs:225
cargo fmt --all -- --checkexits 1 at this head on three hunks: the 32-elementidentityarray inidentity.rs:225and inattestation.rs:495, and theformat!atattestation.rs:502. Both files are this branch's own work (git log origin/main..HEADon them shows the strict-verification commit, and the forgery tests do not exist on main), and the branch head and the merge ref are byte-identical here, so it is not a merge artifact. jatmn raised this today and it is still red. -
[P1] Allocate migration versions above #173's range
crates/gitlawb-node/src/db/mod.rs:923
This branch takes v18pinned_cids_clear_legacy_equal_cidand v19node_state. Open #173 takes v18pinned_cids_cid_indexand v19pinned_cids_repo_provenance, running through v25. The runner keys the applied set on the integer alone, so whichever merges second skips its DDL entirely: no error,schema_migrationsstill reads healthy, and the column is simply absent. main's max is v17 today, so v26 and up is the safe range while #173 is open. The reservation comment at:906-921claiming "#135/#173 holds through 14" is stale and should be corrected in the same commit. -
[P1] Give the pin-boundary re-derivation a budget of its own
crates/gitlawb-node/src/reconciliation.rs:518
scan_deadlineis computed once at:369and handed to the full scan, the mid-scan refilter, and then both pin-boundary re-derivations at:518and:567. When the scan legitimately uses its budget,deadline.saturating_duration_since(Instant::now())is zero,refilter_public_objectsreturnsNone, and the caller turns that into an emptyto_pinwith only a warn (:531,:580). I ran it both ways against the function directly: a fresh budget returns the list, a spent budget returnsNone. So a repo whose scan fills 300s is skipped for both backends every hourly pass, permanently, and the durability backstop does nothing for the large repos it was written for. Fail-closed is right at the pin boundary; the fix is a separate budget for the re-derivation, not a wider shared one. -
[P2] Acquire
pin_semaphorein the sweep before the pin loops
crates/gitlawb-node/src/reconciliation.rs:539
The push path holds a permit across its pin loops (api/repos.rs:2407), and the doc onstate.rs:144describes that pool as the cap on concurrent MB-scale pin loops. The sweep callsipfs_pin::pin_new_objectsand the Pinata twin directly with no acquire (zeropin_semaphorereferences in this file), which makes the largest pinner on the node the one path outside the cap. -
[P2] Cover the pin-boundary re-derivation with its own test
crates/gitlawb-node/src/reconciliation.rs:518
sweep_never_pins_withheld_blob_in_cleartextbinds the property as a set, not per layer. I removed the deny intersection insiderefilter_public_objectsand the test still passes, because the scan-time filter carries it alone. The newest layer, the one added for the irreversible-pin class, is not independently load-bearing, and a rule that narrows between the scan and the pin boundary is not exercised anywhere. A test that mutates visibility after the scan and asserts the newly denied blob is not pinned would pin it. -
[P2] Reconcile
list_pins' doc with what it now returns
crates/gitlawb-node/src/api/ipfs.rs:704
The doc added in this change says the rawpinata_cidis deliberately not surfaced because it leaks infrastructure detail to unauthenticated callers, and four lines laterdisplay_cid = p.cid.or(p.pinata_cid)emits that same value under thecidkey on a route merged without auth (server.rs:231). Renaming the field does not change what a caller learns. Either the fallback is intended and the doc has to say so, or the row stays omitted. -
[P3] Make
gaps_filledcountable againstgaps_found, and fix the bound comment
crates/gitlawb-node/src/reconciliation.rs:617
gaps_foundis the union of missing OIDs across backends (:497-500);gaps_filledsums per-backend successes. One object missing from both and repaired on both reports 1 found and 2 filled, so the ratio an operator watches can exceed 100% while both HELP strings say "objects". Separately, the comment at:364-368says total blocking per repo is bounded atREPO_SCAN_DEADLINE; phase 2 grants a freshREPO_SCAN_DEADLINEat:647and a freshPIN_PHASE_DEADLINEat:689, so the real per-repo worst case is additive and closer to 30 minutes.
One process note: #173 shares ipfs_pin.rs and api/ipfs.rs with this branch and lands first, so plan on a rebase there rather than only a version bump.
…changes Reviewer R1-P1 (delayed-upload race) and R1-P2 (exhausted-budget interaction) for issue-218's reconciliation sweep: - policy-epoch fence (v28 repos.policy_epoch): every visibility-rule and quarantine mutation bumps the epoch; the sweep captures it at each pin dispatch boundary and the pin loops abort the moment it moves, so a narrow landing mid-batch wins over the pre-authorized snapshot (fail closed). - encrypted seal path fenced the same way per blob; sweep acquires the pin semaphore before both public batches and the encrypted seal so the sweep cannot stack unlimited blocking pool work. - encrypt_and_pin takes git_bin + batch_budget and runs each object read under spawn_blocking with a shared read deadline via the new read_object_bounded_spawn_blocking, so a hung git reaps within budget (recovered_pins budget test). - cursor reset: run_pass fetches REPOS_PER_PASS+1 (lookahead) so a full terminal page clears the cursor instead of rescanning it forever. - gaps_filled counts unique objects across both backends so it stays countable against the union gaps_found (R2-P3). - list_pins doc reconciled with the pinata_cid-under-cid fallback it actually emits (R2-P2). - migrations v18/v19 renumbered to v26/v27 to dodge open Gitlawb#173's 18-25 claim. New tests: pin_new_objects_stops_mid_batch_when_policy_moves, encrypt_and_pin_stops_sealing_when_reader_removed_mid_batch, encrypt_and_pin_returns_by_budget_with_a_hung_git, and sweep_clears_cursor_on_exact_page_boundary.
Superseded by a re-review at 639ebaa.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed at 639ebaa9 by execution. The fmt gate that blocked the last round is green, CI is 12/12 on this head, the metrics dedup and the list_pins doc asks are closed, and the one still-open bot thread is moot since ScanContext and escalate_kill no longer exist in the crate. The confidentiality core continues to hold: I could not construct a rule shape that gets a withheld blob onto a public backend in cleartext. Four asks, and the migration one needs no edit from you right now.
Findings
-
[P1] Renumber the migrations once, after #173 merges, not now
crates/gitlawb-node/src/db/mod.rs:919
The reservation comment says #173 claims 18 through 25. Its current head claims through 26, and 26 is exactly the version this branch also takes, so whichever side merges second would skip its own v26 DDL silently: no error, and aschema_migrationstable that looks healthy. Please don't renumber again yet, since #173 is still open and can add versions, and a third renumber against a moving range buys nothing. I'm holding the merge here until #173 lands, at which point take the next free versions above its final max and correct the comment in the same commit. Nothing for you to track; the gate is mine. -
[P1] Hold one pin permit per repo, not two
crates/gitlawb-node/src/reconciliation.rs:790
run_passtakes the global pin permit at :567 and holds it across the whole repo iteration, then takes a second permit at :790 for the same repo's seal phase.max_concurrent_pin_tasksaccepts 1, and at that value the sweep waits on a permit it is itself holding. The second acquire sits one line above thetimeout(...), so nothing bounds the wait. A probe at pool size 1 with public gaps plus a path-scoped rule carrying one reader hangs until a 15s guard fires; the identical probe at pool size 2 passes in 1.68s. The comment at :560 already asserts the first permit covers the seal, which is the behavior worth making true. -
[P1] Capture the encrypted-path fence before the rules recheck, not after
crates/gitlawb-node/src/reconciliation.rs:731
recheck_public_pinreads the rules at :723 andPolicyFence::captureruns at :731, so a rule change landing between the two is baked into the recipient set while the epoch captured afterward already reflects it.is_currentthen reports current for the whole seal loop and the fence never fires for that narrow. The public path does this in the opposite order and is correct: capture at :572, recheck after at :589. That asymmetry inside one function is what makes this look like a slip rather than a choice. Being straight with you: I verified this by reading both orderings, not with a race repro. -
[P2] Give the two pin-boundary re-derivations independent budgets
crates/gitlawb-node/src/reconciliation.rs:473
Onepin_authz_deadlinecovers both re-derivations, and the comment makes clear that bounding both backends as a unit is deliberate. The issue is what a spent budget does:refilter_public_objectsreturnsNoneand the Pinata caller turns that into an emptyto_pinbehind awarnat :659. The IPFS arm always runs first, so a large repo that consumes the 300s leaves Pinata silently skipped every pass, for exactly the repos this sweep exists to protect. Either give each arm its own budget, or make an exhausted budget a reported outcome instead of an empty list. -
[P3] Add the batch budget gate to the seal loop
crates/gitlawb-node/src/encrypted_pin.rs:138
The IPFS and Pinata loops both gate onbatch_budget_gate; the seal loop has the fence check and a read deadline but no budget gate. It stays bounded by the outer pin-phase deadline, so this is consistency and some wasted child spawns rather than a correctness break.
The base is 11 commits behind main and both api/repos.rs and db/mod.rs have moved there since, so fold the rebase into the same pass as the renumber once #173 is in.
|
One input for the sweep's design, because it decides whether
let cid = body
.lines()
.filter(|l| !l.trim().is_empty())
.filter_map(|line| {
let v: serde_json::Value = serde_json::from_str(line).ok()?;
v["Hash"].as_str().map(|s| s.to_string())
})
.next_back()
.unwrap_or(expected_cid.clone());So an empty body, a body carrying no The row is then written, and at Why that matters specifically for this PR: Worth weighting: the plain git objects are the less serious half, since the CID serve path in The trigger is operator or infrastructure configuration rather than an attacker: The sibling sink already does the strict thing, so there is a precedent to copy: Two practical notes if this gets folded in rather than tracked separately. Requiring I have not run this end to end; the claims above are from reading the tree at |
…re rules recheck Reviewer R2-P1 findings on the reconciliation sweep: - P1 permit: run_pass held the global pin permit for the whole repo iteration, then acquired a SECOND one for the same repo's seal phase. With max_concurrent_pin_tasks = 1 the sweep waited on the very permit it held, deadlocking past the guard timeout. The seal phase now reuses the permit the public phase already holds and only acquires when the public phase held none (one permit per repo, never two). - P1 ordering: the encrypted-path PolicyFence was captured AFTER recheck_public_pin's rule read, so a narrow landing between the two was baked into the recipient set while the captured epoch already reflected it and is_current stayed true for the whole seal loop. Capture now runs BEFORE the recheck, mirroring the public path. - P2 budgets: IPFS and Pinata re-derivations previously shared one pin_authz_deadline; IPFS re-derives first, so a large repo that consumed it left Pinata silently skipped every pass. Each arm now re-derives against its own fresh REPO_SCAN_DEADLINE. - P3 budget gate: the seal loop in encrypt_and_pin got the same batch_budget_gate the IPFS/Pinata loops use, so the three loops cannot drift apart in how they report a truncated batch. New test: run_pass_reuses_the_pin_permit_for_the_seal_at_pool_size_one, which reproduces the reviewer's probe (public gaps + path-scoped rule with a reader, pool size 1) and fails on the old double-acquire (verified by reverting the fix: deadlocks until the 60s test timeout).
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Rechecked head b44c951 after the prior draft. Several earlier-round items are closed on this head (pin-permit deadlock, encrypted-fence ordering, per-backend pin-boundary budgets, cursor lookahead, metrics dedup, bounded encrypted reads, PolicyFence mid-batch abort). What remains below is what survives that re-verification.
The per-finding notes below are intentionally narrow. Read Overall guidance first — it explains why this PR has produced findings across many rounds and what to change so the next push is not another drip cycle.
Overall guidance: why findings keep appearing, and how to stop the drip
This PR has been through a long review arc (20+ commits, multiple CHANGES_REQUESTED rounds, beardthelion and jatmn both filing overlapping classes of issues). That is not because the confidentiality core is weak — reviewers have repeatedly confirmed they could not construct a path that gets a withheld blob onto a public backend in cleartext. The drip happens because of how the change is packaged and iterated, not because the sweep idea is unsound.
1. One branch is carrying several independent projects
Issue #218 asks for a periodic reconciliation sweep. This branch also ships, in the same diff:
| Subsystem | What landed here | Could stand alone |
|---|---|---|
| Pin provenance | nullable cid, migration v12 semantics, v26 legacy clear, has_ipfs_cid / record_pinned_cid upsert rules, /api/v1/ipfs/pins synthesis |
Yes — PR 1 |
| Policy fencing | repos.policy_epoch, PolicyFence, bumps on rule/quarantine mutation, mid-batch abort in pin/seal loops |
Yes — PR 2 |
| Durability sweep | reconciliation.rs, cursor in node_state, metrics, spawn gate, bounded git scans |
Yes — PR 3 (depends on 1–2) |
| Crypto hardening | strict identity::verify, attestation forgery tests |
Separate (#309 on main) |
| Unrelated drift | cert threshold, advisory locks, LIKE escaping — regressions vs current main, not new sweep work |
Should never have been in this branch |
beardthelion asked twice to split into three PRs (nullable-cid / process plumbing / sweep). That ask was never taken. The diff grew from ~600 lines to ~1,500. Every extra subsystem multiplies the review surface: a reviewer (or the next fix) touches db/mod.rs and can disturb migration numbering, pin predicates, cert queries, and sweep cursor persistence in one pass.
Why that causes drip: Round N fixes a sweep deadline bug in reconciliation.rs. Round N+1 discovers the fix reused a spent scan_deadline one gate earlier, or that PolicyFence ordering differs between public and encrypted paths, or that migration v26 now collides with #173. The fixes are real, but they are symptoms of reviewing a stack as a single lump.
What to do: Land foundations first, sweep last.
- PR A (pin state): nullable
cid, provenance predicates,list_pins/ CLI contract, migration for legacy rows. Small, reviewable in one round. - PR B (policy epoch):
policy_epochcolumn, bump sites,PolicyFence, wire it on both push and sweep paths (or document push as out of scope). - PR C (sweep):
reconciliation.rsonly, depending on A+B. Cursor, metrics, spawn gate, tests that prove gap-fill end-to-end.
If splitting is impossible now, freeze scope: no more drive-by changes to cert.rs, error.rs, repo_store.rs, or unrelated db queries. Rebase onto main and delete anything that is not sweep-critical.
2. Long-lived branch + merge (not rebase) imports silent regressions
The branch forked from an older main and integrated upstream via merge commit 34f0619. That pattern is the direct cause of the three P1 rebase findings (cert threshold, SHA-256 advisory locks, LIKE escaping). They are not sweep bugs; they are wrong conflict resolutions that would revert security fixes already on main if this merged today.
Why that causes drip: Each review round re-litigates sweep logic while the diff still contains landmines in files the author never meant to change. Reviewers file cert/lock/LIKE findings; author fixes sweep items; next round someone notices the regressions are still present. The cycle feels endless because process debt and feature debt are mixed.
What to do:
- Rebase onto current
mainbefore every push, not once at the end. - On conflict: default to
mainfor any hunk not required for #218. Re-apply only sweep/pin/policy hunks manually. - After rebase, run a three-file smoke check (see P1 finding):
cert.rsHashSet test,repo_store.rsSHA-256 test,dbwildcard test. If any are missing, the rebase failed silently.
3. Fixes for round N often introduced round N+1 findings
Several findings in earlier rounds were introduced by the fix for the previous round, not by the original sweep design:
- Adding
fresh_repore-fetch for phase 1 but initially passing batch-snapshotowner_didin phase 2. - Adding
PolicyFencefor irreversible pins but capturing epoch after rules recheck on the encrypted path (fixed onb44c951) while public path captures before. - Giving pin-boundary re-derivation a fresh
REPO_SCAN_DEADLINEbut leaving the mid-scan refilter on the spentscan_deadline(still open — see P2). - Holding one pin permit per repo but initially acquiring a second for the seal phase (deadlock at pool size 1 — fixed).
- Renumbering migrations to v26/v27 while #173 also claimed v26 — coordination failure, not logic error.
Why that causes drip: The PR is iterating under time pressure on a moving stack. Each fix addresses the specific line a reviewer cited without re-auditing the whole irreversible-publish pipeline (scan → refilter → recheck → fence → re-derive → pin loop) for consistent policy and time bounds.
What to do: After each fix, walk one horizontal slice instead of patching vertically:
For one repo, trace time and policy at every irreversible boundary:
scan_deadline → mid-scan refilter → recheck_public_pin → PolicyFence::capture
→ refilter at pin boundary → pin_new_objects loop (fence per object)
→ encrypted: fence → recheck → recipients walk → encrypt_and_pin loop
Draw a table: stage | deadline variable | policy check | what happens on timeout/narrow. Inconsistencies in that table are where the next drip finding comes from. The remaining P2 (mid-scan refilter) is exactly such an inconsistency: pin-boundary got a fresh budget; mid-scan refilter did not.
4. Tests prove properties as a set, not per layer
The suite improved (sweep_fills_ipfs_gap, mirror skip, exact-page cursor, pool-size-1 permit, withheld-blob cleartext). But several tests bind the property ("never pin withheld blob") while multiple layers implement it (scan filter, mid-scan refilter, pin-boundary re-derive). Reviewers demonstrated that removing only the pin-boundary intersection still leaves sweep_never_pins_withheld_blob_in_cleartext green because the scan-time filter alone carries the test.
Why that causes drip: Author believes a class is covered; reviewer removes the newest layer and files the same class again; author adds another layer; repeat.
What to do: For each irreversible boundary, one test that only that boundary defends:
| Property | Test idea |
|---|---|
| Mid-scan narrow | Add rule after scan object list built, before pin phase; assert denied blob not in to_pin |
| Pin-boundary narrow | Mutate rule after mid-scan refilter, before pin_new_objects; assert upload not sent (mock server call count) |
| Quarantine mid-pass | Quarantine after batch read, before pin; assert zero uploads |
| Exhausted scan budget | Scan consumes full REPO_SCAN_DEADLINE; assert refilter still runs when given fresh deadline (P2 fix) |
| Gap-fill actually repairs | Already have sweep_fills_ipfs_gap — keep it |
Tests should go red when the specific gate under test is deleted, not when any gate is deleted.
5. Pre-existing assumptions were never declared as non-goals
The sweep's design doc (PR body, #218) says it "re-derives the set of objects a repo should have replicated … and produces the missing ones." That implies:
- Missing = not in DB with correct provenance, or
- Missing = not on backend (stronger)
The code implements the weaker predicate (has_ipfs_cid / has_pinata_cid). pin_git_object on main already records a row on empty 2xx bodies (ipfs_pin.rs:190). The sweep amplifies that pre-existing hole but did not introduce it. Reviewers keep returning to it because the PR claims a durability backstop without stating the backstop is DB-row-shaped, not backend-verified.
Why that causes drip: Same finding reappears under "sweep trusts bad rows" because scope was never closed in the PR description.
What to do: In the PR body, add an explicit Non-goals section:
- "This sweep repairs OIDs with no
pinned_cidsrow (or Pinata-only row needing IPFS). It does not verify Kubo still holds the object; false-positive rows from misconfiguredGITLAWB_IPFS_APIrequire a separate fix topin_git_object." - "Push-path pinning does not use
PolicyFencetoday; sweep does. Parity is follow-up #XXX."
Declaring non-goals stops reviewers from re-filing the same scope question.
6. Parallel work (#173) shares files and migration namespace
#173 touches db/mod.rs, ipfs_pin.rs, and api/ipfs.rs — the same files this PR changes. Both claim migration v26. beardthelion is holding merge until #173 lands.
Why that causes drip: Migration/version findings recur whenever either branch moves. This is not fixable by more sweep logic; it is coordination.
What to do: Do not renumber migrations again until #173 merges. After it lands, one rebase, one renumber pass, one reservation-comment update. No further migration edits on this branch until then.
7. What is actually left vs what is cycle noise
| Category | Items | Action |
|---|---|---|
| Process (not sweep logic) | Rebase regressions (cert, locks, LIKE) | Rebase onto main; verify three tests |
| Coordination | Migration v26 vs #173 | Wait, renumber once |
| Real sweep logic | Mid-scan refilter deadline (P2) | Fresh authz_deadline or document deferral |
| Scope decision | pin_git_object false positives (P3) |
Fix or declare non-goal in PR body |
| Quick cleanup | try_get("cid").ok(), RUN-A-NODE doc (P3) |
One-line + doc row |
If the next push addresses rebase + P2 + scope declaration + nits without adding new subsystems, review should converge. If the next push adds more migrations, push-path fencing, or Kubo validation in the same branch, expect another drip round.
8. Recommended sequence for the next (hopefully final) author pass
Do these in order; do not skip ahead:
- Wait for #173 to merge (or get explicit maintainer OK to proceed with renumber).
- Rebase onto current
main. Resolve conflicts by keepingmainfor unrelated files. Run fullcargo test --workspace(or CI). - Renumber migrations to first free versions after #173. Update reservation comment.
- Fix P2 (mid-scan refilter deadline) — one logical commit, one test that fails without it.
- Declare non-goals in PR description (pin_git_object / push-path fence). Optionally file follow-up issues and link them.
- Fix P3 nits (
get::<Option<String>>, RUN-A-NODE row). - Push and stop. No new features, no "while I'm here" cleanups, no bundling strict-ed25519 or error-body changes unless
mainrequires them for conflict resolution.
That sequence attacks root causes (stack size, stale base, inconsistent deadlines, undeclared scope) rather than the symptom list one finding at a time.
Findings
-
[P1] Rebase onto current
mainbefore the next review round
crates/gitlawb-core/src/cert.rs:138,crates/gitlawb-node/src/git/repo_store.rs:733,crates/gitlawb-node/src/db/mod.rs:2228What is wrong. This head is behind current
mainand would land three unrelated reversions if merged as-is:RefUpdateCert::satisfies_thresholdcounts signature entries (.count()) instead of distinct signer DIDs. One maintainer can satisfy a 2-of-3 threshold by duplicating their own valid signature.mainrestored distinct-signer counting in #326 and carriessatisfies_threshold_rejects_duplicated_signature; this head dropped both.advisory_lock_keyis back onstd::collections::hash_map::DefaultHasher.mainuses SHA-256 with domain-separatedowner_slug + ":" + repo_name, stability tests, and the shared-Postgres rolling-upgrade warning indocs/RUN-A-NODE.md.DefaultHasheris not stable across Rust toolchain versions, so shared-Postgres nodes can compute different lock keys for the same repo and lose cross-machine write exclusion.list_ref_certificates_by_prefixbuildsformat!("{}%", prefix)with noESCAPEclause. Prefixes containing%,_, or!match unrelated certificates.mainescapes metacharacters with!as the escape character and has regression tests for literal-wildcard behavior.
Root cause. These are not reconciliation-logic bugs. The branch forked before #326 and related
mainhardening landed, then merge commit34f0619(merge: upstream main into issue-218 reconciliation sweep v2) resolved conflicts by keeping the branch-side copies of files the sweep never intended to touch. The reconciliation work is sound on its own surface; the problem is stale-base drift that would revert upstream security fixes on merge.Author guidance.
git fetch origin && git rebase origin/main(or mergemainonce if you prefer, but rebase keeps the history cleaner).- For each conflict in
cert.rs,repo_store.rs, anddb/mod.rs, takemain's version of the unrelated hunks and re-apply only your reconciliation-specific changes (migrations v26–28,node_state,policy_epoch, nullablecid, etc.). - After rebase, verify the three fixes survived:
cert.rs:HashSetdistinct-signer counting andsatisfies_threshold_rejects_duplicated_signaturetest present.repo_store.rs:pub(crate) fn advisory_lock_keyusing SHA-256;advisory_lock_key_is_stabletest present.db/mod.rs:list_ref_certificates_by_prefixwith!escape logic andlist_ref_certificates_by_prefix_treats_wildcards_literallytest present.
- Run
cargo test -p gitlawb-core cert::testsand therepo_store/dbtests that cover those paths before pushing.
Do not hand-fix the three regressions in isolation on this stale base — rebasing is the root-cause fix.
-
[P1] Allocate migration versions above open #173 before merge
crates/gitlawb-node/src/db/mod.rs:926What is wrong. This branch claims migration v26 (
pinned_cids_clear_legacy_equal_cid), v27 (node_state), and v28 (repos_policy_epoch). Open #173's current head also claims v26 (pin_repair_sweep_discovery_cursor) and continues through v25. The migration runner indb/mod.rskeys the applied set on the integer alone: whichever branch merges second silently skips its own DDL whileschema_migrationsstill reads healthy. On this branch that means either nonode_statetable (sweep cursor never persists across restart) or no #173 pin-repair schema, depending on merge order.Root cause. Both branches independently chose v26 while #173 was still open. Migration version numbers are a global namespace in
MIGRATIONS; two PRs cannot claim the same integer even when the SQL is unrelated. The reservation comment above v17 is now stale relative to #173's 18–25 (and its v26).Author guidance.
- Wait for #173 to merge (beardthelion is already holding the merge gate here).
- Rebase onto the resulting
mainand inspectMIGRATIONSfor the highest applied version. - Renumber this branch's three migrations to the next free integers (e.g. if #173 ends at v26, take v27/v28/v29 for
pinned_cids_clear_legacy_equal_cid,node_state, andrepos_policy_epoch). - Update the reservation comment block above the new entries so the next contributor knows which integers are taken.
- Confirm
db.run_migrations()in tests still applies all entries in order and thatnode_state_roundtrip_and_delete/migration_v12_*tests pass on a fresh DB and on an upgrade path.
No code change is needed until #173 lands; the root cause is merge-order coordination, not sweep logic.
-
[P2] Give the post-scan visibility re-filter its own deadline
crates/gitlawb-node/src/reconciliation.rs:488What is wrong. After the bounded full git scan completes,
run_passcallsrefilter_public_objects(..., scan_deadline)at:488-494using the samescan_deadlinethat the scan just consumed (:402-443).refilter_public_objectswraps its work intokio::time::timeout(deadline.saturating_duration_since(Instant::now()), ...)(:161-162). When the scan legitimately uses most or all of the 300-secondREPO_SCAN_DEADLINE, the remaining duration is zero, the timeout fires immediately, the function returnsNone, and the caller hitscontinueat:499("fresh-visibility re-filter failed, skipping"). That aborts the entire repo iteration — no IPFS/Pinata gap-fill and no encrypted recovery — even though the pin-boundary re-derivations at:598and:659were explicitly given a freshInstant::now() + REPO_SCAN_DEADLINEfor exactly this starvation class (:465-473). Those fresher budgets are unreachable once the mid-scan refilter fails.Root cause. The read phase was designed as one bounded unit (scan + refilter share
scan_deadline, comment at:394-401), but the pin phase was later split into per-boundary fresh budgets without updating the mid-scan refilter. The failure mode is structural: a repo whose scan fills the read budget is permanently skipped every hourly pass, which is exactly the population the durability backstop exists for.Author guidance (pick one approach and document it).
Option A — recommended: fresh budget for the mid-scan refilter (matches pin-boundary pattern).
- After the scan succeeds at
:459, computelet authz_deadline = Instant::now() + REPO_SCAN_DEADLINE;. - Pass
authz_deadline(notscan_deadline) into therefilter_public_objectscall at:488. - Update the comment at
:486-487to say the read scan and the authorization refilter each get their ownREPO_SCAN_DEADLINE(worst case ~10 minutes of blocking per repo, which the file already documents as additive at:32-38). - Add a test that simulates an exhausted scan budget (mock or inject a deadline near
Instant::now()) and asserts the refilter still runs when given a fresh deadline — mirror the pin-boundary starvation test pattern.
Option B — accept the tradeoff explicitly.
If sharing the read budget is intentional, change thecontinueat:499to a softer outcome: log atinfothat the repo is deferred (not failed), and/or persist a per-repo "deferred" marker so operators can see repos starved by read-phase budget. Update the PR description andreconciliation.rsmodule comment to state that repos whose scan consumes the full read budget are intentionally deferred to a later hourly pass. Do not leave the current behavior undocumented — it reads like a bug whengaps_foundcan be nonzero on a prior pass but this pass skips silently.Either way, the root cause is inconsistent deadline policy between the mid-scan gate and the pin-boundary gates; align them or document the divergence.
- After the scan succeeds at
-
[P3] The sweep does not close a pre-existing false-positive IPFS pin hole
crates/gitlawb-node/src/ipfs_pin.rs:190,crates/gitlawb-node/src/reconciliation.rs:510What is wrong.
pin_git_object(ipfs_pin.rs:182-190) accepts any 2xx Kubo/api/v0/addresponse with noHashfield and falls back toexpected_cidcomputed locally from the bytes. A misconfiguredGITLAWB_IPFS_API(proxy returning HTML, health check on wrong port, truncated gateway) therefore returnsOk(expected_cid)andrecord_pinned_cidwrites a row. The sweep then computes missing work exclusively fromfilter_ipfs_pinned_oids/has_ipfs_cid(cid IS NOT NULLatdb/mod.rs:2733-2742), so a falsely recorded row suppresses every future repair attempt. Encrypted recovery copies that depend onipfs_pin::catare gone with no second copy. This behavior is unchanged onmain— the diff does not touchpin_git_object— but the new sweep makes the hole matter because it trusts DB rows as durability evidence.Root cause. Durability is inferred from database presence, not from backend verification. The push path and the sweep share the same
has_ipfs_cidpredicate, so a silent false positive at pin time becomes a permanent blind spot for the backstop.Author guidance (choose scope explicitly).
If in scope for this PR:
- In
pin_git_object, after parsing the response body, require a presentHashfield (like Pinata'sdata.cidcheck atpinata.rs:61-63). ReturnErron empty body, non-JSON body, or missingHash. Log mismatches between returnedHashandexpected_cidat warn level without necessarily failing (Kubo chunking can differ), but do not record a row on missingHash. - Update
delaying_endpointand any tests that assert empty-body success (ipfs_pin.rs:565-566) to return{"Hash":"<cid>"}instead. - Optionally add a sweep-side probe: for a sample of
has_ipfs_cidrows,HEADorcatagainst Kubo and clear/re-pin rows where the backend returns 404. That is heavier but closes the "row exists, object doesn't" class without changing push-path semantics.
If out of scope:
- State in the PR description and in a comment above
filter_ipfs_pinned_oidsusage inreconciliation.rsthat the sweep repairs missing rows, not phantom rows wherepin_git_objectrecorded a CID the backend never stored. - File a follow-up issue for strict Kubo response validation (beardthelion's PR comment is the spec).
The root cause is a contract mismatch: the sweep assumes
pinned_cidsis evidence of backend durability, butpin_git_objectonly proves the HTTP call returned 2xx. - In
-
[P3] Minor decode and operator-doc nits
crates/gitlawb-node/src/db/mod.rs:2723,docs/RUN-A-NODE.mdWhat is wrong.
list_pinned_cidsmaps rows withcid: r.try_get("cid").ok().try_get::<String>fails on SQLNULLand on decode errors;.ok()turns both intoNone. A corruptcidcolumn is indistinguishable from a Pinata-only row (cid IS NULL), so/api/v1/ipfs/pinsmay silently omit or misrepresent the row instead of surfacing a DB error.GITLAWB_RECONCILIATION_SWEEPis documented inREADME.mdand.env.examplebut absent fromdocs/RUN-A-NODE.md. Operators who follow only the runbook cannot discover the default-on hourly sweep or how to disable it.
Root cause. The nullable-
cidchange introduced a convenience decode (try_get().ok()) that conflates expected nulls with error paths. The operator doc was updated in README/.env.examplebut not in the runbook.Author guidance.
- In
list_pinned_cids, replacer.try_get("cid").ok()withr.get::<Option<String>, _>("cid")so only SQLNULLbecomesNoneand decode failures propagate through the existing?/AppError::Dbpath. - Add a row to
docs/RUN-A-NODE.mdin the environment-settings section (mirror theREADME.md:345entry): name, default (true), behavior (hourly bounded sweep when IPFS or Pinata is configured; no-op when neither backend is set), and pointer to disable viaGITLAWB_RECONCILIATION_SWEEP=false. - No test is strictly required for the doc change; for the decode fix, the existing
list_pinsclosed-pool test path is sufficient smoke coverage — a unit test assertingNULLmaps toNoneand a malformed value errors would be a nice addition if you touch the query mapping anyway.
Superseded by a re-review at b44c951.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed at b44c9519 by execution. This round's four fixes all land. The permit reuse is real and its regression test is load-bearing: restoring the old second acquire_owned() turns run_pass_reuses_the_pin_permit_for_the_seal_at_pool_size_one red at the 60s guard, and the fix is green. Both pin-boundary re-derivations now take a fresh budget, the encrypted fence is captured before the rules re-fetch so there is no epoch-blind window, and the seal loop's batch gate matches its IPFS and Pinata siblings. The reconciliation suite is 13/13 locally on this head and CI is 12/12.
What blocks the merge is not in this round's diff. It is the branch's base state, and I checked each item against the live remote rather than against the review history.
Findings
-
[P1] Rebase onto current main; as it stands the merge reverts three security fixes
crates/gitlawb-core/src/cert.rs:138,crates/gitlawb-node/src/git/repo_store.rs:733,crates/gitlawb-node/src/db/mod.rs:2228I diffed each of these against
origin/maindirectly.satisfies_thresholdhere counts signature entries, while main counts distinct signer DIDs through aHashSet, so on this branch one maintainer satisfies a 2-of-3 threshold by duplicating their own signature; main'ssatisfies_threshold_rejects_duplicated_signatureis absent here.advisory_lock_keyis back onDefaultHasher, whose output is not stable across toolchain versions, so two nodes sharing a Postgres can compute different keys for the same repo and lose cross-machine write exclusion; main hashes SHA-256 overowner_slug + ":" + repo_nameand pins it with a golden test that this branch also lacks. Andlist_ref_certificates_by_prefixbuildsformat!("{}%", prefix)with noESCAPEclause, so a prefix containing%or_matches unrelated certificates; main escapes those. None of the three is reconciliation work. Take main's side of those hunks in the rebase and re-apply only the sweep, pin-provenance and policy-epoch changes. -
[P1] Renumber the new migrations above #173's range
crates/gitlawb-node/src/db/mod.rs:926I pulled the version and name pairs from
db/mod.rson all three heads. Main tops out at v17. #173 is open and mergeable and now claims v18 through v26, with v26 aspin_repair_sweep_discovery_cursor. This branch also claims v26, forpinned_cids_clear_legacy_equal_cid. The runner keys the applied set on the integer alone, so whichever of the two merges second skips its own v26 in full with no error and a healthy-lookingschema_migrations. Wait for #173, then take the next free integers and refresh the reservation comment, which the renumber has outrun. -
[P2] Give the mid-scan re-filter its own deadline
crates/gitlawb-node/src/reconciliation.rs:488This round gave both pin-boundary re-derivations a fresh
REPO_SCAN_DEADLINE, with a comment explaining that a spent deadline computes a zero remaining duration and turns the repo into a permanent hourly skip. The mid-scan re-filter at:488still gets the scan's ownscan_deadline, and it is the one whoseNonehitscontinueat:499, before the encrypted seal phase at:724. So the failure it causes is larger than the one that was fixed: both backends and the recovery-copy seal are dropped for that repo. I probed the function in both directions on this head, fresh deadline returnsSome, spent deadline returnsNone. Worth noting the band is wider than an exactly-exhausted budget, since a scan that truly runs out times out at:455instead. The re-filter re-walksreplicable_blob_set_boundedandall_blob_oidsover the same repo, so any scan that finishes late enough to leave less than that second walk needs produces the skip, which is the large-repo population the backstop exists for. Either give it its own budget or make thecontinuean explicit logged deferral and say so in the module comment.
The branch is 11 commits behind main and db/mod.rs and api/repos.rs both moved upstream in that window, so the rebase needs a re-review of those two resolutions regardless of the findings above. jatmn's P3s from earlier today still stand as written and I am not adding to them.
…ciliation-sweep-v2
R2-P1: the post-scan re-filter reused the scan's `scan_deadline`, so a repo whose bounded scan consumed its whole budget computed a zero remaining duration, timed out immediately, and aborted the repo iteration before any pin/seal work - permanently skipping exactly the large repos the sweep exists for. The pin-boundary re-derivations already got fresh per-arm budgets; the mid-scan gate now uses the same pattern (`authz_deadline`). Adds a unit test proving a spent deadline starves the re-filter (immediate None) while a fresh deadline lets it complete.
… Hash R2-P3 false-positive hole: pin_git_object accepted any 2xx /api/v0/add response and fell back to the locally computed expected_cid when the body carried no Hash. A misconfigured GITLAWB_IPFS_API (proxy returning HTML, health check on wrong port, truncated gateway) therefore returned Ok(expected_cid) and record_pinned_cid wrote a row the reconciliation sweep then trusts as durability evidence - a permanent blind spot for the backstop. A missing Hash now fails the pin with an explicit error (mirrors Pinata's data.cid check); a mismatched Hash logs a warn without failing, since Kubo chunking can legitimately differ. delaying_endpoint now returns a real Hash, and a new test proves a 2xx-without-Hash is rejected.
…weep toggle
R2-P3 decode nit: list_pinned_cids used try_get("cid").ok(), which conflated a
SQL NULL (a legitimate Pinata-only row) with a decode failure on a corrupt cid
column and silently misreported the latter as the former. Only NULL now maps to
None, via try_get(...)? so a corrupt column surfaces as an error through the
existing anyhow path. Tests cover both: a Pinata-only row lists with cid=null,
and a cid column retyped to bytea makes the whole listing fail.
RUN-A-NODE.md gains the GITLAWB_RECONCILIATION_SWEEP row (default true, no-op
without an IPFS/Pinata backend, disable with =false), mirroring README.md.
Superseded: re-reviewed at 07a4878, where all three findings are addressed.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed at 07a48788 by execution. Every ask from the last two rounds is closed, and I checked the three rebase restorations against origin/main rather than against the commit messages. satisfies_threshold counts distinct signer DIDs through a HashSet again and its duplicate-signature test is present; I swapped the set back to a Vec and the test goes red, restoring it goes green. advisory_lock_key is back on SHA-256 over owner_slug + ":" + repo_name with both golden tests, and list_ref_certificates_by_prefix carries ESCAPE '!'. The migrations sit at v27-v29, clear of the v18-v26 that open #173 holds and of the v30-v31 that #333 takes. The branch contains current main with nothing behind, cargo metadata --locked passes, and the full suite is green locally and on CI.
The one CI failure is not yours. Quality-signal triage died on a GitHub-side Could not resolve to a node with the global id of 'PR_kwDOSPq2nc71QP-v' while listing labels, after two 429s downloading the action. It needs a rerun, not a commit.
I was ready to approve this round. What stopped me is one claim in the durability core that the code does not support, and it is worth fixing precisely because everything around it is careful.
Findings
-
[P2] Either wire the stale-CID repair or drop the claim that the sweep performs it
crates/gitlawb-node/src/db/mod.rs:4716,crates/gitlawb-node/src/db/mod.rs:2818,crates/gitlawb-node/src/ipfs_pin.rs:369record_pinned_cidbecame an upsert, and the comment above its test says an object pinned once with the wrong bytes "is re-pinned by the sweep and the row overwritten". The sweep cannot do that.filter_ipfs_pinned_oidsselects gaps withcid IS NOT NULL, andpin_new_objectsskips on the same predicate throughhas_ipfs_cid, so an oid carrying a wrong-but-presentcidis filtered out before the one production call site atipfs_pin.rs:476is ever reached. I seeded a row withcid = 'QmStaleWrong'for a real blob oid, ranpin_new_objectsover it against a live mock, and read the row back unchanged. The existing test passes because it calls the DB helper directly, so it cannot see the gap. Either make the gap predicate re-offer rows whose recorded CID does not match the locally computed one, or delete the sweep half of the claim and keep the upsert as push-path repair only. -
[P3] Bind the fresh re-filter deadline at its call site
crates/gitlawb-node/src/reconciliation.rs:498The fresh
authz_deadlineis right, but nothing holds it there.refilter_starves_on_spent_deadline_but_runs_on_fresh_deadlinedrivesrefilter_public_objectsdirectly, so it pins the function's contract and not the wiring: I reverted the call site toscan_deadlineand the reconciliation suite stayed green.REPO_SCAN_DEADLINEis a module const computed insiderun_pass, so a call-site test needs it plumbed through the signature first. The same gap covers the pin-boundary re-derivations accepted in earlier rounds, so it is one seam to open rather than a fix to retrofit here. -
[P3] Delay and jitter the first sweep pass
crates/gitlawb-node/src/reconciliation.rs:171The worker's first statement is
run_pass, not a sleep, so every process start with a pin backend configured begins a full scan at second zero. On a single node that is harmless and arguably useful. On a rolling restart it synchronises every node onto the same disk, git and backend work at the same instant. A short randomised initial delay costs nothing and removes the herd. -
[P3] Correct the two stale v18 migration comments
crates/gitlawb-node/src/db/mod.rs:2753,crates/gitlawb-node/src/db/mod.rs:4746Both still say v18 cleared the legacy
cid = pinata_cidrows. The clearing migration is v27 here after the renumber.
Two notes, neither an ask.
The missing-Hash fix at ipfs_pin.rs:199 closes the half it claims, and the other half now matters more than when jatmn logged it as pre-existing. A present-but-arbitrary Hash is still written straight into pinned_cids.cid, which is the resolver key oids_for_cid looks up. That is exactly the wrong-CID row the P2 above proves the sweep cannot repair, so the two compose into a permanent one. It predates your branch, main has the same passthrough, and I am filing it against main rather than growing this PR.
The CodeRabbit thread on reconciliation.rs:277 about ScanContext.canceled is moot: ScanContext and escalate_kill are gone crate-wide at this head. It can be resolved.
One merge-order fact. This shares ipfs_pin.rs and api/repos.rs with open #173 and api/repos.rs with #330. The v27-v29 allocation is clear only while #173 keeps v18-v26, so if #173 grows another migration before it merges, this needs one more renumber.
|
Correction to my last review: strike the resolver-key sentence in the note about the Kubo I checked What a mismatched |
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
crates/gitlawb-node/src/ipfs_pin.rs:
pin_git_objectno longer fabricates a CID from a 2xx response that carries noHashfield — a misconfigured GITLAWB_IPFS_API (proxy returning HTML, wrong-port health check) now fails the pin with an explicit error instead of writing a pinned_cids row the sweep would then trust as durability evidence. A mismatched Hash still logs a warning without failing (Kubo chunking can legitimately differ).crates/gitlawb-node/src/db/mod.rs: New migrations v27/v28/v29 (pinned_cids legacy equal-cid backfill, node_state cursor table, repos policy epoch). Migration numbers start at v27 to stay clear of fix(node): gate GET /ipfs/{cid} tree objects so a withheld subtree's structure can't leak (#135) #173's v18–v26 while that PR is open.
list_pinned_cidsnow maps only SQL NULL toNone(Pinata-only rows) and surfaces a corruptcidcolumn as an error instead of silently conflating the two.Non-goals
PolicyFence. It keeps the visibility-based filter it already runs today; this PR only adds the periodic sweep as the backstop.Hashcheck inpin_git_objectcloses the hole that could have created them.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
GITLAWB_RECONCILIATION_SWEEPsetting, enabled by default.