fix(node): implement reconciliation sweep as durability backstop (#218) - #244
fix(node): implement reconciliation sweep as durability backstop (#218)#244Gravirei wants to merge 24 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. |
|
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)
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.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed head 8fd6302e. The anchor ask from my last round is closed and closed correctly: plan_seal returns SkipUnchanged once the recipients tag is stable (encrypted_pin.rs:95-98), the block comment and the log now say the anchor is one-shot, and the new test pins it. The sweep's own premise is load-bearing too. I gutted the pin dispatch in both backend arms of run_pass and reconciliation::tests went red, so the repair behavior is genuinely covered.
The problem is one commit earlier. 91d05783 removed assert_all_refs_are_commits from all_object_paths and from blob_paths. The reasoning in that commit message holds for the first: all_object_paths feeds an allow-list, empty paths are explicitly denied at visibility_pack.rs:1396 and :1423, and absence there means withhold. blob_paths feeds a deny-list on the serve path, where absence means serve, and it has no catch-all phase and no empty-path filter. The same removal has opposite polarity on the two sides.
Findings
-
[P1] Keep
blob_pathsfailing closed on a ref that does not peel to a commit
crates/gitlawb-node/src/git/visibility_pack.rs:357
withheld_blob_oids_boundedbuilds its set fromblob_paths, a commit-only walk, andsmart_http.rs:611-635then enumerates the served objects withgit rev-list --objects --alland drops only what is in that set. Those two enumerations are not the same set, and the guard was what forced them to agree. I built the shape: a blob committed under/secret/, an annotated tag pinning it, then the commit orphaned.git rev-list --objects --alllists the blob, the per-commitls-tree -rwalk does not, and a clone readstopsecretout of it. Before this commitblob_pathsreturned an error and the filtered serve failed closed atrepos.rs:1712. Keep the guard on the deny-list callers and drop it only fromall_object_paths, which is what the sweep actually needed, or enumerate non-commit ref targets and withhold everything reachable from them. -
[P1] Fix the tail test's sentinel rather than ignoring the test, and repair its must-not twin
crates/gitlawb-node/src/api/repos.rs:10094
The commit message says the tail is "spawned belowrelease" today. It is not:repos.rs:2307-2316spawns it abovereclaimed.release(...)at:2330, gated onreceive_result.is_ok(), byte-identical toorigin/main. The test also came from3ced62fa, the #174 fix, not #215. What actually broke is the marker. The test polls the fake git log forfor-each-ref, which the tail only ever reached throughassert_all_refs_are_commits; the commit above removed the last such call the tail makes. I ran it three ways: green onorigin/mainin 1.84s, red at this head with--ignoredin 11.42s, and green again at this head with the ignore removed and the sentinel repointed torev-list. So the behavior is fine and the guard is what needs fixing. The twin at:10198matters more: it proves a failed receive-pack spawns no tail by assertingfor-each-refis absent, and no code path reaches that command any more, so it passes whether or not a tail spawns. It is not ignored, so CI is still counting it. -
[P1] Decode the now-nullable
cidasOptionin the two readers that still takeString
crates/gitlawb-node/src/db/mod.rs:3231
v27 dropsNOT NULLand clears the legacy equal-cid rows, andrecord_pinata_cidwritescid = NULLon every Pinata-only push from here on (db/mod.rs:3956).list_pinned_cidswas hardened for that, with a comment explaining exactly why.cid_for_oid(:3231) andpinned_cids_after(:3293) were not: both read the column withRow::get::<String, _>, which istry_get().unwrap()in sqlx 0.8.6, so an unexpected NULL panics rather than erroring. Both have production callers,ipfs_pin.rs:235inside the pin loop andipfs_pin.rs:896in the legacy repair sweep, and that second query is deliberately unfiltered, so on any node that has ever pinned a Pinata-only object the first batch after upgrade walks straight into it. -
[P2] Move the new migrations off v27 through v31
crates/gitlawb-node/src/db/mod.rs:1139
#327 and #384 also claim v27, #384 and #386 claim v28, and #333 and #347 claim v30 and v31. The consequence is stated in this file already, in the v17 reservation note at:913: the runner keys on the integer alone, so whichever side merges second has its entry skipped in full, with no error and aschema_migrationsthat still reads healthy. If v27 is the one skipped,pinned_cids.cidstaysNOT NULLand every Pinata-only insert fails at runtime; if v30 is,list_pinned_cidsselects a column that does not exist andGET /api/v1/ipfs/pins500s. Pick a reserved gap the way v17 did. -
[P2] Do not advertise a CID this node's own resolver will not serve
crates/gitlawb-node/src/api/ipfs.rs:2174
list_pinned_cidsonmainfiltered the listing withis_raw_cidv1(db/mod.rs:3392there) precisely so a legacy provider key was never handed to a client, because/ipfs/{cid}recomputes the raw CID and 404s a mismatch. This branch drops that filter and additionally aliasesp.cid.or(p.pinata_cid)into thecidfield, whileoids_for_cid(:3164) still matches onlypinned_cids.cid. Sogl ipfs listnow prints CIDs that aGETagainst the same node cannot resolve. If widening the listing is deliberate, retire the U4 rationale explicitly rather than by deletion. -
[P3] Delete the two contracts that still name the guard you removed, and make the inverted tests assert the set
crates/gitlawb-node/src/git/visibility_pack.rs:787
:787still says the serve path is safe because "those go throughblob_paths, which runs the guard first", and:1272still contrasts the lenient walk with a blob allowed-set that runs it. Neither is true now, and:787names the exact property the first finding breaks, so it will mislead the next reader. Separately,skips_a_ref_pointing_at_a_blobandskips_an_annotated_tag_of_a_blobend atassert!(result.is_ok())and never look at the returned set, so they are green under an implementation that withholds nothing; the fixture's secret blob is still commit-reachable, so addingassert!(withheld.contains(&secret))costs a line.migration_v27_clears_legacy_equal_cidhas the same shape: I replaced itsUPDATEwithSELECT 1and the test stayed green, because both assertions go throughhas_ipfs_cid, whose v30 predicate is already false for a row with apinata_cid. Assert the column directly.
Not asks, recorded only: tree_structurally_safe has no depth cap and memoizes successes only, on a route its own comment calls anonymously reachable, so a deep or deny-heavy repo shape costs one git ls-tree per tree per encounter with the shared deadline as the only bound. The pin permit taken at reconciliation.rs:710 is held across the 300s withheld walk at :937 and the Irys anchor, which the comment above it does not cover. gaps_found under-reports whenever cap_missing truncates or a repo is skipped before the count, and nothing exported distinguishes that from a healthy repo. The PR body's reuse list names replicable_blob_set, which has no caller outside its own module test. And the two new .cargo/audit.toml suppressions are unrelated to this change, with RUSTSEC-2026-0258's own comment naming an available fix while the file header says every entry has none.
One process note: rebase and watch merge order. #382 intersects visibility_pack.rs in ten hunks and #324 touches config.rs; both move surfaces this review rests on.
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.
Merge readiness
- This head is mergeable against
mainatbfc44f9, and the captured checks are green. However, open PRs #327, #333, #347, #384, and #386 overlap the migration range or adjacent visibility/configuration surfaces. If any lands first, please rebase and have the resolved diff reviewed again.
Findings
-
[P1] Restore fail-closed handling for non-commit ref targets
crates/gitlawb-node/src/git/visibility_pack.rs:357
Removingassert_all_refs_are_commitsmakes the blob path map commit-only while the filtered-pack object enumeration can still include an object reachable through an annotated tag. For example, after a commit containing/secret/filebecomes unreachable but an annotated tag still points at that blob,rev-list --objects --allsupplies the blob to the pack while the path walk supplies no/secretentry, so the withhold set is empty and the protected blob is served/pinned. Keep the new support for valid shapes only when the target can be classified; otherwise preserve a fail-closed outcome. -
[P2] Advance the reconciliation continuation only after work is dispatched
crates/gitlawb-node/src/reconciliation.rs:668
ipfs_lastandpinata_lastare captured before the policy fence, fresh visibility re-filter, and pin call, then persisted even when those paths return an empty vector without handing any OID to a backend. On an at-cap missing set, a temporary policy/DB/refilter failure rotates the whole unattempted prefix behind the remaining backlog on the next pass, despite the database contract saying the offset is the last OID actually attempted. Persist an offset only for dispatched work (and keep the existing bounded retry rotation). -
[P2] Do not advertise a Pinata-only provider CID as this node’s resolver key
crates/gitlawb-node/src/api/ipfs.rs:2174
For rows whose localcidis NULL, this putspinata_cidin the backward-compatiblecidfield. The local/ipfs/{cid}lookup is keyed by the raw local resolver CID, however, so that provider CID cannot resolve from this node; clients that follow the listed CID get a 404. Preserve the explicit remote/provenance fields, but do not present a provider-only CID as the node-local resolver key. -
[P2] Repair and re-enable the disconnect-tail regression test
crates/gitlawb-node/src/api/repos.rs:10094
This PR marks the test ignored even though the production tail is currently spawned aboverelease. The test waits forfor-each-ref, but this PR removed the last tail-path use of that command, so the marker is stale; the failure-direction twin uses the same marker and is also vacuous. Point both assertions at a command the tail still executes and run the test in CI, so a future reorder cannot silently restore dropped post-receive replication after a disconnect. -
[P2] Remove the stale advisory suppressions
.cargo/audit.toml:38
The new ignore describesh2 0.4.13, but the PR head (likemain) locksh2 0.4.18; it also describeslru 0.16.4while the lock contains0.16.3. These unrelated ignores are now inaccurate and would silence the audit gate if the vulnerable versions re-enter the lockfile. Remove them, or replace them with current, evidenced advisory handling.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed head 1f915ef4 by execution. Most of the 08-30 round landed and landed well: the migrations renumbered to v32-v36 and I re-fetched every open PR head to confirm nothing else claims that range, the nullable-cid decode is fixed, audit.toml now matches the lockfile, the tail test's #[ignore] is gone with both markers repointed at rev-list, and the listing no longer aliases a Pinata CID into cid.
Four of the five asks below come from one block: the for-each-ref phase 2 added to blob_paths. It does not close the leak it was written for, its two tests do not bind it, its policy is applied on only one of blob_paths' two consumers, and it is what turned CI red.
Findings
-
[P1] Restore the required test checks
crates/gitlawb-node/src/api/repos.rs:4048
test (stable)andtest (beta)both fail on this head with one failing test,upload_pack_shares_one_deadline_across_walk_and_filtered_serve, the #174 filtered-arm shared-deadline guard, returning 500 where it requires 504. I reproduced it locally and traced the mechanism rather than guessing: the fake git answersfor-each-refwith the single tokenrefs/heads/main, phase 2 doessplit_once(' ')and bails onNone, so the walk errors instead of being reaped. Changing that one fixture arm to emit two tokens turns the test green. The fixture is what is stale, not the production path, but the guard proves nothing while it is red. -
[P1] Peel the tag in the phase-2 ref walk
crates/gitlawb-node/src/git/visibility_pack.rs:490
%(objecttype)reports the type of the ref's own object, which for an annotated tag istag, soif kind == "blob" || kind == "tree"never sees the referent. I checked against stock git and then in the function itself: a blob written withhash-object -wand reachable only through an annotated tag is absent from the phase-1 commit walk, present inrev-list --objects --all(whatrev_list_keepserves from), and absent from the withheld set. The same blob behind a direct ref-to-blob is withheld, so phase 2 works for that shape and only that shape. Ask for the peeled referent (%(*objectname) %(*objecttype), orrev-parse <oid>^{}) and insert it. -
[P1] Apply the empty-path decision on the allow side too
crates/gitlawb-node/src/git/visibility_pack.rs:854
allowed_blob_set_for_caller_boundedconsumes the sameblob_pathspairs and callsvisibility_check(..., "")on the new empty-path entries, which returnsAllowon a public repo because no glob matches the empty path. Executed on a public repo with zero rules and an anonymous caller: the same OID came backdeny_side_withholds=true allow_side_admits=true. The serve filter withholds it and the/ipfs/{cid}gate hands it over. Route both consumers through one owner-only decision and add an allow-side denial test. -
[P2] Rebuild the two ref tests on a commit-unreachable blob
crates/gitlawb-node/src/git/visibility_pack.rs:3481
Injectingif falseat the phase-2 filter leaves bothskips_a_ref_pointing_at_a_blobandskips_an_annotated_tag_of_a_blobgreen. The fixture's secret blob is committed atsecret/b.txt, so phase 1 withholds it under/secret/**no matter what phase 2 does. That is my error, not yours: my 08-30 P3 asked for exactly theassert!(withheld.contains(&secret))line that turned out vacuous. What is actually needed is a fixture where the blob is reachable only through the ref (commit, capture the OID, orphan the commit, then tag). -
[P2] Advance the reconciliation continuation only for dispatched work
crates/gitlawb-node/src/reconciliation.rs:678
Unaddressed this round, andreconciliation.rsis not in the diff.ipfs_lastandpinata_lastare still captured at:678above the pin permit, bothPolicyFencecaptures, and both pin loops, then written at:887and:895under nothing butif ipfs_enabled/if pinata_enabled. An at-cap missing set that a transient fence or refilter failure empties still rotates its whole unattempted prefix behind the backlog. -
[P2] Carry
gl ipfs listalong with the wire change
crates/gl/src/ipfs_cmd.rs:122
The server-side call was right, but the client readspin["cid"].as_str().unwrap_or("?")and prints it as the entry heading, and nothing in the CLI readspinata_cid. A Pinata-only row now renders as a bare?. Nocrates/glchange in this round.
Not asks, recorded only: cid_for_oid and pinned_cids_after conflate SQL NULL with a decode error through .ok().flatten(), harmless while sha256_hex is NOT NULL but out of step with list_pinned_cids' error contract. The must-not disconnect twin's fixed 750ms sleep can pass vacuously under scheduler pressure; a bounded poll would close it.
Merge order: #382 intersects visibility_pack.rs in ten hunks and #324 moves config.rs. Neither has landed, so I reviewed this branch where it stands on bfc44f9; if either merges first, the resolved diff wants another look.
Superseded by my re-review on head 1f915ef.
`upload_pack_shares_one_deadline_across_walk_and_filtered_serve` — the Gitlawb#174 shared-deadline guard for the FILTERED serve arm — was red on both `test (stable)` and `test (beta)`. The fake git answered `for-each-ref` with the single token `refs/heads/main`, a ref NAME, where `blob_paths` phase 2 asks for `--format=%(objectname) %(objecttype)`. The phase-2 parse fails closed on a line it cannot read, so the walk returned an error and the request surfaced as a generic 500 instead of being reaped into a 504. That silently repurposed the test from "the filtered serve shares one deadline" into "the walk errors" — the guard proved nothing while it was red. The fixture is what was stale, not the production path: the arm now emits the column shape the walk parses. A commit tip has no peeled referent, so two columns is the whole line. The 1.2s walk cost stays on `ls-tree` and the serve's on `pack-objects`, so the two phases remain independently attributable.
`%(objecttype)` reports the type of the ref's OWN object, which for an
annotated tag is `tag`. The phase-2 `if kind == "blob" || kind == "tree"`
arms therefore never saw the referent, and a blob reachable ONLY through
an annotated tag escaped the withheld set — while `rev_list_keep`'s
`git rev-list --objects --all` does peel tags and served it. Phase 2
closed the leak for a direct ref-to-blob and for that shape only.
The walk now asks for the peeled atoms `%(*objectname) %(*objecttype)`
and inserts the referent with an empty path, alongside the direct
blob/tree tips it already handled. Line parsing moves to a field-count
match: two columns for a non-tag tip (the peeled atoms expand to empty),
four for a tag tip, anything else still fails closed.
Measured against git 2.50, the `*` atoms peel the WHOLE chain — a
tag-of-a-tag-of-a-tag-of-a-blob reports the blob — so a `tag` peeled
type does not occur on stock git. That arm is kept anyway, because
`push_delta.rs`'s ref-type guard documents `%(*objecttype)` as a
one-level peel: under such a git it finishes the chain with
`rev-parse <tip>^{}` plus a `cat-file -t` probe rather than bailing and
500ing every clone of a repo that merely carries a nested tag. Both
children are bounded by the walk's shared deadline and are unreachable
on the common one-line-per-ref path.
…nd 8 P1)
`blob_paths` has two consumers and only one of them carried the
empty-path policy. `withheld_from_pairs` (the deny side, what the
smart-http serve filter excludes) withheld a phase-2 empty-path entry
from everyone but the owner; `allowed_blob_set_for_caller_bounded` (the
allow side, what `GET /ipfs/{cid}` hands over) called
`visibility_check(..., "")` directly. On a public repo no glob matches
the empty path, so that returns `Allow`: the serve filter withheld the
OID while the IPFS gate served the bytes — the leak phase 2 exists to
close, reopened one layer over.
The policy moves into `pair_decision`, which both consumers now call, so
the two gates cannot disagree and a future change to the empty-path rule
moves both at once. `withheld_blob_recipients_bounded` routes through it
too: an unclassifiable blob grants a recovery copy to the owner only,
never to a rule's named reader whose grant was written against a path
this object does not have.
No behavior change for non-empty paths — `pair_decision` delegates to
`visibility_check` unchanged.
…round 8 P2) `skips_a_ref_pointing_at_a_blob` and `skips_an_annotated_tag_of_a_blob` were vacuous. Both hung their ref on `fixture()`'s `secret` blob, which is COMMITTED at `secret/b.txt`, so phase 1's per-commit `ls-tree` already produced `(secret, "/secret/b.txt")`, the `/secret/**` rule already denied it, and `withheld.contains(&secret)` passed with phase 2 deleted outright. Both now use `orphan_blob`, a blob written straight to the object store that no commit's tree names, so the ONLY route into the withheld set is the `for-each-ref` phase. That is also the real leak shape: `rev_list_keep`'s `git rev-list --objects --all` does follow a ref to such a blob, so an under-withheld one ships in the clone pack. Verified by mutation: with `if false` injected at the phase-2 filter, every test below goes RED (they were green under that mutation before). New coverage: - `ref_only_blob_is_denied_on_the_allow_side_too` — the allow-side denial for the shared `pair_decision`; it also asserts deny and allow agree on one OID, and that the owner IS still admitted, so it cannot pass by denying everything. Reverting the allow side to `visibility_check` turns it red on exactly that assertion. - `skips_a_nested_annotated_tag_of_a_blob` — real-git tag-of-a-tag. - `peels_a_tag_whose_peeled_target_is_still_a_tag` — drives the one-level-peel fallback through the fake-git seam, so that arm is tested rather than dead.
…d work (round 8 P2)
`ipfs_last` / `pinata_last` were captured from the missing set above the
pin permit, both `PolicyFence` captures and both pin loops, then written
under nothing but `if ipfs_enabled` / `if pinata_enabled`. Every stage in
between can legitimately produce nothing — a fence capture that fails, a
quarantine/visibility recheck that says skip, a pin-boundary
re-derivation that errors — and each is transient.
Advancing past OIDs that were never attempted is not a harmless retry
delay: `missing_oids` rotates strictly past the stored offset, so the
whole unattempted prefix lands BEHIND the entire backlog on the next
pass. For an at-cap repo — the only kind the continuation exists for —
the backlog never drains inside one cap window, so those objects are
starved indefinitely. That is a durability hole in the durability
backstop.
The offset now moves only for work actually handed to a backend, tracked
at the dispatch boundary as `to_pin.last()` (not the missing set's last:
an OID the pin-boundary re-derivation dropped was never offered). It is
recorded before the call, so a pin phase that times out mid-batch still
counts as dispatched.
The write becomes three outcomes rather than two, via one
`next_offset_write` helper so the backends cannot drift apart:
* work dispatched -> advance to the last dispatched OID
* nothing missing, query OK -> None, marking the row done
* nothing dispatched, or the missing-set query failed
-> write NOTHING, leaving the resume point
a capped pass already paid for
`*_scan_ok` is what separates the last two: an empty missing set means
"everything is pinned" or "the filter query failed and we know nothing",
and only the first should mark the row done.
Test: `sweep_leaves_offset_untouched_when_nothing_was_dispatched`, using
a `#[cfg(test)]` seam on the pin-boundary re-derivation — that stage
cannot be starved from the outside, because the mid-scan re-filter runs
first on the same rules and budget and makes the sweep `continue` well
before the offset write. It asserts the stored offset comes back
byte-identical after a declining pass, then that the same repo does
advance it once dispatch happens, so the assertion cannot pass on a dead
write site. Restoring the old semantics turns it red.
The server-side wire change landed without its client half. `cmd_list`
read `pin["cid"].as_str().unwrap_or("?")` as the entry heading and
nothing in the CLI read `pinata_cid` at all, so a Pinata-only row
printed as a bare `?` — the listing showed nothing usable for an object
that is in fact durably stored.
`cid` is legitimately null for such a row: it is the key
`GET /ipfs/{cid}` resolves against, and the node stopped aliasing a
Pinata provider CID into it (round 3) precisely because that CID 404s
there. So the fix is on this side.
Rendering moves into `render_pin`, which leads with the node-resolvable
CID when there is one and otherwise the provider CID, labelled so nobody
feeds it back to this node's resolver. A dual row keeps both visible. A
`backends:` line names where the bytes are, read from the writer-owned
`local_pinned` / `pinata_pinned` booleans rather than re-inferred from
CID shape — which is what the node's own comment warns against.
Older nodes send neither flag; those `cid`-only rows fall back to CID
presence and render as they always did
(`render_pin_handles_a_legacy_cid_only_row`).
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.
Merge readiness
- [P1] Restore the required test checks
crates/gitlawb-node/src/api/repos.rs:4068
The changedblob_pathsphase now expectsfor-each-refto emit<oid> <type>, but this load-bearing filtered-upload-pack fixture still emits onlyrefs/heads/main. The walk therefore errors before it reaches the shared-deadline assertion; the current stable and beta CI jobs both fail on this test. Update the fixture for the new command contract and keep the test exercising the intended 504 behavior.
Consolidated guidance
These findings are not seven unrelated edge cases. They come from a small number of contracts that this PR now spans but does not yet model in one place: Git reachability-to-visibility classification, authorization-to-external-side-effect dispatch, reconciliation progress, and the pin-listing API/CLI boundary. Please address those root contracts as a cohesive revision rather than patching only the named lines; otherwise a local fix is likely to leave the same mismatch in a sibling path.
-
Create one fail-closed classification contract for non-commit refs and use it everywhere. The smart-HTTP deny set, CID allow set, reconciliation object set, and encrypted-recovery walk must agree on what a direct blob/tree ref, annotated tag, nested tag, tag-of-tree, and malformed/missing target mean. Fully peel tags before classification. If an object's path cannot be established, represent that explicitly as unclassifiable and deny it in every path-sensitive consumer—do not encode it as an ordinary empty string that one consumer treats as deny and another treats as allow. Add a table-driven test matrix that exercises each shape through both filtered smart HTTP and
/ipfs/{cid}, with public, denied-path, and authorized-reader cases. -
Keep Git path bytes intact until the visibility engine has made its decision.
ls-tree -zis deliberately NUL-delimited because filenames can contain whitespace and non-ASCII bytes. Parsing may normalize the metadata portion, but must never trim, lossy-decode, or otherwise transform the filename. Add regression cases for trailing space, leading whitespace, tabs/newlines where representable, and non-UTF-8 paths; an unrepresentable path must fail closed rather than quietly become a different path. -
Define a linearization point for policy changes and external publication. An epoch read followed by an HTTP request is only an optimistic observation, not a fence. Decide whether a visibility/quarantine mutation waits for in-flight publication or publication holds a policy lease that mutation invalidates before it can proceed; then apply that protocol consistently to IPFS, Pinata, and encrypted envelopes. Test the interleaving where a rule narrowing commits after the final check but before the request is accepted, and assert that no new public or recipient-visible artifact is created.
-
Make reconciliation progress reflect an effect, not a plan. A continuation cursor must advance only from an explicit attempt/result record for the backend concerned. Model the states separately—candidate, authorized, dispatched, persisted success, and retryable failure—so a DB/refilter/fence failure cannot look like work completed. Add at-cap tests for each pre-dispatch failure and for partial backend success, then assert the next pass retries every undispatched OID while still rotating fairly after actual attempts.
-
Treat the pin response as a versioned server/client contract. The PR correctly avoids calling a provider CID a local resolver key, but changing
cidfrom a required string to nullable requires every first-party consumer, docs, and fixture to understand local-only, Pinata-only, and dual-backed rows. Add a CLI/API compatibility test for all three shapes that verifies what is displayed, whether it is locally fetchable, and how provider-only information is labelled. -
Make test doubles share the command contract of production helpers. The failed CI test is a symptom of fixtures independently guessing Git output. Centralize fake
for-each-refrecords or make the fixture emit the exact format consumed by the parser. For each new visibility or reconciliation boundary, run the relevant focused test and the stable CI-equivalent grouping before requesting review.
Once this model is in place, please re-run the entire visibility/pinning suite instead of only the tests immediately adjacent to a fix. The failure modes here cross consumers: a change that repairs public pinning can still leave smart HTTP, CID serving, encrypted recovery, or the CLI on a different interpretation of the same state.
Findings
-
[P1] Peel annotated tags before classifying non-commit refs
crates/gitlawb-node/src/git/visibility_pack.rs:485
%(objecttype)istagfor an annotated tag, not the tag's target type, so phase two drops an annotated tag that points at a blob or tree.rev-list --objects --allstill includes the peeled blob for the smart-HTTP pack, butblob_pathsnever puts it in the withhold set. A blob reachable only through such a tag can consequently bypass a path-scoped deny. Peel the tag target (or otherwise fail closed for it) before deciding whether the object is classifiable. -
[P1] Do not authorize an unclassifiable ref target using the empty path
crates/gitlawb-node/src/git/visibility_pack.rs:854
Phase two represents direct blob/tree refs as(oid, ""). The deny-side consumer deliberately withholds that shape, but the CID allow-list passes it tovisibility_check; on a public repo the empty path is allowed because it matches no deny rule. That makes/ipfs/{cid}serve an object whose real repository path is unknown. Treat empty-path targets as denied on the allow side as well. -
[P1] Preserve trailing whitespace in Git paths before evaluating visibility
crates/gitlawb-node/src/git/visibility_pack.rs:1071
git ls-tree -zpermits filenames ending in spaces, butrecord.trim()removes those bytes before the rule check. For example, a deny for/secret /**is evaluated against/secretfor a realsecretentry, allowing the structural tree and exposing the true entry name and child object ID. Skip only the final empty NUL record; do not normalize the parsed filename. -
[P1] Make the policy fence cover the dispatch race
crates/gitlawb-node/src/ipfs_pin.rs:2065
is_current()performs a separate epoch read and the irreversible POST begins only after that await returns. A visibility or quarantine mutation can commit in that interval, so the sweep can publish plaintext using the old decision despite the new fence. Serialize the policy decision with dispatch (and apply the same guarantee to Pinata and encrypted sealing) so a completed narrowing cannot race an upload. -
[P2] Advance the per-backend continuation only after work is dispatched
crates/gitlawb-node/src/reconciliation.rs:668
ipfs_lastandpinata_lastare taken from the capped missing set before permit acquisition, fence capture, fresh refiltering, and pin calls, then persisted even if those paths dispatch no OID. With more than the per-pass cap, one transient DB/policy/refilter failure rotates that untouched prefix behind the backlog, contrary to the offset's stated “last attempted OID” contract. Save progress from the objects actually handed to each backend. -
[P2] Carry the Pinata-only response shape through
gl ipfs list
crates/gl/src/ipfs_cmd.rs:122
The API now returnscid: nullpluspinata_cidfor a valid Pinata-only row, but the supported CLI still prints onlycid, rendering every such successful pin as?. Display the provider CID/provenance appropriately (without presenting it as a local resolver key) so the new state remains usable to CLI users.
…isibility rules handling
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed head 4e9729f1 by execution. CI is 12/12 green on this head and the empty-ident break is fixed the right way, on the bare repo where the tagger identity is actually needed. The pair_decision unification is real and complete: I traced all six call sites and found no consumer still making its own empty-path decision, and the cursor rework holds up under every failure arm I drove, not just the happy one.
What blocks is narrower than the last two rounds but sits at the centre of the change. Two of the round's marquee guards do not hold when I mutate the code they protect, and the path-byte fix landed on one of the file's two ls-tree parsers.
Findings
-
[P1] Drop the
record.trim()intree_structurally_safe
crates/gitlawb-node/src/git/visibility_pack.rs:1168
The round fixes byte preservation on the blob walk and leaves the sibling tree walk normalizing. I built the shape and ran it: a repo with a directory namedsecret(one trailing space) holdingf.txt, a rule of/secret /**, anonymous caller. The deny side withholds the blob and the allow side admits its parent tree, on the same input, because the trim turnssecretintosecretbeforeentry_pathreachesvisibility_check. That allow-set feeds/ipfs/{cid}and the sweep's pin set, so the consequence is an egress one. Both new whitespace tests pass under the defect because both drivewithheld_blob_oids. -
[P1] Withhold what a tree-valued ref target reaches, not just the target
crates/gitlawb-node/src/git/visibility_pack.rs:517
Phase 2 inserts the ref's target OID and stops there, so for a tree tip (direct, or peeled from an annotated tag) the tree's children are never classified.git rev-list --objects --all, which is what the filtered pack serves from, does list them. Executed on a bare repo whose secret blob is reachable only as a child of amktreetree published as a tag: the tree is withheld, the blob is not, and the blob is in the pack. Tag-of-tree was named explicitly in the last round's guidance, andorigin/mainstill hasassert_all_refs_are_commitsas this function's first statement, so this is a fail-open direction rather than a gap being narrowed. -
[P1] Make the third fence load-bearing before it ships
crates/gitlawb-node/src/db/mod.rs:3655
Nothing tests it.record_pinned_cid_with_source_fencedhas exactly one call site in production and none in tests, so no test can observe the comparison. Mutating the guard toif false && ...leaves the pin suite at 132 passed and the reconciliation suite at 26 passed, and--listmatchingfenceorepochreturns nothing. The mechanism itself is sound, and I checked the half that is easy to get wrong:set_visibility_ruleandremove_visibility_rulereally do take the same row lock, implicitly, throughUPDATE repos SET policy_epoch. That is worth keeping. It just needs a test that fails when the fence stops working: epoch bumped givesErrwith no row landed,i64::MAXlands the row. -
[P2] Fence the Pinata record too, or say why it is exempt
crates/gitlawb-node/src/pinata.rs:454
Both backends dispatch under a fence captured from the same repo row, and both POSTs are equally irreversible, but only the IPFS record got the third fence. Pinata still records through a bare autocommit upsert straight after its own POST, and itspinned.pushis unconditional, so the pair reaches the cid map that drivesupsert_branch_cidand the gossip. If IPFS-only is deliberate, the contract should say so rather than leaving the asymmetry to be inferred. -
[P2] Rebuild the consumer matrix so it can fail
crates/gitlawb-node/src/git/visibility_pack.rs:4320
direct-blob,annotated-blobandnested-tag-blobare three labels over oneblob_oid, and that OID is independently a direct ref tip, so all three phase-2 classification arms can be deleted individually and every row still passes. Neutering the peel insert leaves the matrix green while turning the olderskips_an_annotated_tag_of_a_blobred, which is the wrong test failing. Consumer 4's non-owner arm compares recipients againstcaller.unwrap_or("??"), and the fixture's rule carries no reader DIDs, so that assertion holds for any implementation. Give each ref shape its own blob, use a reader DID that is actually inreader_dids, and hoist the two caller-invariant consumers out of the loop. -
[P2] Run the generated script in the new helper test
crates/gitlawb-node/src/test_support.rs:243
fake_git_with_refs_emits_the_column_shapeasserts the output contains two substrings and never executes it. Built from that test's own two refs the script is a shell syntax error:;;is emitted once per ref inside a singlefor-each-ref)arm (:209), so the first one closes the arm and the secondechoparses as a pattern.sh -nexits 2 with "word unexpected". The helper is green and unusable. Move the;;out of the loop and add ansh -nassertion, which is the check that would have caught it. -
[P2] Tie the wire-form test to the closure it names
crates/gitlawb-node/src/reconciliation.rs:1319
progress_state_to_wire_matches_the_closurenever callsnext_offset_write. It assertsProgressState's match arms against themselves, which cannot fail while the enum compiles, so it cannot detect the drift the name promises. The underlying issue is that the enum is a second copy of live logic: makingnext_offset_writereturn aProgressStateand callingto_wire()at the two write sites would leave one encoding, make the table genuinely testable, and clear the dead-code warning without an#[allow]. -
[P2] Make the fence real when the repos row is absent
crates/gitlawb-node/src/db/mod.rs:4830
SELECT ... FOR UPDATEon a predicate matching no row takes no lock, and the helper folds the miss to0throughunwrap_or(0).PolicyFence::capturefolds a missing row to0the same way, so the comparison passes with nothing serializing behind it. Verified both directions against Postgres: with a session holding the lock, an insert against the missing id returned in 6ms unblocked, while the same shape on an existing row blocked the epoch update for just over two seconds. The comment saying every record goes through the lock is not true for that case. -
[P2] Take the row lock only when there is a fence to compare
crates/gitlawb-node/src/db/mod.rs:3655
The locked read runs before thei64::MAXsentinel is examined, so the push path, which passes the sentinel precisely because it has no fence, still takes an exclusive lock on the repos row and performs no comparison. Every pin record now queuestouch_repo, the quarantine toggle and rule writes behind it. Moving the read inside the sentinel check keeps the fence and drops the contention. -
[P3] Correct the two comments that describe code that is not there
crates/gitlawb-node/src/git/visibility_pack.rs:1307
The tree allow-set hunk is unreachable as written:object_pathsonly ever insertsformat!("/{path}")and has no catch-all phase, so no empty path reaches this call and the owner carve-out the comment describes cannot fire. Reverting the hunk leaves both suites green. Separately, the note at:478says the*atoms peel the whole chain on stock git so thetagarm is dead; on git 2.43 the round's own fixture reports a peeled type oftag, so that arm is live in production and each nested tag costs two extra git children with no ceiling on ref count.
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.
Consolidated guidance
This PR has gone through many review rounds because the findings are not independent edge cases. The change crosses four contracts that are currently modeled in several places: Git reachability-to-visibility classification, policy authorization-to-external-publication ordering, reconciliation work/progress state, and bounded object scanning. A local fix in one consumer can leave a sibling consumer on a different interpretation, so the next review discovers the same contract mismatch through another path.
Please address these as root contracts rather than patching only the five named lines:
-
Produce one byte-safe, fail-closed Git object classification and reuse it everywhere.
Smart HTTP,/ipfs/{cid}, public reconciliation, and encrypted recovery should consume the same answer for an object: its type, whether its path is known or unclassifiable, the applicable visibility decision, and—where a tree is involved—the descendants covered by that decision. A parser must never return success with a partial result. Direct refs, annotated tags, nested tags, blob/tree targets, malformed targets, and non-UTF-8 names should enter this model once rather than being rediscovered by each consumer.The regression matrix should cross ref shape with caller/policy and consumer: anonymous, authorized reader, and owner; public, private, quarantined, and path-scoped repos; smart-HTTP deny set, CID allow set, public pin set, and encrypted-recipient set. For every row, assert that all consumers agree on the classification. Include raw non-UTF-8 filenames and verify the actual filtered pack—not only the intermediate set—contains no withheld bytes.
-
Define one linearization protocol for policy changes and irreversible publication.
The required invariant is: once a visibility or quarantine narrowing commits, no publication based on the older policy may subsequently become externally visible. An epoch read is useful for detecting stale preparation, but it does not order the commit with a later HTTP request. IPFS, Pinata, and encrypted envelopes need the same protocol: a policy lease/lock held through dispatch, mutation waiting for in-flight publishers, or another mechanism that prevents/compensates the stale external effect. The implementation choice is open, but a post-upload DB veto alone cannot satisfy the invariant.Add deterministic interleaving tests for all three sinks. Pause the backend after the final authorization check, commit a rule removal or quarantine change, then release the request. Assert that no new plaintext object or stale-recipient envelope survives, no local success record/announcement is emitted, and the next pass has a well-defined retry outcome.
-
Model reconciliation as explicit, independent phases and outcomes.
Public pinning and encrypted recovery are related, but “no public object to pin” is not “no encrypted work.” Keep their eligibility and execution independent after the common repo-level gates. Within each backend, distinguish candidate, freshly authorized, dispatched, backend-accepted, durably recorded, retryable failure, and unknown outcome. Continuation state, logs, and metrics should derive from the appropriate stage rather than from an overloadedVec<(sha, cid)>that means different things to different callers.Exercise empty public work with non-empty encrypted work, policy changes before and during dispatch, DB failure after backend acceptance, ambiguous timeout, partial success across backends, restart/resume, and the next sweep's retry behavior. Assertions should cover the external effect, DB state, continuation, and metrics together so one layer cannot report success while another still sees a gap.
-
Apply bounds before expensive expansion and keep membership work linear.
The 50,000-object repair cap does not bound work that happens while discovering and classifying those objects. Use indexed OID membership, batch Git queries where possible, and pagination/streaming or another pre-expansion bound so a large or dirty object database cannot consume every authorization deadline before the cap is reached. Prefer deterministic tests that assert command/instruction counts or bounded pages over wall-clock-only tests.
The goal is one cohesive revision with shared primitives and end-to-end tests. Fixing these contracts centrally should close the current findings and reduce the chance that another consumer-specific variant appears in the next round.
Findings
-
[P1] Abort the non-commit tree walk when a filename is non-UTF-8
crates/gitlawb-node/src/git/visibility_pack.rs:424
walk_tree_oids_innerinserts the current tree OID, runsgit ls-tree -z, and returnsOk(())when any filename is not UTF-8. That is a successful partial classification: none of the tree's child blob/tree OIDs are inserted. A direct tree ref or annotated tag peeling to such a tree is valid Git input;git rev-list --objects --allstill enumerates both the tree and its descendants. The smart-HTTP keep side therefore removes the known tree OID but keeps the missing child blob OIDs and passes them explicitly topack-objects, exposing their bytes to an anonymous/non-reader fetch.Before this PR, the all-ref guard rejected non-commit ref targets and aborted the walk. The new tolerant ref support is useful, but it must preserve that fail-closed outcome. Parse the record metadata without decoding the filename, or return an error for an unclassifiable tree; do not return success until every reachable descendant is withheld. Add a direct-tree and peeled-tree fixture containing a raw invalid byte and assert both the deny set and the final pack omit the child blob.
-
[P1] Serialize policy narrowing with the irreversible backend upload
crates/gitlawb-node/src/ipfs_pin.rs:2077,crates/gitlawb-node/src/pinata.rs:414,crates/gitlawb-node/src/encrypted_pin.rs:225
Each path awaitsPolicyFence::is_current()and starts its HTTP POST only after that database read has completed. The following interleaving remains possible: derive an allow/recipient snapshot; pass the final epoch check; commit a visibility removal or quarantine change; let Kubo/Pinata accept the request. The public object or stale-recipient envelope is then created after the narrowing completed. The later fenced IPFS/Pinata record can reject local bookkeeping, but it cannot retract the backend object; encrypted recording is not post-upload fenced at all.This is the same linearization requirement from the earlier consolidated guidance, not a request for an extra best-effort check. Establish one ordering protocol shared by all three effect paths so policy mutation and publication cannot cross in that order, or compensate the external effect before reporting/anchoring success. The regression test must pause each backend after the last epoch read, commit the narrowing, release the backend, and prove that no newly created artifact remains visible to the removed audience.
-
[P1] Run encrypted recovery even when there are no public objects to pin
crates/gitlawb-node/src/reconciliation.rs:665
Thiscontinue, and the second one after fresh refiltering at line 712, exits the repository before encrypted phase 2 at line 1112. A public path-scoped repo whose only reachable object is a direct blob ref is a concrete counterexample: the anonymous public classifier correctly treats its empty path as unclassifiable and removes it fromobject_list, whilewithheld_blob_recipients_boundedassigns that same blob to the owner recovery set. The public list is empty, but encrypted work is non-empty. Every hourly pass takes the same early return, so a lost or failed encrypted copy is never repaired.Keep the common repo-level eligibility checks, but let public pinning no-op without suppressing encrypted recovery. Add equivalent direct-blob and direct-tree cases, plus the mid-pass refilter-to-empty case, and assert that public backends receive no plaintext while the expected owner recovery envelope is present after the sweep.
-
[P2] Index object membership before the repeated full-scan classifier
crates/gitlawb-node/src/git/visibility_pack.rs:759,crates/gitlawb-node/src/git/visibility_pack.rs:852
all_object_pathslaunches an unusedgit rev-parse <commit>^{tree}child for every historical commit even though root OIDs are later recomputed byroot_tree_oids. It then loops over everycat-file --batch-all-objectsrow and usesblob_set.iter().any(...)ortree_set.iter().any(...)to decide whether that OID already has a path. WithOobject rows andPpath pairs, that phase is O(O × P) rather than indexed lookup. The sweep runs this classifier for the initial scan, the fresh refilter, and again at backend dispatch boundaries; the 50,000-object cap is applied only after classification succeeds.On the large histories this durability backstop is meant to repair, the redundant subprocesses and quadratic membership scan can consume each five-minute budget before any object reaches a backend, producing the same fail-closed skip every pass. Remove the unused per-commit probe, maintain a separate OID index while preserving the path-pair set, and move the effective work bound ahead of unbounded object-store expansion (or page/stream the classifier). Add a scale regression that fails on quadratic membership or per-commit root subprocesses without relying only on machine timing.
-
[P2] Count a Pinata gap as filled only after its durable outcome is known
crates/gitlawb-node/src/reconciliation.rs:1006
Reconciliation states thatpin_new_objectsreturns only objects whose DB record was written and uses every returned pair to incrementgitlawb_reconciliation_gaps_filled_total.pinata::pin_new_objectshas a different contract: after a successful provider POST it pushes(sha, cid)even whenrecord_pinata_cidreturns an error (pinata.rs:451-468,515). A closed/failed DB or policy-fence rejection can therefore produce a “filled” log and metric while the row is absent; the next sweep queries the DB, detects the same gap, and may count it again. A timed-out autocommit is explicitly an unknown outcome but is also counted as success.Return a reconciliation-specific outcome that separates provider acceptance from durable local recording (and preserves an explicit unknown state), or verify persistence before counting a filled gap. Keep the existing push-path behavior if its announcement contract requires provider-success pairs, but do not reuse that return value as proof of reconciliation durability. Test definite DB failure, fence rejection, and ambiguous timeout through the next pass, asserting DB state, retry selection, logs, and metric deltas together.
Expected validation before the next review
- Run one table-driven object-classification matrix through all four consumers and the final filtered pack, including direct/annotated/nested blob and tree refs, malformed targets, raw non-UTF-8 names, and every caller class.
- Run deterministic paused-backend policy-race tests for local IPFS, Pinata, and encrypted envelopes; the narrowing must commit before backend release and no stale artifact may remain.
- Run reconciliation lifecycle tests covering independent public/encrypted phases and every candidate → authorized → dispatched → accepted → persisted/retry/unknown transition, including restart and the following pass.
- Run a large-history/object-store test that enforces indexed membership and bounded discovery before the repair cap, rather than merely increasing the timeout.
- Run the full visibility, smart-HTTP, IPFS, Pinata, encrypted-recovery, reconciliation, API/CLI, stable, beta, and MSRV suites after the shared contracts are changed. Focused tests alone have repeatedly missed disagreements between sibling consumers.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed head 5243547f by execution. CI is 12/12 green and the sweep's own premise is
covered for real: I gutted the gap-fill dispatch at reconciliation.rs:909 (passing an empty
vec in place of to_pin) and sweep_fills_ipfs_gap_and_persists_cursor went red, so the test
that matters is load-bearing rather than decorative. Ten of the round-9 asks landed.
The problem is what the round-9 fix brought with it. walk_tree_oids_bounded was added in
4bcb88db to stop a mktree tree tip serving blobs it never withheld, and the same commit
gave that walker a non-UTF-8 arm that reopens the identical leak. I confirmed it end to end
rather than by reading.
Findings
-
[P1] Fail closed on a non-UTF-8 tree listing instead of returning Ok
crates/gitlawb-node/src/git/visibility_pack.rs:424
Whengit ls-tree -zreturns bytes that are not UTF-8, this arm inserts the tree OID and
returnsOk(()), so every blob under that tree is missing from the withheld set while
rev-list --objects --allstill serves it. I built a tree whose entry filename carries a
raw0xFF 0xFE, pointedrefs/tags/direct-treeat it, and ran the real walker: the tree
is withheld, the child blob is not, and the blob is in the served set. Phase 1 already
handles this input correctly by bailing at:526, so the two walks disagree on the same
bytes. The comment above the arm claims it fails closed; it does not. Making itbail!the
way:526does flips the probe to a fail-closed error and leavesvisibility_pack::at
55 passed andreconciliation::at 28 passed, so the strict form costs nothing here.
This reaches all three phase-2 shapes (:630direct tree,:643annotated-tag-of-tree,
:687nested peel), not just the one.fails_closed_on_non_utf8_pathdoes not cover it:
that test commits, so it exercises the phase-1 path and never enters this walker. -
[P2] Bound the tree walk by child count, not only depth and deadline
crates/gitlawb-node/src/git/visibility_pack.rs:457
The walk stops atMAX_TREE_WALK_DEPTHand the shared deadline, and nothing else. There is
no memo of already-walked tree OIDs, so a tree reachable from N ref tips is walked N times,
and a wide shallow tree spawns onels-treechild per subtree well inside the depth bound.
Expiry is fail-closed, so this is cost and availability rather than disclosure, but the
ceiling is currently wall-clock rather than anything structural. -
[P3] Name the migration test after the version that actually drops the constraint
crates/gitlawb-node/src/db/mod.rs:5932
migration_v12_makes_cid_nullable_and_preserves_classificationand its "pre-v12" comment
point at a migration that no longer exists: the versions run 1-11, 17-26, 32-36, and the
cidDROP NOT NULLis in v32 at:1163. The test still drives a real path, so this is
naming drift rather than dead coverage, but the next person reading it will look for a v12.
On scope
This is round 10, and the playbook's aggregate check now fires on all three criteria rather
than the two that trigger it. The diff has grown to +8422/-449 across 23 files; the P1 above
traces directly to the previous round's remedy, which I confirmed with git log -S (both the
new walker and its non-UTF-8 hole land in 4bcb88db); and a wrong answer here serves a secret
blob in cleartext to an anonymous clone. Fixing this P1 in place is the right immediate step,
but I do not think an eleventh round of line-level findings is the way this lands. The pin
sweep and the visibility-walk rework are separable, and splitting them would let the walk
changes get a review that is not competing with a durability feature for attention.
…er (Gitlawb#218 round 10 P1) walk_tree_oids_inner previously returned Ok(()) on a non-UTF-8 ls-tree -z listing, inserting only the tree OID and skipping every child blob/tree OID. A direct tree ref (or an annotated tag peeling to a tree) is valid Git input; git rev-list --objects --all still enumerates the tree and every descendant, so the keep-side removed the tree from the served set but passed the child blob OIDs to pack-objects, exposing their bytes to an anonymous clone. Phase 1 already bails on the same input at the blob_paths walk; this fix puts the tree-walk on the same fail-closed outcome. The new fails_closed_on_non_utf8_tree_tip test pins the invariant for both a direct refs/tags/direct-tree ref and an annotated tag-of-tree (the two non-commit shapes round 9 added tolerance for), confirming the walker bails rather than returning Ok with a partial withheld set.
…tlawb#218 round 10 P1) A path-scoped repo whose only reachable object is a direct blob or direct tree ref yields an empty public list (the anonymous public classifier removes the only object from the served set) while withheld_blob_recipients_bounded still assigns that object to the owner recovery set. The early `continue` on object_list.is_empty() (and the second one after the mid-pass refilter) suppressed encrypted recovery too, and a lost or failed encrypted copy was never repaired. Track empty-public-work as a flag and gate the public phase on it. Encrypted phase 2 runs regardless. The backend enable flags and `_pin_permit` move out of the gated block so phase 2 can read them when the public phase did not run.
The test name and its docstring/comments still say "v12", but the cid-nullable migration is at v32 (line 1163). The test itself is correct — it drives the real path — so this is naming drift rather than dead coverage. A future reader looking for the migration would otherwise chase a v12 that no longer exists.
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.
Merge readiness
- [P2] Restore the required formatting gate
crates/gitlawb-node/src/reconciliation.rs:712
The current head failscargo fmt --all -- --check, which reproduces the only red required GitHub job (fmt + clippy); targeted clippy itself passes. Format the new reconciliation block and the changedvisibility_pack.rstest, then rerun the combined gate.
Overall guidance
The number of findings here does not come from nine unrelated edge cases. Most trace back to three contract boundaries that the implementation currently represents with comments, shared tuples, and timeouts rather than explicit state:
- Authorization decision versus irreversible effect. A policy snapshot is checked, an external upload happens later, and a DB fence is then treated as if it also ordered the earlier provider effect. These are three different events. The security invariant must be stated at the provider-publication boundary, including what happens when policy changes while a request is already in flight.
- Provider acceptance versus durable local state versus advertised state. The push path legitimately needs to know that Pinata returned a CID even when a DB write is uncertain, while reconciliation needs proof that the durable gap record landed, and the listing API needs a resolver key the node can actually serve. Reusing the same
(sha, cid)shape for all three meanings is why a provider upload becomes a false repair and why legacy provider CIDs become local resolver keys. - A bounded call versus bounded, fair lifecycle progress. Five-minute subprocess deadlines and a 50,000-upload cap limit individual stages, but they do not bound data materialized before the cap, repeated DAG traversal, time spent holding a global permit, or which encrypted objects eventually receive an attempt across passes. The backstop needs a work/progress invariant across the whole pass and across restarts, not only timeouts around individual calls.
Several tests currently lock in the local implementation behavior without asserting the end-to-end contract: the reader-removal race accepts one stale envelope, the legacy-listing test requires a key known to 404, and the single-permit test proves permit reuse without checking that unrelated push work can proceed during the intervening history walk. Please use the invariants below as the source of truth and make the tests exercise the complete failure path through the downstream consumer/effect.
A root-cause-oriented way to close this review is:
- Write down the state transitions for each lane:
planned → authorized → provider accepted → durably recorded → advertised, including stale-policy, timeout, unknown-commit, restart, and compensation outcomes. - Give those outcomes distinct types or otherwise make them impossible to confuse at call sites. In particular, do not let “provider returned a CID” satisfy a caller that requires “durability record committed.”
- Put work admission, traversal limits, and continuation decisions before or inside the operation that expands the work. A timeout remains a last-resort safety net, not the primary pagination strategy.
- Add one table-driven integration matrix covering local IPFS, Pinata, and encrypted recovery across policy changes before/during/after upload, DB success/failure/unknown outcome, cap exhaustion, restart, and concurrent push traffic.
- Keep the visibility decisions and existing compatibility behavior unchanged unless a finding explicitly identifies that contract. The requested fixes do not require publishing empty-path objects anonymously, changing push gossip semantics, or treating Pinata provider CIDs as local resolver keys.
Please address these as one contract pass rather than patching only the cited line in each item. The locations identify where the failures become observable; the root causes often span the producer and consumer on either side.
Findings
-
[P1] Serialize policy narrowing with the irreversible publication
crates/gitlawb-node/src/encrypted_pin.rs:219
Failure path: all three lanes check the captured epoch and then await an HTTP upload. A visibility-rule removal or quarantine can commit after that check but before the backend finishes publishing. The later fenced DB write on local IPFS/Pinata can refuse the bookkeeping row, but it cannot retract the provider effect; encrypted recovery does not perform a fenced record at all.encrypt_and_pin_stops_sealing_when_reader_removed_mid_batchstages this exact interleaving and requiressealedto remain non-empty, so the test proves an old-recipient envelope survives a committed removal.Root cause: the DB record is being used as the linearization point for an external side effect that already happened. Ordering record-vs-policy transactions does not order publication-vs-policy.
Required outcome: after a narrowing commits, this sweep must not leave a newly published plaintext object or newly published old-recipient envelope authorized only by the stale epoch. Establish a real ordering around the provider effect or a compensation/revocation outcome whose failure semantics are defined and enforced. Preserve the existing fail-closed classification and do not assume that merely omitting the DB row removes content from Kubo or Pinata.
Regression coverage: exercise policy change before dispatch, during an intentionally delayed upload, and after provider success but before DB persistence for each backend. Assert backend-visible effects and recipient decryptability, not only the returned vector or local row.
-
[P2] Give valid direct-ref objects a durability copy
crates/gitlawb-node/src/reconciliation.rs:1182
Failure path: the new non-commit-ref walk correctly represents a direct blob/tree or peeled tag target with an empty path.pair_decisionintentionally makes that shape owner-only, so anonymous public pinning excludes it. However, the sweep enters encrypted recovery only whenhas_path_scoped_ruleis true. In a public repo with no path rule, the object is therefore excluded from public pinning and never offered to the owner-encrypted lane. The same result repeats every pass.Root cause: “needs encrypted recovery” is inferred from the presence of a path-scoped rule rather than from the classified candidate set. Empty-path owner-only objects are a second reason encrypted work can exist.
Required outcome: every valid reachable direct-ref object must receive at least one durability copy consistent with its established owner-only visibility. Do not solve this by making empty-path content anonymous; route it to a visibility-correct recovery lane or otherwise preserve an owner-authorized copy.
Regression coverage: add public/no-rules cases for direct blob, direct tree, annotated tag-to-blob/tree, and nested tags. Simulate the missing-copy state and assert eventual durable recovery plus continued anonymous denial.
-
[P2] Bound and deduplicate the non-commit tree traversal
crates/gitlawb-node/src/git/visibility_pack.rs:416
Failure path:walk_tree_oids_innerinserts the tree OID intooutbut ignores whether that insertion was new, then startsgit ls-treefor every tree edge. A legal tree can contain many differently named entries pointing to the same child tree; repeating that shape at several levels causes the same small set of OIDs to be walked once per path. None of this exceedsMAX_TREE_WALK_DEPTH, so subprocess work can amplify until the deadline on every reconciliation attempt.Root cause: the output deduplication set is being mistaken for a traversal/work bound. It deduplicates the final values but not the work used to discover them, and depth does not bound breadth or repeated DAG edges.
Required outcome: each unique tree should be expanded at most once per classification, and the whole operation needs an absolute entry/tree/process budget that fails closed when exceeded. Preserve strict non-UTF-8 handling and complete recursive coverage of genuinely new child trees.
Regression coverage: construct wide and multi-level trees with thousands of names pointing to a shared child OID. Assert bounded subprocess/entry counts and a deterministic fail-closed result when the explicit work limit is exceeded.
-
[P2] Apply the repair bound before full-graph expansion
crates/gitlawb-node/src/git/visibility_pack.rs:785
Failure path: the sweep buffers the completecat-file --batch-all-objectsresult, all reachable path/tree pairs, several copied sets/vectors, and DB-filter results beforecap_missingtruncates the upload list. It repeats substantial classification at the mid-scan and backend boundaries. Withinall_object_paths, the per-commitrev-parse <commit>^{tree}result is unused and roots are later recomputed in a batch; the catch-all then tests each object throughHashSet<(oid,path)>::iter().any, producing object-count × path-pair work.Root cause: the 50,000 limit is an effect cap, but the PR describes it as the per-repo work bound. Discovery has no matching pagination/cardinality contract, and membership is indexed by
(oid,path)when this phase asks only whether an OID has been seen.Required outcome: enforce a bounded amount of discovery, allocation, DB filtering, and repeated authorization work before dispatch, while retaining complete fail-closed classification for the admitted window. Remove the unused subprocess and use OID-indexed membership for OID queries. The fix may stream/page candidates or persist discovery progress; it must not silently treat undiscovered content as publicly allowed.
Regression coverage: use a repository whose object/path count exceeds the cap by a large factor. Assert bounded peak candidates/DB chunks/process calls, deterministic continuation across passes, and eventual coverage without weakening withheld/dangling-object filtering.
-
[P2] Count only durable Pinata records as filled gaps
crates/gitlawb-node/src/pinata.rs:451
Failure path: after Pinata accepts the upload,record_pinata_cidcan fail, time out, or reject a stale policy epoch. The function logs that result and still appends(sha, cid)at line 515. That is intentional for the pre-existing push/gossip consumer, but reconciliation says the vector contains only successfully recorded pins and incrementsgaps_filledfrom it. The pass can therefore report success while the durable row is absent and the next pass still sees the same gap.Root cause: one return type represents two different facts: provider acceptance for push gossip and durable record completion for reconciliation.
Required outcome: let the existing push path retain provider-success information, but give reconciliation separate, unambiguous evidence of durable Pinata state before it advances success accounting. Unknown DB outcomes must remain unknown/retriable rather than being collapsed into “filled.”
Regression coverage: inject definite DB failure, DB timeout/unknown outcome, and stale-epoch rejection after a successful Pinata response. Assert the push consumer still receives the provider outcome it needs, while reconciliation reports no durable fill and retries safely.
-
[P2] Keep unrepaired provider CIDs out of the resolver-key fields
crates/gitlawb-node/src/db/mod.rs:3957
Failure path: the base implementation omitted stored CIDs that failis_raw_cidv1until the repair sweep rewrote them. This PR removes that filter, returns every non-null legacy provider key, and projects it unchanged into bothcidandlocal_cid./ipfs/{cid}independently recomputes the raw-content CID and rejects the mismatch, so a client can list a purported local resolver key and immediately receive 404. The changed test now requires this behavior while its comments acknowledge the resolver rejection.Root cause: “row has something to advertise” is conflated with “row has a node-local resolver key.”
pinata_cid, provider history, and local raw-content resolution are distinct namespaces/provenance facts.Required outcome: keep unrepaired or non-raw provider identifiers out of
cid/local_ciduntil they have a key the local resolver accepts. Continue listing legitimate Pinata-only rows throughpinata_cidand the provenance booleans; the fix must not regress remote-only visibility.Regression coverage: seed legacy Kubo/provider, Pinata-only, local-only, and dual rows; call the list endpoint and then follow every advertised local
cidthrough the resolver. Every advertised local key must be servable, while remote-only entries remain visible only in their provider field. -
[P2] Release the pin permit before the recipient history walk
crates/gitlawb-node/src/reconciliation.rs:1156
Failure path: when the repo has public gaps,_pin_permitis acquired before those uploads and deliberately kept until the end of the repo iteration. After public effects finish, the code performs offset writes, captures/rechecks policy, and can spend five minutes inwithheld_blob_recipients_boundedbefore encrypted upload; it can then retain the permit through manifest work.mainpasses this same semaphore to normal post-push pinning. Atmax_concurrent_pin_tasks = 1, unrelated push durability waits throughout the non-pin history phase.Root cause: avoiding a second acquire/deadlock was solved by extending one critical section across two pin phases and all intervening preparation. Permit ownership now follows a repo iteration rather than the scarce external operation it is meant to bound.
Required outcome: no pin permit should be held during history traversal or unrelated DB/anchor work. Each actual pin/seal phase must remain globally bounded, and the control flow must not reacquire while already holding a permit. This can be satisfied by ending the first guard before preparation and acquiring a new guard only when encrypted work is ready.
Regression coverage: with a single permit, pause the recipient walk after public upload and start unrelated push pinning. Assert the push can acquire the permit during the walk, then assert encrypted upload waits/acquires normally without deadlock.
-
[P2] Guarantee multi-pass progress for encrypted recovery
crates/gitlawb-node/src/encrypted_pin.rs:132
Failure path: encrypted candidates are regenerated into an unorderedHashMap;encrypt_and_pinwalks that iteration order only until the shared 120-second budget gate breaks. Local IPFS and Pinata persist a last-dispatched OID and rotate later passes, but encrypted recovery records no cursor and defines no stable ordering. A set of persistently slow uploads can repeatedly consume the budget without any invariant that unattempted healthy objects move into a future window.Root cause: a per-call timeout limits how long one pass runs but is being used as a substitute for pagination/fairness state. Successful recipient tags reduce completed work, but they do not guarantee progress past a persistent failing subset.
Required outcome: every still-eligible encrypted candidate must receive a deterministic future attempt under repeated capped passes, including across restart. Preserve recipient-tag idempotence and retry failed/unknown effects; do not mark an object complete merely because it was selected or dispatched.
Regression coverage: create more work than one batch can process, make early candidates persistently slow/failing, run several passes including a restart, and prove healthy suffix candidates are eventually sealed while failed candidates remain retriable.
-
[P3] Make the pin-list documentation match the nullable wire shape
crates/gitlawb-node/src/api/ipfs.rs:2133
Failure: this public contract says Pinata-only rows copypinata_cidinto an always-non-nullcid, while the implementation, response tests, and CLI intentionally preservecid: nulland expose the provider value separately.Root cause: comments from an earlier response shape remained after the resolver/provenance contract changed.
Required outcome: document
cid/local_cidas nullable node-local resolver keys andpinata_cidas the provider identifier, including local-only, remote-only, dual, and legacy/unrepaired shapes. Keep the working wire behavior rather than changing it to match stale prose.
Needs maintainer decision
-
.cargo/audit.toml:39adds a global ignore for RUSTSEC-2026-0253, whose official advisory affects both lockedlruversions and describes safe-Rust use-after-free/double-free conditions. The dependency predates this PR, but the repository-wide risk acceptance does not; unlike existing ignores, no tracking issue is linked. Please either remove/split this waiver or explicitly accept it in a separately tracked security decision with a retirement path. -
Open PR #382 changes the same withheld-tree/full-scan contracts. It is not in
main, so it is not a current rebase blocker; please decide merge order and deduplicate whichever PR lands second. -
The prior human request to split the 8.6k-line security-sensitive diff remains reasonable because this also carries visibility, CID/provenance, strict-signature, and audit-policy changes. Whether to waive that request is a maintainer scope decision rather than a code defect.
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.