feat(storage): storage-agnostic BlobStore layer + push-latency quick wins - #79
feat(storage): storage-agnostic BlobStore layer + push-latency quick wins#79kevincodex1 wants to merge 14 commits into
Conversation
|
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 PR replaces hard-coded Tigris storage with configurable S3-compatible, filesystem, and IPFS backends. ChangesPluggable Storage Backend and Repository Archiving
Estimated code review effort: 5 (Critical) | ~100 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
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. |
beardthelion
added
kind:feature
fa8a9f3 to
7b7e690
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: 8
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/storage/archive.rs (1)
162-184: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up the temp dir when the swap phase fails.
The unpack-error path removes
tmp_dir, but the swap phase does not: ifremove_dir_all(local_path)orrenamereturnsErr, the freshly-extracted.{file_name}.tmp-extract.<uuid>dir is orphaned. Because each extraction uses a fresh UUID and nothing reclaims leftovers, repeated failures (e.g. permission errors, open handles) leak temp dirs indefinitely.Secondary concern:
remove_dir_all(local_path)beforerenamedestroys the only valid local copy first, so a subsequentrenamefailure leaves the repo missing — underminingsync_down_if_stale'slocal_path.exists()self-heal fallback.🧹 Proposed fix: scope cleanup over the whole swap
- let lock = publish_lock(local_path); - let _publish = lock.lock().expect("publish lock poisoned"); - if local_path.exists() { - std::fs::remove_dir_all(local_path).context("removing stale repo dir")?; - } - std::fs::rename(&tmp_dir, local_path).context("swapping extracted repo into place")?; - Ok(()) + let swap = (|| -> Result<()> { + let lock = publish_lock(local_path); + let _publish = lock.lock().expect("publish lock poisoned"); + if local_path.exists() { + std::fs::remove_dir_all(local_path).context("removing stale repo dir")?; + } + std::fs::rename(&tmp_dir, local_path).context("swapping extracted repo into place")?; + Ok(()) + })(); + if let Err(e) = swap { + let _ = std::fs::remove_dir_all(&tmp_dir); + return Err(e); + } + Ok(())🤖 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/storage/archive.rs` around lines 162 - 184, The swap phase in the archive extraction does not clean up tmp_dir if either the remove_dir_all(local_path) call or the rename call fails, leaving orphaned temporary directories. Additionally, removing local_path before confirming the rename succeeds risks losing the only valid copy if rename fails. Ensure that tmp_dir is cleaned up in all failure paths during the swap phase by wrapping the entire lock-acquisition and swap logic so that if any error occurs after acquiring the publish lock, the tmp_dir is removed before returning the error. Consider the order of operations to minimize data loss risk by only removing the old local_path after confirming the rename will succeed.
🧹 Nitpick comments (2)
crates/gitlawb-node/src/storage/ipfs.rs (1)
72-73: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid
body.to_vec()to prevent doubling upload memory.Line [72] copies the full blob before upload. For large repo archives this spikes memory usage and allocator pressure; use a streaming multipart part instead.
Suggested fix
- let part = reqwest::multipart::Part::bytes(body.to_vec()).file_name("blob"); + let part = reqwest::multipart::Part::stream(reqwest::Body::from(body)).file_name("blob");🤖 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/storage/ipfs.rs` around lines 72 - 73, The multipart Part creation uses body.to_vec() which copies the entire blob into memory before uploading, doubling memory usage for large files. Replace the reqwest::multipart::Part::bytes() call with a streaming approach such as reqwest::multipart::Part::stream() to avoid the unnecessary copy and pass the body content directly as a stream instead of converting it to a vector first.crates/gitlawb-node/src/storage/fs.rs (1)
114-117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDon’t silently skip directory read failures in
list().Lines [114]-[117] convert I/O failures into partial results. Returning
Ok(keys)after hidden errors can mislead future admin/GC/migration callers into acting on incomplete inventories.🤖 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/storage/fs.rs` around lines 114 - 117, The list() function silently ignores directory read failures by using an Err(_) => continue pattern in the std::fs::read_dir(&dir) match statement, which causes it to return Ok(keys) with incomplete results. Replace this silent failure handling with proper error propagation or handling that ensures callers are informed of failures rather than acting on incomplete inventory data. Instead of continuing on read_dir errors, return an Err result or log and return the appropriate error indication so that admin, GC, and migration processes receive accurate information about the success or failure of listing all keys.
🤖 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/api/repos.rs`:
- Around line 555-561: The `tokio::spawn(guard.release(true))` call has two
issues: the advisory lock release at the end of the `release()` method silently
discards errors with `let _ =`, making lock-release failures invisible to
operators, and the spawned task is not integrated with graceful shutdown,
potentially leaving locks acquired indefinitely. Fix by adding proper error
logging (via warn! or error!) for the advisory lock release query failure
instead of using `let _ =` to discard the error, and integrate the spawned task
with axum's graceful shutdown mechanism using a task tracker or shutdown signal
to ensure the task completes or is cancelled gracefully during shutdown.
In `@crates/gitlawb-node/src/git/repo_store.rs`:
- Around line 178-192: The cache-miss path currently uses unwrap_or(None) on the
head_etag call, which swallows any transient storage errors and returns None,
causing the function to fall through and return a non-existent local_path that
makes the caller see a misleading "repo not found" error. Replace the
unwrap_or(None) on the head_etag(...).await call with the ? operator to
propagate storage errors instead of silently converting them to None, so real
errors surface to the caller rather than being hidden as "not found" responses.
- Around line 79-116: The sync_down_if_stale function uses permissive self-heal
fallbacks for both read and write paths, but this causes a lost-update issue for
writes: when the remote etag differs from the cached etag (remote is newer) and
download fails, the function falls back to the stale local copy, which then gets
uploaded and overwrites the newer remote archive. Add a freshness-requirement
parameter to sync_down_if_stale that gates the self-heal behavior: when
freshness is required (write path), propagate errors from the HEAD check (around
lines 82-89) and download operation (around lines 105-115) instead of falling
back to the local copy; when freshness is not required (read path), keep the
current permissive fallback. Update calls from acquire_fresh to pass false
(freshness not required) and acquire_write to pass true (freshness required).
In `@crates/gitlawb-node/src/storage/fs.rs`:
- Around line 75-82: The fixed temporary file extension `tmp-put` created by
`path.with_extension("tmp-put")` on line 75 causes a race condition when
concurrent `put` operations occur on the same key, as both writes will use the
same temp file path and corrupt each other's data. Generate a unique temporary
filename for each write operation instead of using a fixed extension, such as by
appending a UUID, random suffix, or timestamp to the temporary filename to
ensure each concurrent write has its own isolated temp file before the atomic
rename into place.
In `@crates/gitlawb-node/src/storage/ipfs.rs`:
- Around line 166-168: The list() function currently skips non-success responses
from the files/ls endpoint by using continue, which silently treats
authentication, network, and server errors as empty successful listings. Instead
of continuing when !resp.status().is_success() is true, you should properly
handle the error by returning a Result error that propagates the failure to the
caller, ensuring that list() accurately reflects whether the operation succeeded
or failed rather than returning partial or empty results on error conditions.
- Around line 25-29: The reqwest::Client::new() call in the new method does not
configure any timeout values, which allows requests to the IPFS API to hang
indefinitely if the service becomes slow or unreachable. Replace the default
Client::new() call with a Client::builder() pattern that explicitly sets timeout
values using methods like timeout() to define a reasonable connection and
request timeout duration, then call build() to create the configured client.
This ensures that storage operations have bounded execution times and prevents
indefinite hangs.
In `@crates/gitlawb-node/src/storage/mod.rs`:
- Around line 67-70: The selection order documentation comment in the storage
module starting with "Selection order:" is incomplete and does not reflect the
actual implementation. Update the comment block to include the auto-detection of
the ipfs backend when the GITLAWB_IPFS_API environment variable is set, which
occurs in the actual implementation around lines 84-86. Add ipfs as an
additional auto-selection option to the documented order after the fs fallback
so the documentation accurately describes all auto-detection paths in the
storage backend selection logic.
- Around line 91-145: The function currently downgrades to local-only mode
(returning None) when explicitly configured backends fail to initialize, which
can silently accept writes without the intended durable backend and cause data
persistence issues. In the match statement, change the error handling for the
s3, fs, and ipfs branches so that when backend creation fails (in the Err
branches of S3BlobStore::new, FsBlobStore::new, and IpfsBlobStore::new), return
the error to propagate it up the call stack rather than logging a warning and
returning None. This ensures that if a backend is explicitly configured but
cannot be initialized, the application will fail at startup rather than silently
degrading to local-only mode.
---
Outside diff comments:
In `@crates/gitlawb-node/src/storage/archive.rs`:
- Around line 162-184: The swap phase in the archive extraction does not clean
up tmp_dir if either the remove_dir_all(local_path) call or the rename call
fails, leaving orphaned temporary directories. Additionally, removing local_path
before confirming the rename succeeds risks losing the only valid copy if rename
fails. Ensure that tmp_dir is cleaned up in all failure paths during the swap
phase by wrapping the entire lock-acquisition and swap logic so that if any
error occurs after acquiring the publish lock, the tmp_dir is removed before
returning the error. Consider the order of operations to minimize data loss risk
by only removing the old local_path after confirming the rename will succeed.
---
Nitpick comments:
In `@crates/gitlawb-node/src/storage/fs.rs`:
- Around line 114-117: The list() function silently ignores directory read
failures by using an Err(_) => continue pattern in the std::fs::read_dir(&dir)
match statement, which causes it to return Ok(keys) with incomplete results.
Replace this silent failure handling with proper error propagation or handling
that ensures callers are informed of failures rather than acting on incomplete
inventory data. Instead of continuing on read_dir errors, return an Err result
or log and return the appropriate error indication so that admin, GC, and
migration processes receive accurate information about the success or failure of
listing all keys.
In `@crates/gitlawb-node/src/storage/ipfs.rs`:
- Around line 72-73: The multipart Part creation uses body.to_vec() which copies
the entire blob into memory before uploading, doubling memory usage for large
files. Replace the reqwest::multipart::Part::bytes() call with a streaming
approach such as reqwest::multipart::Part::stream() to avoid the unnecessary
copy and pass the body content directly as a stream instead of converting it to
a vector first.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b25ebb6-e61a-4ce6-a7ae-1636bdde8f45
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/gitlawb-node/Cargo.tomlcrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/git/mod.rscrates/gitlawb-node/src/git/repo_store.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/storage/archive.rscrates/gitlawb-node/src/storage/fs.rscrates/gitlawb-node/src/storage/ipfs.rscrates/gitlawb-node/src/storage/mod.rscrates/gitlawb-node/src/storage/s3.rs
💤 Files with no reviewable changes (1)
- crates/gitlawb-node/src/git/mod.rs
|
please have a look into this @beardthelion |
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Reviewed the merged state against current main (clean merge), built it, ran the storage and repo_store unit tests, and traced the write and consistency paths. The BlobStore abstraction and the Tigris-to-archive migration are clean and complete, and the fs backend's atomic swap and per-method key validation hold up. My concerns are all in the push-latency changes: the write-back default has a data-loss path, and it leans on an advisory-lock pattern that isn't sound over a connection pool. Highest first.
Findings
-
[P1] Default
async_uploadto false; ack-before-upload can lose an acked push on a rolling deploy.crates/gitlawb-node/src/config.rs(GITLAWB_ASYNC_UPLOADisdefault_value_t = true),api/repos.rs:561. With it on,git_receive_packacks the client and uploads in a detached task. If the process stops in that window (a SIGTERM during a rolling deploy is the routine trigger, not just a crash), storage keeps the old archive. On restart the in-processversionscache is empty, so the write-pathsync_down_if_stale(require_fresh=true) finds no cached etag, downloads the stale remote, and overwrites the newer local copy atrepo_store.rs:102-113. Lazy migration does not save it:acquireonly uploads when storage has no object (:154-175), so an already-present stale archive is never re-synced. The "local copy survives; lazy migration re-syncs" comment does not hold. Default this to false, or add a durable local-newer marker plus a visible signal on upload failure and a shutdown drain before making it the default. -
[P2, pre-existing] The advisory lock is acquired and released on different pooled connections.
repo_store.rs:233locks viafetch_one(&self.pool);:260/:494unlock viaexecute(&self.pool). Session-scoped Postgres advisory locks only release on the connection that took them, and sqlx hands out an arbitrary pooled connection per query, so the unlock can land on a different session and silently no-op (the boolean result is not checked) while the lock persists. A later same-session acquire is re-entrant and succeeds spuriously; a different session blocks 60s then 500s. This is already on main, so the PR does not introduce it, but the write-back change leans on the lock being held through the upload and now runs the unlock from a detached task that is dropped on crash, so it depends on a guarantee that is not actually there. Worth fixing as part of leaning on it: hold one connection (pool.acquire()) for the lock's lifetime and run lock+unlock on it, or use a transaction-scopedpg_advisory_xact_lock. -
[P2] The S3 client sets no request timeout.
storage/s3.rs:31.S3BlobStore::newbuilds the client with no timeout config, and the AWS Rust SDK sets none by default, so a hung S3 endpoint blocks the calling task unbounded. Underasync_uploadthat task holds the advisory lock, so one stuck upload blocks every later push to the repo. The IPFS client already sets connect and total timeouts; S3 should match, viaaws_sdk_s3::config::timeout::TimeoutConfigon the client builder or atokio::time::timeoutwrapper. -
[P2] "Fail closed when the backend is unreachable" only covers missing config, not connectivity.
storage/mod.rs:69. The doc says a configured-but-unavailable backend returns Err, butS3BlobStore::newonly loads credentials and builds the client with no network call, so an unreachable bucket boots clean and fails on the first live request. Scope the comment to config validation, or add a cheap probe against the bucket at boot if real fail-closed is intended. -
[P2] The risky new paths have no tests.
compress_repo/decompress_repoare pure filesystem functions but there is no round-trip test and nothing covering the "corrupt archive leaves the original intact" atomicity claim, which is the whole download-durability story.sync_down_if_stale'srequire_fresh=true(the lost-update guard) and the etag-skip are also untested (for_testingforcesarchive=None). Both are cheap to add with the fs backend and a test constructor that accepts an archive.
P3s, briefly: IPFS list() doesn't reject traversal in its prefix the way the other four methods do (ipfs.rs:159), latent since it has no caller (a traversal-segment check that still allows the empty list-all prefix, or just drop list()); absent-vs-error detection keys on Kubo error-string substrings (ipfs.rs:68), and since Kubo's error Code is always 0 there's nothing structured to switch on, so a pinned comment is the realistic fix; IPFS put() returns Err after a successful write when the follow-up stat fails (ipfs.rs:104); and several doc comments still say "Tigris" after the rename (repo_store.rs:131-133,212,282, archive.rs:4).
The abstraction is the right call and the migration is complete. Once the async_upload default and the advisory-lock soundness are sorted I'm glad to take another pass. @kevincodex1
|
please check again @beardthelion |
Superseded: the five findings from this review are resolved on d35a1c8; re-reviewing the new head separately.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Thanks, this addresses the prior round cleanly. I re-reviewed the merged state on the new head: async_upload now defaults false, the advisory lock is acquired and released on a single pinned connection (sound on every exit path I traced), the S3 client has operation/attempt timeouts, ipfs put no longer fails on a post-write stat, and the new compress/decompress and sync_down_if_stale tests exercise real behavior. All five prior findings are resolved.
One regression the lock fix introduces, which is why I'm holding for another round.
Findings
-
[P2] Bound concurrent pushes per DID before pinning a pool connection per push.
git/repo_store.rs:236(thepool.acquire()now held for the whole write),server.rs:178(push routes).acquire_writeholds one connection from the shared pool across the write, the 60s lock-retry, and the upload, andgit_write_routeshas no per-DID rate limit (unlikecreation_routesatserver.rs:96). Because identities are permissionless, a single actor can fire enough concurrent or slow pushes to exhaust the default 10-connection pool (db/mod.rs:269, noPgPoolOptions), after which every DB-touching handler node-wide blocks onpool.acquire()for the ~30s acquire timeout and then fails. It's bounded (30s acquire timeout, 300s S3 op timeout) and self-recovers, but it's repeatable and reachable, and the previous connection-per-query design didn't have it. Bound concurrent writes per DID ongit_write_routes(therate_limit_by_didlayer creation already uses, or a semaphore), and/or give lock connections a dedicated, sized pool. Notpg_advisory_xact_lockhere: the lock has to outlive the transaction (it is held across the post-commit upload), so a transaction-scoped lock would release before the upload and reopen the stale-archive window the lock exists to close. -
[P3] Add
#[must_use]toRepoWriteGuard.git/repo_store.rs:456. The connection pinning is correct, but a guard dropped withoutrelease()returns its connection to the pool with the session advisory lock still held (sqlx does not reset session state on check-in), so it lingers until that backend connection is evicted. Every current caller releases on all paths, so this is not reachable today;#[must_use]is cheap insurance against a future caller that forgets.
The lock-soundness approach is right and the rest is solid. Once the push path is bounded I'll approve. @kevincodex1
|
@coderabbitai please do full review of this PR |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 8
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/repo_store.rs (1)
161-190: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftSerialize background archive uploads with the write lock.
Both lazy migration and
init()upload archives from background tasks without holding the repo advisory lock. A first push can write and upload a newer archive while one of these older background uploads is still compressing; if the backgroundputfinishes last, it overwrites the newer storage object. This is especially reachable for a newly created repo where the empty initial archive races the first push.Move these uploads behind the same advisory-lock path and re-check storage under the lock before
upload, or makeinit()upload synchronously before the repo is exposed.Also applies to: 316-329
🤖 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/repo_store.rs` around lines 161 - 190, The background archive uploads in the spawned async task lack serialization with the repo advisory lock, creating a race condition where an older background upload can overwrite a newer archive pushed by the user. Acquire the repo advisory lock before performing the archive.upload() call within the tokio::spawn task, re-check if the archive already exists in storage after acquiring the lock (to avoid redundant uploads), and only proceed with the upload if the lock is held and the storage check confirms it's needed. Apply this same fix to the other location mentioned at lines 316-329 where archive uploads occur outside the advisory lock path.
🤖 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/api/repos.rs`:
- Around line 556-560: The comment describing the write-back ack behavior
incorrectly claims that lazy migration provides recovery for the
crash-durability window when async uploads are deferred. This is inaccurate
because lazy migration does not reliably re-sync when an older archive already
exists in storage. Update the comment to accurately reflect the actual
durability tradeoff: if the node stops after sending the ack but before the
upload completes, storage remains stale and a restart may roll the local cache
back from storage. Remove the claim that lazy migration re-syncs and instead
describe the realistic recovery behavior and the latency-durability tradeoff
being made.
In `@crates/gitlawb-node/src/config.rs`:
- Around line 155-162: The `guard.release()` method returns unit type `()`
instead of a `Result`, causing upload errors to be silently logged and never
propagated to callers. This violates the durability guarantee of the strict path
when `async_upload=false`, where clients receive acknowledgment even though the
durable upload failed. Modify the `release()` method to return a `Result` type
so that errors can be properly propagated, then update all write endpoint call
sites (repos, pulls, issues) that call `guard.release().await` to handle and
propagate the `Result` in the strict path, ensuring the client only receives an
acknowledgment after confirmed successful durable upload.
In `@crates/gitlawb-node/src/git/repo_store.rs`:
- Around line 346-356: The archive.upload() error handling currently only logs
warnings without propagating the failure, causing synchronous write operations
to succeed even when durable storage updates fail. Modify the function
containing this match statement to return a Result type, and change the Err(e)
case to return an error instead of just logging it. For synchronous callers
(async_upload=false), let them handle the returned error to fail the request
appropriately. For asynchronous paths, spawn a background task that can still
log any returned errors. Apply these changes to all similar upload failure
handling patterns throughout the file, including the related code at lines
503-516.
- Around line 519-521: When the write fails and is skipped (in the else block
where the warn! log is), the cached etag for this repo must be invalidated to
prevent subsequent write operations from using stale cache information. Add a
call after the warn! statement to drop or clear the cached etag version
associated with self.repo_name, ensuring that the next write operation will
perform a fresh pre-write download and not operate on inconsistent local state.
In `@crates/gitlawb-node/src/storage/archive.rs`:
- Around line 180-190: The current implementation deletes the old repo before
attempting the rename, which means if the rename fails the old repo is lost. To
fix this, instead of immediately removing the old repo with remove_dir_all when
local_path exists, first move it to a temporary backup location using
std::fs::rename. Then attempt to rename the extracted tmp_dir into place. If the
rename succeeds, clean up the backup directory. If the rename fails (in the
error handling block), restore the original repo by renaming the backup back to
local_path before removing the failed tmp_dir, ensuring the old repo is
preserved on failure.
In `@crates/gitlawb-node/src/storage/fs.rs`:
- Around line 98-103: In the `head` method, replace the error detection logic
that uses `path.exists()` with explicit error kind checking by checking if
`e.kind() == std::io::ErrorKind::NotFound` instead, since `exists()` returns
false for both missing files and permission/IO errors, causing real errors to be
silently masked. Additionally, locate the code around line 127 that contains
`rd.flatten()` in what appears to be a directory listing operation and remove
the `flatten()` call to ensure per-entry errors are properly propagated with
context instead of being silently dropped, as dropping errors causes partial
results to be reported as success and misleads callers that depend on complete
directory listings.
In `@crates/gitlawb-node/src/storage/mod.rs`:
- Around line 47-50: The BlobStore trait methods `get` and `put` require
buffering entire objects as Bytes in memory, which causes out-of-memory issues
for large repository archives. Add streaming/spooling APIs to the BlobStore
trait (such as stream-based get and put methods) that can handle objects
incrementally without loading the full content into memory at once. Modify the
archive upload/download code paths to use these new streaming methods instead of
the current whole-object `get` and `put` contract, allowing large `.tar.zst`
files to be processed in chunks.
- Around line 148-152: The validate_key function is missing validation for
backslashes in blob keys, which could cause path confusion on Windows since blob
keys are documented as forward-slash-delimited. Add a check in the validation
logic that rejects any key containing a backslash character using the contains
method, similar to the existing null byte check. Include this validation
alongside the other blob key checks (the traversal segment check and the
absolute path/null byte check) to ensure backslashes are caught during input
validation rather than relying on downstream conversion like the list function
does.
---
Outside diff comments:
In `@crates/gitlawb-node/src/git/repo_store.rs`:
- Around line 161-190: The background archive uploads in the spawned async task
lack serialization with the repo advisory lock, creating a race condition where
an older background upload can overwrite a newer archive pushed by the user.
Acquire the repo advisory lock before performing the archive.upload() call
within the tokio::spawn task, re-check if the archive already exists in storage
after acquiring the lock (to avoid redundant uploads), and only proceed with the
upload if the lock is held and the storage check confirms it's needed. Apply
this same fix to the other location mentioned at lines 316-329 where archive
uploads occur outside the advisory lock path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 604e1276-dc51-4f03-a84b-b577d38cfffe
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
crates/gitlawb-node/Cargo.tomlcrates/gitlawb-node/src/api/issues.rscrates/gitlawb-node/src/api/pulls.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/git/mod.rscrates/gitlawb-node/src/git/repo_store.rscrates/gitlawb-node/src/git/tigris.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/state.rscrates/gitlawb-node/src/storage/archive.rscrates/gitlawb-node/src/storage/fs.rscrates/gitlawb-node/src/storage/ipfs.rscrates/gitlawb-node/src/storage/mod.rscrates/gitlawb-node/src/storage/s3.rs
💤 Files with no reviewable changes (3)
- crates/gitlawb-node/src/git/mod.rs
- crates/gitlawb-node/src/db/mod.rs
- crates/gitlawb-node/src/git/tigris.rs
| async fn get(&self, key: &str) -> Result<Option<Bytes>>; | ||
|
|
||
| /// Store an object, returning its metadata (including the new etag). | ||
| async fn put(&self, key: &str, body: Bytes) -> Result<ObjectMeta>; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Avoid making large repo archives a whole-memory BlobStore operation.
get/put require entire objects as Bytes, so repo archive upload/download paths must buffer the full .tar.zst in memory. Large repositories can OOM the node during push/fetch; consider streaming/spooling APIs for archive objects instead of a whole-object contract.
🤖 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/storage/mod.rs` around lines 47 - 50, The BlobStore
trait methods `get` and `put` require buffering entire objects as Bytes in
memory, which causes out-of-memory issues for large repository archives. Add
streaming/spooling APIs to the BlobStore trait (such as stream-based get and put
methods) that can handle objects incrementally without loading the full content
into memory at once. Modify the archive upload/download code paths to use these
new streaming methods instead of the current whole-object `get` and `put`
contract, allowing large `.tar.zst` files to be processed in chunks.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 1187-1197: In the error handler for the release_after_write call
(within the map_err block), add cleanup logic to remove the local fork directory
stored in disk_path before returning the AppError. This prevents orphan data
from accumulating when the durable upload fails, which can cause issues on retry
attempts due to the destination path already existing. The cleanup should happen
in the error path before constructing and returning the AppError with the
failure message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d006b80-0156-44ab-a8a3-778966abe6f7
📒 Files selected for processing (6)
crates/gitlawb-node/src/api/issues.rscrates/gitlawb-node/src/api/pulls.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/git/repo_store.rscrates/gitlawb-node/src/storage/archive.rscrates/gitlawb-node/src/storage/fs.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/gitlawb-node/src/storage/archive.rs
- crates/gitlawb-node/src/storage/fs.rs
- crates/gitlawb-node/src/git/repo_store.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
-
[P2] Clean up the local fork directory when fork creation fails after the clone
crates/gitlawb-node/src/api/repos.rs:1187-1218
Aftergit clone --mirrorsucceeds,release_after_writeis called without any cleanup on failure. If the durable upload fails, the handler returns an error but leaves the cloned mirror atdisk_path. A later fork with the same name (or a retry of this one) will then fail because the directory already exists. The same leak happens ifstate.db.create_repofails after the upload. Removedisk_pathbefore returning from either error path. CodeRabbit's latest review explicitly requested this cleanup. -
[P2] Don't block the Tokio runtime in
FsBlobStore::headand::put
crates/gitlawb-node/src/storage/fs.rs:39-51andcrates/gitlawb-node/src/storage/fs.rs:69-107
headandputboth callSelf::meta_of, which usesstd::fs::metadatadirectly inside an async trait method. That blocks the async executor thread while stat'ing local files. Forput, move the stat into the existingspawn_blockingclosure; forhead, usetokio::fs::metadata(or aspawn_blockingcall) instead of the synchronous helper. -
[P3] Advisory-lock acquisition timeout is shorter than the backend operation timeout
crates/gitlawb-node/src/git/repo_store.rs:234-250
acquire_writeretriespg_try_advisory_lockfor 60 seconds, but the S3 backend sets a 300-second operation timeout (crates/gitlawb-node/src/storage/s3.rs:44-47) and the IPFS backend sets the same 300-second total timeout (crates/gitlawb-node/src/storage/ipfs.rs:30-31). A single large-repo upload can therefore hold the advisory lock long enough for concurrent pushes to the same repo to be rejected after only 60 seconds. Align the lock-acquisition timeout with the configured storage operation timeout, or use a blocking advisory-lock call with a matching timeout.
Both findings from this review are resolved on the current head: advisory locks now use a dedicated bounded pool (no handler-pool starvation), and RepoWriteGuard is #[must_use]. Dismissing so the state matches the current head; jatmn's open findings still stand.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed the current head against merged-state. The three findings from the last round are genuinely addressed: the advisory-lock acquire is now bounded with an explicit timeout, archive temp dirs are cleaned on every error path, durability errors propagate to callers, and the #[must_use] guard plus isolated lock pool are all in place and behave as described. The blocking-stat item is mostly handled too (the multi-MB write moved into spawn_blocking); what's left is narrow and noted below. Owner gating on the four touched routes is intact and the blob-key/traversal defenses check out.
One thing should change before merge: the storage auto-detect quietly repurposes an existing IPFS pinning config as the repo-archive backend on upgrade. The rest are polish.
Findings
-
[P2] Require an explicit opt-in before selecting the IPFS repo-archive backend.
crates/gitlawb-node/src/storage/mod.rs:96— auto-detection falls through toipfswheneveripfs_apiis set and no s3/fs backend is configured. Butipfs_apialready exists on main and today only drives the per-object encrypted pinning path (sync.rs,encrypted_pin.rs,ipfs_pin.rs); repo storage on main is Tigris-or-local, never IPFS. So a node that setGITLAWB_IPFS_APIpurely for pinning, with local-only repo storage, silently starts routing whole-repo archives into IPFS MFS after this upgrade, with no operator action. Gate the ipfs backend behind an explicitGITLAWB_STORAGE_BACKEND=ipfsrather than auto-selecting it fromipfs_api, so the pinning config keeps its existing meaning. -
[P2] Don't strand the advisory lock when the write-back upload is cancelled.
crates/gitlawb-node/src/git/repo_store.rs:577and the spawn atcrates/gitlawb-node/src/api/repos.rs:560— inasync_uploadmoderelease()runs in a detached, untrackedtokio::spawn, and thepg_advisory_unlockis the last step of its linear control flow rather than aDrop. If the runtime shuts down (or the task is otherwise cancelled) mid-upload, thePoolConnectionreturns to the pool with the session lock still held, and subsequent pushes to that repo burn the full retry window before failing. The root weakness (unlock not tied toDrop) predates this PR, but the detached write-back makes it materially easier to hit.async_uploaddefaults off, so this isn't a default-path break. Direction: tie the unlock to a guard that runs on drop, and/or track in-flight write-backs and drain them on shutdown. Flagging the direction rather than asserting a fix, since an async unlock inDropis not a one-liner. -
[P3] Move the remaining
statoff the runtime in the fs backend.crates/gitlawb-node/src/storage/fs.rs:102(head) and:95(put's trailingmeta_of) still callstd::fs::metadatasynchronously on the async runtime, while the rest of the backend usesspawn_blocking/tokio::fs. A singlestatis cheap, so low impact, but it's the residual of the earlier blocking-IO item and worth finishing for consistency. -
[P3] Reconcile or sweep the
.bakorphan left by a crash mid-swap.crates/gitlawb-node/src/storage/archive.rs:185— the move-aside/rename swap is correct on its error path, but a hard kill between the backup rename and the new rename leaveslocal_pathabsent with an orphaned.bak-UUIDsibling. It self-heals on the nextacquire()(the swap is on the download path, so storage still holds the copy and re-downloads), so this is litter plus a narrow transient, not data loss. A startup sweep of stale.bak-/.tmp-extract.dirs would close it. -
[P3] Fold the duplicated advisory-lock retry loop into one helper.
crates/gitlawb-node/src/git/repo_store.rs:235and:351—acquire_write(60 attempts) andupload_under_lock(30) carry the samepg_try_advisory_lockloop verbatim; they differ only in attempt count and error text. Extract a private helper taking both as params.
Advisory, not blocking: the new fail-closed storage::build() branches, the release() upload-failure path, and the async_upload write-back have no test coverage; the IPFS backend has none at all and detects not-found by string-matching Kubo error bodies, which will misclassify silently if Kubo changes its wording.
The structural change is solid and the storage trait is the right shape. Once the IPFS auto-detect is gated I'm happy to take another pass.
|
@kevincodex1 this one's gone stale but the work is worth finishing. Nothing on main has replaced it, repo storage is still Tigris-only, and the BlobStore trait is the right shape. Two things stand between it and merge:
Do you want to rebase and take it the rest of the way, or shelve it? Your call either way. |
bd7c026 to
a7f12a1
Compare
|
@coderabbitai please do an intensive review on this |
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/api/repos.rs (1)
2644-2649: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "Tigris" reference in test assertion message.
Every other comment in this diff was renamed from Tigris to the generic "storage" terminology; this assertion string was missed.
- "receive-pack advertisement must be throttled before the Tigris acquire" + "receive-pack advertisement must be throttled before the storage acquire"🤖 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/api/repos.rs` around lines 2644 - 2649, Update the assertion message in the receive-pack throttling test to replace the stale “Tigris” terminology with the generic “storage” terminology, while preserving the existing status assertion and test behavior.
🧹 Nitpick comments (2)
crates/gitlawb-node/src/api/repos.rs (1)
1661-1664: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winOrphaned remote archive when
create_repofails after a successful upload.By this point
release_after_writehas already succeeded (the archive is durably uploaded);cleanup_local_forkonly removes the local clone, leavingrepos/v1/{forker_slug}/{fork_name}.tar.zstin storage with no DB row ever pointing at it.RepoArchive::delete()already exists (currently#[allow(dead_code)]) — exposing it throughRepoStoreand calling it here would close the leak.♻️ Proposed fix (needs a small `RepoStore` wrapper around `RepoArchive::delete`)
if let Err(e) = state.db.create_repo(&record).await { cleanup_local_fork(disk_path).await; + if let Err(cleanup_err) = state.repo_store.delete_archive(&forker_did, &fork_name).await { + tracing::warn!(fork = %fork_name, err = %cleanup_err, + "failed to remove orphaned storage archive after fork error"); + } return Err(e.into()); }🤖 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/api/repos.rs` around lines 1661 - 1664, Update the create_repo failure path around state.db.create_repo to delete the already-uploaded remote archive before cleaning up the local fork and returning the error. Expose RepoArchive::delete through the RepoStore API, then invoke that wrapper with the uploaded archive identifier/path in this branch, preserving the original database error while handling cleanup failures consistently with existing conventions.crates/gitlawb-node/src/main.rs (1)
266-298: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdvisory-lock pool size is a hardcoded constant, unlike every other tunable in this file.
Every other capacity knob here (
GITLAWB_PUSH_RATE_LIMIT,GITLAWB_CREATE_RATE_LIMIT,GITLAWB_SYNC_TRIGGER_RATE_LIMIT, etc.) is env-overridable.ADVISORY_LOCK_POOL_SIZEgates push concurrency directly — a push pins one connection for its whole lifetime — so operators can't tune it for their traffic without a rebuild/redeploy.♻️ Proposed fix
- const ADVISORY_LOCK_POOL_SIZE: u32 = 16; + let advisory_lock_pool_size: u32 = std::env::var("GITLAWB_ADVISORY_LOCK_POOL_SIZE") + .ok() + .and_then(|v| v.trim().parse().ok()) + .unwrap_or(16); let lock_pool = sqlx::postgres::PgPoolOptions::new() - .max_connections(ADVISORY_LOCK_POOL_SIZE) + .max_connections(advisory_lock_pool_size) .connect(&config.database_url)🤖 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/main.rs` around lines 266 - 298, Make the advisory-lock pool capacity configurable through the existing application configuration or environment-variable parsing instead of the hardcoded ADVISORY_LOCK_POOL_SIZE constant. Add a tunable setting with an appropriate default, then use that setting in PgPoolOptions::max_connections within the startup flow before constructing RepoStore, preserving the current default concurrency.
🤖 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/api/repos.rs`:
- Around line 2644-2649: Update the assertion message in the receive-pack
throttling test to replace the stale “Tigris” terminology with the generic
“storage” terminology, while preserving the existing status assertion and test
behavior.
---
Nitpick comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 1661-1664: Update the create_repo failure path around
state.db.create_repo to delete the already-uploaded remote archive before
cleaning up the local fork and returning the error. Expose RepoArchive::delete
through the RepoStore API, then invoke that wrapper with the uploaded archive
identifier/path in this branch, preserving the original database error while
handling cleanup failures consistently with existing conventions.
In `@crates/gitlawb-node/src/main.rs`:
- Around line 266-298: Make the advisory-lock pool capacity configurable through
the existing application configuration or environment-variable parsing instead
of the hardcoded ADVISORY_LOCK_POOL_SIZE constant. Add a tunable setting with an
appropriate default, then use that setting in PgPoolOptions::max_connections
within the startup flow before constructing RepoStore, preserving the current
default concurrency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68b283c4-1543-4bc5-9501-c19aea706914
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
crates/gitlawb-node/Cargo.tomlcrates/gitlawb-node/src/api/issues.rscrates/gitlawb-node/src/api/pulls.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/git/mod.rscrates/gitlawb-node/src/git/repo_store.rscrates/gitlawb-node/src/git/tigris.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/state.rscrates/gitlawb-node/src/storage/archive.rscrates/gitlawb-node/src/storage/fs.rscrates/gitlawb-node/src/storage/ipfs.rscrates/gitlawb-node/src/storage/mod.rscrates/gitlawb-node/src/storage/s3.rs
💤 Files with no reviewable changes (3)
- crates/gitlawb-node/src/git/mod.rs
- crates/gitlawb-node/src/git/tigris.rs
- crates/gitlawb-node/src/db/mod.rs
|
hey guys please review again @jatmn @beardthelion |
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed the moved head (d30af0d2) against merged state. The storage abstraction is well-built and the authz surface is untouched: owner-push, branch protection, quarantine, path-scoped read gates, and the git_upload_pack 404-deny all still sit above the new storage layer, and pool()'s removal has no stranded callers. One data-loss defect in the new write-back path blocks it, plus two should-fix issues. Findings below, highest first.
Findings
-
[P1] Don't let a failed write-back upload roll the local copy back over an acked push
crates/gitlawb-node/src/git/repo_store.rs:85
WithGITLAWB_ASYNC_UPLOAD=true,git_receive_packacks the client, thenguard.release(true)uploads in a detached task (api/repos.rs:962). If that upload fails (a transient S3/IPFS error, or the node restarts first),releaseinvalidates the in-memory etag and storage still holds the pre-push archive. The next access runssync_down_if_stale(require_fresh=true), whose logic assumes an etag mismatch means "remote is newer" and downloads it over local -- but here remote is older, so the acked commit is silently overwritten. The comment atapi/repos.rs:957("the local copy survives... until the next successful push re-uploads") is inverted: the next push pulls stale storage down before it can re-upload. Recovery needs a persisted local generation/marker so "local is ahead of storage" is distinguishable from "storage is ahead of local," rather than treating any mismatch as storage-wins. The default strict path is safe; this is the opt-in write-back path only. -
[P2] Bound the whole-archive in-memory buffering
crates/gitlawb-node/src/storage/mod.rs:47
get/putmove the entire.tar.zstthrough RAM as oneBytes, andcompress_repobuilds a fullVec<u8>, with no per-op size cap or concurrency guard. An unauthenticated cold-cache clone of a large public repo forces the whole archive resident, and N concurrent archive ops is N x archive-size toward OOM.GITLAWB_MAX_PACK_BYTESbounds the protocol body, not this buffer. The buffering carried over fromtigris.rs, but the fresh trait signature is where to fix it (stream get/put through a temp file) or to defer explicitly with a tracking issue rather than bake the limit in. -
[P2] Detach the lock connection when
pg_advisory_unlockitself fails
crates/gitlawb-node/src/git/repo_store.rs:517
On an unlock-query error,unlockonly logs and lets the pinned connection return to the pool, whileDropand the acquire poll-error path bothdetach()to close the socket and free a possibly-held lock. A transient error there repools a connection that may still hold the advisory lock, wedging that repo's writes until the pool recycles the connection (the lock pool sets nomax_lifetime). MirrorDrop:detach()the connection on unlock failure instead of repooling it. -
[P3] Clean up local/storage state on the two orphan paths
crates/gitlawb-node/src/git/repo_store.rs:246
initleaves the bare repo dir on disk when its upload fails, so a retry of the same name hits an existing destination; andfork_repo(api/repos.rs:1640) removes the local mirror on a post-upload DB failure but leaves the already-uploaded archive orphaned in storage with no owning row. Neither is data loss, but both leave litter that can block or confuse a retry. -
[P3] Add the new storage knobs to
.env.exampleand the README config table
crates/gitlawb-node/src/config.rs:143
GITLAWB_STORAGE_BACKEND,GITLAWB_S3_BUCKET/_ENDPOINT/_FORCE_PATH_STYLE,GITLAWB_STORAGE_FS_DIR, andGITLAWB_ASYNC_UPLOADare new operator-facing settings but appear in neither.env.examplenor the README env table (which still lists onlyGITLAWB_TIGRIS_BUCKET). The clap--helpdocs are good; this is just the operator-doc surface the repo already maintains. -
[P3] Minor cleanups
crates/gitlawb-node/src/storage/mod.rs:65
list()ships fully implemented across all four backends but is#[allow(dead_code)]with no production caller (only tests) -- wire a consumer or drop it until one exists. Two receive-pack test strings still say "Tigris" after the rename (api/repos.rs:2648). And the PR body's "cross-node consistency holds" overstates it: the advisory lock serializes writers only, so another node can still serve a stale archive mid-upload -- worth narrowing the claim.
Core design is sound and the gates are clean; the P1 is contained to the opt-in write-back path and fixable without a redesign.
Address beardthelion + CodeRabbit round on d30af0d: - [P1] Persisted pending-upload marker: written before every post-write upload, cleared only on success. sync_down_if_stale now distinguishes "local is ahead of storage" (failed/interrupted upload — serve local, never download) from "storage is ahead" (download), so a failed write-back upload can no longer roll a stale archive over an acked push. Stale markers without a local copy are dropped; the cache-miss download path clears leftover markers. Two regression tests. - [P2] Detach and close the lock connection when pg_advisory_unlock fails, mirroring Drop — never repool a possibly-locked session. - [P3] init() removes the local repo dir when its upload fails; fork deletes the already-uploaded archive when create_repo fails (new RepoStore::delete_archive). - Error precedence in git_receive_pack: a failed push now reports the git failure even when the lock release also errors. - Advisory-lock pool: acquire_timeout raised to match the 300s lock wait; pool size now env-tunable (GITLAWB_ADVISORY_LOCK_POOL_SIZE). - Dropped list() from the BlobStore trait until a production consumer exists (kept as a test-only helper on the fs backend). - Documented all storage knobs in .env.example and the README env table; fixed the two remaining "Tigris" strings; corrected the async_upload docs to describe the marker semantics.
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] Persist the write-back marker before acknowledging the push
crates/gitlawb-node/src/api/repos.rs:949
tokio::spawnreturns beforeRepoWriteGuard::release()is first polled, but that method is where the pending-upload marker is written. If the process stops in the interval after the successful receive-pack response is sent and before the task runs, the acknowledged local commit has neither an uploaded archive nor a marker. On restartsync_down_if_staletreats the old archive as newer and replaces the local repository with it. Create the durable intent marker before spawning/acknowledging (and retain it if the task is cancelled), then add coverage for that shutdown window. -
[P1] Do not let a pending-upload marker bypass every later freshness check
crates/gitlawb-node/src/git/repo_store.rs:96
After a failed upload, node A retains this marker and returns its local copy before checking storage. Node B can subsequently acquire the advisory lock, upload a newer archive, and release it; A's next write still skips the download and overwrites B's archive with A's divergent copy. The marker needs to record/reconcile the version it was based on, or otherwise retry safely under the lock, rather than making local state authoritative indefinitely. -
[P1] Fail lazy migration when the storage existence check is unknown
crates/gitlawb-node/src/git/repo_store.rs:367
archive.exists(...).await.unwrap_or(false)converts a failed HEAD request into "archive absent" and uploads the node's local cache. A restarted node can therefore overwrite a newer shared S3/IPFS/FS archive whenever HEAD transiently fails but PUT succeeds; the advisory lock only serializes writers and cannot establish freshness. Propagate or retry the HEAD error and upload only after a successful absence check. -
[P1] Scope fork cleanup to the attempt that created the archive
crates/gitlawb-node/src/api/repos.rs:1667
Two same-owner/name fork requests on different nodes can both pass the preflight lookup and upload to the same archive key. One DB insert wins; the loser unconditionally removes both itsdisk_pathand that shared archive after its uniqueness error, deleting the winner's durable repository and leaving its live DB row unable to hydrate on another node. The same danger applies to an ambiguous DB error after a committed insert. Serialize/claim creation and never delete a key that may belong to an existing repo. -
[P1] Do not report irreversible Git mutations as failed requests
crates/gitlawb-node/src/api/issues.rs:71
create_issue,close_issue, andmerge_prnow return a 5xx when the post-mutation archive upload fails, even though the Git mutation is retained locally behind the pending marker. A caller that retries issue creation receives a new UUID and later publishes both the supposedly failed issue and the retry; for a merge, the target ref is already merged while the PR database status remains open. Use a recoverable accepted-operation state/compensation, or make the mutation and durability outcome externally idempotent instead of returning an ordinary request failure after commit. -
[P2] Use a collision-resistant filesystem archive revision
crates/gitlawb-node/src/storage/fs.rs:39
The fs backend callssize-mtimean etag, and RepoStore treats equality as proof that its local copy is current. The configured fs backend explicitly supports mounted storage, where timestamp precision is not guaranteed to be nanoseconds: two different same-size archive writes in one timestamp tick can have the same token. A stale node then skips the required download under its advisory lock and uploads over the newer archive. Use a content hash or reliably changing persisted generation, or disable the etag fast path for this backend. -
[P2] Clean up a partially created mirror when
git clone --mirrorfails
crates/gitlawb-node/src/api/repos.rs:1620
The new cleanup closure is defined only after a successful clone.git clonecan create its destination before returning a non-zero status (for example during a fetch/write failure), and this branch returns without removing it. A later fork retry then fails because the destination exists. Define and invoke the cleanup on the clone-failure path as well. -
[P2] Compensate failed repository creation after the synchronous archive upload
crates/gitlawb-node/src/api/repos.rs:230
RepoStore::initnow durably uploads the empty archive before thiscreate_repoinsert. If the insert loses a concurrent-create race or fails transiently, the handler returns with the newrepos/v1/...archive orphaned; the local bare directory also blocks a retry. The local-directory leak existed before this PR, but making the archive upload synchronous makes the durable orphan deterministic. Give create-repo the fork flow's compensation, without deleting an already-successful concurrent creation.
… sidecar Address jatmn review round on 8c4ec8e: - [P1] Write the pending-upload marker BEFORE acking a write-back push (RepoWriteGuard::mark_pending, called pre-spawn): a process stop right after the ack can no longer leave the acked commit unprotected. - [P1] The marker now records the storage etag the local write was based on. sync_down_if_stale compares it against the current remote etag: unchanged = local strictly ahead (serve local); advanced = genuine cross-node divergence — the write path fails closed with an explicit reconciliation error instead of silently overwriting either side, and the read path serves local with a warning. - [P1] Lazy migration propagates a failed storage existence check instead of treating it as "absent" and overwriting a possibly-newer shared archive; the migration retries on a later access. - [P1] Creation flows are claim-first: create_repo and fork_repo insert the DB row (the claim on owner/name) before creating anything durable, and compensate failures by deleting only their own row (by generated id) and their own local dir. The fork path no longer deletes a storage archive at all — a racing winner's archive can never be removed. - [P1] create_issue/close_issue/merge_pr no longer fail the request when the durable upload fails after a committed git mutation: the mutation is locally durable and marker-protected, and failing forced callers into non-idempotent retries (duplicate issue UUIDs, merged-but-open PRs). The failure is logged; storage re-syncs on the next upload. - [P2] fs backend etag is now a per-write UUID persisted in a .etag sidecar (legacy size-mtime fallback for pre-existing objects): two same-size writes in one coarse-timestamp tick can no longer alias, so the skip-redundant-download fast path is sound on mounted filesystems. - [P2] fork cleans up a partially-created mirror when git clone itself fails; init() cleans its dir on init_bare failure too. 524 tests (3 new: divergence detection, sidecar etag rotation, marker base-version protection).
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed the merged state at 75060ea. The two new commits genuinely close both prior rounds: the marker is written before the ack, the versioned base distinguishes local-ahead from cross-node divergence and fails the write path closed, exists() errors propagate instead of reading as "absent", create/fork are claim-first with compensation keyed strictly by their own row id (and undo_claim never touches a storage key, so a racing winner's archive is safe under any topology), unlock failure now detaches the pinned connection, the fs etag is a per-write sidecar UUID, and the operator docs cover every new knob. I built and checked the merged state and traced each of those to the fixing lines. Two new problems in the marker machinery itself keep this at request-changes; both are contained and the fixes are small.
Findings
-
[P1] Preserve the pending marker's base etag instead of overwriting it on re-mark
crates/gitlawb-node/src/git/repo_store.rs:764
Every upload-failure arm removes the repo'sversionsentry (repo_store.rs:364, 720, 733), and the serve-local-under-marker branch ofsync_down_if_stale(96-150) never repopulates it, so the next write re-marks the marker with an empty base. I reproduced this with a throwaway test: fail one upload (marker holds the true base), write again with the upload failing too, and the marker now contains "" while storage never moved; the third write then dies on the "storage advanced ... reconcile manually" bail, and since issues, closes, and merges all go throughacquire_write, the repo's whole write surface is wedged until someone hand-deletes the marker file. Two consecutive upload failures is just "storage was down for a minute spanning two writes", and this hits the strict path as well as async_upload. Direction: havemark_pending_uploadreturn early when a marker already exists. The first marker's base is by construction the last storage etag this node confirmed, and it stays correct for every write stacked on the same undiverged local copy, so divergence detection is unharmed. Add the two-consecutive-failures regression test; it goes red on the current code. -
[P1] Run the fork's post-clone upload under the advisory lock
crates/gitlawb-node/src/api/repos.rs:1694
release_after_write(repo_store.rs:339) callsarchive.uploadwith noLockedConn, and claim-first ordering makes the fork visible togit_receive_packbefore the upload finishes. A push to the fresh fork uploads under the lock; the fork's unlocked upload can compress the pre-push (or mid-receive, torn) tree and PUT after it. The push's upload succeeded, so no marker protects it: the next require_fresh sync sees the fork's etag, downloads, and rolls back an acked push. This is a race, not a certainty (fork must compress before the push lands and PUT after), but it is exactly the windowupload_under_lock's own comment describes, andinit()already routes through it. Direction: acquire the lock insiderelease_after_write(or callupload_under_lockwithskip_if_exists=false). -
[P2] Make the marker wiring's tests exercise the production writers
crates/gitlawb-node/src/git/repo_store.rs:1037
All three new marker tests plant the marker by callingmark_pending_uploaddirectly; nothing drivesguard.mark_pending()(repos.rs:976) orrelease()'s own marker writes, so deleting the load-bearing before-ack line leaves the suite green. jatmn's round explicitly asked for coverage of the ack-to-first-poll window and it is still uncovered. Add a test that acquires a guard over the fs-backed store, callsmark_pending(), drops the guard without release (the crash), and asserts the next sync serves local; plus one assertingrelease(true)with a failing archive leaves the marker. -
[P2] Fall back to strict upload-before-ack when the marker write itself fails
crates/gitlawb-node/src/git/repo_store.rs:764
mark_pending_uploadwarns and continues on a write error, and repos.rs:976 acks unconditionally, so the "marker must be on disk BEFORE the ack" comment is not enforced: a full disk at the wrong moment silently reopens the crash window this mechanism exists to close. Direction: return Result frommark_pendingand take the synchronous release path when it fails. -
[P2] Write the fs etag sidecar before renaming the blob into place
crates/gitlawb-node/src/storage/fs.rs:97
The blob is renamed into place first and the sidecar written after, so a crash between them leaves new content under the old etag. Etag-equality consumers then skip the download and serve stale, which is the same failure the sidecar was built to remove, relocated into a crash window. Writing the sidecar (its own tmp+rename) before the blob rename inverts the failure into a harmless redundant download. -
[P2] Use try_exists() for the pending marker probe
crates/gitlawb-node/src/git/repo_store.rs:97
Path::exists()returns false on EACCES/EIO, not just NotFound, so a transient stat error skips the whole marker branch and the write path falls through to etag-compare-and-download, rolling back the pending local write. This delta fixed the identical two-valued collapse forarchive.exists()inupload_under_lock; the same treatment belongs here, with Err reading as marker-present on the write path. -
[P2] Clear the stale marker on repo recreation
crates/gitlawb-node/src/api/repos.rs:1650
undo_claimremoves the clone dir and the row but not the sibling marker that the failedrelease_after_writejust wrote. Recreating the same owner/name re-creates the local dir, and once the new repo's archive exists, the leftover empty-base marker trips the divergence bail and wedges the recreated repo's writes. Clearing any pre-existing marker ininit()afterinit_barecovers every recreate path; removing it inundo_claimtoo keeps the litter down. -
[P2] Close out the whole-archive buffering thread explicitly
crates/gitlawb-node/src/storage/mod.rs:47
get/putstill move whole archives through RAM andcompress_repostill builds a full Vec; unchanged at this head and the thread is still unresolved. Streaming is a real design change and deferring it past this PR is a reasonable call, but make it explicit: a tracking issue referenced from the PR, and resolve the thread. -
[P2] Add a bounded re-upload retry behind the 2xx-on-failed-upload paths
crates/gitlawb-node/src/api/issues.rs:82
Returning 2xx when the git mutation committed but the upload failed is the right call and I consider that settled: a 5xx invites retries that mint duplicate issue UUIDs or contradict an already-merged ref, and the marker keeps the local copy authoritative. What is missing is the second half: nothing ever retries the upload, so a repo that gets no further writes stays divergent indefinitely, visible only as one log line at failure time. A bounded background retry (or a startup/periodic sweep of pending markers) plus a counter for outstanding markers closes the loop. -
[P2] Add a failing-store test double for the new error branches
crates/gitlawb-node/src/git/repo_store.rs:394
No mock or failing BlobStore exists in the crate, so thehead_etagErr branches (marker and main flow, both require_fresh values),upload_under_lock's exists-Err propagation (the fix for theunwrap_or(false)finding; reverting it stays green today), andinit's cleanup-on-failed-upload are all structurally untestable. One small injectable failing store unlocks all of them. -
[P3] Skip the marker when no storage backend is configured:
mark_pending(repo_store.rs:672) writes markers even witharchive: None; they are inert until a backend with pre-existing archives is enabled, then wedge those repos. Early-return when there is no archive. -
[P3] Harden the fs etag test against revert: back-to-back writes get distinct mtimes, so
assert_neon consecutive put etags also passes with the legacy size-mtime etag. Also assert the sidecar file's content equals the returned etag. -
[P3] Fix the claim-first comments' scope: with per-node Postgres (three fly apps, per-node DATABASE_URL), "a concurrent same-name create loses at the insert" holds within one node only; the cross-node safety argument is that compensation never deletes a storage key. Worth one comment tweak so the next reader builds the right model.
Two candidates from this pass were checked and dropped: init's unconditional upload cannot clobber a racing first push (upload_under_lock compresses under the lock, so it snapshots post-push state), and the disk_path-UNIQUE-as-claim concern does not apply given per-node databases.
The remaining fixes are small and local; with the two P1s and the wiring tests in place this is close.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
P1: Do not acknowledge write-back pushes when the pending marker cannot be persisted
mark_pending awaits a method that suppresses its std::fs::write error (crates/gitlawb-node/src/git/repo_store.rs:673-677, 764-773). In the current git_receive_pack flow (crates/gitlawb-node/src/api/repos.rs:318-343), an asynchronous upload can therefore be acknowledged before the node stops or the upload fails, without a pending marker surviving. A later normal freshness download can then overwrite the acknowledged local repository. Make marker persistence fallible and atomic, and do not acknowledge the push or spawn its release unless that persistence succeeds.
P1: Serialize the fork archive publication with writes to the new fork
The database row makes a fork addressable before its clone and archive publication complete (crates/gitlawb-node/src/api/repos.rs:1624-1701). release_after_write calls archive.upload without the per-repository lock (crates/gitlawb-node/src/git/repo_store.rs:335-367), while normal writers use acquire_write. A concurrent push can upload a newer snapshot and then have the delayed fork snapshot overwrite it in the remote archive. Hold the same write lock across publication, including the necessary clone phase, or route fork publication through the locked write protocol.
P2: Reject a zero-sized advisory-lock pool at configuration parsing
The unrestricted u32 configuration value reaches max_connections (crates/gitlawb-node/src/config.rs:174-179, main.rs:284-291). A value of zero permits startup, but every acquire_write waits the configured 300 seconds and then fails, effectively disabling writes. Require a Clap range of 1.. and add a parsing test.
…gence checks, stable lock hash Address beardthelion + jatmn rounds on 75060ea/f154768, plus findings from an internal adversarial review pass: Reviewer findings: - Marker persistence is fallible and atomic (tmp+rename); the write-back path only acks after the marker is durably on disk, else it falls back to strict upload-before-ack. An existing marker's base is preserved on re-mark so consecutive upload failures can't corrupt it into a false divergence (regression test per the reported repro). - release_after_write (fork publication) now uploads under the per-repo advisory lock, closing the torn-snapshot race against pushes to a freshly addressable fork; on divergence it refuses and leaves the local copy marked instead of clobbering the concurrent writer. - Startup sweep (retry_pending_uploads) re-uploads marked repos and clears markers; diverged repos stay marked and are counted in a new gitlawb_pending_upload_markers gauge. - try_exists for the marker probe (EIO must not read as "no marker"); markers skipped entirely in local-only mode; stale markers cleared on repo recreation (init) and in fork's undo_claim. - fs backend: etag sidecar published before the blob rename; sidecar content asserted in tests; GITLAWB_ADVISORY_LOCK_POOL_SIZE rejects 0 at parse; claim-first comments scoped to per-node databases. - New FlakyStore failing-backend double + sqlx-backed guard tests cover the ack window, consecutive failures, failed existence checks, init cleanup, and head-failure branches (531 tests). Internal review findings: - retry/fork upload decisions now happen UNDER the advisory lock (upload_locked_with_marker): an unlocked base check could pass, block on a concurrent push's lock, then clobber that push's fresh upload. - The marker base used for the locked check is read from the marker itself, not the versions cache — the cache is invalidated by upload failures and lost to restarts. - Marker cleared before the lock releases, and its base is atomically refreshed to the new etag before unlink, so a crash between upload and clear self-heals instead of wedging on spurious divergence. - Lazy migration skips marked repos instead of stranding their markers. - advisory_lock_key: DefaultHasher (unstable across Rust releases) replaced with a SHA-256 prefix — mixed-toolchain rolling deploys can no longer silently lose cross-machine mutual exclusion. - fs put serializes the sidecar+blob publish pair per key so concurrent same-key puts can't pair one put's etag with another's content. - Known limitations documented in place: unlocked read-download swap (pre-existing) and the fork creation compound-failure window.
|
@coderabbitai make an intensive review on this |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/gitlawb-node/src/storage/mod.rs (1)
47-51: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftWhole-object
get/putstill buffers entire repo archives in memory.This was flagged before and remains unaddressed:
get/putrequire the full object asBytes, soarchive::RepoArchive::upload/download must hold the entire compressed.tar.zstin memory for every push/fetch. Large repositories risk OOM under concurrent pushes.🤖 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/storage/mod.rs` around lines 47 - 51, The storage trait’s get and put methods force whole-object buffering via Bytes, causing RepoArchive upload/download to hold full archives in memory. Replace the get/put payload contract with streaming I/O, then update archive::RepoArchive upload and download and each storage implementation to stream data incrementally while preserving missing-key and ObjectMeta behavior.
🧹 Nitpick comments (3)
crates/gitlawb-node/src/metrics.rs (1)
282-288: 📐 Maintainability & Code Quality | 🔵 TrivialKeep
gitlawb_pending_upload_markersupdated on marker writes/clears
The startup sweep sets this gauge once, but the runtime paths that create and clear pending-upload markers don’t touch it, so the reported count can drift from the actual number of repos ahead of storage. Update it where markers change, or recompute it on each sweep.🤖 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/metrics.rs` around lines 282 - 288, Update the runtime code paths that create and clear pending-upload markers so they also refresh gitlawb_pending_upload_markers through set_pending_upload_markers, keeping the gauge synchronized with the current marker count. Locate the marker write and removal logic and preserve the existing startup sweep behavior.crates/gitlawb-node/src/git/repo_store.rs (1)
717-800: 🧹 Nitpick | 🔵 TrivialOperational note: size
lock_pooland observe wait time against the 300s poll window.Each writer pins one
lock_poolconnection for the whole acquire→upload lifetime, and a contended waiter can poll for up toLOCK_ACQUIRE_TIMEOUT_SECS(300s). Enough concurrent slow pushes to the same or different repos can saturate the boundedlock_pool, at which pointpool.acquire()itself starts blocking new writers. Consider a metric on lock-acquire latency /lock_poolutilization and sizing the pool for expected push concurrency.🤖 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/repo_store.rs` around lines 717 - 800, The lock pool requires operational sizing and visibility for pinned connections during advisory-lock acquisition and uploads. Add metrics for lock-acquire latency and lock_pool utilization, and review or adjust lock_pool capacity to support expected concurrent pushes without pool acquisition blocking; preserve the existing 300-second LOCK_ACQUIRE_TIMEOUT_SECS behavior.crates/gitlawb-node/src/storage/fs.rs (1)
52-67: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded per-key lock map grows for the life of the process.
LOCKSnever evicts entries — every distinct key everput()leaves a permanentArc<Mutex<()>>in the map. For a long-running node accumulating many distinct repo archive keys over its lifetime, this grows without bound. Consider lock striping (fixed array of N mutexes indexed byhash(key) % N) to bound memory while still serializing same-key publishes.🤖 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/storage/fs.rs` around lines 52 - 67, The put_publish_lock implementation retains one lock per path indefinitely, so replace the growing LOCKS HashMap with a fixed-size lock striping scheme. Hash the path deterministically, select a mutex from a static array using the hash modulo the stripe count, and preserve serialization for identical keys while bounding lock-map memory.
🤖 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/storage/mod.rs`:
- Around line 43-58: Add a public async list method to the BlobStore trait for
namespace iteration, matching the existing object metadata/result conventions.
Implement the method for every BlobStore backend, including fs.rs, s3.rs, and
ipfs.rs, and move or reuse the filesystem test helper so it is available in
production; ensure each implementation returns the expected objects without
changing existing get/put/head/delete behavior.
---
Duplicate comments:
In `@crates/gitlawb-node/src/storage/mod.rs`:
- Around line 47-51: The storage trait’s get and put methods force whole-object
buffering via Bytes, causing RepoArchive upload/download to hold full archives
in memory. Replace the get/put payload contract with streaming I/O, then update
archive::RepoArchive upload and download and each storage implementation to
stream data incrementally while preserving missing-key and ObjectMeta behavior.
---
Nitpick comments:
In `@crates/gitlawb-node/src/git/repo_store.rs`:
- Around line 717-800: The lock pool requires operational sizing and visibility
for pinned connections during advisory-lock acquisition and uploads. Add metrics
for lock-acquire latency and lock_pool utilization, and review or adjust
lock_pool capacity to support expected concurrent pushes without pool
acquisition blocking; preserve the existing 300-second LOCK_ACQUIRE_TIMEOUT_SECS
behavior.
In `@crates/gitlawb-node/src/metrics.rs`:
- Around line 282-288: Update the runtime code paths that create and clear
pending-upload markers so they also refresh gitlawb_pending_upload_markers
through set_pending_upload_markers, keeping the gauge synchronized with the
current marker count. Locate the marker write and removal logic and preserve the
existing startup sweep behavior.
In `@crates/gitlawb-node/src/storage/fs.rs`:
- Around line 52-67: The put_publish_lock implementation retains one lock per
path indefinitely, so replace the growing LOCKS HashMap with a fixed-size lock
striping scheme. Hash the path deterministically, select a mutex from a static
array using the hash modulo the stripe count, and preserve serialization for
identical keys while bounding lock-map memory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af781916-3dca-4f7a-b114-77319e01e6b4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.env.exampleREADME.mdcrates/gitlawb-node/Cargo.tomlcrates/gitlawb-node/src/api/issues.rscrates/gitlawb-node/src/api/pulls.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/git/mod.rscrates/gitlawb-node/src/git/repo_store.rscrates/gitlawb-node/src/git/tigris.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/metrics.rscrates/gitlawb-node/src/state.rscrates/gitlawb-node/src/storage/archive.rscrates/gitlawb-node/src/storage/fs.rscrates/gitlawb-node/src/storage/ipfs.rscrates/gitlawb-node/src/storage/mod.rscrates/gitlawb-node/src/storage/s3.rs
💤 Files with no reviewable changes (2)
- crates/gitlawb-node/src/git/mod.rs
- crates/gitlawb-node/src/git/tigris.rs
CodeRabbit round on 5605b9c: - gitlawb_pending_upload_markers is now delta-maintained by the marker lifecycle itself (+1 on create, -1 on remove), with the startup sweep seeding the absolute baseline — the gauge no longer drifts between sweeps. - FsBlobStore's put publish lock uses a fixed 64-stripe array instead of a per-key map that grew for the life of the process. Skipped with reason: restoring list() on the BlobStore trait — it was removed deliberately at maintainer request until a production consumer (GC/admin/migration) exists; the fs test helper covers the tests that need it. Whole-object buffering remains the tracked streaming follow-up.
|
@coderabbitai please do an intensive review again |
|
✅ Action performedFull review finished. |
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed the merged state at 3cbcb2b, including the gauge and stripe commit that landed mid-review. The points raised literally in the prior rounds are addressed: mark_pending propagates its write error and git_receive_pack falls back to strict upload-before-ack instead of acking (repos.rs:984-999), the zero-sized lock pool is rejected at parse by a guard I confirmed goes red when removed, and the marker gauge is now maintained by deltas at the create and clear sites rather than only at startup. Moving the divergence check under the advisory lock is the right shape. Two things still block: the test meant to prove the ack-window fix does not exercise it, and there is a crash window that permanently wedges a repo whose content already matches storage.
Findings
-
[P1] Clear the etag cache in the ack-window test so it exercises the marker
crates/gitlawb-node/src/git/repo_store.rs:1595
mark_pending_alone_protects_the_ack_windowstill passes with the entire body ofmark_pendingreplaced byOk(()). The test never clearsstore.versions, sosync_down_if_stalereturns on the etag-cache hit before it consults the marker, which a real restart would not have. Addingstore.versions.lock().await.clear();afterdrop(guard)makes it fail with the marker gutted and pass with it restored. I ran all three states, on this head. -
[P1] Do not treat a crash between a successful upload and the marker clear as divergence
crates/gitlawb-node/src/git/repo_store.rs:549
upload_locked_with_markeradvances storage, then rewrites and unlinks the marker. If the process dies in between, the marker still names the pre-upload base while local and storage are byte-identical, and every later freshness check reads remote != base as divergence. I reproduced it on this head: the startup sweep reports 0 reuploaded and 1 still pending, thenacquire_writefails with "advanced while local changes were pending upload ... reconcile manually" on a fully consistent repo. Recovery requires deleting the marker on the node's disk. The doc onclear_pending_upload_after_success(repo_store.rs:1016) covers the rewrite-to-unlink window but not the upload-to-rewrite one. Recording the intended next etag in the marker before the upload, so a marker whose in-flight value matches the current remote is recognised as this node's own completed upload, would close it. I have not compiled that, so treat it as a direction rather than a specified fix. -
[P2] Restore the previous etag when the blob rename fails
crates/gitlawb-node/src/storage/fs.rs:131
putpublishes the sidecar before the blob so a crash yields new-etag/old-content, which the comment argues is the safe direction. That held before; it does not now. Under the new rule a remote etag not matching the marker base is terminal divergence, so a failed blob rename leaves a new etag over stale content and hands the next write the same permanent wedge as the finding above. Reading the old sidecar inside the publish lock and restoring it on the error path keeps the claimed property. The stripe change does not affect this. -
[P2] The fork publication window is documented but still open
crates/gitlawb-node/src/api/repos.rs:1671
Routingrelease_after_writethrough the locked upload closes the literal complaint. The ask was to hold the lock across publication including the clone, and the row still makes the fork pushable before clone and upload finish with no lock held, as the comment itself states. The window is confined to the forker's own namespace, so I am fine deferring it, but it should be a tracked issue rather than a comment. -
[P3] Log the sweep's directory-read failures
crates/gitlawb-node/src/git/repo_store.rs:439
A failedread_dironrepos_dirreturns(0, 0)and a failed per-owner read continues silently, so a sweep that scanned nothing looks identical in the logs to a node with no pending markers. With the gauge now seeded from that same scan, a silent failure also publishes a zero.
Minor: .pending-upload.tmp-<uuid> files are unlinked only on the rename error path, so a crash mid-rename leaves litter nothing collects.
Checked and clean: the sweep's reconstruction of owner and repo from directory names cannot produce another tenant's storage key, since the on-disk slug is the same string local_path and the archive key are built from and the did:key alphabet excludes the substituted characters; the advisory lock key change has no persisted dependency on the old value and the 8-byte truncation is not a practical collision risk, though the one-deploy mixed-binary gap the comment notes is real; the guard in git_receive_pack releases exactly once on all four branches; the new gauge deltas are balanced, since the increment sits after the marker is actually created and both decrements are conditional on the unlink succeeding.
On the open bot thread asking to promote list onto BlobStore: nothing here needs it and implementing it across every backend is scope with no caller. I would close that as won't-do rather than grow the trait.
Superseded by the review on 3cbcb2b.
…ash wedge beardthelion round on 3cbcb2b: - [P1] The pending marker now records the upload's intended etag (the archive's content MD5, written just before the PUT — S3 single-part ETags and the fs backend's etags are exactly that value). A crash anywhere between the PUT landing and the marker clear is recognized on recovery as "storage holds this node's own completed upload": sync and the startup sweep adopt it and clear the marker instead of wedging a byte-identical repo behind "reconcile manually". On backends whose etags are not content MD5 (IPFS CIDs), the intent simply never matches and recovery keeps its fail-safe behavior. - [P1] The ack-window test now clears the in-memory etag cache after the simulated crash (a real restart would have lost it), so it goes red if mark_pending is gutted — previously the cache-hit skip masked the marker entirely. - [P2] fs backend etags are content MD5 (persisted in the sidecar, legacy size-mtime fallback unchanged): different content still always differs, identical content is now the same token, and the etag a PUT will produce is predictable. put() restores the previous sidecar when the blob rename fails, so a failed publish can't leave a new etag over old content — which the marker rules would read as terminal divergence. - [P3] The startup sweep logs read_dir failures loudly instead of reporting a clean zero (which also seeded the gauge), and collects marker-tmp litter left by a crash mid-rename. The fork publication window stays deferred as agreed, pending a tracked issue. New regression test simulates the post-PUT crash state and asserts heal-not-wedge; the consecutive-failures test now pins the marker BASE line (the in-flight line legitimately changes per attempt).
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] Make the filesystem blob publication recoverably atomic
crates/gitlawb-node/src/storage/fs.rs:128
The sidecar ETag is renamed into place before the matching blob. If the process dies between the two renames, storage contains the old archive bytes but the new archive MD5. A pending marker has already recorded that MD5, so recovery treats the stale bytes as this node's completed upload, clears the marker, and caches/serves the old repository as the new generation. Publish the bytes and generation as one recoverable unit (or ensure recovery can reject an ETag whose bytes do not match) before using the sidecar as the freshness authority. -
[P1] Do not base crash recovery on a client MD5 for arbitrary backends
crates/gitlawb-node/src/storage/archive.rs:62
The marker records the archive MD5 beforeput, then identifies a completed upload by comparing it with the backend ETag. That is not a valid contract for the advertised IPFS backend (which returns a CID), or for standard S3 configurations such as SSE-KMS, where an ETag is not the object MD5. A crash after a successful PUT but before marker removal is consequently classified as divergence, leaving the marker in place and blocking every later write even though local and remote contain the same archive. Persist and compare a backend-valid generation/content token, or provide a recovery path that validates the actual object rather than assuming its ETag. -
[P1] Fail the upload before PUT when in-flight provenance cannot be persisted
crates/gitlawb-node/src/git/repo_store.rs:1107
record_inflight_uploaddrops both write and rename failures, but its callers still execute the PUT. If that rewrite fails and the process stops after the PUT succeeds, only the old-base marker remains; recovery sees the new remote generation as external divergence and wedges the repo. The intent rewrite needs to return an error and prevent the PUT/early acknowledgement unless it has been atomically recorded. -
[P1] Do not acknowledge issue and merge mutations with no recoverable durable state
crates/gitlawb-node/src/api/issues.rs:73
RepoWriteGuard::releaselogs a marker-write failure and continues; if the subsequent archive upload also fails, it removes the version cache but there is no marker for startup or the next access to protect the local mutation. These handlers deliberately log and ignore that release error after the Git mutation, so a later fresh write downloads the stale archive over an acknowledged issue, close, or merge. Either propagate this combination as a failed request or retain durable retry/provenance state before reporting success. -
[P1] Keep a new repository non-addressable until creation/fork publication is committed
crates/gitlawb-node/src/api/repos.rs:210
Both creation paths insert a visible DB row before initialization/publication, then delete that row and its directory when their own later upload fails. A client can push through the visible claim in that window and successfully upload its update; the original create/fork request can then run its compensation and delete the pushed repository's local directory and DB row. The fork code even documents a narrower version of this race. Hold the repository lock across the claim-to-publication lifecycle or expose the row only after it can no longer be compensated away. -
[P1] Do not offer the IPFS MFS backend as shared archive storage
crates/gitlawb-node/src/storage/ipfs.rs:40
Every operation uses Kubo's local MFS path (/gitlawb/<key>). No shared mutable pointer, CID registry, IPNS publication, or peer lookup is written, so two nodes using separate Kubo daemons read and write different namespaces. A push accepted on one node is invisible to another, violating the cross-node archive/lock assumptions behind this feature. Either restrict this backend to a single shared Kubo/MFS instance or publish and resolve a shared versioned mapping. -
[P1] Preserve exclusion while migrating the advisory-lock key
crates/gitlawb-node/src/git/repo_store.rs:1155
Replacing the base branch'sDefaultHasherkey with a SHA-256 key means old and new nodes acquire different Postgres advisory locks for the same repository during the required rolling deployment. They can both sync, mutate, and PUT competing snapshots, so the later upload loses an acknowledged push. The comment acknowledges this gap but leaves it live. Acquire both generations of the lock (or otherwise perform a compatible lock-key migration) until no old nodes remain.
…dual lock keys jatmn round on 7e9fde0 (all seven findings): - Recovery validates heal candidates by CONTENT, not etag: it fetches the remote archive and compares its MD5 against the marker's recorded in-flight hash. This is backend-independent — correct under IPFS CIDs and SSE-KMS etags, and immune to the fs backend crashing between its sidecar and blob renames (stale bytes under a fresh etag now read as divergence, never as a completed upload). The fs backend's etag is the persisted content MD5, so a failed publish also restores the previous sidecar. - record_inflight_upload is fallible and aborts the PUT when the intent cannot be durably recorded — an upload must never outrun the marker that explains it. - Issue/close/merge handlers verify the pending marker actually exists before treating a failed release as recoverable; with no marker the request fails instead of acking an unprotected mutation. - Creation and fork publication run inside RepoStore::create_published, which holds the per-repo advisory lock for the whole claim-to- publication lifecycle: a push through the just-visible row serializes behind publication, so failure compensation can never destroy a concurrently accepted push. release_after_write is gone. - The ipfs backend is explicitly restricted to a single shared Kubo instance (MFS is daemon-local): startup warning + module/config/README docs state the deployment requirement. - Writers acquire BOTH the new SHA-256 advisory-lock key and the legacy DefaultHasher key (all-or-nothing per attempt, partial grabs released before retrying), so writer exclusion holds across a mixed-binary rolling deploy; the legacy key drops once the fleet is upgraded.
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] Publish filesystem archive bytes and their freshness token atomically
crates/gitlawb-node/src/storage/fs.rs:128
The filesystem backend publishes the new.etagsidecar before it renames the new archive body. A crash in that interval leaves the old bytes paired with the new MD5. Another node can download those old bytes and cache the new token; when the intended bytes are later retried, they have the same MD5, so that node will keep treating its stale copy as current and can overwrite the recovered push on its next write. Use a single atomically-published generation/manifest (or otherwise make body and version recover as one unit) rather than allowing the token to lead the content. -
[P1] Do not use per-node PostgreSQL locks as the cross-node write serializer
crates/gitlawb-node/src/git/repo_store.rs:347
The new comments explicitly describe Fly apps as having separate Postgres instances, but these advisory locks are acquired from each node's localDATABASE_URL. Two nodes sharing an archive backend can therefore both acquire their local lock, read the same archive, accept independent pushes, and PUT competing snapshots; the later upload silently loses the first acknowledged push. This needs a serializer/CAS at a shared boundary (or an enforced shared-Postgres deployment), not a session lock local to each node database. -
[P2] Only clean up a repository directory that this creation attempt created
crates/gitlawb-node/src/git/repo_store.rs:440
create_publishedremoveslocal_pathand its pending marker after every failed build/publish without establishing that this invocation created either. If a create/fork collides with a pre-existing local repo (for example a DB-missing legacy repo, or a shared-mount race),init_bare/git clone --mirrorfails and this error path recursively deletes that existing repository; it also removes its recovery marker if directory deletion fails. Track successful ownership of the destination and clean only a directory/marker created by this attempt. -
[P2] Restrict the startup orphan sweep to the exact temporary-directory format
crates/gitlawb-node/src/storage/archive.rs:178
The new startup cleanup recursively deletes any dot-prefixed directory containing.tmp-extract.or.bak-. Peer mirroring intentionally accepts names outside the API validator, so a legitimate mirrored repo such as.project.bak-2026is stored as.project.bak-2026.gitand matches this broad predicate. It will be deleted on every restart. Parse and require the precise generated.{repo}.git.(tmp-extract|bak)-{UUID}form before removing a directory.
Superseded: re-reviewed the current head 1d5b479.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed the merged state at 1d5b479a (clean merge, base current with main). CI is green across all 15 checks. The storage abstraction itself is good work and I want it in: the trait, the three backends, the archive layer and the key/traversal validation are all sound, and the authorization surface is untouched (no gate added, removed or reordered anywhere in the diff).
The problem is no longer any single defect. Eleven maintainer review rounds and fourteen commits in, the shape of the loop is the finding: the original commit was +1003, the rounds since have added +2457 on top of it, and repo_store.rs alone went from +186 in the original change to +1648 across the fixes. Three of the four issues raised in the latest round are defects in remedies from earlier rounds, not in the original design. The sidecar-before-blob ordering exists because I asked for it; the unconditional creation cleanup exists because jatmn asked for clone-failure cleanup; the startup sweep exists because I asked for one. Each round has been individually correct and the aggregate is still going backwards.
So I'm settling the scope rather than running another round. Details below, findings first.
Findings
-
[P1] Split the crash-recovery protocol out and land the storage layer on its own
crates/gitlawb-node/src/git/repo_store.rs:96
The BlobStore trait, the three backends,RepoArchiveand the config surface are ready. The pending-marker machinery (marker lifecycle, versioned base, in-flight provenance, content-validated recovery, startup sweep, dual lock keys) was designed incrementally in review comments, and it is where every remaining defect lives. Land this PR withasync_uploadfixed off and the strict upload-before-ack path only, and drop the marker protocol entirely from it. Then bring the write-back path back as its own change with a written design for the freshness contract, so we can review one protocol instead of six accumulated patches to one. That is a scope call, not a criticism of the fixes: the individual rounds landed what was asked for each time. -
[P1] Publish the filesystem archive body and its freshness token as one unit
crates/gitlawb-node/src/storage/fs.rs:140
Confirming jatmn's finding, and the deciding detail is on the reader side rather than the writer side. A crash between the two renames leaves new-token/old-bytes; a second node then takes the plain freshness path, reads the token atrepo_store.rs:187, downloads the old bytes, and caches that token atrepo_store.rs:217without ever validating content. When the intended bytes are retried they carry the same MD5, so that node's cached token still matches andrepo_store.rs:203keeps skipping the download indefinitely. The content validation added last round only protects the node holding the marker, which is not the node that loses the push. Reversing the two renames does not fix it either; the body and its token have to publish as one recoverable generation. -
[P2] Reserve the lazy-migration slot before spawning, not after it succeeds
crates/gitlawb-node/src/git/repo_store.rs:283
acquire()spawns a migration task whenever the repo is not in themigratedset, and the set is only populated in the success arm at:295. A repo whose upload keeps failing therefore spawns a fresh task on every read, and each task now takes the per-repo advisory lock from the bounded pool thatmain.rs:284sizes for pushes. Eight sequential reads produce seven upload attempts on a failing backend; I ran that. The reads are anonymous on a public repo (list_commitsinapi/repos.rs:360takesOption<Extension<AuthenticatedDid>>), so this hands an unauthenticated caller a lever on push availability that the base commit did not have, since background migration there took no lock and no dedicated pool. Claim the key under the same mutex guard as the check before spawning, and give the opportunistic path a short lock budget instead of the 300s write budget. -
[P2] Only remove a repository directory this attempt actually created
crates/gitlawb-node/src/git/repo_store.rs:440
init_barebails when the destination exists (git/store.rs:9), and the failure path then recursively deletes it. A create against a name whose directory is already on disk without a DB row (a legacy repo, or one whose row was compensated away) destroys that repository. I ran it:create_publishedon a pre-existinglegacy.gitreturns "initializing bare repo" and leaves the directory gone.clear_pending_uploadat:447also runs when the directory removal failed, so a partial failure takes the recovery marker with it. Capture whether the destination existed beforebuild, and gate both the removal and the marker clear on having created it. -
[P2] Match the exact generated swap-directory name before deleting anything
crates/gitlawb-node/src/storage/archive.rs:178
The predicate is a leading dot plus a substring, so it deletes any repo directory whose name happens to contain.bak-or.tmp-extract.. Peer mirroring builds its path straight from the peer-supplied string atsync.rs:239without going throughvalidate_repo_name, so a mirrored.project.bak-2026lands as a directory this sweep removes on every restart. I ran both cases against the real sweep:.project.bak-2026.gitand.notes.tmp-extract.draft.gitare each removed. The existing test only offers a liverepo.gitand a.keepfile as negatives, so neither shape is covered. Parse the generated form and require a trailing UUID rather than matching a substring. -
[P2] Take the publish lock before reading the etag being rolled back
crates/gitlawb-node/src/storage/fs.rs:125
prev_etagis captured at the top of the blocking task, but the stripe lock is not taken until:136. If another put to the same key completes in between and this put's blob rename then fails, the rollback at:152writes the pre-existing token back over the other put's content. That is the one direction the sidecar design exists to prevent: a token that says "unchanged" over content that changed. Move the capture and the rollback inside the guard. Narrow (it needs a rename failure racing a same-key put), and I confirmed the ordering by reading rather than by running it. -
[P3] Fix two claims in the PR description, and state the lock's precondition in the code
crates/gitlawb-node/src/git/repo_store.rs:348
The description still advertisesliston the trait; it was dropped and now survives only as a test helper on the fs backend. It also says the lock being held until the upload finishes means "cross-node consistency holds", which overstates it in the same wayacquire_write's doc comment does: the lock excludes writers that share one Postgres, and during the window other nodes still serve the stale archive, whichconfig.rs:171correctly documents. On jatmn's related point, machines of one Fly app share aDATABASE_URL, so they do exclude each other, and I could not find the code comment that finding cites about separate Postgres instances. The gap worth closing is documentary: say plainly that a fleet sharing one archive backend must share one database, and stop calling this cross-node consistency.
The whole-archive-in-memory thread from the earlier round is still open, and last round's recovery check made it heavier by fetching the entire remote object to hash it (repo_store.rs:254). If the scope split above happens that thread goes with the recovery protocol; if it doesn't, it needs an answer here.
|
Follow-up to my last round, settling something that spans this PR and #215 rather than sitting inside either. The advisory-lock layer belongs to #215. Both PRs replace Worth being explicit about one trap: do not rebase the dual-key transition mechanically. Its legacy half is main's The dedicated lock pool should move with it. To be clear about what that decision is and is not: it settles where the code lands, not whose mechanics are better. Yours are better in three places I want carried across rather than lost. Findings
Nothing else changes from the last round: the scope split and the four findings there still stand. |
3 participants
Replace the hardcoded Tigris client with a backend-agnostic object store.
BlobStore trait (get/put/head/delete/list) with backends:
RepoArchive composes a bare repo into one repos/v1/{slug}/{repo}.tar.zst object on top of any backend (tar+zstd moved here; git/tigris.rs removed). RepoStore rewired from Option to Option.
Config (backward compatible): GITLAWB_STORAGE_BACKEND, GITLAWB_S3_BUCKET, GITLAWB_S3_ENDPOINT, GITLAWB_S3_FORCE_PATH_STYLE, GITLAWB_STORAGE_FS_DIR, GITLAWB_ASYNC_UPLOAD. GITLAWB_TIGRIS_BUCKET kept as a legacy alias.
Push-latency quick wins:
cargo check clean; storage::fs + repo_store unit tests pass.
Closes #89
Summary by CodeRabbit
Release Notes