fix(node): implement reconciliation sweep as durability backstop (#218) - #244
fix(node): implement reconciliation sweep as durability backstop (#218)#244Gravirei wants to merge 34 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.
…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.
…awb#218 round 10 P2) all_object_paths was doing O(O×P) work in two places: the catch-all cat-file branch checked `blob_set.iter().any(|o| o == oid)` and `tree_set.iter().any(...)` for every reachable object, and the phase 1 loop also did a per-commit unused `git rev-parse <commit>^{tree}` probe whose result was discarded (the root-tree OID is recomputed by `root_tree_oids`). On the 50k-object repos the sweep exists for, the quadratic scan and the redundant per-commit child process can consume each authorization deadline before any object reaches a backend, producing a permanent hourly skip. Maintain a separate `blob_oids` / `tree_oids` HashSet alongside the path-pair sets so the membership check is O(1). Drop the unused per-commit rev-parse probe.
…Gitlawb#218 round 10 P2) walk_tree_oids_inner stopped at MAX_TREE_WALK_DEPTH and the shared deadline. A wide shallow tree reachable from N ref tips or from multiple parents still spawned one ls-tree child process per subtree well inside the depth cap, and the wall-clock bound could not stop that — expiry was the only stop. The ceil was wall-clock only. Add a structural invocation cap (MAX_TREE_WALK_INVOCATIONS) the walker fails closed at, and a memo of walked tree OIDs so the same tree reachable from multiple ref tips is walked once. Round 10 P2.
…round 10 P2) pin_new_objects pushed (sha, cid) on provider POST success regardless of whether record_pinata_cid persisted the row. A closed/failed DB write or an explicit-transaction source record timeout meant the next sweep queried the DB, found no row, and re-offered the same gap — with reconciliation having counted a fill that never happened. Track a `db_record_durable` flag. On hard failure of either record_pinata_cid (non-timeout) or record_pin_source (any failure, since it's a multi-statement transaction), set the flag false and skip the `pinned.push`. The `BoundedDbError::Elapsed` autocommit case stays as the previous comment's "may have committed" reasoning: the push fires so the reconcile still sees the fill. Round 10 P2.
…ocess mutex (Gitlawb#218 round 10 P1) The PolicyFence's epoch read was a one-way read: a visibility narrow (rule insert, quarantine set) could commit between the fence's epoch read and the upload's HTTP POST, and the upload's DB-record INSERT's epoch check would see the new epoch and reject — but the provider already accepted the object. Encrypted envelopes had no fence at all. Add a per-repo in-process mutex that PolicyFence acquires at capture and releases at drop. The narrow paths (set_repo_quarantine, set_visibility_rule, remove_visibility_rule) acquire the same lock for the duration of their DB transaction. A narrow therefore blocks until the in-flight batch finishes, and an in-flight batch waits for the narrow to commit before it even reads the epoch. Multi-process / multi-node is a known gap: a Postgres advisory lock per repo would extend the same guarantee across processes. Deferred to a follow-up.
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.
…ontract The previous test asserted that a successful upload with a timed-out post-upload source record still returned the (sha, cid) pair. That assertion pinned the buggy behavior the reviewer called out: the reconcile would count a fill that was not durable, then re-offer the same gap. The test name and the `record_pin_source` arm of `pin_new_objects` are unchanged; the assertion now reflects the new contract: when the post-upload source record times out (its `mark_pin_sources_incomplete` already sets the source set to incomplete, so the next pass re-offers), the pair is suppressed so the reconcile sees the gap as not-filled. Round 10 P2 follow-up.
clippy::too_many_arguments on walk_tree_oids_inner (8 args after the round 10 memo + invocation counter threading). rustfmt re-ran on the if/else blocks added in the has_public_work refactor (round 10 P1 Gitlawb#42). No behavior change.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed at 244ba36c. Six commits landed since the last round and the direction on most of them is right: the fmt gate is green again, the tree walk is bounded by ls-tree invocation count, and the classifier no longer scans quadratically. The problem is the policy-mutex commit. It closes the epoch/POST race by holding a per-repo lock across the whole pin batch, and that deletes a confidentiality property the suite already pinned. test (stable) and test (beta) are red on this head for exactly that reason.
Findings
-
[P1] Do not hold the policy lock across the whole pin batch
crates/gitlawb-node/src/ipfs_pin.rs:1455
1634a042makesPolicyFencehold a per-repo mutex from capture through drop, andset_visibility_rule/remove_visibility_rule/set_repo_quarantineacquire the same lock. The commit message states the consequence plainly: "A narrow therefore blocks until the in-flight batch finishes." That removes the mid-batch abort.encrypted_pin::tests::encrypt_and_pin_stops_sealing_when_reader_removed_mid_batchnow fails on both stable and beta atcrates/gitlawb-node/src/encrypted_pin.rs:566, sealing 3 of 3 where it must seal fewer, andencrypted_pin.rsis not in this round's diff, so production behavior moved under a test that was already green. The trade goes the wrong way: before, a reader removal landing mid-batch cost one in-flight object; now the removal waits and every object in the batch is sealed to the reader being removed. The same lock sits on the request path (crates/gitlawb-node/src/api/visibility.rs:114and:153) with no acquire timeout, so an owner revoking a reader can block behind a background sweep for the batch budget. Close the epoch/POST window without extending one critical section over the batch, and keep that test green rather than rewriting it to match. -
[P2] Bound the policy-mutex key allocation and correct its doc claim
crates/gitlawb-node/src/ipfs_pin.rs:1490
PolicyMutexes::lockrunsBox::leak(repo_id.to_string().into_boxed_str())unconditionally, before the registry lookup. The map is keyed by&'static strand hashes by content, so the entry is correctly reused, but every call after the first discards a fresh 36-byte allocation that is never freed. I ran the same key handling standalone: 1000 calls against a single repo id give 1 registry entry and 1000 distinct leaked keys. The doc comment says the leak is "bounded by the number of distinct repos ever observed"; it is bounded by call count, and this is on an hourly per-repo sweep plus every visibility write. Look the key up first and leak only on insert. -
[P2] Test the empty-public-list path the round-10 fix exists for
crates/gitlawb-node/src/reconciliation.rs:676
d9992a4badds 51 production lines and no test lines; all five hunks land between 662 and 1151, abovemod testsat 1347. Nothing in the suite referenceshas_public_work, and every fixture that could reach it commits a file at a public path. The two that look all-withheld,sweep_skips_mirror_rowsandsweep_skips_private_repos_before_scan, bail before the scan and never get there. The branch the commit exists to enable is unexercised, and the old skip-empty behavior is unpinned too, so the change is untested in both directions. -
[P3] Give the annotated-tag case its own input
crates/gitlawb-node/src/git/visibility_pack.rs:3941
The two assertions callwithheld_blob_oids(&bare, &rules, true, OWNER, None)with identical arguments at:3941and:3951, so the second is a re-run of the first rather than a peel case. Both refs live in the same bare clone on both calls, anddirect-treesorts beforetag-of-tree, so the walk fails closed on the direct ref and the peel arm never runs either time. A regression in the tag-of-tree shape would not be caught. Give the peel shape its own tree or its own clone. -
[P3] Finish the migration test rename and fix two stale comments
crates/gitlawb-node/src/db/mod.rs:5962
The function ismigration_v32_...now, but the body still narrates v12: the seed boundary istake_while(|m| m.version < 12)at:5962, and "Apply migration v12", "cid must be nullable after v12", and "new post-v12 row" remain at:6017,:6030,:6074. The test does drive the real v32DROP NOT NULL, so this is drift, not dead coverage. Two comments inreconciliation.rsare wrong the same way::751says the empty-refiltered pass "only need[s] to skip the offset bookkeeping" when nothing is skipped (it falls through to the offset loads, andnext_offset_write(true, false, None)returnsDrained, which clears the cursor), and:710says the second earlycontinuewas "replaced with the flag flip above" whenhas_public_workis computed pre-refilter and cannot govern that case.
One note on order, not a finding: #285, #382, and #134 all touch surfaces this PR moves (api/repos.rs, git/visibility_pack.rs, api/ipfs.rs). Whichever lands second will need the overlap reconciled, and the blast-radius reasoning here assumes none of them has merged yet.
…tion sweep P1: remove the whole-batch policy mutex. PolicyFence no longer holds a per-repo lock from capture through drop and the narrow paths (set_visibility_rule, remove_visibility_rule, set_repo_quarantine) no longer acquire it, so a visibility narrow commits immediately and the batch aborts on its next is_current check instead of the narrow blocking behind the sweep. Accepted residual is a single in-flight object; the fenced DB record still refuses to land a raced row as durable. encrypt_and_pin_stops_sealing_when_reader_removed_mid_batch is green again. PolicyMutexes is deleted, resolving the key-leak P2 by removal. P2: cover the round-10 has_public_work branch with sweep_seals_withheld_blob_when_public_list_is_empty: a direct-blob-ref repo yields an empty public list with a non-empty owner recovery set; asserts no public gaps/fills, no cleartext pin, exactly one POST (the seal envelope), and a recorded encrypted copy. P3: peel-case isolation in the non-UTF-8 tree-tip test (delete the direct-tree ref before the second call so the peel arm must run); v12->v32 rename drift in the migration test (boundary, comments, and the stale (2)->true doc line); accurate comments for the pre-refilter flag and the post-refilter fall-through.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed at 34b59790. Round-11 fixes verified by execution: cargo fmt --check and clippy -D warnings are clean on this head, encrypt_and_pin_stops_sealing_when_reader_removed_mid_batch passes again after the policy mutex removal, sweep_seals_withheld_blob_when_public_list_is_empty covers the empty-public path, and the tag-peel fixture now deletes direct-tree before asserting the peeled arm. I gutted the first IPFS to_pin dispatch and sweep_fills_ipfs_gap_and_persists_cursor went RED, so the gap-fill premise is load-bearing.
Open PRs #285, #382, and #134 still touch surfaces this diff moves; whichever lands second will need overlap reconciled.
Not an ask, recorded only: gitlawb_reconciliation_gaps_found_total counts the post-cap work queue (50k/backend), not the repo's full missing set, so operators should not read it as total backlog depth.
Not an ask, recorded only: migration_v32_makes_cid_nullable_and_preserves_classification still narrates the old v12 contract in a few comment lines; the take_while boundary and assertions are v32-correct.
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
- The branch is based on current
main(bfc44f926d08c0bf774e2c05dd76b245871294f1), GitHub reports it mergeable without conflicts, and all 13 reported checks are green. The blockers below are failure-path and lifecycle problems the current suite does not exercise.
Why this PR keeps producing follow-up findings
The recurring problem is not that the reconciliation idea is unsound. It is that several boundaries are represented by the same value even though they mean different things:
- selected for a batch;
- actually entered/attempted by the backend loop;
- accepted by Kubo or Pinata;
- committed to the local database;
- safe to advertise through the node resolver;
- complete enough to advance durable continuation state.
The current implementation repeatedly promotes an earlier state into a later one: the tail of a selected vector becomes “last dispatched,” an elapsed provider-record transaction becomes “durable,” and a legacy non-null column becomes proof that Kubo stored bytes. The same structural issue appears in resource ownership: one semaphore guard represents both provider use and an entire repository pass. Review fixes have mostly added local branches, booleans, and explanatory comments around those conflated states, which leaves the next consumer free to make the same incorrect inference.
Please address these as explicit state-machine contracts rather than another set of one-line conditionals. In particular:
- Make backend results carry separate fields for
last_attempted, provider acceptance, and confirmed database durability. The caller should not reconstruct those facts from a planned vector or a success list. - Keep scan/prepare, provider-effect, persistence, and publication phases distinct. Acquire scarce provider permits only around provider effects.
- Treat migrated legacy provenance as
unknownunless it can be verified. Column shape is not evidence that an external side effect happened. - Add failure-injection tests at transitions, not just happy-path unit tests: budget exhaustion before the suffix, provider success followed by DB cancellation, old-row migration followed by a sweep, list-then-resolve, and a stalled preparation phase competing with a normal push.
- Avoid folding more signature, dependency-policy, or API cleanup into this branch while repairing reconciliation. If practical, split the durability state machine, legacy migration/repair, and API projection into independently reviewable commits or PRs with their own lifecycle tests. That will make the next review about the fixes rather than rediscovering interactions across an 8.8k-line diff.
The findings below are limited to behavior introduced, worsened, or activated by this PR. I am intentionally not carrying forward concerns that depend on a stronger, undocumented revocation model, a different allowed-at-any-path tree policy, or a merely theoretical scheduling sequence.
Findings
-
[P2] Give empty-path direct refs an owner recovery path even when there are no path rules
crates/gitlawb-node/src/reconciliation.rs:1188Failure path: A blob, tree, or peeled tag referent reachable only through a non-commit ref has an empty path.
pair_decision("", ..., None)deliberately rejects that object for anonymous/public replication because no visibility glob can classify it. That is the correct fail-closed public decision. However, encrypted recovery runs only underhas_path_scoped_rule(&fresh_rules2). For a public repository with no visibility rules, or only a whole-repository/rule, the object is removed from the public set and phase 2 is skipped. If its previous recovery copy was lost, every hourly pass reaches the same result and the durability backstop never repairs it.Root cause:
has_path_scoped_ruleis being used as an optimization based on the older invariant that “without a path-scoped rule, no individual object can be withheld.” The new empty-path policy creates an owner-only withheld class independently of path rules, so that invariant no longer holds.Required outcome: Keep empty-path objects denied to anonymous callers and to readers whose grants cannot match a path, but route them into an owner-authorized recovery lane regardless of whether any path-scoped rule exists. This could be driven by the actual non-empty withheld-recipient result rather than by the rule-shape shortcut; the important part is that an unclassifiable reachable object gets a durability copy without becoming public.
Regression coverage: Exercise a direct blob ref, direct tree ref, and peeled tag target under both zero rules and root-only rules. For each case, prove that no plaintext public pin is written, the owner-encrypted copy is created, and a subsequent sweep recreates it after deleting the recovery record/copy. The existing test adds
/secret/**, so it cannot catch this gate. -
[P2] Put a real resource bound before full repository materialization
crates/gitlawb-node/src/reconciliation.rs:610Failure path: Before
cap_missing(50_000)executes, one pass bufferscat-file --batch-all-objects, walks every reachable commit/path/tree, builds complete allowed/all/reachable sets, collects the complete filtered object list, and sends that full list through database filtering. An admitted repository with a very large history therefore consumes memory and blocking-pool/CPU work proportional to its entire object/path graph every hour before the advertised object cap affects anything. The five-minute child-process deadline limits elapsed subprocess work; it does not bound the number or aggregate size of values already emitted and retained in the Rust collections.Root cause:
MAX_OBJECTS_PER_REPOis an effect/upload cap, while its contract says it prevents a large repository from monopolizing the blocking pool or hourly budget. Discovery and visibility classification have no matching page, cursor, cardinality, or admission bound, so the cap is applied after the resource it claims to protect has already been consumed.Required outcome: Bound discovery/allocation before full materialization, or add a separately enforced admission/resource ceiling that makes the intentional full scan safe. A resumable ordered discovery cursor is one option, but the exact mechanism is less important than preserving both properties: bounded work per pass and eventual coverage across passes. Do not meet the bound by classifying undiscovered objects as public; incomplete classification must remain fail-closed.
Regression coverage: Build a repository materially larger than
MAX_OBJECTS_PER_REPO, instrument candidate/set sizes and Git invocations, and prove one pass stays within the chosen bound. Run enough passes—including a restart—to show deterministic progress beyond the first window while denied and dangling objects remain excluded. -
[P2] Persist the last object actually attempted, not the tail of the selected vector
crates/gitlawb-node/src/reconciliation.rs:953Failure path: Both public backend arms assign
ipfs_dispatched/pinata_dispatchedfromto_pin.last()before enteringpin_new_objects. Those backend functions consume the vector sequentially and may stop atbatch_budget_gatebefore later elements are visited. If an early upload consumes the 120-second budget, the suffix was never attempted, but the stored cursor is still the maximum planned OID. On the next pass,missing_oidsfinds nothing greater than that maximum and falls back to the same sorted head-first order; the same slow prefix can therefore prevent the untouched suffix from ever reaching a provider.Passing ownership of a vector to a sequential function is not equivalent to dispatching every element in it. The comments currently make that exact assumption.
Root cause: The backend result exposes durable successes but not attempt progress, so the caller invents progress from the plan before the effect occurs. This also conflates “attempted but failed/unknown” with “never entered,” even though cursor fairness needs to distinguish them.
Required outcome: Return explicit effect-side progress from each backend, such as a typed result containing
last_attemptedplus the durably recorded successes, and persist the cursor after the backend call. Pre-dispatch failures must leave the cursor untouched; an actually attempted failure may rotate to the tail but must remain missing/retriable; an unvisited suffix must remain ahead of the continuation. Keep IPFS and Pinata progress independent.Regression coverage: Use a batch with at least three ordered OIDs and make the first or second object consume the remaining budget. Assert the stored cursor is the last object whose loop body was entered—not
to_pin.last()—then run the next pass and prove the previously untouched healthy suffix is attempted. Cover both backend implementations because they have separate loops and cursor rows. -
[P2] Do not classify an elapsed Pinata transaction as durable
crates/gitlawb-node/src/pinata.rs:480Failure path: Pinata accepts an upload, then the bounded
record_pinata_cidfuture expires while waiting on the database. This branch still describes the writer as a single autocommit upsert and setsdb_record_durable = true. At this head, however,Db::record_pinata_cidstarts an explicit transaction and reaches durability only attx.commit()indb/mod.rs:4189. Cancellation before COMMIT leaves no row; cancellation while COMMIT is in flight is at best an unknown outcome. In both cases the pair is returned as if persistence were confirmed. Reconciliation then incrementsgaps_filled, and push-side consumers can derive branch/gossip CID state without confirmed resolver bookkeeping.Root cause: The database writer changed from autocommit to a multi-statement transaction, but its timeout classification was copied forward unchanged. The Boolean
db_record_durableand returned pair also combine two facts that have different consumers: Pinata accepted the bytes, and the node durably recorded a resolvable association.Required outcome: Treat
Elapsedas non-durable unless a bounded read-after-timeout proves the exact row and applicable fence epoch. Preserve provider acknowledgement for any caller that legitimately needs it, but represent it separately from confirmed database durability; reconciliation metrics and durable continuation may use only the latter.Regression coverage: Inject (1) provider success followed by cancellation before commit, (2) a commit whose result is unknown to the client, (3) a definite DB error, and (4) a stale policy epoch. Assert that provider acknowledgement can still be surfaced where appropriate, while no unconfirmed case increments
gaps_filled, advertises a local resolver association, or advances state as repaired. -
[P2] Do not infer historical Kubo durability from the legacy row shape
crates/gitlawb-node/src/db/mod.rs:1256Failure path: Migration v35 sets
local_ipfs_provenance = TRUEfor every legacy row wherecid IS NOT NULL AND pinata_cid IS NULL. Before this PR, a 2xx Kubo response with noHash—including a wrong-port health endpoint, HTML proxy response, or truncated body—fell back to the locally expected CID and wrote exactly that row even though nothing proved Kubo stored the bytes. After upgrade, v35 marks the row locally durable,filter_ipfs_pinned_oidstrusts it, and the reconciliation backstop permanently skips the object. The new strictHashresponse parsing prevents new phantom rows but cannot validate rows already produced by the old behavior.Root cause: The migration treats a database column pattern as evidence of an irreversible external side effect. The old schema did not preserve enough provenance to distinguish a successful Kubo add from the pre-fix 2xx/no-
Hashfallback, so the correct migrated state is unknown, not true.Required outcome: Migrate ambiguous local-only rows as unknown and verify or conservatively re-pin them before setting local provenance true. If backend verification cannot prove the raw object copy, an idempotent re-upload is preferable to permanently suppressing repair. Preserve known Pinata-only/dual state and do not rewrite provider history as local evidence.
Regression coverage: Seed the exact pre-v35 row produced by a 2xx/no-
Hashresponse, apply migrations, and run reconciliation. The object must be verified or re-pinned rather than filtered out. Add controls for a genuine legacy local pin, a Pinata-only row, and a dual row so the conservative migration does not destroy known state. -
[P2] Keep legacy provider CIDs out of node-local resolver fields
crates/gitlawb-node/src/api/ipfs.rs:2184Failure path: The base implementation withheld stored CID values that were not valid raw CIDv1 resolver keys until repair rewrote them. This PR removes that filter and copies any
p.cidvalue into bothcidandlocal_cid. A legacy Kubo/Pinata provider identifier can therefore appear as a node-local key before repair, or permanently if repair cannot find a source. Following that advertised value through/ipfs/{cid}recomputes the raw-content CID and rejects the mismatch, so the list endpoint promises a local object the node immediately refuses to serve.Root cause: The historical
cidcolumn has contained more than one namespace, but the response projection now treats non-nullness as proof that the value is both a raw node resolver key and locally durable. CID syntax, writer provenance, provider history, and local resolvability are separate facts.Required outcome: Populate
cid/local_cidonly when the value passes the raw local-resolver contract and local provenance is confirmed. Keep remote-only state visible throughpinata_cidandpinata_pinned; do not hide those rows, and do not copy a provider identifier into a local field merely to keepcidnon-null.Regression coverage: Seed local-only, Pinata-only, dual, legacy provider-key, and failed-repair rows. For every non-null local CID returned by the list endpoint, follow it through
/ipfs/{cid}and require successful resolution. Remote-only rows should remain present but carry their usable identifier only in the provider field. -
[P2] Release the global pin permit during recipient discovery and other non-provider work
crates/gitlawb-node/src/reconciliation.rs:897Failure path: Once a repository has a public gap,
_pin_permitremains live across both public backend arms, cursor writes, policy capture/rechecks, the entirewithheld_blob_recipients_boundedhistory walk (with its own five-minute deadline), encrypted work, and manifest handling. Normal post-push durability uses the same semaphore. Withmax_concurrent_pin_tasks = 1, finish the public upload and stall recipient discovery: an unrelated successful push cannot acquire the only permit even though the sweep is not talking to any provider. Because the phase budgets are additive, the node-wide delay can be minutes.Root cause: A previous self-deadlock risk from reacquiring the semaphore was avoided by extending one critical section across the repository's whole reconciliation state machine. The guard now represents “this repo is being processed,” not “one scarce provider operation is active.”
Required outcome: Scope the permit to provider effects. Acquire immediately before the public IPFS/Pinata work and drop after those effects; do offset writes, fresh policy reads, and recipient history traversal without it; acquire a new permit only when encrypted upload is ready; and release it before manifest anchoring or unrelated bookkeeping. Structure ownership so no branch can reacquire while already holding a guard, while retaining the configured global provider concurrency ceiling.
Regression coverage: With a one-permit semaphore, pause recipient discovery after a public upload and start an unrelated post-push pin. The push must acquire during the paused walk. Then release discovery and prove encrypted upload waits for/acquires the permit normally, never exceeds concurrency, and does not deadlock.
-
[P3] Delete completed continuation state instead of refreshing tombstones forever
crates/gitlawb-node/src/db/mod.rs:3107Failure path:
save_reconciliation_offset(..., None)upsertsdone = TRUEand refreshesupdated_at.load_reconciliation_offsettreats that row exactly like absence, but every later healthy drained pass writes the tombstone again.clear_reconciliation_offsethas no production caller, and the table has no foreign key/cascade to repositories. Every drained repo/backend therefore retains a permanent row, generates an hourly write/WAL update, and can leave an orphan after repository deletion/recreation.Root cause: “No continuation exists” is represented as durable state even though absence already has the required fresh-start meaning and the migration contract says completed state prunes back to zero. The cleanup method and repository deletion lifecycle were never connected.
Required outcome: Make
Draineddelete the(repo, backend)row, or add equivalent pruning plus repository-lifecycle cleanup. Preserve the distinctIdlebehavior: a transient pre-dispatch failure must keep a real prior cursor rather than clear it.Regression coverage: Start with an advanced cursor, drain the missing set, and assert the row is deleted. Run another empty pass and assert it does not recreate a tombstone. Delete/recreate a repository identity and prove no stale backend offset survives.
-
[P3] Make the pin-list documentation match the nullable response contract
crates/gitlawb-node/src/api/ipfs.rs:2133Failure path: The public handler documentation says a Pinata-only row copies
pinata_cidinto an always-non-nullcid. The implementation, response tests, andglconsumer intentionally preservecid: null/local_cid: nulland expose the provider identifier separately. An API client implementing the documented contract will therefore assume a local resolver key exists where the server intentionally emits none.Root cause: Documentation for an earlier effective-CID fallback remained after the response was split into local resolver and provider provenance fields.
Required outcome: Document
cidandlocal_cidas nullable node-local resolver keys,pinata_cidas the provider identifier, and the provenance booleans as the backend-state indicators. Keep the tested nullable wire behavior; changing the response to match the stale prose would reintroduce the resolver-key defect above.Regression coverage: Keep one response-shape matrix for local-only, remote-only, dual, and legacy/unrepaired rows, and make the CLI expectations consume the same documented meanings.
Needs maintainer decision
-
.cargo/audit.toml:39adds a repository-wide waiver for RUSTSEC-2026-0253, affecting both locked, reachablelruversions. The advisory requires a panicking key destructor plus unwind-catching, and I did not establish that trigger in current callers, so I am not presenting it as a vulnerability finding. The risk acceptance is nevertheless new and, unlike existing exceptions, has no linked tracking issue or retirement owner. Please either track the upgrade/removal explicitly or keep the audit failure visible until it can be resolved. -
The current tests intentionally allow one provider upload already in flight when a visibility rule or reader set narrows; they require later objects to stop. If the product requires revocation commit to linearize against every concurrent publication—or requires compensation for an encrypted envelope that finishes and is publicly anchored after removal—that is a stronger policy contract than the current tests establish. Please decide and document that guarantee before changing the fence again; the earlier whole-batch lock fixed one race by making revocation wait for the entire batch, which violated the prompt-revocation behavior the suite also expects.
-
Open PR #382 overlaps the withheld-tree and full-scan contracts; #384 overlaps database migrations/storage; and #314 overlaps strict signature work. This branch also combines reconciliation, visibility/CID security changes, API/CLI behavior, migrations, signature hardening, and dependency-policy changes despite the repository's one-change-per-PR guidance. Please choose merge order before more repair commits, and split independently valuable work where practical so fixes on one state machine do not invalidate another branch's assumptions.
…tion sweep Reworks the sweep around explicit state-machine contracts instead of adding conditionals: backend results carry last_attempted plus confirmed durability separately (PinBatchOutcome), provider permits scope to provider effects only, discovery pages by commit window with a persisted cursor, legacy provenance migrates as unknown, and the pin list advertises only validated raw local keys. - Empty-path owner recovery without path rules: phase 2 drops the has_path_scoped_rule gate and runs on the actual recipient result, with a dedicated recovery recheck (no anonymous-listability requirement); matrix test over blob/tree/tag refs x zero/root rules, including recreate-after-delete. - Pinata Elapsed is non-durable unless verify_pinata_record proves the exact row and fence epoch; loop test with row-locked repos. - list_pins gates cid/local_cid on is_raw_cidv1 plus confirmed provenance; response-shape matrix uses real CIDs with a legacy row and GET /ipfs follow-through; docs match the nullable wire contract. - Permits acquired per provider effect and dropped after; no nesting at pool size 1 (test renamed to the new shape). - Scan discovery pages SCAN_COMMIT_WINDOW commits oldest-first with a node_state cursor (advance on drained/empty/attempted, delete on cover); refilters and phase-2 recipients run on the same window; walk-layer paging/invocation/union tests plus a 1050-commit cursor-to-completion sweep test. - v35 migrates legacy rows as unknown (no TRUE backfill); v35 test asserts unknown + preserved Pinata history; sweep re-derivation test re-pins instead of filtering. - Drained completions DELETE continuation rows (no tombstones); FK cascade on repo delete; row-count regression test. - Audit: lru waiver gets an explicit retirement owner plus a weekly drift guard; revocation model recorded as a product decision on PolicyFence (prompt revocation, single-in-flight residual).
The windowed-union test creates its annotated tag in the bare clone, which carries no config. It passed locally only via the developer's global git identity; CI has none, so 'git tag -a' failed there. Verified with an empty GIT_CONFIG_GLOBAL/SYSTEM.
read_object_bounded_returns_by_deadline_with_a_hung_git reads the fake's pid file with no readiness handshake; a slow scheduler yields 'No such file or directory' (stable-only red, identical beta green). Unrelated to Gitlawb#218 (store.rs untouched); re-enable with a readiness signal from the fake.
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.