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.
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.
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.
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).
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
Reconcile the advisory-lock series with main after #173, #330, and related
landings. Conflict resolution in repo_store, repos, issues, pulls, ipfs, and
main.
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.
- 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
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
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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
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
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
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
beardthelion