Repository navigation
feat(storage): storage-agnostic BlobStore layer + push-latency quick wins - #79
kevincodex1 wants to merge 16 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 |
|
✅ 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. |
…ver #173 lock/test seams)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // platforms refuse to rename onto a non-empty dir, hence the move-aside.) | ||
| let backup = if local_path.exists() { | ||
| let b = parent.join(format!(".{file_name}.bak-{}", uuid::Uuid::new_v4())); | ||
| std::fs::rename(local_path, &b).context("moving existing repo to backup")?; |
| } else { | ||
| None | ||
| }; | ||
| match std::fs::rename(&tmp_dir, local_path).context("swapping extracted repo into place") { |
| Err(e) => { | ||
| // Restore the previous copy so the repo isn't left missing. | ||
| if let Some(b) = backup { | ||
| let _ = std::fs::rename(&b, local_path); |
…4 unlock shape Two CI failures from the storage-abstraction merge, both in this layer. CodeQL (rust/path-injection). Every path derived from a repo path — the pending-upload marker, its rename-temp, and the swap phase's .bak- and .tmp-extract. work dirs — is rebuilt with join/with_file_name, and a rebuilt path is a new question no earlier validation answers. Add validated_sibling_path (single-segment name, containment under the repo parent, explicit Component::Normal walk) and validated_repo_path_in (the same walk for a repo path arriving as a parameter, applied where the filesystem work happens rather than trusted across a call boundary), then route every marker/backup/temp construction and the sweep's entry-derived repo paths through them before any fs call. pending_upload_marker is fallible as a result; the cleanup paths that cannot propagate treat a rejected path as "no marker", the same conservative answer they already give for an absent one. INV-22 F4 gate. The merge moved the advisory unlock from RepoWriteGuard::release into LockedConn::unlock and, in doing so, took the connection out of self BEFORE awaiting pg_advisory_unlock — the exact shape #174 F4 forbids, since a cancellation mid-await then leaves Drop with conn == None and strands the session lock. Restore the reviewed shape: unlock through the still-owned connection, dispose of it only when the unlock errors (#174 F3b), and set the new released flag only once the await resolves so the Drop backstop stays live until then. tests/inv22_gates.rs follows the code to its new home rather than being deleted with it; the invariant is unchanged.
| // it up front: if the rename of the new copy then fails, we restore the | ||
| // backup so `local_path` is never left without a valid repo. (Most | ||
| // platforms refuse to rename onto a non-empty dir, hence the move-aside.) | ||
| let backup = if local_path.exists() { |
The four lifecycle findings on this branch share one root cause: a resource
was tracked by a coarse state ("release started", "the path exists", "a row
with this name exists") when the safety question was about the IDENTITY and
the PUBLICATION STAGE of one specific write attempt.
This is the vocabulary that makes those questions answerable:
- `PublishAttemptId` — minted before the request is built, carried with the
bytes as object user metadata, read back off the store to decide whether
what is published is THIS attempt's work. That turns "did my request
succeed", which a lost response makes undecidable, into "are the published
bytes mine", which the store can answer.
- `PublishStage` / `PublishStageCell` — how far one attempt got, observable
from OUTSIDE the future doing the work. A cancelled handler never returns
an outcome, so the stage it had reached is the only thing that can classify
it.
- `UploadError` split by what a failure ENTITLES A CALLER TO DO rather than
by which HTTP status came back. Destructive compensation is licensed only
by `proves_not_published()`.
Deliberately free of any object-storage type. #79 deletes `git/tigris.rs` and
replaces it with a BlobStore layer; this module is what survives that swap,
with each backend supplying only its own error classifier.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Merge readiness
- [P1] Resolve the failing CodeQL check before merge
crates/gitlawb-node/src/storage/archive.rs:292
The current head's CodeQL check fails with nine new high-severity alerts in code changed by this PR. GitHub reports the PR as mergeable but blocked; rerun and resolve the alerts on the final head before merge.
Findings
-
[P1] Keep sidecar rollback inside the filesystem publish critical section
crates/gitlawb-node/src/storage/fs.rs:125
prev_etagis read before the stripedput_publish_lockis acquired. The lock is then dropped whenwrite_and_swapreturns, but the error handler restores that earlier snapshot afterward. A concrete interleaving is: writer A reads the old sidecar, publishes A's new sidecar, and fails renaming A's blob; writer B then takes the stripe and successfully publishes B's blob plus B's sidecar; finally A resumes its unlocked cleanup and overwrites or removes B's sidecar using A's stale snapshot.head()now reports an etag that does not describe B's bytes, andsync_down_if_stalecan mistake a stale cached etag for current storage, skip the download, and later upload its stale repository over B's archive.The root cause is treating rollback as outside the atomic publish transaction. Make the per-key critical section cover the prior-sidecar snapshot, sidecar/blob publication, and every rollback action; do not let a failed operation mutate sidecar state after another same-key operation has committed. Add a deterministic interleaving test where A fails after publishing its sidecar and B succeeds before A's cleanup, then assert that
head()reports B's etag and bytes. -
[P1] Retain the in-process repo lease through async write-back
crates/gitlawb-node/src/api/repos.rs:2363
The async path spawnsguard.release(true), which intentionally retains the PostgreSQL advisory lock until the storage upload finishes, then dropsleasebefore returning the response. A later push for that repo therefore passes the in-process lease, acquires the per-source and global write permits, and blocks inacquire_writebehind the background task's advisory lock. Enough requests from different source keys can occupy the shared write pool for the storage timeout window and shed unrelated repositories—the exact global-admission starvation that the lease-before-admission ordering at lines 2172–2181 is intended to avoid.The root cause is that the two serializers no longer cover the same write lifetime: the lease protects only the request until acknowledgement, while the advisory lock protects the still-running write-back operation. Transfer a clone/ownership of the existing lease into the write-back task and release it only after
guard.release(true)completes (including its error path), or restructure admission so a same-repo request cannot obtain shared write permits until it can actually proceed past the prior advisory lock. Preserve the opt-in early acknowledgement and the existing advisory-lock serialization; add a test with async upload stalled that proves a second same-repo push remains parked at the lease without reducing available global write permits.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed the merged state at 9ce694b0. This round is current main merged in plus one commit fixing the two CI failures that merge caused. Neither the scope call from my last round nor any of its seven findings is answered, and the CodeQL half of this round's own fix did not land: the check is still failing on this head, with nine open rust/path-injection alerts all stamped 9ce694b0. The F4 restore is right, and its gate's region markers each occur exactly once in the file, so it is scoped where the docstring says it is.
Findings
-
[P1] Land the storage layer without the crash-recovery protocol
crates/gitlawb-node/src/git/repo_store.rs:96
Repeating last round's call unchanged, because nothing here addresses it and the new defect below is again a defect in the marker machinery rather than in the abstraction. The BlobStore trait, the three backends,RepoArchiveand the config surface are ready. Ship them withasync_uploadfixed off and the strict upload-before-ack path, and drop the pending-marker protocol from this PR. Bring the write-back path back as its own change with a written freshness contract. Until that happens I am not going to approve this PR, so further rounds on the marker code inside it are not worth your time. -
[P2] Cover the four
decompress_reposinks CodeQL is still flagging, or dismiss the alerts with the reason written down
crates/gitlawb-node/src/storage/archive.rs:300
The new barriers cover the sibling paths (tmp_dirat:262, the backup at:296), but the three renames and thecreate_dir_allstill consumelocal_pathas it arrived, and that parameter is whatrust/path-injectiontraces. I checked the reachable callers (repo_store.rs:293and:399) and each passes a validated path, so I do not believe there is a live traversal. The problem is that the commit message asserts the component walk is a barrier "static analysers (CodeQLrust/path-injection) recognise", and the check disproves that on this head. Either re-establish the barrier onlocal_pathinsidedecompress_repothe waysync_down_if_staledoes, or dismiss the nine alerts with the caller argument attached. A required check that stays red is not a merge path. -
[P2] Reject a traversal component, not the
..substring, when deriving sibling paths
crates/gitlawb-node/src/git/repo_store.rs:1612
validate_repo_nameaccepts a trailing dot, so a repo nameda.lives ata..git, and every sibling this round derives from that directory (.a..git.pending-upload) contains..and is rejected byvalidated_sibling_path.sync_down_if_stalepropagates that with?at:174, and bothacquire_fresh(:424) andacquire_write(:481) run through it, so with storage enabled that repo is neither readable nor writable. I ran it:validate_repo_name("a.")returnsOk(())whilepending_upload_markeron the resulting directory returnsErr(sibling file name contains a parent-directory reference), withplain.gitas the passing control.POST /repos(api/repos.rs:238) and the fork route (:3118) both reject dots outright, so the reach is the mirror path:notify_syncgates onvalidate_repo_slug, which acceptsa., and a known peer can enqueue it. Check the derived name for aParentDircomponent, or normalise the directory name before building siblings from it.
Not asks, for the record. My own P2 from last round about the swap-directory sweep was wrong about reachability and I withdraw that half: I said a peer-mirrored .project.bak-2026 reaches the substring predicate at archive.rs:179 because sync.rs built the path without validating the name. The #272 fix arrived with this merge, sync.rs:231 validates the slug now, and validate_repo_name rejects a leading dot. No creation surface I can find admits one. The predicate is still looser than the names it generates, but that is hardening, not a live path. The rest of that round (filesystem publish atomicity, the migration slot reservation, the create-failure directory removal, the prev_etag capture ordering) is unchanged at this head.
Superseded by the round on 9ce694b, which restates the scope call and carries the still-open findings forward.
euxarist
left a comment
euxarist
left a comment
There was a problem hiding this comment.
The abstraction is the right idea and both maintainers agree on it; my concerns are the concrete blockers rather than the shape. Current head is CodeQL-blocked with nine rust/path-injection alerts in archive.rs, and I can confirm two P1 interleavings from the diff: (1) in the fs backend the sidecar etag is captured before put_publish_lock and the rollback runs after the guard drops, so a stale sidecar can overwrite a newer published state and sync_down_if_stale will then trust that etag to skip downloads; (2) the async receive-pack path drops the lease on the handler tail while the spawned release(true) still holds the advisory lock, so the lease and the lock no longer cover the same window. There is also a sibling-path bug: a repo named a. produces .a..pending-upload, which validated_sibling_path rejects, making the repo unreachable via the mirror path. Given the collisions with #215 (advisory-lock keying) and #285 (same upload sites), I think this should land after #285's disposition, with the marker/recovery protocol split out or given a written freshness contract.
…igpine#285) * fix(node): close the advisory-lock probe's session if it is dropped mid-acquire A cancelled .await does not cancel an already-sent SQL statement, so a pg_try_advisory_lock whose future is dropped still takes the lock server-side while the caller abandons the result, leaving nothing to release it. The connection then returns to the pool holding the lock and wedges that repo until sqlx recycles the session. Introduce LockProbe, which owns the connection across the in-flight try-lock and closes it in its own Drop if it is still held. close_on_drop is a one-way setter, so the arming lives in Drop rather than being set up front and cleared on success; disarming is Option::take, which is what into_conn does once an acquire is actually observed. This is now the only place that issues pg_try_advisory_lock. The committed gate drops a probe without taking its connection, which is the state a cancellation leaves behind, and polls a standalone observer until the lock frees. Deterministic on purpose: the timing sweep that found this window leaks roughly 1 in 600, which is not something a CI gate can rest on. Observed RED before this change with the lock still held for the full 10s window. Refs #279 * fix(node): add a dedicated, lazily-connected advisory-lock pool Pinning a connection for the lock's lifetime is only safe if those connections come from somewhere other than the pool serving ordinary request handlers, otherwise a push burst starves every other query. Add GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS (default 32) and a Db::lock_pool builder, with the sizing tradeoff documented on the field and in .env.example: every in-flight write pins one connection here, so the value is a hard ceiling on simultaneous writes node-wide. The pool connects lazily on purpose. The main pool must connect eagerly because it runs migrations, which is why it needs connect_db_with_retry's backoff and degraded-server handoff; that function is not a generic retry helper and the lock pool is built well after the db-ready handoff has already resolved. A lazy pool has no startup work, so it adds no new way for the process to fail to boot and needs no second copy of that machinery. If Postgres is unreachable when the first write arrives, that write fails on the pool's own acquire timeout, like any other database-backed request. Pure configuration, so no proof-first cycle: the knob is covered by a parse/default/reject-zero test. Db::lock_pool has no caller until the guard wiring lands, hence the temporary dead_code attribute. Refs #279 * fix(node): hold the lock-owning connection in RepoWriteGuard (#279) Postgres advisory locks are session-scoped: only the backend that took one can release it. acquire_write took the lock through fetch_one(&pool) and release unlocked through execute(&pool), two independent checkouts, so the unlock usually landed on a session that held nothing and returned false. Measured on main: two writers on one node and the same repo BOTH acquired, 50 of 50 sequential cycles leaked, and 100 writes left 100 orphaned advisory locks on the server. The guard now owns the PoolConnection that took the lock, drawn from the dedicated lock pool, and releases on that same session. The retry loop probes through LockProbe so a cancellation mid-acquire cannot strand the lock, and hands the connection back before each backoff so a spinner on a contended repo does not pin a slot while idle. Pool exhaustion is deliberately not retried. It is a different condition from lock contention, and retrying it would spend all 60 attempts on a capacity problem unrelated to this repo while reporting it as someone else holding the lock. Both #279 acceptance tests were observed RED first: the exclusion test admitted the second writer, and the leak test reported 1 lock held where 0 was required. Both GREEN after. Full crate suite 516 passed. Db::pool() is removed because this change was its only caller. Refs #279 * fix(node): free the advisory lock when a write guard dies without releasing A guard can exit without reaching release(): an early ? on the pre-write download, a panic, or an axum handler future cancelled when the client disconnects. Its session still holds the lock, so returning that connection to the pool would block every future write to the repo until sqlx recycles it, which on the 0.8.6 defaults is ten minutes idle or thirty minutes lifetime. Close the session instead and let Postgres free the lock at session end. One hazard found by writing the teardown test rather than by reasoning: PoolConnection::drop spawns onto the runtime for both closing and returning, and panics outright when no runtime handle exists. That panic would fire inside a Drop and abort the process during unwind. It is not introduced by this commit, it comes with owning a PoolConnection at all, but this is where it becomes reachable. So Drop checks for a runtime first and, with none, leaks the handle deliberately rather than panicking: the process is already exiting and socket teardown ends the session, which is what frees the lock at exit anyway. Observed RED before the fix, with the lock still held for the full 10s poll window against a standalone observer on a no-reap pool, and the teardown case panicking in sqlx-core connection.rs:208. Proven load-bearing after: neutering the close_on_drop call turns the drop test RED again. The must-not case (a released guard reuses its backend pid across four writes on a pool sized 1) passes in both states, so the signal is specific to the abandoned-guard path. Full crate suite 519 passed. Refs #279 * fix(node): read the advisory unlock's result instead of discarding it pg_advisory_unlock reports "you did not hold this lock" as a false RETURN VALUE plus a server WARNING, never an error, so let _ = execute(...) could not distinguish a real release from a no-op. Three blindnesses stacked in those four lines: execute discards the row, the boolean lives in the row, and let _ discarded the Result too. Read it through fetch_one into (bool,). A false means this session's lock state is not what we believe it is, so the connection stays in the guard for Drop to close rather than being handed back to the pool as clean. Only a confirmed unlock returns it. A query error gets the same treatment, since the lock must not outlive a session we can no longer reason about. Note this is the only unlock site: the pre-write download's error path returns through the guard, so Drop covers it and there is no second place to keep in sync. RED before: the connection came back on the same backend pid after an unlock that returned false. GREEN after, and proven load-bearing by treating false as success, which turns it RED again. The must-not case (a normal release still reuses its backend) passes in both states, so this does not over-close the happy path. Full crate suite 520 passed. Refs #279 * fix(node): bound the object-storage transfers that run under the write lock Two transfers happen while the per-repo advisory lock is held: the archive download inside acquire_write, which runs after the lock is taken and before the guard exists, and the upload inside release. Both were free before the guard pinned a lock-pool connection, because the lock's connection went back to the pool immediately. Now an unbounded stall holds a lock-pool slot for its whole duration, so enough concurrent stalls deny every write on the node with no reaping path. Add GITLAWB_LOCK_HELD_TRANSFER_TIMEOUT_SECS (default 300) and route both through it. A timed-out upload is UNKNOWABLE rather than failed, so the timeout arm takes no compensating action: the PUT may well have landed. The lock is released either way, which trades a narrow last-writer-wins window for not wedging the repo behind a stalled transfer. Deliberate, and stated here rather than discovered later. Note what is NOT bounded by this knob: acquire_fresh's download. It runs before any lock is taken, so it holds nothing. An earlier revision of this change put the bound there by mistake, because both functions contain an identical download call and the first match won. Two disclosures. Coverage: the committed tests cover the bound mechanism, not the wiring. Driving a genuinely stalled transfer through acquire_write needs either the object-store abstraction (out of scope) or a process-global AWS_ENDPOINT_URL_S3 mutation, which would make the suite order-dependent under the concurrent runner. That a stalled transfer is bounded is therefore verified by reading, not by execution. Behavior: on timeout with a local copy present, the download falls into the pre-existing self-healing fallback and the write proceeds against that local copy. This widens the conditions reaching that path from corrupt-or-unreachable to include merely-slow, so a stale tree could now be written and re-uploaded on a slow link. Kept consistent with the existing failed-download behavior rather than inventing new semantics here; changing it is its own decision. Full crate suite 522 passed. Refs #279 * fix(node): authorize before taking the write lock in close_issue close_issue acquired the per-repo advisory lock, then ran the owner-or-author check, then returned 403 with the lock still held. That was harmless only because the lock excluded nothing. Making it work, as the preceding commits do, turns this ordering into a denial primitive: any caller with repo read access could take the write lock on demand and be refused the write, while a legitimate writer burned its 60-attempt retry budget against a lock held by someone with no write authorization. On a public repo that is every permissionless identity. This series creates the exposure, so it closes it in the same series. The owner check is cheap and moves above the lock outright. The author fallback needs the issue's git-JSON blob, since there is no author column, so it now reads through acquire() rather than acquire_write(): an issue's author is set at creation and never changes, so reading it outside the lock races nothing, and only the mutation needs exclusion. Two deliberate behavior choices. A non-owner whose authorship cannot be established gets 403 rather than 404, so this route does not tell an unauthorized caller whether an issue exists. And the owner's existing 404-for-a-missing-issue path is preserved by re-reading under the guard, which the mutation wants anyway. Swept the other three acquire_write sites: merge_pr owner-gates at :200 before acquiring at :214, the repo write path checks did_matches at :916 before :933, and create_issue's read gate is legitimate because that caller IS authorized for the action it performs. close_issue was the only one with the wrong order. RED before: with an independent session holding the repo's lock, a stranger's request sat in the retry loop until the 3s deadline fired (Elapsed), which is the wedge itself. GREEN after, refused immediately. Proven load-bearing: disabling the pre-lock refusal restores the Elapsed. Full crate suite 523 passed. Refs #279 * fix(node): address the review findings on the advisory-lock series Seven fixes from a seven-reviewer pass. Eight of the nine substantive findings were defects introduced by this branch, and two contradicted claims made in its own earlier commit messages. P0, the worst of them. The under-lock download's timeout was folded into the same Err as a corrupt archive, so it fell through to the use-the-local-copy fallback and the write proceeded. Two ways that destroys data: we do not know whether we hold the latest tree, so re-uploading can overwrite another node's newer archive; and the abandoned download's extraction runs in an uncancellable spawn_blocking that ends in remove_dir_all + rename over the same path, so git would have been running against a directory a background task was about to delete. Now the Option is branched rather than collapsed: a genuine fetch error keeps the local-copy fallback, a timeout refuses the acquire and lets the guard's Drop free the lock. LockProbe closed its connection on ordinary contention, so a 60-attempt spinner tore down 60 backends -- the exact behavior an earlier commit claimed it avoided. The test written to guarantee that asserted the HOLDER's lock count, which cannot observe the probe's own connection, so it passed throughout. The correct predicate turned out to be narrower than the first attempt at this fix: not "we saw an answer" but "we positively know nothing was acquired," because a true answer means the lock IS held and dropping without handoff leaks it exactly as a cancellation would. The first attempt reopened U1's leak and U1's gate test caught it. tigris.exists() ran unbounded inside the lock-held span, so the knob's promise was not kept and the docs were wrong about the shape. The HEAD and the download now share one budget, so worst-case occupancy is one budget per span rather than two. close_issue's pre-lock author read used acquire(), whose fast path returns on a cached directory and never contacts object storage, so on a multi-node deploy an issue author would be refused their own issue. Now acquire_fresh, which refreshes without taking the lock -- the property the comment already claimed. That comment also asserted authorship is immutable, which a reviewer disproved by force-pushing a forged blob at refs/gitlawb/**; reworded to the real justification (a pre-check whose mutation re-reads under the guard). The underlying pushability is pre-existing and larger than this branch. The retry loop had no wall-clock cap, reaching ~360s against a 120s proxy idle timeout that was itself lowered from 600s after held connection slots caused a production outage. Capped at 90s, with the bail message naming the real bound. All four acquire_write call sites stringified their error, destroying the anyhow downcast that error.rs documents explicitly ("without this, every database outage surfaces as a 500 instead of a 503"). PoolTimedOut is in that 503 set, so a saturated lock pool told clients not to retry something transient. Now propagated. Tests: six that the plan required and the first pass never wrote. The R4 pool-isolation test (named in the plan as proving the pool split, entirely absent), close_issue's owner and non-owner-author twins (INV-21c, a two-principal gate with only its deny arm covered), a waiter-does-not-block- an-unrelated-repo case, and a config test for the transfer knob. The runtime-teardown test no longer returns green when DATABASE_URL is unset. Drop's no-runtime branch now detaches via leak() instead of mem::forget: PgConnection has no Drop impl, so the socket closes synchronously and the lock frees immediately rather than at process exit. Evidence. The probe predicate is proven load-bearing in BOTH directions: forcing always-close reddens the churn regression, forcing never-close reddens the cancellation gate, and only the correct predicate satisfies both. The author twin reddens when the fallback is removed. One honest limit: the author twin does NOT redden when acquire_fresh is reverted to acquire. With tigris disabled in tests the two calls are identical, so that fix is correct by reading, not by execution -- the same seam that leaves the transfer bound's wiring untested. Full crate suite 529 passed, clippy -D warnings clean. Refs #279 * fix(node): log lock-pool saturation in the request path, not via readiness Completes the shed-in-path half of the availability story. F7 already made a pool timeout a retryable 503 by letting the sqlx downcast through; this adds the operator-facing half, with the pool's own size/idle counters so an incident can distinguish "the pool is full" from "the database is gone" without reproducing it. Deliberately NOT a readiness probe. /ready gates Fly routing with no fail-open, so failing it on a saturated pool would pull this node's READS out of service too and push its write load onto peers carrying the same load. That is the downward spiral AWS's health-check guidance names and the SRE Book documents independently. A lock-pool readiness probe would also add nothing on the reachability axis, because both pools are built from the same database_url, so the existing app-pool ping already answers it. The error is still returned via .context() rather than replaced, because anyhow preserves downcastability through context layers and the 503 mapping depends on it. Cost is bounded by construction: one line per failed acquire, and a failed acquire has already paid a multi-second pool timeout. Refs #279 * fix(node): refuse a write when object storage is unknowable, and shed contention as 503 Four defects a seven-reviewer pass found in the previous two commits, three of them cases where one arm of a match contradicted its neighbour. The under-lock refresh read a failed Tigris HEAD as "no archive" (`exists().await.unwrap_or(false)`), skipped the refresh, and wrote against a possibly-stale tree, then re-uploaded over it. That is the outcome the timeout arm twenty lines below explicitly refuses. A failed HEAD and a failed download leave us knowing different things, so they no longer share a branch: only a download failure after a successful HEAD establishes that the local copy is a sound thing to fall back to and re-upload. A HEAD failure now refuses the write. Lock contention that ran out the acquire deadline surfaced as a 500 carrying the owner slug and repo name in the client-visible body, while pool exhaustion fifteen lines up returned a deliberate 503. Contention is transient and ordinary, so it now maps through a typed `RepoBusy` to a retryable 503 with a fixed body; the detail stays in the log at the raise site. `lock_not_taken` was assigned after the try-lock answered, so an error left a previous `true`-derived value standing and could return a lock-holding session to the pool. It is now cleared before the statement is sent. `leak()`'s no-panic property in the guard's no-runtime Drop branch depended on `min_connections == 0` without saying so; a future tuning change would have silently re-armed a panic inside Drop. Made explicit with the reason. Two tests that did not bind: - `waiter_on_one_repo_does_not_block_another` proved only that two lock keys do not collide. It now samples the pool counters across more than two backoff cycles. RED at 0/50 samples with `drop(probe)` moved after the sleep, the exact defect it names, which the previous version survived. - the exhaustion test's `Ok(Err(_)) | Err(_)` was satisfied by its own outer timeout. It now requires `PoolTimedOut` and asserts the slots are accounted for. RED with the pool sized N+1. `contended_acquire_sheds_as_repo_busy_not_internal_error` is new and drives the deadline path for real; the deadline became a field so it does not wait 90s. RED at 500-vs-503 with the downcast removed. The acquire deadline's docstring claimed a total under the 120s proxy idle timeout, which the 300s under-lock transfer bound in the same function contradicts. It bounds the wait only, and now says so. The backoff is clamped to the remaining budget. * fix(node): re-authorize close_issue under the guard instead of only re-checking existence The guarded re-read matched `Ok(Some(_))` and discarded the blob, so the authorization decision rested entirely on the pre-lock read. A comment above claimed the opposite. That is not a narrow window: `acquire_write` re-downloads the archive after locking, so the tree that gets mutated is routinely not the one the author was read from. With owner-push enforcement defaulting to false and branch protection covering only `refs/heads/*`, `refs/gitlawb/issues/*` is pushable, so a forged author blob landing between the two reads was honored. The blob is already in hand under the guard, so re-asserting owner-or-author costs a deserialize. A non-owner whose issue has vanished gets 403 rather than 404, matching the pre-check's existing refusal to reveal existence. `owner_can_still_close_after_the_reorder` seeded the owner as their own issue's author, which made it unable to fail: with the owner check disabled the author fallback granted the close and the test stayed green. It now seeds a third party, so only the owner arm can grant. RED with `is_owner = false`. Also drops the claim that the author twin covers the acquire-vs-acquire_fresh distinction. `RepoStore::for_testing` hardcodes `tigris: None`, so the two calls are identical in every test here and reverting that line leaves the twin green. Separating them needs an object-storage seam. Stating the gap beats asserting coverage that does not exist. * style(node): rustfmt the new contention test Whitespace only; cargo fmt --check gates the push. * fix(node): shed an unrefreshable write as a retryable 503 with a fixed body The two refusal arms added for the under-lock transfer bound returned bare anyhow errors, so AppError's From impl (which downcasts only sqlx::Error and RepoBusy) landed them in Internal: a 500 internal_error whose body was the error string. Two defects in one. A transient object-storage failure told the client not to retry, and the timeout arm's message interpolated the owner slug and repo name straight into the response body, contradicting the fixed-body policy the RepoBusy arm sets six lines above it. RepoUnavailable follows RepoBusy exactly: a fieldless type raised with the operator detail in a context string, downcast to its own rung, mapped to a 503 whose body interpolates nothing. The detail stays in the log at the raise site. The timeout arm logs at error! where its sibling logs at warn!, deliberately: a 300s stall pinned a lock-pool slot for five minutes and is the condition the transfer bound exists to surface, so it must keep paging when the handler classifier demotes the ordinary blip case. * fix(node): refuse a fresh-copy write when the archive HEAD cannot be read acquire_fresh collapsed a failed HEAD into "no archive exists" via unwrap_or(false) and served the local copy, while the sibling path under the lock already refused on the same condition. A push that hit a storage blip got an advertisement built from a possibly-stale tree, uploaded a whole pack, and was then refused by acquire_write for the reason the advertisement had already swallowed. The authorship pre-check on close_issue read the same way, which is an infrastructure failure resolving toward a denial. A failed HEAD now raises RepoUnavailable, so both callers surface it as the retryable 503 rather than a stale success. The download-failure fallback is unchanged: a present-but-unreadable archive is still self-healed from local. info_refs keeps its map_err for every other failure so the read path's error vocabulary does not move; only the typed error takes the From chain. A test-only TigrisClient constructor pointed at a closed port makes both refusals executable, so this is no longer verified by reading. It also proves the under-lock arm end to end, which the earlier draft had recorded as an untestable gap. * fix(node): log expected transient acquire failures at warn, not error Both acquire call sites logged every failure at error, including RepoBusy, which the raise site already logs at warn. Ordinary write contention paged. The previous commit widened the problem: info_refs now raises RepoUnavailable on a storage blip, so that site would have started paging on the condition this series just classified as transient and retryable. A classifier over the two typed refusals picks the level at both sites, mirroring the startup path's permanent-vs-transient split. Anything it cannot classify still logs at error, so an unknown failure keeps paging. * fix(node): log the read failure the close_issue pre-check folds into a denial The authorship pre-check treated a get_issue I/O error and a genuinely absent issue as the same None, with no log line. The client answer is deliberately identical, since a caller who cannot write must not learn whether the issue exists, but the two are not the same event and an operator had no way to tell a real authorization denial from a filesystem or parse failure behind it. Splitting the arm leaves the 403 exactly where it was and makes the read failure visible in the log. * test(node): poll for the closed session instead of sleeping a fixed 300ms The release-invariant test slept 300ms for a Drop-spawned close before comparing backend pids, which is a coin flip on a loaded CI runner. It now polls pg_stat_activity for the captured pid on a standalone connection, the same discipline poll_until_free documents: a pooled observer would be handed the session under measurement and hide the effect. The conversion was checked against the failure it exists to catch rather than assumed. Pooling the session on an unlock that returned false makes the test fail after the poll deadline, not hang, and no_reap_pool disables idle timeout and max lifetime so nothing but the close under test can retire that backend. A generous deadline would have been the same defect as the sleep. * fix(node): raise a failed fresh download with no local copy as RepoUnavailable acquire_fresh already refused a failed Tigris HEAD as RepoUnavailable, so the handler layer mapped it to a retryable 503. A failed GET on the same path still returned a bare anyhow error, which the info/refs map_err closure stringified to AppError::Git and answered 500. One endpoint therefore told the client a transient object-storage blip was permanent or retryable depending on which call failed, and close_issue's pre-check inherited the same split through its bare ?. Raise it at the source instead of at each consumer: From<anyhow::Error> for AppError already downcasts RepoUnavailable out of the context chain, so both callers pick up the retryable mapping without touching either. The new test drives HEAD 200 with GET 500, which is the exact state the refusal is for: archive present per HEAD, GET failed, no local copy to fall back on. * test(node): assert the second writer sheds as RepoBusy instead of timing out two_writers_on_the_same_repo_are_not_both_admitted wrapped the second acquire_write in an 8-second outer timeout and only checked the future had not finished. That passes for any stall, including lock-pool saturation or a slow CI box, so it could not tell a working shed from an unrelated hang, and it cost 8 seconds on every suite run. Use with_lock_acquire_deadline and assert the typed RepoBusy downcast while the first guard is still held, matching what contended_acquire_sheds_as_repo_busy_ not_internal_error already does. It now also fails loudly if a second writer is admitted, which the timeout version could not distinguish. * fix(node): read the close_issue author pre-check from a non-mutating snapshot The non-owner author fallback refreshed through acquire_fresh, which publishes into the live repo directory: its extract step removes the existing directory and renames the new one into place. That runs with no write lock held, so any signed non-owner could trigger a directory swap underneath an in-flight guarded write on the same path. read_snapshot downloads to a throwaway temp dir and hands back a RepoSnapshot that removes it on drop, so the pre-check still sees fresh data and the live path is never touched. download_to grows a publish flag to serve both shapes from one path. The wedge invariant still holds: a stranger is refused without waiting on the write lock. * fix(node): bound every await in the lock-acquire loop by the deadline The remaining budget was checked before the pool checkout but bounded neither the checkout nor the pg_try_advisory_lock query that follows it. A checkout starting just under the deadline could wait out the pool's own acquire timeout, and a slow query could be accepted after the budget was spent, so the advertised wall-clock cap held only on the fast path. Both awaits now run under the remaining budget and shed as RepoBusy when it runs out. The probe's Drop closes its session, which cannot hold a lock it never confirmed taking, so the query-timeout arm is a plain shed. * test(node): add an S3 mock that really enforces conditional writes The fence work landing next is only as good as what the tests can observe, and a mock that answers 200 to every PUT would make the whole suite vacuous. This one holds the object and its ETag, refuses a mismatched If-Match and an If-None-Match "*" over an existing object with 412, and mints a fresh ETag per successful PUT so two byte-identical archives never share a token. Capture-then-replay is deliberate rather than parking a handler and hoping it resumes: when tokio drops an SDK future the client can tear the connection down and cancel the server task with it. Replaying what arrived models the arm that matters (body fully transmitted, commit decided later) with no timing in it. Six tests pin the semantics in both directions so a hollowed mock cannot hide. * fix(node): stop pretending a held lock fences a late upload The timeout arm claimed that keeping the advisory lock protected a successor from an abandoned PUT. It does not. release takes mut self, so the guard drops the moment it returns and Drop closes the session; measured on this branch, a successor took the same repo's lock 5ms after release returned while the PUT was still in flight. The comment and warn now say what is actually true: the outcome is unknowable, the PUT may still land, the lock releases normally, and a conditional upload is what keeps a late publish from overwriting a successor's archive. Two tests replace the claim. Session disposition is the observable that separates the two shapes, so the unlock is pinned to run and be confirmed on the guard's own session with the connection returned to the pool, checked by backend pid. Successor admission is pinned too, but noted as not what proves the point, since the lock frees within milliseconds either way. * feat(node): let the storage client express and detect a conditional write upload takes an UploadPrecondition (IfMatch, IfAbsent, Unconditional) rather than an Option<String>, so the absent case and the deliberate no-fence case are distinguishable at the type level and a caller cannot lose the fence by passing None. head_etag reads the current ETag alongside exists, which keeps exists and its other callers untouched. A failed precondition has to be classified off the raw HTTP status: PutObjectError models no PreconditionFailed variant, so a 412 arrives as Unhandled with nothing useful on it. 412 is always a lost precondition and 409 is one under IfAbsent. 404 deliberately is not: no archive delete exists on this line, so a 404 on a conditional PUT means a wrong bucket or endpoint, and reporting that as retryable would send clients into a loop against a permanent fault. The three background uploads outside the write guard now publish with IfAbsent. They fire only where the archive is expected absent, and leaving them unconditional would defeat the fence from the side: init uploads an empty bare repo, so a push landing just before it could have its archive replaced by that empty one. A refusal there means someone else already published the key, which is logged as the correct outcome rather than a failure. * fix(node): fence the release publish so an abandoned upload cannot win acquire_write now reads the archive's ETag under the advisory lock and the guard carries it, so release publishes conditionally on the generation it actually refreshed from. An upload abandoned by an earlier writer no longer overwrites a successor's archive: the store rejects it, because the ETag it was written against is gone. A refused precondition gets exactly one supersede-retry, never a loop. The distinction that makes this sound is that a 412 is a definite answer, unlike the timeout arm where nothing is knowable, and the retrying writer still holds the lock, so whatever landed underneath was written without one and its tree is not the authority. Two losses in a row refuse instead of escalating. That retry is what keeps ordinary pushes working now that init publishes create-only: a first push racing init's upload of the empty repo loses once and then wins, rather than surfacing a 503 on the most common operation there is. release returns a must-use outcome and the four publishing handlers propagate it before any trust bump, webhook, or success body, so a publish the store refused reads as a retryable 503 instead of a 201. The three release(false) sites deliberately do not map it: they publish nothing, and a 503 there would shadow the 403 or 404 the route means to return. The download-failure fallback also publishes fenced now. That arm knows the stored generation (its HEAD succeeded, only the GET failed), so publishing unconditionally from it would reintroduce the same overwrite. * test(node): drive the abandoned-upload race end to end, and probe the real backend The headline test is the one that had to exist. Writer A's release is parked past its transfer bound and returns with the outcome unknowable, B acquires and publishes, and A's captured PUT is then replayed: the store answers 412 and B's archive survives. The create-only variant covers the arm whose real-world failure is silent rather than loud. The control is what makes those attributable. With no interleaved B, an abandoned-then-replayed PUT whose generation still matches lands. Without it the headline would only show that replays get rejected, not that staleness is what rejects them. Header assertions come last in all three, so a lost fence reds on the outcome it is about rather than on a wire-format check. A mock cannot prove Tigris honors any of this, and the vendor requires a Single-region or Multi-region bucket for conditional operations, so against a Global or Dual-region bucket the fence is a silent no-op. The credentials-gated probe checks both arms against the real endpoint and cleans up unconditionally, including when an assertion fails, which is the case it exists to catch. It accepts 412 or 409 on the create-only arm because both mean the precondition was enforced and both are already classified as a loss. * fix(node): reconcile F-series tests and test seams with the rebased guard shape The rebase onto main carried main's #174 F-series guard tests forward. The branch's guard replaces main's locked/released/test_pre_unlock_gate Drop mechanics with close-on-drop (runtime) and leak (off-runtime), so two tests that asserted the replaced mechanics are reconciled: the off-runtime disposal test now asserts the leak observable (the slot never returns to idle) and the detached-unlock-returns-connection test is dropped, its invariant covered by the U-series drop-frees-the-lock gates. The pre-unlock gate seam is restored (test-only) so the mid-unlock cancellation tests keep their deterministic park. ipfs.rs tests are updated to the branch's sync test-client constructor and 4-arg RepoStore::new. Refs #279 * fix(node): address the 2026-08-10 review round on the advisory-lock series - P1: serve the receive-pack advertisement from a non-mutating snapshot instead of acquire_fresh, so the unlocked advertisement cannot delete or swap the live repo directory under a concurrent guarded write (the stale-ETag success ordering). acquire_fresh loses its only production caller and is removed. - P1: read-gate, rate-limit, bound, and cancellation-clean the close_issue pre-lock snapshot, so a signed non-owner cannot drive unbounded Tigris downloads and blocking extractions, and an abandoned extraction no longer leaks its temp dir. - P1: classify raw 409/412 responses as a lost conditional write via SdkError::raw_response() in both upload and the publish supersede-retry, so an unparsable error body cannot acknowledge a write that was never published. - P2: wrap the cold-cache under-lock download failure in RepoUnavailable so it sheds as a retryable 503 like the HEAD arm, not a permanent 500. - P2: surface a refused create-only fork upload as PreconditionLost and refuse the fork, so an orphan archive cannot shadow a fork's DB record. Refs #279 * fix(node): record why the close-issue read gate cannot carry a synced row The gate added for the pre-lock snapshot reads a repo row's own visibility rules and public flag. A row synced from a peer is stored public and carries none of the owner's rules, so for that class the gate can only return allow. Refusing instead would deny the repo's real owner and the issue's real author on any node whose only copy is a synced one, and it would not buy the protection it appears to, because a synced row's recorded owner comes from the peer that sent it. Every other read gate in the API reaches the same verdict for such a row, so this is left as is and stated at the call site rather than special-cased here. The test that covered the gate seeds a locally created repo and cannot observe this. Adds one that pins the property directly: the gate allows an arbitrary caller on a synced row, and the handler refuses that caller anyway, because the owner-or-author check is what decides this route. * fix(node): address jatmn review on merged advisory-lock head Align .env.example db pool with validate floor, move snapshot cleanup into the blocking extract task, remove fork clones on PreconditionLost, restore upload_site_reached test plumbing, and use owner DIDs in receive-pack tests after #330 owner-push default. * fix(node): cap close_issue snapshot work on the read pool Take a git_read_semaphore permit before the non-owner author snapshot so parallel close attempts cannot each drive a full archive extraction while still under the hourly rate bucket. Format owner-DID test helpers. * fix(node): mark close_issue mirror visibility gate for push hook Annotate list_visibility_rules with mirror-rows-handled so the pre-push surface detector accepts the synced-row read gate. * fix: unblock pre-push after main merge Read CappedBody.text in gl whoami (#381 shape), restore legacy CID sweep wiring dropped in the merge, route rate-limiter cleanup through AppState, and allow build_lock_pool for test-only callers. * fix(node): carry transient acquire failures through every write handler Extend acquire_write_app_error so RepoBusy and RepoUnavailable map to fixed-body 503s on issue and pull paths, matching receive-pack. Fix the flaky u2 drain exhaustion test via a drain_faults seam and add handler regressions for contention and storage unavailability on create_issue. * fix(node): drop unused logcap helper after drain_faults seam * fix(node): route fork disk paths through validated_repo_disk_path CodeQL flagged remove_dir_all on a user-derived path in fork_repo. Use the same three-layer barrier as RepoStore::local_path and reject names the validator refuses before clone. * fix(node): harden release-upload outcomes and replication tail gating Treat timed-out or failed archive uploads as non-durable in into_result, gate pinata/gossip on confirmed publish durability while keeping inv22 spawn order, split lock-pool vs main DB pool validation, map LockPoolBusy through From<anyhow::Error>, and attach Retry-After on repo write contention errors. * fix(node): close jatmn round on publish authority and durability gating Revoke under-lock refresh swaps when the advisory lock ends, fail closed on unrecorded release outcomes before Pinata/gossip, compensate fork archives when create_repo fails, recompute acquire backoff from the live deadline, and route publish swaps through validated_repo_disk_path for CodeQL. * fix(node): harden issue close/create and fork publish paths on #285 Move close_issue rate limiting after the read gate so denied readers see 404 not 429, roll back local issue refs when publish refuses, re-check swap authority under publish_lock, propagate fork upload failures before DB insert, and update lock-pool config help text. * fix(node): close write-attempt ownership gaps on advisory-lock release Make publish swaps claim-exclusive via CAS, invalidate the local read cache on definite publish refusals, harden fork create compensation and clone cleanup, and keep snapshot temp dirs owned through the async handoff. * fix(node): gate replication tail on publish durability and split close_issue limits Wait for a confirmed release outcome before post_receive_replication_tail runs any walks or coalescing work. Give close_issue its own per-IP rate bucket so it cannot drain receive-pack quota. Add regression tests for both paths. * fix(node): add ValidatedRepoDiskPath for CodeQL and reconcile tail durability Introduce a validated-path newtype with the same inline join and component walk main uses, so path-injection sinks only accept sanitised repo paths. Reconcile the publish-durability gate with the disconnect-safe tail spawn via PublishDurabilitySlot and explicit UploadUnknowable handling. * fix(node): close three jatmn findings on advisory-lock PR Retry fork create confirmation before compensating or returning, schedule background recovery when the lookup stays unavailable, install publish durability outcomes before the recorded flag, and size the lock pool for non-push mutations (default 40). * fix(node): verify adversarial findings on advisory-lock PR Fail closed when publish durability never records, bound pg_advisory_unlock with the same transfer budget as upload, and cap close_issue pre-lock snapshots at git_acquire_timeout_secs instead of the write-lock transfer bound. * fix(node): compensate under the write lock and harden tail durability handoff Run create_issue rollback through release_compensating so issue refs are deleted only while the advisory lock is still held and only on definite publish refusals, not UploadUnknowable. Replace the tail durability slot's async try_lock drop with a std mutex so cancellation during polling cannot skip installing UploadUnknowable. * fix(node): refuse stale under-lock refresh and gate tail durability on release Under-lock GET failures now always shed as RepoUnavailable, even when a local copy exists, so a transient download error cannot publish over a newer stored generation. PublishDurabilitySlot only synthesizes UploadUnknowable after release starts, and receive-pack marks that boundary before spawning the tail. Adds regression tests for both paths; inv22 F4 gate accepts the release wrapper. * style(node): rustfmt advisory-lock gap-closure commits * fix(node): satisfy clippy on stale-local download test path join * feat(node): add a publish attempt/stage boundary The four lifecycle findings on this branch share one root cause: a resource was tracked by a coarse state ("release started", "the path exists", "a row with this name exists") when the safety question was about the IDENTITY and the PUBLICATION STAGE of one specific write attempt. This is the vocabulary that makes those questions answerable: - `PublishAttemptId` — minted before the request is built, carried with the bytes as object user metadata, read back off the store to decide whether what is published is THIS attempt's work. That turns "did my request succeed", which a lost response makes undecidable, into "are the published bytes mine", which the store can answer. - `PublishStage` / `PublishStageCell` — how far one attempt got, observable from OUTSIDE the future doing the work. A cancelled handler never returns an outcome, so the stage it had reached is the only thing that can classify it. - `UploadError` split by what a failure ENTITLES A CALLER TO DO rather than by which HTTP status came back. Destructive compensation is licensed only by `proves_not_published()`. Deliberately free of any object-storage type. #79 deletes `git/tigris.rs` and replaces it with a BlobStore layer; this module is what survives that swap, with each backend supplying only its own error classifier. * feat(node): stamp uploads with an attempt id and preserve dispatch ambiguity `upload` now reports its progress into a `PublishStageCell` and stamps every PUT with an attempt id in object user metadata, so both of the questions a cancelled or unanswered publish raises become answerable: - `PreparingArchive` is marked before the blocking compression and `PutDispatched` immediately before `send()`. The conditional PUT is not constructed until compression returns, so a caller abandoned in that window definitely never attempted publication — a fact nothing could previously observe, because the only report was the return value of a future that no longer existed. - `attempt_landed()` HEADs the key and compares the stored attempt id, which is what lets a client whose response was lost recover its own committed write instead of guessing. `UploadError::Other` is replaced by `NotPublished` (proven never committed) and `Ambiguous` (may have been dispatched and may have committed). The single backend-aware classifier is `classify_put_failure`: a 4xx is an answer the server gave before storing anything and proves non-publication; a 5xx does not, and neither does a timeout, a dispatch failure, or a response the SDK could not read. `SdkError` is `#[non_exhaustive]`, so the fallback arm is the cautious one. `delete` is replaced by `delete_if_attempt_matches`, which reads the attempt off the object and fences the DELETE with `If-Match` on the generation it came from. A second name lookup before an unconditional delete would only narrow the window in which a successor's object can be erased; the conditional delete closes it. * fix(node): key durability, cache validity and fork cleanup on the attempt Closes the four lifecycle findings by consuming the attempt/stage boundary at the sites that were deciding on coarse state. P1 — cancellation before PUT dispatch is not publish durability. `PublishDurabilitySlot` is armed on the guard's publish STAGE rather than on a "release started" flag, and its `Drop` classifies an abandoned handler by how far the attempt actually got: a cancellation during compression records a definite refusal, a cancellation after dispatch records unknowable, and a cancellation after the store acknowledged records `Released` (which the flag could never say). `publish_durability_confirmed` now requires `Released`, so the detached tail no longer does IPFS/Pinata/P2P/GraphQL/Arweave/peer work for refs that exist only in a rejected local write. A guard with no storage backend seeds `NoBackend`, keeping Tigris-less deployments' tails running. P1 — an unknowable generation is quarantined, not served as ordinary cache. `release` writes a sidecar marker naming the unresolved attempt beside the live tree (never inside it: it would be tarred into the next archive), and `acquire`'s existing-path fast path reconciles it before serving. Only the store confirming it holds that attempt lifts the quarantine; anything else is a retryable `RepoUnavailable`. The tree is deliberately NOT deleted — the PUT may have landed and this can be the only local copy. A confirmed publish, an under-lock refresh, and cache invalidation each clear the marker, so the quarantine is a bounded refusal rather than a standing outage. The release-side timeout arm also stops treating every stall alike: a bound that expires at `PreparingArchive` is a definite non-publication, and a bound that expires after dispatch spends one short, separately bounded HEAD trying to reconcile the attempt before falling back to unknowable. P1 — fork confirmation and cleanup bind to the creating attempt. The DB row id is minted up front and used as the attempt id, so the object's metadata, the clone's sidecar stamp and the row all name one attempt. `confirm_fork_repo_row` looks the row up BY ID and reports Ours / Foreign / Absent, so a concurrent create, mirror registration or retry can no longer be returned as this request's own commit. Every destructive step — `compensate_fork_archive`, its background retries, and `ForkCloneGuard::drop` — is conditional on the resource still belonging to this attempt, so recovery that observed `None` before a successor committed can no longer erase that successor's archive or directory afterwards. P2 — response-loss ambiguity is preserved rather than compensated. Guarded writes map an ambiguous publish to `UploadUnknowable` + quarantine instead of `UploadFailed` + cache invalidation + the caller's undo, so `create_issue` no longer deletes an issue ref whose archive is durable. Fork creation reconciles by attempt id: a create-only PUT that committed and lost its response is RECOVERED and the row inserted, and an unresolved one keeps its only local clone and refuses retryably. The fork name is no longer fenced behind the attempt's own orphan. Tests, each RED-checked by reverting its guard: - slot_drop_during_compression_records_a_definite_non_publication - slot_drop_after_dispatch_records_unknowable - slot_drop_after_the_store_acknowledged_records_released - slot_drop_with_no_storage_backend_records_released - publish_durability_confirmed_accepts_only_released - publish_durability_confirmed_refuses_quickly_after_unrecorded_slot_drop - receive_pack_cancelled_during_compression_publishes_nothing_and_runs_no_tail (+ receive_pack_that_completes_its_publish_still_runs_the_tail as control) - a_bound_that_expires_before_dispatch_is_a_definite_failure - an_unresolved_publish_quarantines_the_tree_and_a_later_read_refuses - a_quarantined_tree_is_served_once_the_store_confirms_the_attempt - the_next_write_clears_an_inherited_quarantine - a_confirmed_publish_leaves_the_tree_readable (control) - a_guarded_write_whose_response_is_lost_is_not_compensated - a_put_that_commits_and_loses_its_response_is_ambiguous_and_reconcilable - a_closed_response_with_no_http_status_is_ambiguous - upload_classifies_404_as_a_definite_non_publication - upload_classifies_500_as_ambiguous_not_definite_failure - a_generation_that_moves_between_the_head_and_the_delete_is_not_deleted - an_attempts_own_object_is_deleted_and_a_foreign_one_is_not - fork_confirmation_never_claims_a_concurrent_attempts_row - fork_confirmation_recognizes_this_attempts_own_committed_row - fork_confirmation_reports_absent_when_nothing_owns_the_name - fork_clone_guard_leaves_a_successors_mirror_alone - fork_recovery_resuming_after_a_successor_deletes_neither_object_nor_path - fork_compensation_removes_what_this_attempt_still_owns - fork_publish_that_loses_its_response_stays_recoverable - a_fork_whose_publish_lost_its_response_is_recovered_not_fenced - a_fork_whose_publish_is_unresolved_keeps_its_clone_and_refuses_retryably - an_ordinary_fork_publishes_and_commits (control) - mock_round_trips_the_attempt_metadata_a_put_stamped The S3 mock gains attempt metadata, conditional DELETE, a commit-then-lose- the-response mode, a delivered-but-not-committed mode, and a per-key object store (fork creation touches the source and fork keys in one request, and a single slot would have let an assertion about one silently read the other). The compression seam is a condvar gate so no test holds a guard across an await. * fix(node): settle the cache contract on the abandoned path and the attempt The publication boundary this branch introduced was applied at call sites that each had to be remembered, and five were not. Close the eight gaps that survived, grouped by what actually failed rather than by symptom. The marker's lifecycle was answered by the wrong event in three places. The swap that replaces a live tree now owns the clear, so a cache-miss download no longer leaves a stale marker that fences the archive it just installed, and the under-lock refresh no longer lifts a quarantine on a HEAD that downloaded nothing. init clears a predecessor's marker. A cancelled handler never reached release, so it applied the tail contract and not the cache contract. RepoWriteGuard::drop now settles the tree it abandoned, keyed on how far the publish actually got, and a guard that handed out its path for writing marks the tree whatever stage it reached. A marker naming no attempt is a definite non-publication rather than a permanent refusal: it invalidates and the next read serves the stored generation, so an abandoned first push cannot wedge a repo at 503. Only an unreadable sidecar still fails shut. Both of the store's live-path hand-outs now go through one confirming helper, so read_snapshot can no longer return an unconfirmed tree as an ordinary snapshot. The gate binds ahead of the lazy-migration spawn, because a quarantined tree backfilled to object storage by an IfAbsent PUT would publish exactly the refs the marker exists to withhold. The marker no longer rests on a single fs::write; an unwritable sidecar falls back to an in-memory record that reads consult. Fork cleanup claims ownership by renaming the stamp before it touches the tree, so a successor that claims the same path cannot lose its directory to a predecessor's check-then-unlink, and the directory-absent path no longer unlinks a stamp it does not own. A create-only precondition loss whose reconciliation HEAD fails is refused retryably instead of reported as a permanent name conflict, and its clone is released so the retry can re-clone rather than finding the destination occupied. The pin sweep skips a tree whose publish is unresolved, so provider CIDs are not rewritten from refs that may never become durable. That is the sidecar marker, distinct from the operator-level quarantined row flag. 26 tests, each written and observed failing before its fix. Suite 1211 passed, 0 failed, 1 ignored; fmt, clippy -D warnings and --locked clean. * fix(node): settle the abandoned tree at the disconnect, not at reaper exit The settlement that marks an abandoned write ran from RepoWriteGuard's Drop, and on a client disconnect that guard is held by the Arc clone riding the admission guard into the detached reaper, so it ran only once the process group had been reaped. Reads take no advisory lock and consult only the sidecar, so for the width of that window a concurrent fetch was served the abandoned refs off the live tree with no marker beside it. Confirmed by execution before the fix: no marker on disk, acquire returning the live path, and the abandoned ref still on the tree it served. Only the lock release has to wait for the reaper; the settlement does not. TreeSettlement is a token carrying the path, the publish stage and the hand-out flag, holding the exhaustive stage match that used to live in Drop. The handler takes it out of the guard before the guard is shared with the reaper, so on a disconnect the handler future drops first and the marker lands at the disconnect instant while the reaper still holds the lock. A successful release disarms it. Drop still settles a guard nobody took the token from, so the arms have one home. The regression test reads inside the window and asserts the abandoned refs are not served. It samples pg_locks from an independent session first, so a run where the reaper had already finished reports itself inconclusive rather than passing for the wrong reason. Suite 1212 passed, 0 failed, 1 ignored. The five settlement mutations are load-bearing, including one that reverts this split and reddens the window test. * fix(node): let exactly one classifier settle an abandoned tree Moving settlement into a token the handler holds left two classifiers able to run for one write. release() classifies the tree and only then awaits the unlock, while the handler disarms the token after that await returns, so a cancel in the unlock gap dropped a still-armed token and settled a second time. After a definite refusal that is a quarantine marker written beside the tree release had just deleted, which sends every later read down the refuse-and-reconcile path for a repo whose state was already resolved. Observed before the fix: the tree gone, the marker there. The arm is now an AtomicBool shared between the guard and its token. release stores false at the classification point, above the unlock await, and settle takes the arm with compare_exchange, so whichever classifier gets there first is the only one that acts. tree_settled stays, and now has a test that says why: after the handler takes the settlement the arm is still true, so tree_settled is the only thing stopping the guard's own Drop, which on a disconnect runs in the detached reaper, from settling again at reaper exit. The reap-window test was passing for weaker reasons than it claimed. Its fake git took SIGTERM and let the group die, so the reaper could free the advisory lock before the probe read; it now traps TERM and re-parks so only SIGKILL ends it. It asserts the marker exists at the disconnect instant rather than only mentioning it, and the served-refs half no longer passes when acquire fails for an unrelated reason. Suite 1214 passed, 0 failed, 1 ignored. Nineteen mutations load-bearing, including one that reverts the release-side disarm and one that reverts the settlement split. * test(node): use the test's own pool now that Db::pool is gone --------- Co-authored-by: Kevin Codex <kevin@gitlawb.com>
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