record_pin_source's doc already names this residual and accepts it: the
pin_sources_incomplete marker is one boolean per object rather than per
(object, repo), so a genuine record from a third repo clears a marker that a
different repo's failed record set. Its two neighbouring cases are tested,
the same repo re-recording and a no-op insert that must not clear, and this
one sat between them described in prose and guarded by nothing.
The test drives the real path rather than writing rows. Repo A first-pins the
object. Repo B genuinely holds it and pushes, its skip-branch record fails on
the definite-error arm, and the code marks the set incomplete. An anonymous
read is then SERVED from B, which is the assertion that matters: it proves the
unrecorded holder is reachable while the marker stands. Repo C pushes, its
record succeeds, and the clear fires inside C's transaction. The identical
read now 404s, with the preload counter proving the fallback scan never ran.
Nothing here changes behavior. It converts an accepted tradeoff from a comment
into a guard, so widening it takes a deliberate edit rather than going
unnoticed. The doc comment says the residual fails closed, and that holds:
the provenance loop and the fallback scan both serve through the same
gate_and_serve, so clearing the marker only ever removes a gated search and
can never serve an object the caller could not already read. The window is
exactly one to fifteen sources, because at cap the count-guarded insert is a
no-op and the marker survives.
If someone implements the per-(oid, repo) marker the doc proposes, this test
is expected to go red. It should be updated to the new behavior, not deleted.
Proven load-bearing: removing the clear from record_pin_source leaves the
second read at 200 and turns the test red on the marker assertion.
The boot sweep resolved each legacy row's bytes only through
pin_sources_for_oid. A row predating provenance has repo_id NULL and no
pin_repo_sources entry, so the source set is empty, the `for repo_id in
sources` body never runs, and the cursor advances past it. list_pinned_cids
also filters those rows out on is_raw_cidv1, so they were both unadvertised
and unrepairable: nothing on the node would ever fix them. The sweep exists
precisely for pre-provenance rows, and that was the shape it could not
handle.
For a source-less row, discovery now probes a bounded set of warm local repos
for the object and, on a hit, repairs the key through the existing
repair_legacy_provider_cid.
What it records is the part worth reading closely. It calls record_pin_source
additively and then mark_pin_sources_incomplete. It does NOT call
backfill_pin_provenance, and repo_id stays NULL. Reading identical bytes out
of a repo proves that repo HOLDS the object, not that it is the source: forks,
a vendored file, a shared LICENSE blob and the empty tree all collide across
repos. Since backfill_pin_provenance guards on AND repo_id IS NULL, the first
claim written is permanent, so an exclusive claim would let probe order decide
which repo's visibility rules gate a public read surface, with no correction
path. It would also disable the resolver's own fallback: needs_scan is
sources.is_empty() || at_cap || incomplete, so a non-empty below-cap unmarked
set reads as complete and stops the scan that would have found a servable
copy. The additive record plus the incomplete marker keeps that fallback
alive and makes nothing permanent.
Candidates are quarantined-filtered. list_all_repos is a bare SELECT over
repos with no quarantine or visibility filter, while the resolver's own legacy
scan loads list_quarantined_repos and gates on it, so discovery loads the same
set and drops those ids. A quarantined repo is hidden from every reader, so it
must not become a recorded source. Private non-quarantined repos stay in the
list: an additive source record does not widen what the resolver will serve,
since every source is gated independently.
The candidate list and the warm filter load once per pass, lazily, on the
first source-less row, and the is_dir sweep runs under spawn_blocking rather
than inline on a worker. Nothing goes through repo_store.acquire, so a cold
repo is never pulled back from remote storage; a repair pass must not become
a bulk restore.
Two classification rules matter more than they look. A cold or quarantined
candidate does NOT mark the row retryable. The candidate list is every repo on
the node rather than the row's holders, and sweep_legacy_provider_cids rewinds
the cursor whenever any retryable skip occurred, so counting a cold candidate
would make every source-less row retryable on every run on a cold-storage
node: the cursor would rewind forever, the sweep would never drain, and each
pass would pay full discovery cost. Only a read error on a warm candidate is
evidence about the row. Conversely, cap exhaustion IS retryable rather than
terminal, and candidates are ordered oldest-first by (created_at, id) rather
than by id: repo_id derives from the owner DID, which anyone can grind, so a
first-N-wins cap over an id-sorted list would let someone bury the true holder
permanently behind cheap registrations.
MAX_LEGACY_DISCOVERY_PROBES bounds the expensive unit, a bounded object read
from a warm repo. A candidate dropped cold, quarantined, or unsafe costs no
probe. Its doc states the contract it encodes rather than its resemblance to
the resolver's own cap, which happens to share the value under a different
contract.
The source record is best-effort and says so at the site rather than claiming
a later pass heals it. Once the key is rewritten the row is raw CIDv1, so the
cost gate skips it free and nothing revisits it; the healing path is the
resolver's needs_scan fallback, which the incomplete marker keeps available.
The no-remote-fetch property is asserted from both sides. On the effect side,
a cold candidate's disk path still does not exist after two full runs, with a
control proving the sweep declined an available copy rather than finding
nothing to take. On the call side, a source scan of this module (comments and
string literals stripped, four anti-vacuity assertions) rejects the
fetch-capable call shapes, and a mutation planting a real acquire in the probe
loop turns it red. What that pair does not cover, and the test says so: a
fetch reached transitively through git::store or db, and git's own promisor
lazy-fetch on a partial clone. Neither shape exists in the tree today.
Concurrency around the marker is executed rather than assumed. Discovery
records then marks, in that order, because a successful record_pin_source
clears the marker. Under a proven lock-wait interleaving, a concurrent record
committing before discovery's mark leaves the row marked, and two concurrent
passes over one row converge on a single source with the marker set. Both the
swap and the drop of that pair are red-checked.
One residual is pinned by a test rather than papered over, and it is not
introduced here. pin_sources_incomplete is one boolean per object, not per
(object, repo), so a genuine pusher whose record_pin_source commits strictly
AFTER discovery's mark clears it, leaving a non-empty below-cap unmarked set
that tells needs_scan to stop scanning. No ordering inside this sweep can
prevent that, and record_pin_source's own doc already states the same residual
for its existing call sites; closing it needs a per-(oid, repo) marker, which
is a change to the marker's shape. It is bounded in practice: the clearing
writer really holds the bytes, so every source in the set is servable, and for
these rows it cannot be a regression, since before discovery ran they were
keyed on a provider CID and were neither served nor advertised.
Tests: fifteen regressions driving the sweep entry points. The repair, cheap
second pass, probe cap and multi-holder cases were observed RED first. The
must-not cases carry mutations instead, since a guard against something
happening has no honest pre-fix red: quarantined holder never serves, a cold
candidate is neither fetched nor materialized and does not rewind the cursor,
an unsafe candidate name is dropped without aborting the pass while a later
safe candidate still repairs, a raw-CIDv1 row never enters discovery, and a
provenanced row still resolves through the source loop alone. Every guard
carries a red-check mutation with an attributed message, including three on
the design itself: one adds backfill_pin_provenance beside the additive record
and must redden the no-exclusive-claim assertion, one neutralizes the
incomplete marker, and one swaps the record and mark order.
#254 landed after this branch was last pushed and it moves the same surface.
It converts get_by_cid's sqlx failure arms from AppError::Internal to a bare
Into<AppError> conversion, so a connection-class failure downcasts to
AppError::Db and answers 503 db_unavailable instead of a 500 (#251).
Both textual conflicts were spurious alignments rather than real overlaps.
Git matched this branch's provenance path against main's legacy-scan preload
because both regions end the same way, and oids_for_cid does not exist on main
at all. Resolved to this branch's structure, which already contains main's
preload in its own place with all three of its queries clamped.
The semantic change is the part that mattered and it was applied by hand
afterwards, not inherited from the resolution. All ten clamped DB awaits in
get_by_cid now use the bare conversion. Taking this branch's side wholesale
would have silently reverted #251 across every one of them, including the four
this branch added, since the arm shape they were written to mirror predated
#254.
Suite green on the merged tree with fmt, clippy, and metadata --locked clean.
api/repos.rs holds the global pin_semaphore across ipfs_pin::pin_new_objects
and across pinata::pin_new_objects. Inside both, batch_budget_gate only gates
between objects and the git read was already clamped, but the DB calls were
bare awaits. One stalled query parked the permit past every budget, and once
all pin permits were so held, post-push IPFS and Pinata replication stopped
for every repository on the node. The pool defers rather than sheds, so the
queue behind it only grows.
The bound set is derived callee-transitively from each budgeted region's call
graph, not from the shape of the fix. A grep for db./retry_db_record over the
loop bodies cannot see a DB await behind a call site, and that is not
hypothetical: it missed repair_legacy_provider_cid, whose own cid_for_oid and
repair_legacy_provider_cid awaits run under the permit from both lanes while
its deadline argument bounds only the spawn_blocking git read. Kubo: six
direct sites plus those two transitive ones. Pinata: eight direct sites, two
more than Kubo because its post-upload path has its own record_pin_source and
incomplete marker. Completeness here is the enumeration, scoped to the call
graph at this head, not a compile-enforced guarantee.
db_bounded takes the ABSOLUTE deadline, so a slow predecessor cannot hand a
later call a fresh budget. What its Elapsed arm MEANS is a property of the
operation, not of the timeout. A multi-statement transaction whose tx.commit()
is never reached definitively did not land: record_pin_source is
pool.begin(), INSERT, conditional UPDATE, tx.commit(), so a future cancelled
while the INSERT waits on a lock cannot have committed. A single autocommit
statement may well have landed server-side after the client future was
dropped, since tokio cancels the future but not the statement Postgres is
running; mark_pin_sources_incomplete and record_pinata_cid are that shape.
Each site maps its elapsed arm on that basis. Reads skip the object.
Provenance and backfill warn and continue, since the backfill is idempotent
under its repo_id IS NULL guard. A timed-out record_pin_source writes the
incomplete marker, exactly as its definite-error arm does, because the source
definitively was not recorded and leaving the set incomplete AND unmarked is
the state this change exists to prevent: /ipfs would read a non-empty
below-cap source set as complete and 404 a copy the repo would serve. The
arms stay separate so the warn still distinguishes a stalled batch from
scattered per-object failures.
The durability writes take a floored remainder. batch_budget_gate only
guarantees PIN_READ_FLOOR before an object starts and the add gets the whole
remainder, so a successful add can finish with ~0 budget left; an unfloored
bound would fail a write that today completes in milliseconds. That leaves
bytes in Kubo with no pinned_cids row, and on the Pinata side it is worse,
because pinned.push is unconditional there, so repos.rs would build cid_map
and gossip publish_ref_update for a CID that /ipfs will 404. The floor is
re-taken from now at each call, so graces chain within one iteration: the
worst case is deadline+4s on the Kubo skip branch and deadline+6s on the
Pinata add path, about 5% of a 120s PIN_BATCH_BUDGET. It does not stack per
object, because batch_budget_gate breaks at the next iteration's first
statement.
pin_new_objects_gated now takes the batch budget instead of hardcoding
PIN_BATCH_BUDGET. The permit-release regression asserts a bound that IS the
budget, so with the constant inlined it cost 120s of wall clock; passing it
drops the test to 3.4s. Production passes the same constant.
Both twin doc comments are updated together. Each previously stated that the
DB round-trips were untimed inside the budgeted region, which is now false.
The Pinata permit's total hold across pinata_object_list_for_refs stays
unbounded and is called out as such: repos.rs acquires the permit before that
pre-loop walk, so this change closes the DB-stall trigger, not the git-walk
one.
Tests: thirteen regressions, the stall cases observed RED first with the bare
await sitting on the lock until the outer wrap fired. Every guard is proven
load-bearing by a mutation carrying an attributed message, so a RED is pinned
to the named property rather than to seam removal.
Three properties needed a fixture built specifically to bind them, because
the obvious test passed either way. The record floor: a zero remainder still
landed the write, since tokio polls the inner future before the timer and a
local UPDATE round-trips inside one tick, so the marker's table is now locked
and released inside the grace window, which fails without the floor and lands
with it. The absolute deadline: every single-object stall test passes under a
per-call duration too, so a helper unit test now runs two calls against one
shared deadline and asserts the second is cut off by the remainder its
predecessor left, with the mutation granting a fresh duration equal to the
budget rather than larger than it. The Pinata post-upload elapsed arm: it had
been changed in lockstep with the two covered sites but nothing executed it,
so a fixture now lets the upload succeed and stalls only the record that
follows.
get_by_cid builds its walk admission (the global permit plus the
per-caller permit) and then made several DB awaits with no deadline while
those scarce permits were held. A stalled pool or a blocked query pinned
every walk slot for the whole stall, so later /ipfs requests shed capacity
503s long after GITLAWB_IPFS_REQUEST_BUDGET_SECS had elapsed. Any
unauthenticated caller can reach it.
The clamped set is derived from the admission-held region itself, not from
the two sites the review named. Walking the handler body found four bare
awaits:
oids_for_cid, pin_sources_for_oid, pin_sources_at_cap,
pin_sources_incomplete
The last two were not named in review. Already clamped and left alone: the
three per-source lookups and the legacy-scan preload trio. Two negatives
worth recording: gate_and_serve is the only callee reached from inside the
region and holds no DB awaits of its own, so the region does not extend
transitively, and ipfs_work_rate_limiter.is_throttled is in-memory, not a
round trip. Ten DB awaits in the body, zero bare after this change.
Each clamp takes the arm shape the per-source lookups already use, so a
timeout returns the budget-tainted retryable 503 and the early return
RAII-drops the admission. budget_shed and remaining are hoisted above the
oids_for_cid await so the provenance-path sites share one definition; the
legacy-scan preload keeps its own in its nested scope.
Tests: four stall regressions, one per clamped site. The first two lock the
table their query reads and were observed RED first, with the bare await
sitting on the lock until the 10s outer wrap fired.
The marker pair needed a test-only seam rather than a lock. Both queries read
tables that earlier queries in the same request touch first, so a LOCK TABLE
aimed at them stalls an earlier clamp instead, and budget exhaustion has the
same problem because an earlier query elapses before the pair is reached. The
remaining window between pin_sources_for_oid returning and the pair running
is a single get_repo_by_id round trip, measured at 0.14 to 0.17 ms, which is
too small to time a lock into without flaking. So there is a cfg(test) stall
armed per target, placed INSIDE the timeout wrapper so the clamp is what
fires. Every seam item is cfg(test), including the function itself, so a
non-test caller would not compile, and a full release build is clean.
Each of the four asserts the same contract: a 503 inside the budget, the
budget taint in the body, the walk permit returned, and a follow-up request
admitted rather than capacity-shed. All four clamps are proven load-bearing
by reverting the clamp alone with an attributed message, and the mutations
cross-isolate: reverting one turns only its own test red, so each test
stalls at its own query rather than a sibling's.
Cross-model review round, findings corroborated by the in-process reliability
reviewer.
Both channel waits were unbounded. If the reader thread never reached its read,
or never returned after the peer close, the test wedged the binary instead of
failing, and cargo test has no per-test timeout to rescue it. Both are now
recv_timeout with bounds far above any real scheduling delay, so hitting one is
a genuine defect report rather than a flake, and the failure names which wait
stalled.
The handshake paragraph also overstated what the handshake buys. It listed "the
reader thread not yet scheduled to reach the read at all" among the states the
handshake removes, which reads as though the whole scheduling window is closed.
It is not: the handshake removes thread spawn from the window, and a reader
descheduled between its own Started send and its read is still a false-green
path. That path is now stated plainly instead of being left to the following
paragraph to correct.
Re-verified after the change: probe RED 50/50 under a deleted
set_nonblocking(false) at full-suite parallelism, RED on the named message under
a deleted set_read_timeout, 50/50 green unmutated, fmt and clippy clean.
Two findings from review of the probe test.
The helper has two postconditions and only one was guarded. Deleting
set_read_timeout left every test green while a stalled peer would park the
shim's only accept-loop thread forever, since handle_conn runs inline in that
loop. std exposes read_timeout() as a getter, so this half is checkable exactly
with no timing component. Confirmed load-bearing: deleting the line reds the
probe on the new message.
The probe also normalized one socket and then read from a try_clone of it, so
it only proved anything if blocking mode is shared across the two. On Linux it
is, because try_clone is dup(2) and O_NONBLOCK lives on the shared open file
description. On Windows try_clone goes through WSADuplicateSocketW and WSASocketW
(std os/windows/io/socket.rs), which mints a new socket descriptor, and whether
FIONBIO follows it is a question this test would have depended on without being
able to answer. It would have failed silently and green, on the one platform
where the underlying defect is real. The clone was unnecessary, so the reader
now owns the socket the helper normalized and the question does not arise.
Also correct two overstatements in the probe's own doc comment. It called
itself deterministic while its verdict is a 250ms window, and it described the
residual false-green path as "the few instructions between the signal and the
call" when the window is a full cross-thread round trip: signal, read, return,
channel send, and this thread waking. Both now say what the code does.
Re-verified after the change: probe RED 50/50 under a deleted
set_nonblocking(false) at full-suite parallelism, RED on the named message under
a deleted set_read_timeout, and 50/50 green unmutated. The end-to-end test came
in at 39/50 on this run against the first mutation, a third sample alongside 43
and 47, so its doc comment now reports the spread instead of two numbers.
The regression test added alongside the shim fix could pass with the fix
removed. It synchronized on a channel send that happens after accept() but
before handle_conn runs, so the request bytes could already be buffered by
the time the shim read them, and the read then succeeded on a non-blocking
socket. Measured at 43 and 47 of 50 full-suite runs catching a deleted
set_nonblocking(false), so it went false-green roughly one run in eight.
Extract the normalization into normalize_accepted_stream and assert its
postcondition directly, with no client and no request-arrival race. The new
probe puts a connected socket into non-blocking mode, calls the helper, and
checks that a reader parks instead of returning WouldBlock. Blocking mode has
no getter to query: std exposes none and Winsock has no FIONBIO getter at all,
so the property has to be observed through behavior.
The reader signals Started immediately before its read and the main thread
waits for that signal unbounded before opening the 250ms window. Without the
handshake the window would also be satisfied by a thread that has not been
scheduled yet, which would reintroduce the same silent hole one level down.
Peer close comes after the assertion, never before; closing first unblocks the
read and fails the test even when the helper is correct.
Also correct a platform claim this PR had in five places. The bug is not
Windows-only. An accepted socket inherits the listener's flag on BSD-derived
stacks, so macOS and the BSDs are exposed too; Linux is the exception because
std uses accept4 there, which does not inherit file status flags
(rust-lang/rust#67027). A macOS contributor could hit this today while the
comments said it was impossible. The macOS half is documented rather than
executed, and the comments now say so.
Verification, full-suite runs at default parallelism:
helper body mutated (set_nonblocking(false) deleted): probe RED 50/50
call site mutated (helper call deleted): probe GREEN 0/50,
integration test 45
and 46 of 50
unmutated: 50/50 green
The call-site row is the probe's blind spot, measured rather than assumed: it
calls the helper directly, so only the integration test notices a deleted
call, and it does so at a sampled rate rather than reliably. That residual is
recorded in the integration test's doc comment.
start_shim sets the listener non-blocking so its accept loop can poll the
stop flag. On unix accept() hands back a blocking socket, so that never
showed; on Windows the accepted socket inherits the listener's FIONBIO
state and std does not reset it, so every connection arrived non-blocking.
handle_conn reads with read_line(..).unwrap_or(0), which turns the
resulting WouldBlock into "peer closed" and returns without writing a
response, dropping the connection and aborting the client mid-request. The
shim accepts as soon as the handshake completes, strictly before the
request bytes arrive, so the window was open on every connection and
closed only because the 5ms accept poll usually lagged the client. That is
what made real_git_withheld_shaped_first_post fail on windows-latest with
os error 10053 and zero POSTs recorded.
Normalizing at handle_conn entry also restores the 30s read timeout on the
next line, which a non-blocking socket silently made meaningless.
The regression test forces the Windows socket shape explicitly, so it is
load-bearing on every platform rather than only the one that exhibits the
bug, and it establishes the accept-before-request interleaving with a
handshake rather than a sleep: a timing-based version cannot flake red but
can flake green under load, which would make the guard vacuous.
Rebased onto current main. Keep budget-timeout wrappers on get_by_cid;
convert sqlx failures with Into<AppError> (not AppError::Internal) so
pool closed/timeouts become 503 db_unavailable. Same for list_pins and
arweave anchors, with closed-pool regressions.
repair_legacy_provider_cid built its own deadline from git_timeout and never
consulted the batch deadline, but both pin loops call it with a pin_semaphore
permit held. At shipped defaults that is a 600s git_service_timeout_secs against
a 120s PIN_BATCH_BUDGET, so one legacy row whose cat-file wedges could hold a
global pin slot for five times the budget the batch is supposed to cost and
starve every other repo's pin work. The loop's budget gate cannot help: it runs
at the top of the next iteration and cannot preempt a call already in flight.
The helper now takes the deadline rather than deriving it, since its two kinds
of caller can afford different holds. Both pin loops clamp to
min(batch deadline, now + git_timeout). The boot sweep holds no permit and has
no batch to overrun, so it keeps the plain git_timeout.
The Kubo side has had this since the repair landed and the Pinata side inherited
it from the lockstep work in the previous commit, so both get a test: a wedged
cat-file with a 60s git_timeout and a 2s batch budget must return on the batch
order. Both were RED at 62s before the clamp and are load-bearing by mutation.
Also stops the read-bound comments implying both arms bind in a default
deployment. At shipped defaults the batch deadline is always the tighter one;
the git_timeout arm is what an operator who lowers that knob gets.
Add a regression test for verify_strict: a signature forged under a weak (small-order) public key satisfies the verification equation but must be rejected. The non-strict verify path accepts it.
Co-Authored-By: cairn-code <[email protected]>
Rebased onto current main. Keep Overloaded/UnresolvableDid arms; make
Internal and non-unavailable Db responses opaque with server-side logs,
and pin exact JSON in unit + anonymous route tests.
Two gaps remained after the main merge closed the unbounded-read one. The Kubo
skip branch repairs a legacy provider-CID row to the raw-content resolver key on
re-push and the Pinata skip branch did not, so a Pinata-first node left those
rows unresolvable until the deferred sweep caught them. And the Pinata object
read was bounded only by the batch deadline, so one wedged cat-file could hold
the pin permit for the whole budget.
Both are fixed by threading git_timeout into pinata::pin_new_objects, which it
needed anyway: repair_legacy_provider_cid takes a Duration, and the read bound
is now min(batch deadline, now + git_timeout), the same shape the Kubo side
uses. repair_legacy_provider_cid becomes pub(crate).
The returned pairs still diverge on purpose and the twin docs say why: Kubo omits
a record-failed pin because its return is log-only, Pinata keeps it because its
return feeds the announcement cid_map. Pinata's return behavior is unchanged.
Four tests drive pinata::pin_new_objects rather than the Kubo twin, because a
repair call merely present in pinata.rs proves nothing about the Pinata lane:
the repair runs on the skip branch, the codec cost gate reads no bytes for a
row that is already canonical, a failed repair does not break the batch, and the
upload path never runs the repair. The two must-not guards have no obtainable
pre-fix RED, so they are proven by mutation instead: removing the cost gate and
moving the repair onto the upload path each turn one red.
On record_pinned_cid_with_source exhausting its retries, pin_new_objects warned
and then pushed the object onto the returned vector anyway, so the post-push log
told operators an object was pinned when the durable index had no row for it and
the resolver could not serve that CID. The push is now conditional on the record
succeeding.
The twins deliberately diverge here and the old comment said the opposite, that
the push mirrored pinata to keep them structurally identical. The Kubo return is
log-only, so dropping a record-failed pin costs nothing. The pinata return is a
real input: api/repos.rs builds the sha-to-cid map from it, which drives
upsert_branch_cid and the p2p ref-update gossip, so omitting rows there would
silently drop branch-to-CID records. Both comments now say so, since the next
reader's instinct will be to re-align them.
Test asserts the record-failed object is absent from the return while the add
mock still fired, and that a second object is still attempted rather than the
batch breaking. Verified RED against the unconditional push before the fix, and
load-bearing by re-injecting it.
Two doc drifts jatmn flagged on #173, both docs-only.
The /ipfs walk cap is min(internal ceiling, GITLAWB_IPFS_MAX_REPOS_WALKED),
where the internal ceiling is MAX_PIN_SOURCES + 1 = 17. config.rs, README, and
.env.example all described a max() floor instead, so an operator reading them
would think the default of 64 was the effective ceiling and that raising the
knob bought headroom. Neither is true. The runtime is not changed: two tests set
the knob to 1 and depend on the cap actually being 1, so wiring the documented
floor would make the knob inert and break them.
db_max_connections has defaulted to 48 for a while and four comments still said
20. Two of them did arithmetic on it. main.rs and repo_store.rs justified the
separate advisory-lock pool by "20 is below 32", which is now false and made the
separation look like a consequence of the defaults; it is structural, since a
push holds its connection for the whole receive-pack at any pool size. Also adds
a RUN-A-NODE troubleshooting entry for the boot failure an in-place upgrade hits
when it is still configured with the old pool size of 20.
#174 landed on main at 5981257, so the integration merge this branch carried is
now redundant with main's own history. Five files conflicted, and two more
auto-merged into states that did not compile.
store.rs: main migrated the bounded-read family from a per-call timeout:
Duration to a shared deadline: Instant. The auto-merge took main's
batch_check_probe alongside this branch's object_type_bounded, which compose,
and produced two read_object_bounded definitions with incompatible signatures
and no conflict marker. Took main's convention throughout and re-applied this
branch's additions on top: object_size_bounded, the wedged-child reaper test,
and the doc enrichments.
ipfs_pin.rs: the pin read is bounded by the earlier of the batch deadline
(#174) and the object's own git_timeout (#173). Neither implies the other and
both are load-bearing: with only the batch deadline, one wedged cat-file holds
the pin permit for the whole 120s budget; with only git_timeout, a batch of
slow reads runs past the budget. Each direction was verified by injecting it
and observing the RED.
api/repos.rs: kept main's choice of the literal "git" over state.git_bin for
the pin path's cat-file, since that knob is the walk binary and tests point it
at a fake. The parameter stays so the seam is still drivable from a test, and
the reason now sits at the call site.
main.rs: dropped main's free sweep_rate_limiters, superseded by
AppState::sweep_rate_limiters, which sweeps the same limiters plus
ipfs_work_rate_limiter. Both call sites already used the method.
db/mod.rs: dispositioned main's two new peers-table test writers in the
completeness ledger.
Migration numbering is unchanged. v17 is byte-identical on both sides, and
main's own reservation comment already allots 18-23 to this work.
Resolves jatmn's #192 P2. run_bounded's clean-exit teardown only terminated
the job when one had been established; when assign_job returned None it did
nothing, so a git-remote-gitlawb helper descendant that outlived the `git
fetch` leader while holding the inherited stdout/stderr pipe write-ends stalled
the reader joins on the helper's ~300s HTTP timeout, past the bound run_bounded
promises. The timeout path's reap_tree (taskkill /T) fallback cannot rescue the
clean path: once the leader has cleanly exited it no longer roots the
descendant in the parent-PID tree, so taskkill /T cannot reach the orphan.
The Job Object is the only reliable Windows teardown, so make it mandatory:
assign_job now returns JobHandle and panics per-step if the job cannot be
established, and both teardown paths unconditionally terminate it. Nested
unnamed jobs with KILL_ON_JOB_CLOSE are standard on the shipped
x86_64-pc-windows-msvc / CI runners, so this costs no coverage; it converts a
silent ~300s hang into a loud, actionable failure and strengthens the bound
(KILL_ON_JOB_CLOSE kills the descendant at latest when JobHandle drops).
reap_tree, whose only Windows use was the removed None arm, is narrowed to
cfg(all(not(unix), not(windows))).
The change is entirely cfg(windows). Verified by cross-compiling the tests for
x86_64-pc-windows-gnu (compiles clean, clippy -D warnings clean, no dead_code
from the reap_tree narrowing) and by the unchanged Linux git-remote-gitlawb
suite (green). The Windows runtime teardown is reasoned-not-run here; CI's
Windows job is the runtime gate.
run_bounded's clean-exit descendant reap was Unix-only: on Windows a
git fetch leader that exits cleanly leaves the git-remote-gitlawb helper
blocked in its ~300s HTTP request holding the inherited pipes, and
taskkill /T can't reach a descendant once the leader is gone, so the
reader joins blew past the promised bound. The child is now assigned to
a Job Object at spawn (the analog of the Unix process_group(0)), and the
job is terminated on both the timeout and clean-exit paths, which kills
the whole tree regardless of leader liveness. KILL_ON_JOB_CLOSE is the
safety net. The clean-exit regression is now cross-platform so it
exercises this on a future Windows CI lane (#228).
Unix tests unchanged and green; the Windows path is compile-verified for
x86_64-pc-windows-msvc (runtime coverage lands with #228's CI lane).
The test accepted any nonzero git exit (with posts>=1) as the recorded #191
withheld rejection, so a truncated response or a later HTTP failure after the
first POST could pass green. Assert the specific `expected ACK/NAK` signature so
only that break passes, and pin LC_ALL=C so the diagnostic stays the untranslated
English string on a git build with l10n installed. (#192 F6)
The gitlawb_fetches_total gate keyed on whether the request carried `done`,
which both undercounts a no-done completion (a client can finish a flush-
terminated have round with `ACK <oid> ready` + pack and no `done` -> counted 0)
and, on the filtered path, counts a rejected response per POST (the pre-#191
two-POST negotiation the client refuses was counted twice).
Derive completion from the response instead. upload_pack / upload_pack_excluding
now return whether they served a pack; response_served_pack detects the PACK
magic in both side-band-64k and raw framing. The plain path counts when the
response delivered a pack (catches no-done, skips negotiation-only rounds so an
N-round fetch counts once); the filtered path stays gated on the finalizing
`done` round until #191 makes that negotiation valid. Also tighten
request_finalizes_fetch to reject 0003 framing and trailing bytes after the
`done` pkt-line. (#192 F1/F2 + CodeRabbit repos.rs:761)
init() did an unsynchronized REGISTRY.get() check then OnceLock::set(...).
expect(...) for each metric, so two concurrent callers both passed the check
and the loser panicked at 'set INFO once'. Two tests call init(), so the
parallel test runner could hit this. Guard the body with std::sync::Once so it
runs exactly once. Verified: a 32-thread barrier stress-test panics before the
fix and passes after (run in isolation; not committed since the process-global
registry makes the race non-deterministic in the shared-binary suite). (#192)
A non-UTF-8 or non-hex 4-byte length prefix was collapsed to 0000 via
unwrap_or, so a garbled header was handled exactly like a flush -- masking
corrupted/desynchronized protocol input and letting accumulated have lines
go out as a synthesized flush round. Propagate the parse error instead;
a real 0000 still parses as a valid flush. (#192)
run_bounded only tore down the process group on the deadline path. When the git
leader exits on its own but a descendant (the git-remote-gitlawb helper mid-HTTP
request) outlives it holding the inherited stdout/stderr write-ends, the reader
joins blocked on the helper's ~300s timeout, past the deadline run_bounded
promises. Close the group on the clean-exit path too, guarded by a kill(-pgid, 0)
liveness probe so a recycled pid is never signalled (the process-group ID stays
reserved while any member is alive). Adds a leader-exits-first fixture and a
regression test.
Verified by execution: with the clean-path reap removed the new test blocks ~10s
on the descendant (RED); with the reap it returns promptly.
git_upload_pack recorded a fetch and observed a pack size on every successful
POST. In HTTP stateless-RPC the plain serve path streams a pack only on the
finalizing `done` round, so an N-round incremental fetch was reported as N
completed fetches. The filtered path (upload_pack_excluding) instead serves a
self-contained full pack on the single POST that reaches it, with no `done`, so
gating purely on `done` would drop those private (path-scoped) fetches to zero.
Count once on the POST that actually completes a fetch: the `done` round on the
plain path (request_finalizes_fetch parses pkt-lines and fails closed on a
malformed body), or the one filtered-serve POST (served_filtered_pack).
should_count_fetch captures the decision; a completed fetch is exactly one such
POST, so a multi-round negotiation is never double-counted.
The observe_pack_size call still measures the request body, not the served pack;
that mislabel predates this change and is left as a follow-up.
Verified by execution: a three-round plain fetch and a no-`done` filtered serve
each record exactly one completion; reverting the gate to count per POST makes
the plain fetch record 3, and breaking the decision to AND drops both to 0.
The large-output and descendants tests re-invoke the test binary as their
fixtures instead of spawning sh/seq, so they run on x86_64-pc-windows-msvc;
the non-unix timeout path tears down the whole fetch tree with taskkill /T
before joining the drainers instead of killing only the leader. Also folds
the receive-pack POST block into post_pack_round (review nitpick), removing
the duplicated status-check/forward logic.
Address jatmn's two P2 review findings on real_git_fetch.rs.
F1 (INV-22): run_bounded killed only the top-level `git fetch` on the
timeout path, so the git-remote-gitlawb helper descendant kept the
inherited stdout/stderr pipe write-ends open and the reader joins blocked
on the helper's ~300s HTTP timeout instead of the 30s deadline. Spawn the
child with process_group(0) and, on timeout, tear the whole group down
(SIGTERM, ESRCH poll, SIGKILL escalation, bounded cap, then reap) before
joining, mirroring gitlawb-node/src/git/smart_http.rs. libc is added as a
cfg(unix) dev-dep; non-unix keeps the child.kill() fallback so the
always-on test still compiles. New regression run_bounded_reaps_descendants_holding_the_pipe:
a backgrounded sleep inherits the pipe and outlives a 1s-deadline leader;
reverting to a leader-only kill turns it RED (elapsed ~10s).
F2 (INV-21): the non-success branch of real_git_withheld_shaped_first_post
accepted any `git fetch` failure and only printed the POST count, so a
failure before the first POST (broken advertisement, helper lookup,
connection) passed as the expected withheld rejection. Assert posts >= 1
before accepting the nonzero exit. Forcing a pre-POST failure (0 POSTs)
turns the assertion RED.
F1: a nonempty have-batch terminated by EOF means git aborted mid-negotiation, but
the fetch loop folded RoundEnd::Eof into the Done arm, appended a synthetic
0009done, and POSTed it — so a cancelled private fetch still issued an authorized,
signed upload-pack request and made the node generate and stream a full pack to a
client that had gone away. Split the arm: only a real done pkt finalizes; a
nonempty EOF terminates without a POST. The nonempty_eof_batch_posts_as_final_round
test that locked in the wrong behavior is flipped to assert no POST.
F2: fetch_with_helper piped stdout+stderr but only polled try_wait, so a fetch
emitting more than an OS pipe buffer (~64 KiB) blocked git before it exited and the
harness reported a false 30s deadlock. Drain both streams on reader threads while
enforcing the deadline, so the deadline measures a real protocol hang only.
F3: the integration repos were removed only on the success path (predictable
pid/counter names), so a timeout/panic leaked real git repos and could poison a
later run. Root them in a RAII tempfile::TempDir that drops on unwind.
RED->GREEN: nonempty_eof_batch_aborts_without_posting (pre-fix POSTs a synthetic
done → the aborted-negotiation POST fails against the single-response server; fixed
makes one request, no POST). repos_are_cleaned_up_on_unwind (temp dir gone after a
panic unwinds past the guard). Full git-remote-gitlawb suite 42 unit + 3 integration
green, fmt + clippy clean.
Adds executed coverage for the negotiation branches the first pass left
unrun: an invalid pkt-line length (rejected, not underflowed), a non-EOF
read error (propagated, not swallowed as end-of-stream), an immediately
empty upload-pack request (skips the POST), a nonempty have-batch
terminated by EOF (POSTed as a final done round), and an empty
receive-pack body (skips the POST). Each was mutation-checked to fail
without its guard, so the coverage is load-bearing rather than vacuous.
A code-review pass found the negotiation loop would POST once per bare `0000`
flush that carried no haves, so a malformed `wants + N*0000 + EOF` stream
amplified into N signed POSTs. Real git never sends a content-free mid-negotiation
flush and the peer is upstream of the local git process, so this was not
reachable in practice, but it left the loop unbounded per input. Skip a round
that carries no new haves (an empty flush) and only POST rounds with have content
or a terminator, which also makes the loop provably bounded by git's finite have
set. The fresh-clone done-with-no-haves round still POSTs.
A committed integration test drives a real `git fetch` through the built helper
against a real `git upload-pack --stateless-rpc`, so the stateful-to-stateless
ACK bridging is proven by execution rather than reasoned. The main.rs mock tests
cannot falsify it: a pre-scripted Cursor never reacts to the server's ACKs. An
in-test shim replicates the node's v0 serving; the fetch is forced to at least
two negotiation rounds (a fixture that resolved in one round fails the test) and
the resulting object graph is verified.
The second scenario drives the withheld-blob shape (a full pack on the first
POST, as upload_pack_excluding does) through real git and records the outcome:
real git rejects a pack where it expected an ACK continuation ("expected
ACK/NAK, got ..."), so a multi-round fetch of a withheld repo does not complete.
The helper forwards and terminates cleanly rather than hanging, so this is a
node-side concern, not a helper defect, left as a follow-up per the plan's
Withheld-Path Decision. The test guards the helper's non-hang behavior and
surfaces the break if the assumption ever changes.
git-remote-gitlawb advertised `connect`, so git spoke the stateful native
protocol, but handle_connect collapsed the exchange into one GET plus one POST.
A multi-round fetch (more than ~32 overlapping commits) deadlocked: git sent a
flush-terminated have-batch and blocked for an ACK/NAK the helper never sent,
while the helper blocked reading for a `done` git never sent.
Turn Phase 2 into a per-round stateless-RPC client loop. read_upload_pack_round
returns one round at a time (at a flush, done, or EOF) instead of buffering past
flushes waiting for `done`; negotiate_upload_pack captures the wants once and
POSTs a self-contained request per flush-terminated batch: wants, one flush,
every have accumulated so far, and exactly one terminator. Each ACK/NAK is
streamed back to git so it advances, until the done round returns the pack. The
node's `git upload-pack --stateless-rpc` keeps no state between POSTs, so wants
and all prior haves are re-sent every round and no intermediate flush survives
into a body (which would truncate the negotiation server-side).
Signing is preserved per round: every POST carries the Phase-1 decision (signed
after the 404 escalation for a private repo, anonymous for a public one), and a
mid-negotiation denial surfaces through the sanitized error path rather than
reading as an empty or successful fetch. The receive-pack path is unchanged.
Covers the deadlock repro, multi-round accumulation on the wire, per-round
signing for private and public fetches, mid-negotiation denial surfacing, the
withheld-shaped forwarding path, and single-round non-regression.
The git_read_semaphore doc claimed it bounds both info/refs advertisements,
but the receive-pack advertisement draws from git_push_advert_semaphore
(api/repos.rs), so an operator sizing the read pool for both would undercount
their read capacity.
README's settings table also omitted the served-git concurrency env vars that
.env.example already documents: MAX_CONCURRENT_GIT_OPS, MAX_CONCURRENT_GIT_PUSHES,
MAX_CONCURRENT_READS_PER_CALLER, and MAX_CONCURRENT_PIN_TASKS.
GITLAWB_MAX_CONCURRENT_PIN_TASKS caps how many pin loops run at once, not how
much object-id list memory is retained. On the local IPFS path the caller
materializes the list before pin_new_objects_gated acquires, so a parked loop
still holds its full list and the parked count is capped only per repo by
EncryptInflight. config.rs and the pin_new_objects_gated docstring already say
this; the env template still said the knob bounds the memory.
The drain barrier sampled target = count + 1 before connecting the flush and
waited for any next increment. A leak accepted after that sample and before
the flush was counted satisfied the wait; the test then subtracted the flush
as if it were that leak, so leak_count could be 0 with a real leak present
(false green).
Tag the flush connection instead: the accept loop records each accepted
connection's source port, and drain waits until the flush's own source port
is recorded (the flush is never counted). Because accept() is FIFO, when the
flush is recorded every connection that completed its handshake before it is
recorded too, so the post-drain count still covers all egress in flight.
Verified with a probe that reproduces the reported scenario (leak accepted
before the flush is now counted) before removing it.
The tripwire bound a hard-coded 127.0.0.2 alias, which macOS does not route
by default (only 127.0.0.1), so the test could not run on apple-darwin
targets. Switch the fixture to [::1] (IPv6 loopback), which is present by
default on Linux, macOS, and Windows and is NOT covered by NO_PROXY
(127.0.0.1, localhost).
The negative control still holds: hyper-util's proxy matcher has no implicit
loopback bypass — a host is only proxied-around when literally listed in
NO_PROXY (verified in the pinned hyper-util source). Both the positive
control (direct reach of [::1]) and negative control (proxy intercept) still
pass.
- Tripwire: fail loudly (expect) when 127.0.0.2 cannot be bound, instead of
printing SKIP and returning green with zero coverage of the negative
control. Linux routes 127/8 as loopback; macOS needs an explicit alias,
and the message says so.
- Observer: restore a bounded drain/ack barrier (Mutex + Condvar). The
detached accept thread signals after each accept; the test opens a flush
connection and blocks until it is acknowledged. accept() is FIFO, so every
leak connection that completed its handshake before the flush is counted
before the flush. Leaks that start their handshake after the drain remain
the irreducible window, now documented as such.
- support: document that arm_blackhole / disarm_proxy_env mutate process-wide
env vars and are safe only because each consuming binary has exactly one
test; a future second test must be serialized against them.
The stabilise() polling loop cannot prove no more connections are coming,
only that none arrived in the last 100ms. A carefully timed background
leak could schedule its connection after stabilise stabilises and the test
would pass despite a live egress. Replace with a simple counter read
before and after obtain_proof. Connections accepted during the synchronous
call are counted; a leak whose TCP handshake completes after the counter
is read is outside the assertion window — this is now documented as a
known limitation of the single-threaded test harness.
Also narrow the fire-and-forget claim: the observer catches leaks that a
closed-port blackhole would miss (because it counts connections regardless
of error propagation), but the timing race inherent in a detached accept
thread means the guarantee is best-effort for post-return handshakes.
Replace the closed-port proxy with a ProxyObserver (TCP listener with
acknowledged accept-loop flushing) so fire-and-forget leaks whose error
is discarded are caught and the count is synchronized before assertion.
Add a blackhole tripwire asserting obtain_proof fails against 127.0.0.2
with the proxy armed, narrowed to a connect-level error to prevent
vacuous passes. Share arm_blackhole across both test files via a
tests/support module.
Document the observer's coverage scope: it only witnesses leaks on the
single exercised happy path; error/retry branches and non-proxy
transports are outside its guarantee.
Two review findings from the last round.
The object-store readability probe swallowed EACCES on objects/pack exactly
like the benign NotFound of a loose-only store, and never checked the loose
fan-out at all. A present object behind an unreadable directory therefore
looked absent: git cat-file printed a clean missing, the probe said the store
was fine, and /ipfs/{cid} answered 404 for content that exists. The same bool
drives classify_store_fault, so an unreadable store also came back as a
terminal 500 rather than a retryable 503. Only NotFound stays benign now, and
the probe additionally opens the object's own fan-out path. That check is
scoped to the oid (validated as 40 or 64 hex before any path component is
derived) rather than sweeping all 256 directories, so an unrelated broken
directory cannot veto every lookup in the repo, and the caller pays one open
instead of a directory drain it does not need. An unreadable directory now
fails closed: absence cannot be certified through something we cannot read, so
a missing object there sheds a retryable 503 instead of a 404.
Both pin loops held the shared pin permit across a synchronous, unbounded
git read, so a wedged filesystem pinned a slot in a pool that defers rather
than sheds, and blocked a runtime worker while it did. The read now runs
through the bounded, process-group-reaped helper under spawn_blocking, against
the batch deadline, and the Pinata loop gets the batch budget it never had.
The budget gate is shared between the two loops so they cannot drift, and it
declines to start a read it cannot pay for rather than spawning a child only
to reap it.
A fault stops the batch only when the object store is genuinely unreadable,
where every remaining object would fail the same way. An object-scoped fault
skips that object and continues, because one corrupt object should not cost a
healthy batch, and the recovery path re-offers the same list and would break at
the same place forever.
Jatmn's #209 round: known_ungated is empty on main and the three listed
handlers are gated, so the guide must not teach them as exceptions. Restore
the signature/certificate section with a trusted positive case and a forged
rejection case instead of blessing a forged "valid" drive.
Scope the denial-test guidance to each route's actual authorization
rule instead of a blanket non-owner assertion, since several
documented routes (star_repo, replica actions, bounty actions,
author-based closing) intentionally authorize non-owners.
Name the known_ungated exceptions (list_webhooks, list_replicas,
list_protected_branches) instead of implying full repo-scoped read
coverage.
Describe the listed jobs as workflow checks that run on every PR
rather than merge-blocking CI, matching the live main branch ruleset
(one required review, no required status checks).
Encode the contributor-facing gate rules (the three owner-gate forms in
use and the by-design non-owner exceptions, path-scoped reads and the
listings contract, denial semantics, deny-test expectations, migration
versioning, client denial surfacing, MSRV and the exact CI gate set) in
a root AGENTS.md that coding agents load automatically, and point
CONTRIBUTING.md at it. Enforcement labels state only what is on main
today.
Whitelist eight safe AppError variants; opaque Git and anything else.
Add a source-scrape guard for map_err sites, a real sqlx context chain
test, and a tasks(limit) ceiling pin at 200.
Drop the head-file fetch and Rust lexer. Match test attributes on
added lines only, with horizontal-whitespace anchoring, covering bare
#[test], #[cfg(test)], namespaced forms (#[tokio::test(...)]), optional
leading ::, and raw-identifier spellings. needs-tests is advisory, so
the simpler matcher is enough.
Addresses jatmn's review: an exhausted 15-file scan or an oversized/
unreadable blob no longer asserts "no test found" (needs-tests was
being applied on unverified evidence), blobs over 5MB are skipped
before decoding, the "\ No newline at end of file" patch marker no
longer miscounts as an added context line, and #[r#test] /
#[tokio::r#test] raw-identifier attributes are now recognized.
Use function-pointer map_err to clear redundant_closure. Opaque only
when a sqlx::Error is in the anyhow chain so claim/finish business
messages survive. Clamp GraphQL tasks(limit); add mutation coverage.
Addresses jatmn's second round of review on pr-triage's needs-tests check:
hitting the per-PR fetch cap now logs and stops scanning instead of
throwing (which was skipping label reconciliation entirely), file content
comes from the Git blob API instead of the Contents API to avoid the
encoding: none gap for 1-100MB files, the added-line check now requires a
non-whitespace attribute character (not just added whitespace) on the
added line, stripped literals leave a placeholder so they can't glue
adjacent tokens into a false match, and the namespaced-test regex now
accepts a leading :: for absolute-path attributes.
Route query and mutation DB failures through a shared helper that
logs the cause chain server-side and returns a generic client
message, so unauthenticated POST /graphql cannot leak sqlx text.
A diff hunk carries no lexical context from lines outside it, so scanning
patch text alone could misclassify an added line that sits inside a comment
or raw string opened earlier in the file (or outside the hunk's context
window). Fetch the head-version file content via the contents API and lex it
in file order, checking only the lines the diff actually added.
The needs-tests detection regex only matched a literal #[test] or #[cfg(test)
immediately after #[, so it missed namespaced test attributes like
#[tokio::test] or #[async_std::test]. Any PR whose only new tests were async
(the common pattern in this codebase, e.g. crates/gl/src/agent.rs) got falsely
labeled needs-tests even when tests were added in the same commit.
Reproduced against PR #198: it added two #[tokio::test]-only tests in the same
commit as the source change, and the triage bot still commented needs-tests.
Widens the regex to also match any attribute ending in ::test], verified
against representative patches including PR #198's actual diff.
Closes#201
The `test (beta)` lane failed on 17f9eda reading `leader.pid`, NotFound, while
`test (stable)` passed the same commit. The test armed a 100ms watchdog deadline
and the fake git records its pid as its first statement, so a runner slow enough
to delay `/bin/sh` past 100ms gets the whole process group torn down before the
pid is ever written.
Reproduced by delaying the fake git's first statement by 300ms, which yields the
identical NotFound panic at the same line. With the deadline at 2s the same
injection passes at 0.3s, 1.0s and 1.8s of start delay, so the race window moves
from 100ms to 2s.
The property under test is unchanged: a pre-fix build stands the watchdog down on
drain and blocks on `child.wait()` for the child's full `sleep 30`, which still
trips the 10s recv budget. Confirmed by injecting that exact defect, which goes
red on the budget assertion rather than on some other failure.
local_add_refusal had six unit tests and the wiring had none, so
replacing the whole match at the call site with an unconditional "Added
to local peer list." left every test green. That is the shape the store
already names: a helper test plus a status-mapping test does not prove
the call site.
Extract the selection into local_add_report, following the same
print-selection-helper precedent used for the certificate DID hint, and
assert both directions: a refusal never yields the success line, and a
200 never yields the warning. Collapsing the match to always return the
success line now turns that red. The call site is left as a two-line
dispatch carrying no logic.
The scan matched a verb-plus-table needle per line, so it was whitespace
and case sensitive. The identical statement written across lines walked
straight past it, and that form is how the longer queries in this file
are already written, so it is the shape a new writer most likely takes.
Normalize the whole file before matching, mapping offsets back to line
numbers so each statement is still attributed to the function issuing
it. The scan is split into a helper so the property can be asserted
against a synthetic source: the tree contains no multi-line peers write
today, so a test that only ran against the real tree would prove nothing.
Fixtures derive from the needles at runtime for the reason the needles
themselves do, which is that a literal here is found by the scan of this
very file.
The check ran before anything looked at whether the row existed, so it
refused a pure liveness refresh of a row already in the table. Nothing is
protected by that: the row is already there, and an identical-URL refresh
changes no authority. What it did instead was freeze every peer a
deployed database already holds from before this gate, since upsert_peer
validated nothing then and the handler accepted any parseable DID. Those
rows take a 400 on every unsigned announce and cannot be corrected by a
signed one either, because no key resolves.
Now the DID is judged only when no row exists, which is the only place
refusing it prevents anything. The read-then-write race is benign: if a
row appears in between, the write becomes an update and the existing
repoint guard still refuses any http_url change.
The capped read means a peer answering with a body past the cap leaves
truncated JSON behind, and parsing that with ? before the status check
turned a 503 into "invalid JSON response", losing the status the user
needs. A non-JSON error page from a caller-chosen peer URL does the same.
The failure check now runs first and falls back to the body text when
there is no message field. Bound by a test that drives the command
against a mock peer, since reordering the two statements compiles and
passes every unit test in the file.
The peer-add refusal warning added earlier in this branch copied the
node's message straight into stderr, and both announce bodies were read
unbounded, so a hostile --node endpoint, a compromised local node, or a
MITM on plain http could put ANSI or OSC escapes into the user's
terminal. The accepted-announce block had the same hole on the success
path, where the DID and URL come from a peer whose URL the caller chose.
sanitize_node_msg and read_body_capped move from sync.rs to http.rs,
which already owns the wire boundary, and both paths in peer.rs now use
them. Only the display is sanitized: the values forwarded to the local
node stay verbatim, since that is data rather than terminal output.
The split reported the cause in the message but left the code as
bad_request, so a client telling this apart from the other validation
failures on the announce route had to substring-match prose. The auth
middleware already answers unresolvable_did for the same failure on a
request's keyid; the peer-write path now answers the same way. Status is
unchanged at 400, and an unsupported DID method keeps bad_request.
The unproven insert path folded every to_verifying_key failure into
UnsupportedDidMethod, so an oversize, malformed, or non-ed25519 did:key
was told its DID method is unsupported, which is false and leaves the
caller nothing to act on. Split the arm by cause: a parse or resolution
failure now returns UnresolvableDid carrying the underlying reason, in
the same wording the signed path already uses, while a foreign method
keeps methodNotSupported. Every input stays refused in the 400 class.
The refusal test fed local_add_refusal an invented message, so it could
never go red when the node's wording changed. Drive it with the Display
of PeerWriteDenied::UnprovenRepoint and assert the same substring the
node-side pin asserts against a real router response.
The announce route now answers 403 when an unproven caller tries to
repoint an existing peer. gl peer add discarded the local node's reply
with let _ and printed the success line unconditionally, so a refused add
reported as a completed one. The refusal is split into a small function so
the path is assertable, and it is tested for a 403 with a message, a
refusal with no body, and the accepted case. The local add stays best
effort: a transport failure warns rather than failing the command, which
is what the surrounding code already assumed.
The README described the staged rollout as accepting unsigned peers on
these routes, which is no longer the whole rule. With signed peer writes
off, an unsigned announce may still register an unseen peer and refresh a
row whose URL is unchanged, but changing an existing peer's http_url needs
a signature from that peer's own DID, and only a did:key can be registered
unsigned.
The gate restricting unproven inserts to did:key checked the method label
only. Did::from_str validates that the method is one of key, web, or
gitlawb and never inspects the key material, so did:key:notarealkey passed
and created exactly the row nobody can ever correct that the gate exists
to prevent. It now resolves the key the way the signature path does.
Six test fixtures using invented did:key strings stopped inserting once
the gate became real. That failure is the evidence it now reads key
material rather than a prefix, and it means those fixtures were exercising
a path no real DID could take. They carry derivable values now.
Resolving the key put a quadratic base58 decode on a route that accepts
anonymous callers by default. Measured through the router, a 64,000
character did:key cost 8.05s against 4.6ms for the previous label check,
on a tokio worker with no spawn_blocking, and a 1.5 MB body reaches the
handler. The bound sits in to_verifying_key so it also covers the
HTTP-signature keyid, which reads an untrusted string through the same
function. An ed25519 method-id is a fixed 48 characters, so 64 is slack.
Its test asserts the error names the length rather than merely being an
error, because an oversized id fails either way and a check for is_err()
would pass with the guard removed, having already paid the cost.
Three last_ping_ok assertions were vacuous: the seed helper never granted
reachability, so they compared false against false and would have held
against code that never cleared the flag. The helper grants it now and
asserts the seed took. Nothing was hiding behind them.
Drives the must-not set through the production router with real RFC 9421
signatures rather than injected extensions, so the signed path is exercised
as a caller would reach it, in both require_signed_peer_writes modes.
Covers the allowed cases and every rejection: unsigned insert of an unseen
DID, unsigned repoint refused with the row byte-identical after, a
signature from the row's own DID accepted, a signature from a different
DID refused, an identical-URL re-announce refreshing only last_seen, and a
did:web announce refused with no row created.
The signature-from-a-different-DID case is the RUSTSEC-2022-0009 shape,
where libp2p-core accepted a valid signature without checking it derived
the claimed identity. It asserts the key is bound to the row rather than
to anything the request carries.
The announce handler's keyid-mismatch branch had no test at all and
returned 400. It now returns 403 and is tested: a caller denied for want
of proof of control over the targeted row gets the same response class
whether or not it presented a signature.
The completeness ledger derives its set from every writer of the peers
table rather than from upsert_peer's callers, because a caller scan cannot
see a future writer that issues its own SQL. A second table records the
call-site authority for main.rs's bootstrap announce-back, which issues no
SQL and so can never appear in the writer ledger; its authority choice has
no runtime test in this change and is recorded as such.
The type system carries the guard: the authority parameter has no default,
so a new caller cannot omit it. The scan is the backstop for a raw
statement that bypasses upsert_peer entirely, and stays minimal rather
than copying the handler-directory apparatus, which does not apply to a
scan over SQL literals.
upsert_peer ended in an unconditional ON CONFLICT DO UPDATE SET http_url,
so any unauthenticated caller could repoint any peer. A repointed row
steers the sync worker's git remote, the post-receive notify fan-out,
trigger_sync, and the public GET /api/v1/resolve/{did}, none of which
consult the reachability flag that #270's stopgap resets.
An unsigned write may now insert an unseen DID but may never change an
existing row's http_url; changing one requires a verified signature from
that row's own DID. The gate is at the db boundary rather than in the
announce handler because there is a second writer: the bootstrap
announce-back in main.rs reads a peer's own JSON response body with no
proof of anything, and now declares itself unproven, which makes it
insert-only by construction.
The proven variant carries the DID it proves and upsert_peer compares it
against the row being written. A bare proven/unproven flag would say that
something was proven without saying which DID, leaving the real check in
the handler and reproducing RUSTSEC-2022-0009, where a valid signature was
accepted without checking it derived the claimed identity.
An unproven insert is restricted to did:key. Only did:key can ever
authenticate here, so a did:web or did:gitlawb row created unsigned would
become unwritable by anyone once this rule lands.
The unproven path derives its refusal from rows_affected, because an
ON CONFLICT DO UPDATE with a guarding WHERE reports zero rows rather than
raising, and a silent no-op is the fail-open this gate exists to prevent.
The refusal is a typed error the handler maps to 403, downcast out of the
anyhow chain the way sqlx errors already are, so it does not render as a
500.
Observed at c83cbc5 before the fix: an unsigned announce naming an
existing peer returned 200 OK and left the attacker's URL stored. That
test is the RED this change turns green.
Two of #270's reachability tests now drive the proven path. Under this
rule an unproven repoint is refused, so they can no longer express a URL
change through it; they assert the same behavior as before.
The coverage gate matched `# x-release-please-version` anywhere in a manifest,
so an annotated `version` under any other table satisfied it. A crate could
leave the version cargo actually reads unannotated, pass the gate, and still
freeze at release time, with release-please rewriting the decoy line instead:
the exact failure this gate exists to catch.
Scope the match to the [package] table.
IcaptchaCfg::new normalizes an empty GITLAWB_ICAPTCHA_API_KEY to None
via .filter(|s| !s.is_empty()) before storing api_key. No test drove
the empty-string value through new(), so a regression would attach an
empty bearer token to the challenge and answer requests and ship green.
The new test drives an empty key through new() on a trusted operator
origin and asserts it normalizes to None, with a contrast arm showing
a real key still attaches. Mutation check: removing the filter fails
this test and no other.
The merge kept the knob and its clap range but carried over only the
default-value assertion, so nothing asserted that 0 and 1048577 are
rejected. Restored verbatim from the #174 side, beside its
ipfs_max_repo_visits sibling.
Load-bearing: widening the range to 0..=2_097_152 turns it red.
Three gates scanned the "production half" of api/repos.rs by splitting at the
first cfg(test) attribute. Adding test-only items above the code they check
moved that split point above every line being scanned, so all three failed
looking for production code that was still right there.
They now split at the test module, which is what they meant. This is the same
defect already fixed in the ipfs gate during the merge, and no split-on-the-
attribute site remains.
Both behavior-bearing gates were re-proven load-bearing: F2 goes red when the
Pinata object set is re-derived before the pin permit is taken, and the tail
gate goes red when release stops consuming the same success flag. F3 is a
presence check whose marker cannot be removed by a single compiling edit, so it
is not mutation-proven here.
The drain read the repo row and its visibility rules once each. Either read
failing dropped that coalesced push's pins and recovery copy for good: the repo
read returned None on error, and the rules read collapsed through ok(), which
makes the repo read as not announceable. Both fail closed, so nothing leaked,
but nothing sweeps the lost work up either. A single database blip was enough.
resolve_drain_object_list now refreshes through a bounded retry: three
attempts, 50ms doubling, and either read failing is transient. A missing repo
row is the one terminal answer, so it releases at once and spends no retry
budget. Exhaustion logs at ERROR with the repo id and attempt count, so giving
up is observable rather than silent.
The coalescing loop is untouched. It already has the shape the retry needs: a
refresh that gives up skips the pin call, and the next finish_or_take_pending
picks up anything that coalesced while the retry was running.
Eight restored tests cover it, written and observed red against the seam-only
tree first. The five the plan predicted red came up red for the reasons it
predicted, and the three controls stayed green. All eight are mutation-proven:
no-retry, an off-by-one bound, gone burning budget, a rules error read as
empty, an unfiltered candidate set, a duplicated refresh, break-on-giveup, and
a pending slot that never drains each turn their own test red.
A real Postgres pool will not fail on demand, so the drain's error arms have no
way to be driven from a test. This ports the injection table the removed
requeue loop used, keyed on the task's repo id, which is a fresh uuid per test
so parallel cases cannot cross-inject.
The two reads now go through drain_get_repo and drain_list_rules, whose bodies
consult the seam under cfg(test) and then make the same call as before. Error
handling is untouched: the repo read keeps its arms, the rules read keeps its
ok() collapse, and no retry exists yet. The suite is unchanged at 856, which is
the point.
inject and counters carry a dead_code allow until the tests that call them
land.
These covered post-push work being requeued rather than dropped, and were
removed in the merge because they drove the replaced requeue loop. The
behaviour they check is still there, so they re-seam onto the coalescer without
any production change.
The seam moved in three ways. The inflight key is the repo identity key, not
the row id. A coalescing push now merges its (old, new) tip pairs into the
pending slot, so a test that passes an empty vec merges nothing and the drain
never laps: every coalesce carries real commit oids. And the full-scan arm is
reached through the pending-slot overflow, so the leak test coalesces 1025
pairs and asserts the slot degraded to FullScan before the run.
run_encrypt_pin_task_for_test builds the task context the way the production
spawn site does and takes a real owner and name, because the drain re-fetches
by owner and name; a blank name would resolve Gone on every lap.
All four are mutation-proven: drain-does-nothing, stale-empty-rules,
full-scan-skips-the-fail-closed-filter, and a leaked inflight key each turn
their own test red.
The merge deleted coverage that no longer compiled against the reconciled
designs, rather than re-seaming it. Three of those tests have counterparts in
the merged tree after all:
unlock_error_connection_close_is_bounded and
write_guard_dropped_off_runtime_disposes_the_connection came back with
close_conn_bounded and the Drop backstop, which the lock-pool design needed
after all (the after_release hook cannot free a lock when the unlock errors on
a live session). Both now observe the store's derived lock pool rather than the
pool handed to for_testing, which is where the guard's connection actually
lives.
get_by_cid_caps_repos_walked_knob_bounds_the_walks was orphaned when its body
was spliced into a sibling test. It drives GITLAWB_IPFS_MAX_REPOS_WALKED, which
is the binding cap only now that the gate takes the tighter of the two walk
ceilings.
The one test not restored is write_guard_release_when_not_locked: the guard has
a single construction site and it sits below the lock-or-bail, so a guard that
never took the lock cannot exist.
Reconciles the served-git concurrency cap with the /ipfs CID tree gate. The two
branches independently reworked the same three seams, so most of this is picking
one mechanism per seam rather than taking a side per file.
/ipfs walk admission: keeps this branch's provenance-first resolver and its lazy
legacy scan, but drops the detached serve task in favour of #174's shared
Arc<WalkAdmission>, cloned into each spawn_blocking. Both close the permit-release
bypass; the Arc does it without leaving an abandoned request's full legacy scan
running against a held slot. Ported in with it: the request-budget gate at all four
stages with each child deadline clamped to the remainder, the walk timeout computed
inside the closure so queue delay is charged, taint sources in place of a single
truncated flag, and the deterministic-fault 500 gated on nothing else having tainted.
The two per-request ceilings are now composed rather than one shadowing the other.
The visit ceiling was unwired by the merge and the walk cap answered to only one of
the two knobs; both are honoured, the tighter one winning.
Object probe: object_type_bounded takes a Duration and returns ProbeError, so this
branch's call shape keeps #174's absence-vs-unreadable-store discriminator. A clean
`missing` from cat-file is byte-identical whether the object is absent or the pack
is unreadable, and the discriminator is store readability, not git's wording.
Write lock: the advisory-lock pool's after_release hook does not cover an unlock that
errors on a live session. Measured: a poisoned connection returned to the pool still
held the lock 15s later, because the hook's own unlock_all fails the same way. So the
guard now disposes of that connection, in release and in the Drop backstop.
Post-push replication goes to #174's coalescer. This branch's requeue_faults
integration suite has no equivalent seam on that design and is not carried over; it
is recoverable from the pre-merge head.
Also clamps the provenance path's three per-source lookups to the request budget.
They run while the walk permits are held, which is the stall hazard the preload
clamp already covered, and the structural guard found them because it now requires
every occurrence to be wrapped instead of assuming exactly one.
1346 tests pass across the workspace; fmt and clippy are clean under --locked. The
four re-anchored structural guards were mutation-tested and all four go red on the
defect they name.
Both doc comments describing GIT_SERVICE_TIMEOUT_SECS_MAX said the 100-year
ceiling sits "an order of magnitude" under the ~584-year limit of a
u64-nanosecond Instant. The real margin is 3,153,600,000 seconds against
18,446,744,074, a factor of about 5.85. An order of magnitude means ten.
Worth noting how it spread, since the fix is trivial and the pattern is not.
The phrase was written once on the constant, then copied into the
ipfs_request_budget_secs doc by the commit that was at that moment correcting
a different false claim in the same file. Nobody re-ran the division at either
site, because quoting an existing sentence feels like citation rather than
assertion.
The direction of the error is the part that matters: a vague "well under"
would have been true and would have invited a reader to check. The
precise-sounding "order of magnitude" was false and discouraged checking.
Comments only; the diff carries no executable line, checked by filtering it
for non-comment changes. fmt and clippy clean under the CI form.
Review round on the three preceding commits.
The knob's doc said a value past the ceiling "aborts every /ipfs/{cid}
request in a release build". That is wrong, and three reviewers plus an
outside model landed on it independently: the ceiling is 100 years, the
Instant addition does not actually overflow until roughly 584 years on a
u64-nanosecond Instant, so values in between are representable and are
rejected as a deliberate safety margin, not because they panic. Reworded
to say that, since a confidently wrong comment is worse than the vague
one it replaced.
Also on the knob:
- GIT_SERVICE_TIMEOUT_SECS_MAX has two consumers now, so its own doc says
so, and notes this knob derives only the Instant addition and not the
lease-steal multiply.
- The "clears every disable setting" phrasing overclaimed: u32::MAX sits
above the ceiling. Now names the documented sentinels instead.
Operator-facing docs: the sibling knob's ceiling commit updated both
README.md and .env.example in the same change, and this one had not. Both
now carry the accepted range, matching that precedent.
House style: five comment lines added by the preceding commits used em
dashes, which this project does not allow. Four were carried over from the
text being edited and one was copied from the sibling test. All replaced.
Checked mechanically over the added lines rather than by eye.
Two comments restated an argument in full immediately after pointing at
the canonical site for it. Trimmed to the pointer plus the local delta.
Scoping correction worth recording: the two preceding commits claimed "no
behavior change". That holds for repos.rs, state.rs, main.rs and
test_support.rs, but a config.rs field doc IS clap --help output, so the
pin-knob doc edit changes operator-visible help text. No runtime behavior
changes either way.
Verification: full suite 733 passed, fmt clean, clippy clean under the CI
form. The range guard was separately proven load-bearing in both
directions by mutation: reverting to range(1..) goes RED on the rejection
assertions, and tightening the ceiling to 500_000_000 goes RED on the
disable-value assertions.
Known residual, not fixed here: the bound is parse-side only, so a Config
built by direct construction rather than through clap can still hold an
unrepresentable value.
The three handler peeks said they shed "before any DB/disk work", which
reads as an admission bound on the DB window. It is not one. The peek
holds no permit, and permits are only held from the authoritative acquire
below, after the visibility and rate gates, so a burst arriving while
permits are free all proceeds into the DB and none of it sheds. What the
peek actually gives is a cheap 503 for one request once the pool has
ALREADY filled, sparing that request's DB work.
The gate ordering that makes it a peek is deliberate and stays: a denied
or rate-limited request must consume no slot, and one source must not
hold global slots through the DB/visibility window. Reordering to bound
the window would undo that. So this corrects the claims and changes no
behavior; bounding the DB window would need an admission mechanism the
peek is not.
test_support.rs's receive-pack docstring additionally called the peek
"the permit" and "the first statement". Both were wrong at this head: it
is a non-holding peek, and the authoritative write-pool acquire sits
after the per-repo lease. Corrected along with the shed claim. The
"remove the block and this goes red" sentences stay, since those tests
drive an already-saturated pool, which is the case the peek does handle.
Sweep over "before any DB", "before DB", "pre-DB". Left alone with
reason: api/ipfs.rs:142 describes a HELD walk-admission acquire, so its
claim is true; graphql/mutation.rs:201 and test_support.rs:528 are about
auth ordering, unrelated; the repos.rs test docstrings at 4194/4213/4556/
4618/4682 describe test mechanics against a saturated pool, which is the
case the peek delivers.
No behavior change: the diff is comments and docstrings only, checked by
filtering the diff for non-comment lines.
The pin pool's docs claimed it bounded "how many MB-scale lists are held
at once across all repos". It does not. On the local IPFS path the caller
materializes the object list before pin_new_objects_gated acquires, so a
task parked waiting for a permit is still holding its full list, and how
many tasks park is capped only per repo by EncryptInflight. Across
distinct repos that retained memory is unbounded, which is the gap a
reviewer raised against this claim.
Correct the claim at each site rather than implying the pool closes an
exposure it cannot see. The pool does bound concurrent pin loops, and
therefore lists held while being pinned; that part was always true and is
kept. The residual is now named where each claim lives, including on the
config knob, so an operator does not read the knob as a memory cap it
cannot deliver.
Bounding the retention is a real change to the capture shape (the Pinata
twin already avoids it by acquiring before it derives) and is not
attempted here.
Sites corrected: pin_new_objects_gated's docstring and its F2b empty-list
comment, the empty-list test's docstring, AppState::pin_semaphore and
AppState::encrypt_inflight field docs, the max_concurrent_pin_tasks knob
doc, and the construction comment in main.
Sweep over pin_semaphore, F6, and "held at once". Left alone with reason:
ipfs_pin.rs 16/31/174-200/518 describe the loop that genuinely holds the
permit, so their claims are about hold duration and are true; repos.rs
1029-1036 and 2276-2294 describe the Pinata path, which acquires before
materializing, so its "at most pin_semaphore permits' worth exist at once"
holds; repos.rs 5898 claims concurrent loops are bounded, which is the
true concurrency claim; the F6 hits in api/ipfs.rs are that file's own
review numbering for walk-permit ordering, unrelated to this pool;
constructions and test fixtures carry no claim.
No behavior change: the diff is comments and docstrings only, checked by
filtering the diff for non-comment lines.
ipfs_request_budget_secs parsed range(1..), so every positive u64 was
accepted, but get_by_cid derives the request deadline as
Instant::now() + Duration::from_secs(that). That addition is an explicit
overflow check rather than a debug-only one, so an oversized value aborts
every /ipfs/{cid} request in a release build instead of setting a very
long budget. The route is anon-reachable, so an operator typo is felt
publicly.
Reuse GIT_SERVICE_TIMEOUT_SECS_MAX, the ceiling the sibling knob already
carries: the bound is representability, not a policy view of a sane
budget, so the large "set it very large to disable" values operators were
told to use still parse. Reject rather than clamp, matching the other
bounds in this config.
Execution evidence:
- RED before the change: the new test failed with "3153600001 is past the
representable ceiling and must be rejected at parse time" while its
disable-value assertions passed, so the test is not vacuous in either
direction.
- GREEN after: config::tests 13 passed, including the sibling
git_service_timeout regression test.
Sweep for the same defect class, re-run with a widened shape because the
original grep missed a site. Passes: Instant::now() sites; from_secs()
fed by config fields; config fields reaching a Duration through an
arithmetic expression; config fields reaching a tokio timer constructor.
The arithmetic and timer passes each return exactly one hit,
heartbeat_interval_hours (config.rs 150, main.rs 916, operator.rs 161),
which has no range at all and multiplies by 3600 before the Duration.
That knob is a separate defect on a path this branch does not touch, so
it is recorded for a follow-up rather than widened into here. Every other
Instant addition derives from the already-bounded git_service_timeout_secs
or from a constant.
A second-model pass on the previous two commits caught an upgrade footgun in the
first and stale prose in the second.
The 365-day ceiling was tighter than the defect required. The help text has
always told operators to set this very large to disable the bound, and clap
accepted every positive u64, so values like 999999999 (~31 years) are working
production "off" settings that neither overflow `* 2 + 60` nor exceed an Instant
deadline. A node running one of those would have failed to start after this
upgrade, over a value that was never the defect. The ceiling moves to 100 years,
which clears every such setting and stays an order of magnitude under the
~584-year limit of a u64-nanosecond Instant, the tightest representation on any
platform we build for. Every value that worked before still parses; only the
ones that would have panicked are rejected. The test now pins the pre-existing
disable values alongside the rejections so the distinction cannot quietly
regress.
Clamp-and-warn was the suggested remedy and is not what landed: it would keep
accepting a value the node cannot honor and silently substitute a different one,
where the range says what is representable and fails fast, matching how every
other bound in this file behaves.
The lease-bound comment and its test still described the pre-ceiling world. Both
now say what is true, and the test deliberately keeps u64::MAX rather than moving
to the ceiling as suggested: Config is reachable by direct construction, and at
the ceiling the arithmetic does not overflow, so a test pinned there would pass
with the saturation removed and prove nothing. Re-confirmed load-bearing after
the rewording.
Tracing the lease-bound overflow to its other consumers turned up a worse
instance of the same defect. This PR routed git_service_timeout_secs into
build_filtered_pack and blob_paths, and both build a deadline as
`Instant::now() + Duration::from_secs(..)`. That addition panics on overflow in
RELEASE as well as debug, so a large-but-accepted value crashes the serve path
rather than merely disabling a timeout.
Hardening each arithmetic site as it is found would leave the next one open, so
the bound goes where the value enters: clap now rejects anything above 365 days
at parse time, which keeps every derived duration in range at once and fails
fast at boot instead of at the first path-scoped clone. A year is many orders of
magnitude above any real clone or push, so it remains the practical way to
disable the bound.
The test pins both derivations at the maximum and rejects the value one past it
and u64::MAX. RED when the upper bound is dropped from the value_parser.
GITLAWB_GIT_SERVICE_TIMEOUT_SECS accepts every positive u64, so deriving the
per-repo write-lease steal bound as `* 2 + 60` panics every push in a debug
build across the top of the accepted range, and in release wraps to a short
Duration that would let a waiter steal a live push's lease.
Saturating keeps the knob's documented meaning: a timeout that large means the
service bound is effectively off, so the steal bound should be effectively
infinite rather than tiny.
The regression drives git_receive_pack rather than the arithmetic, so the
call-site wiring is what is under test. RED before at repos.rs:1733 with
"attempt to multiply with overflow", GREEN after; re-confirmed load-bearing by
reinstating the unchecked expression.
Review finding from jatmn, round nine. RepoWriteGuard::release closes the
connection when pg_advisory_unlock returns Err (F3b): the await resolved, so
the session is alive and still holds the lock, and returning that connection
to the pool hands the next caller a lock nobody tracks. The Drop backstop
carried the same hazard and only logged.
Both arms are fixed.
The spawned detached unlock now closes the connection through
close_conn_bounded on the error path, instead of letting the async block end
and return it to the pool.
The off-runtime arm was worse than the finding described. It dropped an
already-taken connection with no unlock attempted at all, and this turned out
to panic rather than leak: PoolConnection::drop calls rt::spawn for its
return-to-pool task, whose no-runtime fallback panics with "this
functionality requires a Tokio context". So any off-runtime guard drop
panicked in a destructor today. It now detaches, which gives up the pool slot
and yields a plain PgConnection whose drop closes the socket, ending the
session that holds the lock. Drop cannot await, so detach is the whole
disposal available here. Confirmed against the vendored sqlx 0.8.6 source
rather than assumed, and min_connections is 0 everywhere in this repo.
Three tests, modeled on the release F3b cases, on a pool with the idle reaper
disabled so a reaped session cannot pass the assertion for us. Observed RED
first:
write_guard_dropped_off_runtime_disposes_the_connection ... FAILED
panicked at sqlx-core-0.8.6/src/pool/connection.rs:208:
this functionality requires a Tokio context
then: dropping a write guard off a Tokio runtime must not panic
write_guard_drop_with_failing_unlock_does_not_return_the_connection ... FAILED
timed out waiting for the connection whose detached unlock errored to be
closed rather than returned to the pool still holding the session lock
write_guard_drop_with_successful_unlock_keeps_the_connection ... ok
The success case is green on both sides on purpose: it is the must-not case,
so it only ever catches a widening of disposal onto the healthy path.
GREEN after, and all 12 guard tests pass, so the release coverage did not
regress. Both new guards mutation-proved load-bearing, each red on its own
message, 2/2.
Full suite 729 passed, 0 failed. fmt and clippy clean.
Both new comments said saturating the pool takes ~8 distinct source IPs,
each also rate-limited. Neither half holds for an IPv6 caller with a routed
/64: the sub-cap and the rate limiter both key on rate_limit::client_key,
which with TrustedProxy::None falls through to the full peer address with no
prefix folding, so one caller has 2^64 keys. max_keys does not compensate,
since keys are freed on permit drop and live keys never approach the 100,000
default.
The comments now say what the cap actually bounds, concurrent slots per
resolved client key, and name the residual.
The keying fix stays deferred as a design call, and that deferral is
defensible: full-IP keying predates this PR, so these caps help IPv4 and
single-address callers without regressing anything. Landing a comment that
overstates the guarantee is not defensible, in a round that corrects a
weaker overclaim one screen away.
The per_source_push_cap docstring added earlier in this round repeated the
same claim, so it is corrected here rather than shipping fresh.
Comments only, no behaviour change. check, fmt clean.
The pinata_object_list_for_refs docstring said a visibility tightening that
lands after tail-start is covered by the reconciliation sweep. No such sweep
exists; state.rs says so directly, that a dropped job would be lost forever
precisely because there is none.
The claim is dropped and nothing replaces it. An earlier draft wanted to
substitute a next-push claim, which no site establishes either, and swapping
one unbacked claim for another is the same defect.
Four pre-existing sites make the same claim (repos.rs:30, push_delta.rs at 8,
189 and 281 with warns at 349 and 358). They predate this PR and are left for
a follow-up rather than widening a nine-round diff.
advert_per_caller_cap_sizing_is_never_zero opened with a local closure
re-implementing (pushes / 8).max(1), so it bound nothing. Production derives
that expression twice, and the second site was added after the test was
written. per_source_push_cap now lives in rate_limit.rs next to
PerCallerConcurrency, the type the value feeds, and both main.rs sites and
the test call it.
The finding was confirmed by execution before the fix. Deleting .max(1) from
both production sites with the closure still in place:
injected (2 sites), re-running ... still green
VACUOUS the guard's own defect was injected and nothing failed
0/1 load-bearing
And after, deleting .max(1) from the helper:
injected (1 site), re-running ... red, on the expected message
LOAD-BEARING matched 'advert cap must be >= 1 for pushes=1'
1/1 load-bearing
The first attempt at that second proof came back RED-WRONG-REASON against
'minimum write pool must derive cap 1, not 0': the loop's assertion for
pushes=1 fires first and aborts before that line is reached. Same property,
earlier assertion.
No mutation residue. Test green, fmt and clippy clean.
separator_prevents_the_owner_name_boundary_collision asserted that
("a","b_c") and ("a_b","c") produce different keys. Those differ under any
join at all, bare included (ab_c against a_bc), so deleting the separator
from repo_identity_key left the test green. It could not fail for the reason
it named.
The inputs are now ("a","bc") against ("ab","c"), both abc under a bare
join. The test docstring said the separator was \0; the code joins with /.
Corrected. The docstring on repo_identity_key itself carried the same wrong
collision example, so it is corrected too.
Proven by mutation rather than argued, joining with no separator:
old inputs, injected, re-running ... still green
VACUOUS the guard's own defect was injected and nothing failed
0/1 load-bearing
new inputs, injected, re-running ... red, on the expected message
LOAD-BEARING matched 'owner/name boundary must not be ambiguous'
1/1 load-bearing
Separator restored, no mutation residue. All 6 repo_identity_key_tests pass.
The walk's deadline clamp was computed on the async side, before
spawn_blocking was called, then used inside the closure. Time spent queued
for a blocking thread went uncharged, so the walk ran with a budget larger
than the request's true remainder. The clamp is now derived inside the
closure at task start.
This is the sibling of the upload-pack site fixed in 28a6ca4, and unlike
that one it sits on the anonymously reachable route.
A queue delay that eats the whole remainder saturates the clamp to zero and
the bounded walk fails closed through the existing taint arm, no verdict,
which is the safe direction.
Testing gap, recorded at the site rather than papered over: the queue-delay
path is reasoned, not executed. Observing it needs a runtime with the
blocking pool pinned and parked, and the sqlx::test harness gives no seam
for that. 28a6ca4 shipped with the same gap for the same reason, verified
against its diff and message rather than assumed. The tests in this file
saturate the admission semaphores, never the blocking pool, so there was
nothing to reuse. An assertion on the arithmetic would only restate the
implementation.
cargo test ipfs::: 24 passed, 0 failed.
pin_new_objects runs under a pin_semaphore permit and that pool defers rather
than sheds. Both sinks in ipfs_pin.rs built a bare reqwest::Client::new(),
which carries no timeout, so a silent endpoint parked the pool indefinitely.
Both now use the shared no-redirect client from build_http_client() through a
module-local OnceLock, rather than the hand-rolled twin its docstring forbids.
The client alone does not bound the hold: without a ceiling the loop is O(N)
with N chosen by the pusher. A 120s batch budget is taken once at loop start
and gated at the top of every iteration, so no object's work begins with zero
budget left. The remainder is also passed to each add as a per-request
timeout, which is what lets one large healthy upload run past the client's 10s
default without letting the batch run forever. RequestBuilder::timeout
replaces the client value for that request only and leaks nothing to other
callers, verified by execution against the pinned reqwest.
An earlier version of this commit classified a dead endpoint by
reqwest::Error::is_timeout() || is_connect() and broke the batch. That was
wrong in both directions and is gone. It missed the ordinary dead-peer shapes,
since a peer closing mid-request yields IncompleteMessage and a peer RST
mid-upload yields ConnectionReset, both observed with is_timeout and is_connect
false, so a load-balancer idle timeout evaded it entirely. And it fired on a
slow but alive endpoint, because .timeout() is a total deadline covering body
upload, so a healthy multi-megabyte add was read as a dead endpoint and
abandoned the rest of the push. A wall-clock bound needs neither judgement.
The docstring now states what is bounded and what is not, rather than implying
the hold is closed. Not bounded: the git read, since store::read_object shells
two bare Command::output() calls with no timeout or process-group reaping and
this loop does not run it under spawn_blocking; the DB round-trips; and the
pool itself, since the Pinata replication task holds the same semaphore across
a full git re-derivation with no deadline. Truncation is stated plainly: a
batch stopped at the deadline leaves the rest unpinned and nothing sweeps them
up, so recovery is opportunistic through a later full-scan push.
Tests written first, both RED by assertion rather than by a harness timeout:
pin_new_objects_stops_the_batch_at_its_deadline ... FAILED
the batch must stop partway, not pin all five and not stall on the
first: pinned 5
pin_new_objects_does_not_abandon_the_batch_on_a_slow_but_alive_endpoint ... FAILED
a slow but progressing endpoint must pin both objects: an upload past
the client's 10s default is not a dead endpoint
left: 1, right: 2
Both guards mutation-proved load-bearing against present-but-wrong mutants,
each red on its own message: the loop-top break widened to continue, and the
per-request timeout replaced with None.
Also bounds encrypted_pin.rs's pin_git_object call, where a hang wedged a
repo's EncryptInflight key. Its Drop backstop covers unwind, not hang.
Full suite 730 passed, 0 failed. fmt and clippy clean.
icaptcha-client became a workspace member in #138, after #130 last wrote
release-please-config.json, and nothing updated extra-files. release-please
rewrites a version line only when the line carries the annotation AND the
manifest is listed, so the crate was skipped every release and sat at 0.4.0
while the other five walked to 0.7.1.
Nothing broke: every intra-workspace path dep is bare, so no version
requirement could go unsatisfied; no crate is published to a registry; and
the crate reads no CARGO_PKG_VERSION of its own. The cost was the workspace
Cargo.toml comment asserting a lockstep rule the config did not keep, which
is what the next person adding a crate would read and copy.
Add the missing entry, and gate the rule so it cannot silently lapse again.
The gate derives its required set from `cargo metadata`, so a new crate reds
CI whether or not its author knows the gate exists, and it covers all three
release-silent shapes: a member absent from extra-files, a member whose
version line lost the annotation, and an extra-files path that is no longer
a member.
Next release moves icaptcha-client 0.4.0 to the current workspace version in
one step. Nothing depends on that version, so the jump is cosmetic.
The absent-CID path disambiguates a clean `missing` by re-running the probe,
but the re-probe took the same deadline with no check that any budget was left.
A first probe that nearly exhausted the budget still spawned a second child
that could only be reaped, and the watchdog's SIGTERM grace plus SIGKILL settle
carried the whole call to roughly twice the budget. Measured on
object_type_bounded with a 1s budget and a probe that burns 0.9s of it: two
spawns and 2021ms elapsed.
That is the same ~2x occupancy shape this PR exists to remove, on a different
handler, and it is worse here than on the served-git path: /ipfs/{cid} carries
only optional_signature, and an absent CID drives this branch once per repo, so
an unauthenticated caller spraying well-formed absent CIDs pays the overshoot
per repo. It also degraded an honest absence into a retryable 503.
Re-probe only when the remaining budget covers what the first probe actually
took, since the re-probe runs the same command. Below that, taint to Transient
rather than spawn: an inconclusive disambiguation is not an absence verdict, so
it must not become a false 404 either. The same 1s case is now one spawn and
finishes inside the budget.
Both introduced by this PR (d39fc8d, the F5 disambiguation); neither
object_type_bounded nor the re-probe exists at the merge base, so this is not a
pre-existing condition. The companion test pins that an ample budget still
re-probes, so the check cannot silently disable the disambiguation it guards.
The shared deadline computed the walk's budget on the async side, before the
task was queued on the blocking pool. withheld_blob_oids_bounded starts its own
clock when it actually runs, so a budget measured at queue time handed the walk
a full budget from whenever the pool got to it: the read permit was held for
queue delay PLUS the budget, and only the serve phase saw the outer deadline
already spent.
Computing the remainder inside the closure charges that delay against the shared
deadline, which is what makes the ~1x bound in the docs true rather than
approximate. Not a regression from the previous commit (the old code passed a
constant git_timeout, so the delay went uncharged there too, and the combined
hold was queue delay plus two full budgets), but the claim this PR now prints in
README and .env.example should be exact.
The two shared-deadline tests still pass and still fail on revert; they cover
the budget arithmetic, not the queue-saturation path. Reproducing that needs a
runtime with the blocking pool pinned and parked, which the sqlx::test harness
does not give a seam for, so the queue-delay path is reasoned, not executed.
Two table rows needed correcting for the changes in this PR: the
GITLAWB_GIT_SERVICE_TIMEOUT_SECS row now says the classification walk and the
pack serve share one deadline on the path-scoped upload-pack path, so the value
bounds them combined; and the GITLAWB_IPFS_REQUEST_BUDGET_SECS row no longer
claims the probe and content-read subprocesses are unbounded, while keeping the
object-store-readability sweep named as a live residual.
The other thirteen changed lines are punctuation only, no wording changes: em
dashes replaced with colons, commas, or sentence breaks, and one pair of curly
quotes straightened. The repo's prose-style hook validates a whole file rather
than a diff, so these pre-existing characters were rejecting every edit to
README.md, including the two rows above.
Four prose corrections on this PR's surfaces, no behavior change.
The receive-pack advertisement no longer draws from the write pool, it draws
from a dedicated advert pool, but five comments still described the old
shape: the info/refs per-source block, the cap construction in main.rs, the
State field doc, and a test doc plus its assertion message. The state.rs and
main.rs ones mattered most because they contradicted the operator text being
written in this same commit.
The /ipfs probe and content-read subprocesses each run under their own
deadline now (the lesser of git_service_timeout_secs and the remaining
budget, reaped by process-group teardown), but four surfaces still claimed
they had no duration bound past the pre-start budget check. The correction
stays scoped rather than declaring the route safe: the probe's
object_store_readable check is a synchronous filesystem sweep with nothing to
reap, so a wedged filesystem can still hold the walk slot past the deadline,
and that residual is now stated where the old warning used to be. A grep for
the full phrase misses two of these surfaces, since one wraps across doc-comment
lines and one says "duration clamp" instead, so the sweep uses the fragment.
Both push-side per-source concurrency caps are derived as
max_concurrent_git_pushes / 8 with a floor of 1 and have no environment
variable, so neither was documented anywhere an operator reads. The write-side
one is load-bearing for push availability: it is acquired before the global
write permit, and owner enforcement defaults off, so without it one host
minting disposable did:key identities could monopolize the write pool. They
are described in .env.example beside the read cap rather than in README's
table, which is keyed by variable name.
The hung-walk assertion in visibility_pack printed {err}, showing only the top
anyhow context and dropping the underlying io error. A beta-lane failure
surfaced as "failed to spawn git for-each-ref" with no errno, which made it
undiagnosable. {err:#} prints the chain. The underlying flake is still
undiagnosed: it did not reproduce in 123 local runs and the stable lane passed
on the same commit, so this only improves the next occurrence.
The path-scoped git-upload-pack branch spent git_service_timeout_secs twice:
once on the withheld-blob classification walk, then a second full budget on
the serve. One clone could hold a read-pool permit for ~2x the configured
budget, which is the occupancy hole this PR otherwise exists to close.
Thread one Instant deadline through both phases, mirroring
fail_closed_full_scan_objects and build_filtered_pack: the walk runs against
the remainder and the serve gets what the walk left, computed after the
blocking task's await so it cannot pick up a fresh budget. A walk that
consumes the whole budget saturates the serve to zero, which surfaces as
GitServiceTimeout -> 504 rather than running unbounded. Under-serving one
slow clone is the safe direction versus over-holding the pool.
Both serve arms are covered because a fix that reached only the plain arm
would leave the ~2x hold reachable by any clone of a repo that actually
withholds a blob. Each test was observed RED before the change (status 200,
the serve completing on a fresh budget) and GREEN after, and reverting the
filtered arm alone flips only its own test, so neither guard is a proxy for
the other.
The knob's meaning changed for this path, so .env.example now says the value
bounds the two phases combined rather than each separately.
Spawning the replication tail above guard.release() also moves it above the
Tigris upload, so a ref can be announced while the shared durable copy is still
the old one, and a disconnect there cancels the upload while the detached tail
still pins and announces.
Accepted rather than fixed. Upload-then-announce was never guaranteed: release
tolerates a failed upload by design, warning and continuing to the unlock, so an
announce over a stale Tigris copy was already reachable before this reorder, and
it self-heals through acquire_fresh's local fallback plus the next push. The
alternative, detaching release and the tail together, would return 200 before
the durable copy lands, which changes the client contract more than the window
it closes.
A handler-level "the next acquire_write succeeds" check does not discriminate:
it still passes with the RepoWriteGuard::Drop backstop disabled. Measured with
the backstop off, the lock is held at the drop and reads free about two seconds
later from a session outside the pool. #[sqlx::test] builds its pool with
idle_timeout(1s), so the guard's connection is reaped once the handler future
drops it, the session ends, and postgres frees the lock with no help from the
code under test. acquire_write retries for far longer than that window, so it
waits the release out and reports success either way.
write_guard_release_cancelled_mid_unlock_frees_the_lock is the real proof: it
probes from a connection held out of the pool, 400ms after the drop, inside that
window rather than past it.
The post-receive tail was spawned after `guard.release(...)`, which on a
successful push awaits the Tigris upload and then the advisory unlock while
the handler future is still tied to the client connection. The pack has
already landed on disk by then, so a disconnect in that window left a durable
push whose pins, recovery copy and announce were never spawned: the dropped
tail F2 closed, one step earlier in the handler.
Spawn it above `release`, gated on an explicit `receive_result.is_ok()`. The
`?` below the spawn can no longer be what keeps a rejected push from pinning
and announcing a half-applied repo, so the success check is now a named flag
that `release` consumes too. The tail is read-only on the repo directory and
holds neither the lease nor the advisory lock, so overlapping it with the
upload waits on nothing the handler still owns.
Tests: a handler-level disconnect driven through a parked `release` (new
store-level test seam), plus the must-not direction, a failed receive-pack
that spawns no tail. The U5 source gate now binds the new boundary: spawn
inside `if push_succeeded`, above `guard.release(push_succeeded)`.
The key is logged as the `repo` field on the lease waiter-cap shed and the
steal-bound warning (state.rs), and it was joined with a NUL. Those are the two
messages an operator reads to find a contended repo, and a NUL-hostile log sink
truncates the field at the separator, rendering two different repos' warnings
identically. Before this PR that field was a UUID or a mirror id, both printable,
so the NUL was a regression this change introduced.
Join with '/' instead. It cannot occur in the owner slug by construction - the
slug is owner_did.replace([':','/'], "_") - and it mirrors the shape of the disk
path the key exists to reproduce. A test asserts the key carries no control
characters so this cannot regress silently.
Rotating the U2 lease test's row to a UUID id is not incidental: the mirror path
that seeds it uses 'owner_short/name' as the row id, which now coincides with the
identity key and would leave the test unable to tell the two keys apart. The
rotated id is also the real shape of the bug. Re-verified RED against the reverted
call site after the change.
The post-receive replication tail was spawned at the end of git_receive_pack, after
touch_repo, the trust-score update, per-ref certificate issuance and webhook fan-out.
A client or proxy disconnect anywhere in that window dropped the tail, and with it
this push's pins, recovery copy, and announcements - even though the push was already
durable on disk.
Spawn it immediately after the receive_result ? instead. That is the first point where
the push is known to have landed: guard.release() a few lines up runs on failure too,
so anchoring there would spawn a tail for a push git rejected, pinning and announcing
a half-applied repo - the state release() deliberately refuses to upload to Tigris,
and one a hostile pusher can produce on demand by aborting a pack mid-transfer.
RefUpdate gains Clone so the tail takes the parsed updates while the certificate and
webhook loops keep iterating their own copy.
This makes the tail run concurrently with certificate issuance rather than after it,
so a ref can be announced before its signed certificate exists. Accepted, and stated
here rather than left for a reviewer to find: cert issuance already fails open (errors
are logged and skipped) and the gossip event carries cert_id: None regardless, so no
announce consumer reads a certificate out of it.
Evidence is an ordering gate in inv22_gates.rs rather than a cancellation-race test,
matching the F3 gate's instrument: forcing a disconnect to land inside the cert/webhook
window is timing-dependent and would be flaky, while the ordering is precisely what
changed. The gate is mutation-verified in both directions - moving the spawn back below
the webhooks fails the upper bound, hoisting it above the ? fails the lower one.
Two single-site wiring fixes from the same review round.
withheld_recipients_gated held its git_encrypt_semaphore permit in the async frame
and then awaited spawn_blocking, so dropping the future released the permit while the
uncancellable walk kept running. Every other post-receive scan helper moves the permit
into the closure - replication_withheld_set says so in as many words - so this aligns
the last site with the F4 contract. The regression drops the future mid-walk and
asserts the pool stays at zero while the git child is still alive.
git_info_refs passed a literal "git" to smart_http::info_refs while upload-pack and
receive-pack both pass state.git_bin, so no fake-git harness could drive the
advertisement path. The regression points git_bin at a fake emitting a distinctive
advertisement and asserts it reaches the response body; before the fix the body came
back from real system git (agent=git/2.43.0).
The global and per-source walk permits were handler locals, so a client disconnect
released them the moment the future dropped while the uncancellable spawn_blocking
probe/walk/read kept running against a slot the pool now considered free. A disconnect
flood could admit replacements while prior blocking git children still occupied
threads and PIDs, weakening GITLAWB_MAX_CONCURRENT_IPFS_WALKS and
GITLAWB_IPFS_WALK_PER_SOURCE on a surface any unauthenticated caller can reach.
Hold the pair in an Arc and clone it into each of the three blocking closures. The
permits release when the last clone drops, which collapses both failure directions
into one case: a disconnect leaves the abandoned closure holding the slot, and a
panicking closure leaves the handler holding it. No loop arm changes and the
JoinError arms keep today's taint-and-continue.
A move-and-return shuttle was implemented and rejected. It is correct only if every
arm that continues the loop re-binds the returned value, which the compiler cannot
enforce - the absent-object arm Ok(Ok(None)) => continue, taken on nearly every
iteration of a random-CID scan, drops it - and it forces a second decision about the
panic arms whose least-bad answer would let one broken repo shed every other owner's
/ipfs lookups.
The regression strengthens the existing admission test: it now asserts the slot is
still held after the future is dropped and before the child is killed, with a
child-still-alive precondition so it cannot pass vacuously. Only the walk site runs
under state.git_bin and can be pinned that way; the probe and read deliberately shell
to real git, so a source-scan gate in inv22_gates.rs binds all three clone sites
instead. Removing any one clone turns that gate red.
repo_write_leases and encrypt_inflight keyed on record.id, but RepoStore::local_path
and the pg advisory lock key on the sanitized owner slug plus repo name, and the bare
repo on disk is reused when a repo is deleted and recreated under the same slug. The
row id rotates across that recreate, so both in-process serializers stopped serializing
exactly where the shared objects/ directory still needed them: a disconnected push's
reaper could still be tearing down while a new push took a different key and proceeded.
Add repo_identity_key(owner_did, repo_name) and use it at both sites. Three details are
load-bearing and carry tests: the sanitization is repo_disk_path's replace([':','/'],"_")
rather than normalize_owner_key, which folds a did🔑 prefix and would disagree with
the disk path; the separator is a NUL, without which owner 'a' + name 'b_c' and owner
'a_b' + name 'c' collide; and both call sites pass record.owner_did / record.name rather
than the request path segments, since get_repo normalizes DID aliases and the raw
segments would mint two keys for one directory.
The sanitization is deliberately not injective. Two did:web spellings can fold to one
slug, which would misroute encrypt coalescing if it were reachable, but repos.disk_path
is NOT NULL UNIQUE and holds exactly this derivation, so two such rows cannot coexist.
A test pins that fold in place: making the key injective is what would let two owners
sharing one objects/ directory push concurrently.
resolve_drain_object_list already re-fetches the repo row by owner/name and
deliberately reads visibility rules under record.id, because a delete+recreate
under the same slug gives the row a new id while the bare repo on disk is
reused. The encrypt write a few lines later still took ctx.repo_id, frozen when
the task was spawned, so recovery-pin metadata for a coalesced drain landed
against the deleted row and was unreachable from the live one.
Return the re-fetched id from the resolver and pass it explicitly to
pin_and_encrypt_objects. The parameter is explicit rather than read from ctx so
the two callers stay honest about which id they mean: the snapshot iteration
passes its own spawn-time id, the drain passes the fresh one.
The Arweave anchor is unaffected - EncryptedManifest carries no repo id and
builds its slug from owner and name, both stable across the rotation.
The 300s cleanup task swept the DID, create-IP, push, sync-trigger and
peer-write limiters but not ipfs_rate_limiter, so its expired keys sat
until the map hit max_keys and the inline capacity sweep reclaimed them.
Drive the sweep off AppState in a named sweep_rate_limiters so a limiter
added to the state and forgotten here fails a test instead of leaking
keys, and cover all six with a regression that goes red when any one is
dropped from the sweep.
GITLAWB_REPO_LEASE_MAX_WAITERS was also missing from .env.example and the
README settings table, leaving operators without a way to discover or
tune the cap behind the "repo is busy with another push" 503.
Three findings from the review round.
Moving the Pinata gate to the rules-only predicate was necessary, because a
coalesced push has no walk of its own to consult. But it also meant a push whose
OWN walk failed now acquired a global pin permit and ran a second full
re-derivation that failed the same way. Measured on a repo whose rev-list always
fails: twelve git children and three walk attempts, against four and one before.
With eight pin slots that defer rather than shed, eight such pushes stall pins for
every repo on the node. The gate now excludes a non-coalesced push whose own walk
failed, leaving the coalesced path on the rules-only predicate it needs, and the
pin helper returns before taking a permit for an empty object list.
The connection close on the failing-unlock path had no deadline, and release is
awaited inline while the write lease and both admission permits are still held.
The condition that produces that branch is a connection whose last statement
errored, and a blackholed socket to Postgres is one way to get there, so the await
could pin three admission resources for a TCP timeout. It now runs under a five
second bound; on expiry the future is dropped, which closes the socket and ends
the session anyway, which is what frees the lock. What is tested is the deadline.
That sqlx's close is the thing that stalls in production is reasoned from its
source, not reproduced, and the test says so.
The two tests for that path waited a fixed four hundred milliseconds before
asserting. Replacing the sleep with polling was not enough on its own: the test
pool sets a one second idle timeout, so a connection returned to the pool is
reaped shortly after and the session ends without any help from the code under
test. Both tests passed under mutation once the sleep was long enough to reach
that reaper, which means a straightforward flake fix would have quietly gutted
them. They now run against a pool with no idle reaper, so release is the only
thing that can end the session, and both mutations go red again.
Adds tokio's test-util as a dev-dependency for paused-time assertions. Production
dependencies are unchanged and the build stays --locked clean.
The previous attempt moved the per-source write permit above the lease park to
bound buffered pack bodies. Two reviewers demonstrated that it denies far more
than the source doing the parking. A source parked on one repo's lease was shed
on repos with no contention at all, and because the trusted-proxy setting
defaults to trusting nothing and keying on the socket peer, every pusher behind a
proxy, NAT, or CI runner pool resolves to one key: four parked pushes shed every
push from every pusher to every repo for up to twenty-one minutes. That is the
node-wide shed the permit was moved below the lease to prevent in the first
place, reintroduced at an eighth of the threshold.
The permit goes back below the lease. What is actually unbounded is the waiter
set, so that is what is bounded now, per repo, with a shed past the cap.
The cap counts live waiters, never the entry refcount. The refcount includes the
holder, so capping it would let a lease whose owner died without unwinding pin a
slot forever, which is the permanent wedge the steal timeout exists to survive.
The count is claimed in the same critical section as the refcount and held by a
guard scoped to the cancellable wait, so a cancelled or shed waiter gives it back.
An uncontended acquire takes the fast path and spends no waiter budget; that path
does not barge a queued waiter, which was checked over two thousand rounds rather
than assumed.
Two tests now guard the defect that was just removed: a source parked on one repo
is still served on another, and a parked push does not shed an unrelated pusher
sharing its resolved key. Restoring the old ordering turns both red. Capping on
the refcount instead of live waiters turns the steal-backstop test red. Holding
the waiter slot past the wait turns the fast-path test red.
What this bounds is the parked count, not the parked bytes. At a two gigabyte
pack limit the byte figure stays large, and the total across repos, whose
dominant term is the buffering the signature middleware does before any admission
point at all, is tracked in #263. Past the cap, same-repo concurrency sheds
instead of queueing; that is the trade the knob buys.
The two F1 tests asserted "the capped source shed rather than parked" by racing a
ten second timeout, and drove the holding push in ten millisecond slices. Under
`cargo test --workspace` that is a race against the scheduler rather than a test:
a reviewer saw two of five runs go red, and under load the pair stretched from
1.8s to 19.2s while the fake git holding the lease only lived for 10s, past which
every assertion in both tests is meaningless.
Shed versus park is a question of which path ran, so both are now read as state.
A shed is `Err(Overloaded)`. A park is a second reference on the lease entry,
exposed by a test-only `refs_for`; `acquire` takes that reference synchronously
before the cancellable wait, so a waiter is visible the instant it reaches the
lease, and it stays visible because the steal bound is 1260s. The tests spawn
their pushes and poll until one of the two mutually exclusive states appears, so
a starved machine iterates more rather than failing.
Every wall-clock assertion is gone. The worst of them was a sleep followed by
`!is_finished()`: a runtime that had not yet polled that task satisfied it
vacuously, which means the constant-key mutation could have gone green under
load, and the discriminator is the whole reason that test exists.
All three mutations still go red, now in seconds rather than by timeout expiry:
the per-caller acquire below the lease, the global permit above it, and the
per-caller key replaced with a constant.
Also fixes a third flake found on the way: the acquire-deadline test's follow-up
push inherited the deliberately short two second acquire timeout, which has to
cover a real advisory-lock round trip, so its `Overloaded` was indistinguishable
from the drained-pool `Overloaded` the assertion was actually testing for.
The sqlx `databases_pkey` collisions seen alongside this are machine residue, not
a leak from these tests: sqlx derives the test database name from a hash of the
test path and does cleanup-then-insert without a lock, so any two concurrent test
binaries running the same path collide. The orphan count here held at 53 across
every run.
The post-push tail resolved the withheld set and materialized the object list
before it ever consulted the per-repo coalescer, so rapid pushes to one repo each
parked on the encrypt semaphore and repeated the scan before discovering they
should have coalesced. The gate now runs first, keyed on the cheap
listable-at-root predicate that `replication_withheld_set` already computes from
rules alone before it touches the semaphore.
Three things made this less mechanical than it looks, all of them ways a naive
move would have shipped a regression.
The withheld set had a second consumer. It also gated the Pinata pin and the
branch-to-CID upsert, so a coalesced push that skipped the walk would have pinned
nothing and recorded no CID, while a test asserting merely that the announce
spawn ran would have stayed green. That gate moves to the announce predicate too.
Coalescing must not return from the tail. The announce spawn sits below the
coalescer in the same task, and returning early would drop gossip, the GraphQL
broadcast, Arweave anchoring and peer notify, which are per-push and
non-idempotent. A flag skips the encrypt section and falls through.
The publish paths are gated on announce and iterate the ref updates, not the
object list, so handing the tail the cheap predicate would have made a repo whose
walk is failing start gossiping and anchoring, losing the fail-closed property.
`pinata_object_list_for_refs` already recomputes the real set inside the pin
permit, so it now returns that verdict alongside the list and the tail uses it.
No additional walk is introduced.
Moving the gate earlier also opens a window where a push can coalesce while the
admitted one is still walking. If that walk then fails, the admitted task now
still spawns the drain with an empty snapshot so those tips are consumed, rather
than dropping the guard and discarding them. Note a released guard and a dropped
guard are indistinguishable by the in-flight map, since Drop clears the key
either way, so that case is asserted on drained work rather than on the map being
empty.
Each of the four is pinned by a mutation: walking anyway, returning early,
leaving the Pinata gate on the withheld set, and dropping instead of draining
each turn exactly one test red.
`release` discarded the unlock's result and set `released` regardless, so an
unlock that errored while its session stayed alive (a statement timeout, an admin
cancel) returned a connection to the pool still holding the repo's advisory lock,
with the Drop backstop already disarmed by that flag.
The error is now captured. On failure the connection is taken and closed, which
ends the session and makes Postgres drop the lock server-side, and only then is
the guard marked released, because the lock went with the session rather than
with the unlock. `close()` rather than `detach()` since we are already in an async
fn and can wait for the teardown deterministically.
Disposal is the only mechanism on this path. It cannot coexist with an armed Drop
backstop: disposal needs the connection taken, and Drop returns early when there
is none, while leaving the connection in place so Drop can use it just puts the
locked connection back in the pool when that spawned task ends. The Drop doc now
says so rather than claiming to cover a case it cannot.
The cancellation path is untouched. It was already correct, since `released` is
set after the await, and its test still passes unmodified.
The new tests assert against `pg_locks` from a connection held out of the pool
before the acquire, never by calling `acquire_write` a second time. That shortcut
would pass whether or not the fix is present, because the pool can hand back the
same connection and session advisory locks are re-entrant. The unlock error is
injected by leaving the pinned connection in an aborted transaction, which errors
the next statement while keeping the session and its lock alive, and the test
asserts that precondition before releasing.
Not covered: that the failure path still skips the Tigris upload. The test store
builds with no Tigris client, so the upload site is unobservable here without a
new seam, and that block is untouched by this change.
A push buffers its whole pack before the handler runs, then parks on the per-repo
write lease for up to git_service_timeout_secs * 2 + 60, which is 1260s at
defaults. Both admission permits were taken after that park, so nothing accounted
for waiters: any number of requests could sit on one contended repo, each holding
a fully buffered body, invisible to both semaphores.
The per-source sub-cap now runs before the park. It already exists, is sized at a
quarter of the push pool, and is keyed on the resolved source IP, so parked bodies
are bounded at four per source without a new knob or a new cap to tune.
The global permit deliberately stays below the lease. Moving it up is what
f02c5e1 fixed: a lease-blocked waiter holding a global slot sheds pushes to every
other repo node-wide. A per-source key does not have that shape, since exhausting
it denies only the source doing the exhausting, so the two permits belong on
opposite sides of the park and the comment there now says why.
The load-bearing test is not that a capped source sheds, which any cap satisfies,
including the global one this rejects. It is that a different source still parks
and is served while the first is at its cap. Keying the permit on a constant
leaves the shed test green and turns only that one red. Moving the global permit
above the lease turns the f02c5e1 guard red.
Bounded per source, not globally. A distributed set of sources can still each hold
their four, and the unbounded-total term is the pre-auth buffering in #263 rather
than the lease.
Two claims from earlier commits were argued rather than executed. Both are now
observed.
The legacy-CID sweep's spawn sat inline in main() where no test could reach it,
so the batch size, the inter-batch delay, the detachment and the shutdown select
were all reasoned. The block moves verbatim into `spawn_legacy_cid_sweep`, which
a test calls directly. It seeds rows whose keys are already canonical, so the
cost gate skips each and the persisted cursor reads out walk progress cleanly,
then asserts the cursor stops at the configured batch boundary, the call returns
without blocking, the task is still alive inside its delay, and shutdown ends it.
Never spawning, reading the batch or delay from the wrong knob, or removing the
shutdown branch each turns it red. What remains reasoned is the single call line
inside main(), down from a twenty-six line block.
The claim that a disconnect-interrupted push never uploads to Tigris was
structural: the tests build the store with no Tigris client, so a counter inside
the client arm could never move and would have proved nothing. The counter now
sits at the upload decision, before the client is consulted, and the disconnect
test asserts zero while the success test asserts exactly one. That positive
control is what makes the zero meaningful, and it is itself load-bearing:
releasing the success path with failure semantics drops it to zero, and adding a
Drop impl that uploads raises the disconnect count to one.
The counter observes reaching the upload site, not an S3 request. TigrisClient
wraps a concrete SDK client with no injectable endpoint, and plumbing one through
would be out of proportion to the risk here.
The lock-pool work gave `acquire_write` a distinct exhaustion error and mapped it
to 503 with Retry-After, but only on the push handler. The three other callers,
both issue writes and the PR merge, still turned it into a generic git error and
returned 500. Those are exactly the callers that hold no concurrency permit, so
they are the likeliest to meet an exhausted pool in the first place.
All three now go through the push handler's helper rather than repeating the
mapping, so the log level and the message cannot drift between the four sites.
Each has a test that occupies the single connection of a one-slot pool with a
guard on a different repo, so the failure is pool capacity rather than
advisory-lock contention, and asserts status 503 plus retry-after through
IntoResponse. Reverting the mapping at one site turns only that site's test red.
Worth knowing for review, and left alone deliberately: close_issue takes the
write guard before its author check, so this shed is reachable by any
authenticated caller whether or not they could close the issue. That ordering
predates this change and reordering the handler is not this commit's business,
but it does mean the 503 discloses that the repo's write pool is busy.
Four follow-ups from an adversarial pass over the previous commits.
The marker clear ran unconditionally inside `record_pin_source`'s transaction,
but its INSERT is `ON CONFLICT DO NOTHING` guarded by the cap, so it records
nothing when the repo is already a source. That is the common case, not a rare
one: the pin path calls `record_pin_source` for every already-pinned object, and
on a requeue pass that is the whole-repo enumeration. So a re-push from the repo
that was already recorded cleared a marker set by a different repo's failure and
the public copy went back to 404ing. The clear is now gated on the insert
actually adding a row. A genuine record from a third repo still clears the
marker, which needs a per-(oid, repo) table to close and is documented rather
than papered over.
The sweep advanced its cursor before every skip, which stops a wedge but made a
transient skip permanent: once the walk reached the end of the table the cursor
parked there and no later boot read another row. On a Tigris-backed node the
common skip is "repo is not on local disk", so the sweep would repair whatever
happened to be warm at boot and then never run again, and the new advertise
filter turned those rows from "advertised but 404" into "never advertised, never
repaired". Repairs now report whether a skip was retryable or terminal, and a run
that skipped anything retryable rewinds the cursor once, after the walk, so the
next run re-walks. A permanently unrepairable row is terminal and does not
trigger a rewind, so this cannot become a hot loop.
The sweep also reached its repo paths through the raw join helper, skipping the
three-layer traversal barrier `RepoStore::local_path` applies. A test with a repo
row named `../../escapee` showed it reading and repairing bytes from outside
`repos_dir`. That barrier is now extracted and shared, so the two callers cannot
drift. And the repair's synchronous `git cat-file` moved under `spawn_blocking`,
matching the existing treatment of the withheld walk, so a wedged read no longer
parks a worker thread for the full service timeout.
Finally the resolver's two source-set queries ran ahead of the work-budget peek
that exists to shed a spent caller before doing work. The peek now runs first. A
throttled caller whose set turns out complete gets 429 where it used to get 404,
which is the honest answer since its search never ran, and it removes an oracle
that let a throttled caller tell a complete source set from an incomplete one.
Three regressions from the previous four commits, each demonstrated before being
fixed.
The advisory-lock retry loop checked its connection out before spinning, so a
caller that lost the lock held a pool connection through all sixty one-second
attempts. The old pool-per-query shape returned it between iterations, so a
spinner occupied nothing. That matters more than the pool sizing suggested:
`acquire_write` has three non-push callers (two in issues, one in pulls) that
hold no concurrency permit, and the issue routes carry no rate limit, so any
self-minted DID could fire enough concurrent closes at one repo to strand the
pool and fail authenticated pushes on every repo. The checkout now happens per
attempt and a losing attempt returns the connection before sleeping, so only the
winner retains one. Lock-pool exhaustion also carries its own error now and sheds
503 with Retry-After instead of surfacing as a 500.
Holding the lock on one connection then meant a client disconnect released it
early: the handler future dies mid-receive-pack, the guard drops without
`release`, and `after_release` frees the lock while the detached reaper is still
giving the process group its SIGTERM grace. A second writer was admitted 90ms
after the drop, well inside that window, which is the invariant smart_http
already documents. The write guard now rides the AdmissionGuard into the reaper,
the same seam the admission permits already use, so the lock outlives the group.
The success path still reclaims the guard and releases it synchronously, and a
dropped guard performs no Tigris upload, so an interrupted push cannot publish a
half-applied repo.
The requeue loop's exit on an exhausted re-read discarded a push that coalesced
during the retry window: `requeue_or_release` had cleared the dirty bit without
marking the guard released, so `Drop` removed the key and no pass was ever
attempted. It now falls through with nothing to replicate and lets the tail's
atomic check-and-clear decide. A lap only happens when a push actually coalesced
and each pays a full bounded re-read, so a sustained outage still terminates,
which is tested with ten thousand injected faults under a watchdog rather than
argued.
Also clamps the lock pool's derived size, which previously followed
max_concurrent_git_pushes up to its million-connection ceiling, and documents the
node's total connection budget on that knob.
`repair_legacy_provider_cid` had exactly one trigger: the already-pinned skip
branch inside `pin_new_objects`, which a push only reaches when it re-carries the
object. Normal git negotiation omits objects the node already has, so on an
upgraded node that push generally never arrives. Meanwhile `list_pinned_cids`
kept advertising the stored provider CID that this branch's `/ipfs/{cid}`
deliberately withholds, so the node handed out CIDs it would then refuse to
serve, and previously pinned data stayed unresolvable.
Two halves, both needed. A detached background sweep walks `pinned_cids` in
bounded batches with an inter-batch delay, resolves each row's repo from its
recorded provenance, and reuses the existing repair (and its cost gate, so a row
already keyed on a raw CIDv1 reads no bytes). Migration v16 adds a single-row
cursor table so a restart mid-walk resumes rather than rewinding; the cursor
advances on every row read, before any skip, so an unrepairable row cannot wedge
the walk. A row whose bytes are gone, whose provenance is NULL, or whose repo is
not on local disk is left exactly as it is. And `list_pinned_cids` now omits rows
whose key is not a raw CIDv1, so the window before the sweep catches up
advertises nothing the resolver withholds.
The sweep resolves paths directly rather than through `repo_store.acquire`, which
would pull cold repos back from Tigris and turn a repair pass into a bulk
restore. Defaults are deliberately quiet: 64 rows per batch, 60s between batches.
Ten tests cover the repair, the bytes-gone and unrepairable arms, the cost gate,
the batch bound, cursor resumption, both states of one row through the advertise
filter, and the degenerate empty and no-legacy-rows tables. Removing the filter,
the batch bound, the cursor advance, the bytes-gone early return, or the
inter-batch sleep each turns a test red.
One pre-existing test used placeholder resolver keys that the new filter drops;
its values are now real raw CIDv1s with every assertion unchanged.
The resolver treated a non-empty, below-cap pin-source set as proof that every
source had been recorded, and skipped the bounded scan on that basis. But
`record_pin_source` is best effort at every call site: the ipfs_pin sites were
warn-only once their retries exhausted, and both pinata sites had no retry at
all. An object first pinned from a private repo and later pushed from a public
one whose source write failed keeps a set naming only the private source, so
`GET /ipfs/{cid}` returned 404 for an object the public repo would have served.
Migration v15 adds `pinned_cids.pin_sources_incomplete`, NOT NULL DEFAULT FALSE
so every existing row reads as complete. A record that fails outright sets it; a
later successful record clears it, inside the same transaction as the insert so
the two paths cannot drift. `needs_scan` now ORs the marker in, short-circuiting
after the existing empty and at-cap checks so it costs nothing on the serve path.
Pinata's two bare calls now share `retry_db_record` with ipfs_pin. The ipfs_pin
first-pin path additionally records the CID and its source in one transaction;
Pinata's remains two retried calls.
INV-10 is preserved: a complete set still tail-404s with no preload, asserted on
the existing preload counter. The fallback is not an authorization bypass, and
the test for that asserts the scan actually ran before checking the denial, so
the denial cannot pass vacuously. Dropping the marker term from the gate, forcing
it true, or removing the clear each turns a test red.
The marker is per-object rather than per-(object, repo): a successful record from
one repo clears a marker set by another's failure. That is the conservative
direction, since the marker only ever adds the fallback back.
`requeue_or_release` clears the dirty flag atomically with the decision to loop,
which is correct and stays as it is. The problem was what the pass did next: a
failed `get_repo_by_id` fell into an Err arm that yielded no rules, and
`list_visibility_rules(...).ok()` collapsed its own error the same way. With no
rules, `replication_withheld_set` returns `(false, None)`, the pass skipped its
work entirely, and the loop exited having consumed the dirty bit that stood for
the coalesced push. There is no reconciliation sweep to re-derive it, so that
push's objects were never pinned and its withheld blobs never sealed.
`requeue_refresh_state` now separates the three cases the old code collapsed.
`Ok(Some)` proceeds. `Ok(None)` means the repo really is gone and releases
without touching the retry budget. `Err` from either read is transient and is
retried with doubling backoff up to three attempts. A rules-read error is no
longer indistinguishable from a repo that has no rules.
Exhausting the retries still drops the pass, which is the pre-existing residual,
but it now logs at error level with the repo id and attempt count instead of
disappearing behind a warning. Six tests cover the retried-then-lands path, the
bounded-exhaustion path, repo-gone, the rules-read arm, freshness of the applied
rules, and the coalescing property that had to survive. Reverting the Err arm,
restoring the `.ok()`, raising the bound, or downgrading the error log each turns
a test red.
`acquire_write` took `pg_try_advisory_lock` through the shared pool, then awaited
the Tigris exists/download path before constructing `RepoWriteGuard`. Two bugs
fell out of that. The `tokio::time::timeout` this branch added around the call
can drop the future inside that window, after the lock is held and before the
only unlock (`RepoWriteGuard::release`) exists, wedging every later push to the
repo. And because a session advisory lock belongs to the connection that took
it, running the lock and the unlock through the pool lets them land on different
connections: the unlock returns false and leaks, while a competing acquire that
draws the holding connection re-enters the lock and two writers run against the
same repo concurrently.
`RepoStore` now owns a dedicated lock pool built with an `after_release` hook
that runs `pg_advisory_unlock_all()`. `acquire_write` checks out one connection,
takes the lock on it, and moves it into the guard, which unlocks on that same
connection. sqlx's `PoolConnection::drop` spawns `return_to_pool`, which runs the
hook, so a connection dropped by cancellation still clears its locks; the unlock
is asynchronous with respect to the drop, which the tests account for. The pool
is separate from the main query pool because a push holds its connection for the
whole receive-pack and `db_max_connections` (20) sits below
`max_concurrent_git_pushes` (32).
Seven tests cover the cancellation window, the same-session release, and the
serialization the pool-mismatch bug was breaking. Both the cancellation
regression and the concurrent-writer case were observed red before the fix, and
removing the `after_release` hook or moving the unlock back to the pool turns
them red again.
A cross-model adversarial pass found that run_post_push_replication holds the
per-repo coalescing key until requeue_or_release, but pin_new_objects reached it
only after two unbounded git reads: the U7 legacy-repair read and the
pre-existing pin read, both plain Command::output with no teardown. A wedged
cat-file on a stuck backend hung the task forever, so the key was held until
process death and later pushes only marked it dirty without spawning a
replacement (the same class this PR closed on the /ipfs serve path).
Add read_object_bounded (the bounded twins under one shared deadline) and use it
at both sites; a timeout skips that object and lets the task proceed to
requeue_or_release. Adds a wedge-reap regression proven load-bearing two ways.
Code review found each candidate's size and content reads were each granted a
full git_service_timeout, so a single served candidate could hold the /ipfs
walk permit for 2x the timeout. Share one deadline across both stages (mirrors
build_filtered_pack) so the read holds admission for one timeout total, and
correct the admission-bound comment: the worst case is O(candidates x timeout)
bounded by the walk concurrency cap, not a single deadline.
Before this PR the pin path stored the provider CID (Kubo dag-pb / Pinata) in
pinned_cids.cid, which the new resolver recomputes-and-404s, while
/api/v1/ipfs/pins still advertises the stale value. Repair a legacy row in the
already-pinned skip branch: gate on a cheap stored-CID codec check (only a
non-raw-codec CID reads bytes, so raw rows keep the DB-only skip cost),
recompute the raw CID, and rewrite it while stashing the old value in a new v14
legacy_provider_cid column (distinct from pinata_cid, which gates the Pinata
pin-skip). Rows whose bytes are gone stay withheld. Documents that the deferred
one-shot sweep, not this opportunistic path, fully retires the window.
The three warn-only record sites in the detached post-push task dropped a
transient DB failure silently, leaving a permanently incomplete pin-source set
that makes the resolver 404 a valid public copy. Wrap them in a bounded retry
(inserts are idempotent via ON CONFLICT DO NOTHING); on exhaustion the warn
still fires so behavior degrades to today's. Documents the residual crash/outage
window that only a reconciliation sweep retires.
The route middleware charged ipfs_rate_limiter once per request while the
legacy scan and provenance walk charged the same bucket per probe/walk, so a
one-probe request cost two tokens and GITLAWB_IPFS_RATE_LIMIT=1 admitted the
request at the route then 429'd it at the pre-scan peek.
Add a bounded ipfs_work_rate_limiter, move the in-scan, per-walk, and pre-scan
charges onto it, and leave the route limiter as the pure once-per-request
brake. The work-budget capacity derives from the route limit with a floor of
one full legacy-probe budget per window (no new operator knob), so a
default-config deep search never self-throttles. Adds it to the periodic sweep,
updates all three AppState construction sites, and rewrites the contradictory
limiter comments.
The knob was parsed and documented as the per-request /ipfs fan-out cap but
never read in production: AppState seeded the legacy-probe budget from a fixed
256 constant, so setting the knob did nothing and an operator could still incur
256 acquire/probe operations per request.
Seed ipfs_max_legacy_probes from the knob (default raised to 256 to preserve
shipped behavior); leave the history-walk ceiling on its MAX_PIN_SOURCES+1
constant so a provenanced request is never truncated into a false 503. Updates
the README and .env.example to the shipped behavior and adds cap-honored and
plumbing tests.
Per-repo coalescing dropped every push that arrived while a repo's encryption
task was in flight, and no reconciliation ever processed it, so a withheld
blob added by the coalesced push had its recovery copy permanently absent. The
drop also silently skipped the coalesced push's local-IPFS pin work.
EncryptInflight becomes a dirty-flag map; the detached task loops, and the
atomic check-and-clear sits at the task tail, unconditional on the
has_path_scoped_rule gate and walk success (a public or rules-free repo would
otherwise exit before it ran). Each requeue re-reads repo state fresh and
re-enumerates the pin half through the fail-closed full scan, never bare
list_all_objects, so a coalesced rule change is honored and no withheld or
dangling object leaks. At-most-one-task-per-repo is preserved.
get_by_cid held the global walk permit and the per-source permit as handler
locals, so dropping the request future released both while the spawn_blocking
probe, visibility walk, and object read kept running; a cancel-spam or timeout
client could exceed MAX_CONCURRENT_IPFS_WALKS and the per-source cap, and the
bare cat-file probe/read children had no teardown at all.
Move the gated serve pipeline into a detached task that owns an AdmissionGuard,
awaited by the handler, so admission releases only after the bounded work is
gone; back the /ipfs probe and read with run_bounded_git twins for
process-group teardown. Adds cancel-mid-walk, timeout-reap, and cancel-spam
regressions plus two load-bearing INV-22 rows. Also reflows one U1 test line to
satisfy cargo fmt.
The path-scoped upload-pack branch kept the read and per-caller permits as
handler locals and called upload_pack_excluding with no AdmissionGuard, so a
cancelled filtered clone released admission while KillGroupOnDrop was still
reaping the git group, bypassing the read and per-caller concurrency caps.
drive_git_child now returns the disarmed guard on success so one guard rides
both build_filtered_pack stages (rev-list then pack-objects); the handler builds
the guard from the permits as the plain path does. Adds a disconnect regression
and an INV-22 gate row, both proven load-bearing.
- steal_after: the reclaim is NOT a guarantee that only a leaked lease is
reclaimed. A waiter's timeout starts at acquire(), not at the FIFO head, so a
same-repo backlog whose cumulative wait exceeds steal_after can steal while an
earlier waiter still writes; correctness rests on the retained pg advisory
lock (which serializes the stealer at acquire_write), not on the bound.
- pinata_object_list_for_refs: recomputes from the tail-start rules snapshot,
not a fresh read at pin-worker time, so a rule tightened AFTER tail-start is
not reflected (matches the old retained-list behavior; reconciliation sweep is
the backstop). Removed the overstated 'rule tightened since the push is honored'.
- RepoWriteGuard::Drop: note that on runtime shutdown the detached unlock task
may be dropped before it polls, bounded by pool teardown / connection close.
The deterministic-fault terminal 500 was checked before the transient
truncated_by 503, unconditionally. When one repo is corrupt (deterministic)
while a DIFFERENT repo is transiently skipped in the same scan, the requested
CID may live in the transient repo -- a retryable 503 is correct, but the
handler returned a terminal 500 and a conformant client won't retry, hiding
retrievable content until the unrelated corrupt repo is repaired.
Gate the 500 on truncated_by.is_empty(): it fires only when a deterministic
fault is the SOLE reason nothing served; a co-occurring transient taint falls
through to the retryable 503. A pure deterministic fault still terminally 500s
an absent lookup by design. Adds a co-occurrence regression test.
Code review found a DoS the F3 lease introduced: git_receive_pack acquired the
global and per-source write permits before block-waiting on the per-repo lease,
so a second same-repo push that parked on the lease pinned a scarce global
write-pool slot (1 of 32) for up to steal_after while sending zero bytes. A few
hostile sources stacking same-repo pushes could hold every slot on idle
lease-waiters and shed 503 on every push to every other repo node-wide.
Move both permit acquisitions to after the lease acquire so a lease-blocked
waiter pins no write-pool slot, and keep a cheap non-holding availability peek
before the DB lookup so a push flood on a saturated pool still sheds 503 without
touching Postgres (the authoritative held permit is taken after the lease).
Adds a blocked-waiter-holds-no-permit regression test.
On client disconnect, RepoWriteGuard::Drop released the per-repo advisory
lock immediately (it holds no child/pgid), while KillGroupOnDrop's detached
reaper was still tearing down the git process group for ~4s. A second same-node
push could then acquire the repo and race the first's still-writing objects/
dir and Tigris upload.
Add an in-process per-repo write lease (block-and-wait, keyed like the advisory
lock) that SUPPLEMENTS the retained cluster-wide PG lock. The lease rides into
the disconnect reaper via a clone on the write-path AdmissionGuard (None on all
read paths) and a second clone held across guard.release() for the clean-path
Tigris upload, so it frees only after the group is reaped. A bounded-wait steal
reclaims a leaked lease (block-and-wait has no coalesce degradation). The lease
is always taken before the PG lock and released after, so the two serializers
cannot invert. Deterministic fake-git race test + inv22 gate row.
The detached Pinata task moved each push's full object-id list into the
closure and parked it on pin_semaphore.acquire_owned(); a tokio semaphore
bounds active holders, not the waiter queue, so a slow Pinata backend let N
queued pushes each retain an MB-scale OID list (unbounded memory/tasks).
Capture only the small ref-update tuples and re-derive the object set via the
existing reaped/deadline-bounded git helpers AFTER the pin permit is acquired,
so retained memory is O(ref tuples), not O(object lists). Per-ref effects are
unchanged (still one spawn per push, no coalescing or shedding, so no dropped
announcements). Adds an inv22 gate row forbidding a retained object list across
the acquire, plus re-derivation-equivalence and reaping tests.
list_all_repos and list_visibility_rules_for_repos ran as bare awaits while
holding the scarce IPFS walk permits, so a query blocked in Postgres pinned the
slots past GITLAWB_IPFS_REQUEST_BUDGET_SECS and shed later requests as 503s.
Wrap both in the remaining request-deadline budget; on timeout the RAII permits
drop and the handler sheds a retryable budget 503. Fails closed: a timeout on
the visibility-rules query denies the request rather than serving an unfiltered
listing.
Appends an inv22 gate row asserting both queries stay deadline-wrapped.
Probe existence via `git cat-file --batch-check` (structured `<oid> missing`
on exit 0) instead of matching English `cat-file -t` prose, so a git wording
change cannot flip a miss into an error or vice-versa. A bad-config/corrupt
repo on a readable store is now a terminal, non-retryable 500 (not a false 404,
and not a retryable 503 that would fan out cat-file retries); a transient
unreadable store stays 503. Error bodies are opaque (no raw git stderr).
Layers onto the existing object_store_readable + re-probe disambiguation.
Await pg_advisory_unlock while the pooled connection is still owned by
self; take it and set released only after the await resolves. A cancel
mid-unlock now leaves the Drop backstop armed (conn still Some), so the
session-level advisory lock is no longer leaked onto a pooled connection.
Adds a cancel-mid-unlock regression test (via a test-only pre-unlock gate)
and an append-only inv22 gate row asserting the take/released orderings.
EncryptInflight bounds the outstanding post-push encryption-task set to one per
repo, but its state.rs doc implied a global cap it does not deliver, and each pin
loop (ipfs_pin + pinata pin_new_objects) holds a full per-push object-id list
while walking it — so N distinct authenticated first-pushes could hold N MB-scale
lists at once, unbounded.
Add a global pin_semaphore (GITLAWB_MAX_CONCURRENT_PIN_TASKS, default 8) and gate
BOTH pin loops through it: they DEFER when the pool is full, never drop a pin.
Tighten the state.rs comment to state the bound is PER-REPO, with the cross-repo
residual (throttled by auth + rate limits, memory bounded by the pin permit)
called out honestly.
Also correct .env.example GITLAWB_DB_MAX_CONNECTIONS (20 -> 48): with the #174 F1
boot validation, 20 is below max_concurrent_git_pushes(32)+headroom(8) and would
fail startup — closing the INV-24 doc gap from the F1 change.
Tests: pin_new_objects_gated_defers_when_pin_pool_exhausted (RED without the
permit acquire); max_concurrent_pin_tasks range/default.
object_type_bounded treated any bare `fatal:` cat-file exit as absence. A packed
object whose pack/idx is transiently unreadable (permissions, or a mid-repack
race) emits `could not get object info` — byte-identical to a genuine miss — so a
present object could return Ok(None) and the /ipfs scan a definitive 404 instead
of the intended retryable 503. The stderr cannot separate the two (the CID is
pre-validated, so that string is also the only legitimate absence path), so the
disambiguation must be out of band.
On the collided fatal, probe object_store_readable (objects/ listable, every
pack/idx openable). Unreadable -> Err (taint -> 503). Readable -> re-probe once;
still absent -> Ok(None), now present -> the type. This narrows but does not close
the concurrent-repack TOCTOU (the check samples a different instant), stated
honestly rather than claimed deterministic.
Test: object_type_bounded_unreadable_pack_is_error_not_absence packs a real repo,
chmod 000 the pack, and asserts Err (root-guarded). RED with the readability check
disabled: the present blob returned Ok(None).
fail_closed_full_scan_objects ran phase 1 (replicable_blob_set_bounded) then
phase 2 (all_blob_oids) with a FRESH Instant::now() + timeout for phase 2, under
one held scan permit. A large-but-successful phase 1 plus a full phase 2 held the
permit ~2x the configured budget instead of the one whole-scan budget.
Compute one deadline before phase 1 and share it: phase 2 takes the shared
Instant directly, phase 1 takes the remaining-until-deadline. Total occupancy is
now ~1x. The completeness cost is honest: a repo whose phase 1 nears the budget
under-pins this push (fail closed) rather than over-holding, so size the budget so
both phases normally fit.
Test: full_scan_shares_one_deadline_across_both_phases drives a slow-but-
successful phase 1 that eats the budget; the shared deadline reaps phase 2 and the
scan fails closed (empty). RED before: phase 2's fresh budget completed and kept
the non-blob candidate.
The GET /ipfs/{cid} object-type probe and content read ran bare synchronous
Command::output() on the async worker, budget-checked only before they started.
A hung or corrupt object store therefore pinned a Tokio runtime worker and both
held IPFS admission permits indefinitely, so enough requests exhausted the route
despite GITLAWB_IPFS_REQUEST_BUDGET_SECS. The walk beside them already used
spawn_blocking + the reaped bounded runner; these two stages were left bare.
Extract run_bounded_git_raw (returns ExitStatus/stdout/stderr so callers can
classify exit codes; run_bounded_git stays a thin bail-on-nonzero wrapper for the
walk callers) and add store::object_type_bounded / read_object_content_bounded on
it, preserving object_type's Ok(None)-on-absence vs Err classification so the
serve path's 404-vs-503 semantics are unchanged. Run both /ipfs stages in
spawn_blocking under a deadline clamped to min(git timeout, remaining budget),
with the IPFS permits held across the awaited spawn_blocking.
Test: get_by_cid_hung_probe_is_reaped_and_sheds_503 hangs git on a FIFO alternates
with no feeder; the reaped probe sheds a 503 in bounded time (GREEN), while a
neutralized deadline blocks the handler forever (RED). The existing probe-error /
corrupt-repo tests still assert the absence-vs-error classification at the handler.
The post-receive replication tail (withheld/candidate/full-scan resolution, then
the encrypt+recovery spawn and the announce spawn) ran inline in the request
future and parked on git_encrypt_semaphore before the durable try_begin gate. A
client/proxy disconnect while parked dropped the future and silently lost this
push's pins, recovery copy, and announcements, with no reconciliation sweep to
recover them (state.rs documented this exact residual).
Move the whole tail into an independently owned tokio task and return the git
response immediately, so a disconnect can no longer drop the parked work. Each
push owns its own tail, including its own always-spawned announce, so the
announce is never brought under the per-repo encrypt coalescing (which would
drop a coalesced push's per-ref announcements). Update state.rs to retire the
disconnect-loses-work residual.
Test: receive_pack_landed_push_returns_without_parking_on_scan_pool asserts the
handler returns 200 while the scan pool is held (RED with the tail inline: the
request future parks and the timeout fires; GREEN detached). The burst
serialization test is retimed to poll for scan completion since the withheld
walk is now detached too.
acquire_write took the pg advisory lock, then awaited Tigris, then built the
guard. An outer tokio::time::timeout firing during the Tigris await dropped the
future with the lock held and no guard, so no unlock ran and later pushes to the
repo spun 60s. release() also unlocked on an arbitrary pooled connection, not the
one that locked (session advisory locks are connection-affine), so even the
normal release was unreliable.
Pin a PoolConnection in RepoWriteGuard, build the guard before the lock query,
run the lock loop through it, unlock on that same connection in release(), and
add a Drop backstop that spawns a detached affine unlock (Handle::try_current so
an off-runtime drop logs instead of panicking). Two distinct-session tests
(drop-without-release, affine release) go RED without the fix, GREEN with it.
Pinning a connection per active write is a DB-pool DoS unless the pool clears
the concurrent-write cap: the shipped default (20) was below
max_concurrent_git_pushes (32). Raise the db_max_connections default to 48 and
add Config::validate() to reject db_max_connections < max_concurrent_git_pushes
+ 8 headroom at boot; wire it into startup.
The budget-knob docs claimed the overshoot was bounded by one unclamped
probe; an unclamped subprocess is not a bound, so the README,
.env.example, config help, and module comments now say plainly that a
hung git probe holds the walk slot for the hang's duration. New
regression: an acquire-tainting ghost row ahead of a healthy public copy
must not stop the scan; the later found short-circuit still serves 200.
object_type now distinguishes repo-level git failures (not-a-repository,
error:-prefixed corruption) from genuine object absence, so a corrupt
repo taints the /ipfs scan into the retryable 503 instead of vouching
for a definitive 404; the drain reads visibility rules by the freshly
fetched record id, closing a latent fail-open on id divergence. The four
budget gates collapse into one stage-labelled helper, the three scan
sites share state::acquire_scan_permit (inv22 tripwire repointed and
sever-verified), and the Tigris stall tests use a local accept-and-park
listener instead of a non-routable address.
try_begin now merges a losing push's tip pairs into the in-flight key's
pending slot in the same critical section as the presence check, bounded
at 1024 pairs with a full-scan marker on overflow (force_full_scan
threads to the resolver; empty tips never encode the marker). The task
loop-drains: finish_or_take_pending either hands back the batch with the
key retained, or removes the key and disarms the guard atomically, so a
successor task's key survives the guard drop. Drains re-fetch rules
fresh and replay the tail pipeline under the U4 helper gates only; the
loop holds no task-level pool permit, so a hot repo cannot deadlock the
pool at size 1.
replication_withheld_set's walk arm, resolve_candidates_for_push's git
stages, and fail_closed_full_scan_objects each take a
git_encrypt_semaphore permit before their spawn_blocking and carry it
inside the closure, so a push burst holds at most pool-many concurrent
scans and a started scan finishes with its permit. Deletion-only pushes
and repos without path-scoped rules never park. Contention defers the
landed push's tail (logged with queue_wait_ms), never errors it; the
acquire sites document the accepted disconnect-during-park residual.
GITLAWB_IPFS_REQUEST_BUDGET_SECS (default 600) is captured as one
deadline at handler entry: every acquire and expensive walk is clamped
to the remaining budget, every stage checks remaining before starting,
and exhaustion taints the scan into the retryable 503. A walk in flight
is never aborted at the tokio layer (that would free the walk permit
while the blocking thread still runs); its clamped git deadline plus
group teardown ends it. Docs state the residual overshoot: the in-flight
stage's kill/reap slack plus the unclamped probe subprocesses.
A request-scoped taint list replaces the silent folds: any repo skipped
without a verdict (acquire timeout, probe or read failure, walk failure,
cap or ceiling exhaustion) turns the terminal into a retryable 503 with
Retry-After, and 404 is reserved for a scan in which every candidate
reached a verdict. The walk cap now counts only expensive allowed-set
walks and skip-continues on exhaustion so a plain public copy past the
cap still serves; a new GITLAWB_IPFS_MAX_REPO_VISITS knob (default 1024)
bounds the acquire+probe cost class and stops the scan, documented with
its worst-case object-store fetch count.
drive_git_child hands the AdmissionGuard back on success so
upload_pack_excluding can thread it across the rev-list and pack-objects
stages; on timeout or error it is dropped post-reap, and on future-drop
it rides the detached reaper as before. The upload-pack handler builds
the guard once ahead of the withheld/plain branch, so a client
disconnect mid-filtered-serve no longer frees the read-pool and
per-source permits while the git process group is still being reaped.
An attacker who pins a public object from MAX_PIN_SOURCES repos before the
legitimate public source registers fills pin_repo_sources (record_pin_source is
first-N-wins and drops later sources silently); the buried public source is then
never reached because a non-empty provenance set suppressed the legacy scan, so
anon GET /ipfs/{cid} 404s forever for a public object (re-breaks F1).
U1: db::pin_sources_at_cap reports whether pin_repo_sources is at MAX_PIN_SOURCES
for an oid (the only observable signal that a servable source may have been
dropped, since the write cap never overshoots).
U2: get_by_cid falls back to the bounded legacy scan on a provenance miss when the
set is empty OR at_cap. The scan gates every repo through the real per-caller gate,
so it finds the buried public copy. A complete (non-full) set still fast-404s, so
ordinary denials never fan out to O(repos) (INV-10/F3); the fallback honors the
is_throttled peek.
Regression ipfs_cid_buried_public_source_still_serves_via_scan_fallback: 404 before,
200 after; pin_sources_at_cap_flips_at_max covers the boundary.
- ipfs_pin::pin_new_objects returns the provider Hash (not the raw resolver key),
matching the pinata twin's contract; the DB cid stays the raw content CID. The
return is logging-only here, so this is drift-avoidance, not a functional fix.
- AppError::SearchIncomplete now carries Retry-After: 1 like Overloaded, so both
retryable 503s from the /ipfs handler advertise the retry hint consistently.
A provenanced object has a bounded source set (first-pinner + up to
MAX_PIN_SOURCES additional). With the walk ceiling at 16 == MAX_PIN_SOURCES, an
authorizing public source sorting after 16 path-scoped denials was never reached:
the ceiling truncated the search and returned a false 503 for a readable object.
Raise the ceiling to MAX_PIN_SOURCES + 1 so the whole bounded provenance set is
always tried; the legacy scan stays bounded by MAX_LEGACY_PROBES_PER_REQUEST.
P2-E: the object-type probe and the F6 size+read ran bare git cat-file inside
spawn_blocking while the /ipfs walk permit was held, so a wedged cat-file could
pin the global walk slot for the request's life. Bound both under
git_service_timeout_secs; on timeout free the slot (truncated -> retryable 503)
and skip the repo.
P2-D: the pinata already-pinned branch now backfills NULL first-pinner
provenance in lockstep with the ipfs_pin skip branch (consistency; the object
was already resolvable via the pin_repo_sources union).
P1-A: gate_and_serve held the /ipfs walk permit across a bare repo_store.acquire;
wrap it in git_acquire_timeout_secs (mirroring #174's own handler) so a cold/hung
Tigris acquire cannot pin the global walk slot. On expiry skip the repo and mark
the search truncated (retryable 503, not a false 404).
P1-B: the local-IPFS pin path recorded Kubo's provider Hash as pinned_cids.cid;
for objects above the block size that is a dag-pb root that does not hash the raw
content, so GET /ipfs/{cid} listed then 404'd them under the F2 integrity check.
Record the locally-computed raw-content CID, mirroring the pinata twin.
Rebased onto #174 (fix/served-git-concurrency-cap). #173's incremental history
cannot replay commit-by-commit because #174 rewrote the same handler files in
parallel; this single commit carries the fully-integrated tree (identical to the
verified merge result), with the follow-up fixes as separate commits on top.
Brings in #173: GET /ipfs/{cid} CID->oid resolution via pinned_cids, per-caller
path-scoped blob/tree/commit-tag gating (#135/#173 F1-F6), and the pin-source
provenance table. Adapts #173's tree/commit-tag walks to #174's run_bounded_git
so the held /ipfs walk-concurrency permit is duration-safe (F5).
P2-1: git_info_refs treated every service other than git-receive-pack as a read op
and did the DB/visibility/Tigris work under a read permit, validating the service
string only downstream in smart_http. So an unauthenticated `?service=anything` to
a public repo consumed a read slot + storage work before rejection. Validate the
service is exactly git-upload-pack or git-receive-pack immediately after parsing,
returning 400 before the pre-DB shed and the permit. Handler-layer regression:
with the read pool exhausted, `?service=git-explode` returns 400 (validated first),
not 503 — removing the validation makes it 503 (RED).
U5/INV-24 docs: document the new knobs from U2/U3 in .env.example and the README
env-var table — GITLAWB_GIT_ACQUIRE_TIMEOUT_SECS, GITLAWB_MAX_CONCURRENT_IPFS_WALKS,
GITLAWB_IPFS_WALK_PER_SOURCE, GITLAWB_IPFS_MAX_REPOS_WALKED, GITLAWB_IPFS_RATE_LIMIT.
Each is already enforced on the path it names (U2/U3 tests), so the docs match the
implemented routing.
After a successful path-scoped push the handler spawns a DETACHED task that runs
pin_new_objects then withheld_recipients_gated, which acquire_owned().await's on
git_encrypt_semaphore — it DEFERS (blocks) when the pool is full. The semaphore
caps active walks but nothing capped how many detached tasks were spawned and
PARKED on that await: N rapid path-scoped pushes spawned N tasks, each holding
cloned object lists/rules/paths/keys — an unbounded parked-waiter set (P2-2).
2a54c15 deliberately chose defer-not-shed because dropping the walk loses the
withheld-blob recovery copy and there is no reconciliation sweep to rebuild it.
So bound the OUTSTANDING-TASK set by per-repo coalescing rather than shedding:
EncryptInflight (a bounded Arc<Mutex<HashSet<repo_id>>>) + try_begin — before the
spawn, if a task for the repo is already in-flight skip the duplicate (the pending
walk covers the newer objects); otherwise spawn and move an RAII
EncryptInflightGuard into the task that removes the key on drop (completion, error,
or panic-unwind). This bounds the set to <=1 pending task per repo and never drops
work; withheld_recipients_gated's defer is unchanged.
The second detached post-push spawn (Pinata replication, also runs pin_new_objects)
is scoped out with rationale: it parks on no semaphore (no unbounded waiter set)
and does per-push per-ref work (branch->CID, gossip, subscription, Arweave,
peer-notify) keyed to this push's ref_updates — coalescing it would DROP a later
push's announcements, a correctness regression.
Regressions (mutation-verified RED->GREEN): the outstanding set is bounded to 1
per repo under saturation (never-coalesce -> 32 tasks RED); a coalesced repo's key
is released when its task ends so it is reprocessed, never permanently skipped
(guard-drop no-op -> repo locked out, recovery copy lost forever, RED — the exact
durability regression the fix prevents); distinct repos each admit; cold-set first
push admits; plus an inv22_gates structural tripwire that fails if the handler gate
is removed.
GET /ipfs/{cid} (get_by_cid, publicly reachable via optional_signature) ran
allowed_blob_set_for_caller_bounded in spawn_blocking inside a per-repo loop with
NO global/per-source admission and NO route rate limit — the surface had zero
concurrency or rate control. A permissionless caller could fan out concurrent
full-history git walks (each up to the walk timeout) exhausting blocking-pool
threads and PIDs outside every served-git pool (P1-3).
Acquire a global git_ipfs_walk permit (try_acquire_owned, shed 503) plus a
per-source sub-cap (with_default_max_keys, reject-before-insert, keyed via
client_key/TrustedProxy so XFF can't spoof it) ONCE after CID validation, held as
handler locals across the whole request — including every spawn_blocking walk, so
the slot reflects real blocking-thread occupancy (a tokio timeout cannot cancel
spawn_blocking). None key -> global pool only. Cap repos walked per request so one
request can't serialize N full-history walks. Attach rate_limit_by_ip +
IpRateLimiter to the /ipfs route (mirrors the git/create/peer routers; the
extension is required or the middleware is a silent no-op). Visibility/authz
semantics unchanged.
New knobs: GITLAWB_MAX_CONCURRENT_IPFS_WALKS (32), GITLAWB_IPFS_WALK_PER_SOURCE
(4), GITLAWB_IPFS_MAX_REPOS_WALKED (64), GITLAWB_IPFS_RATE_LIMIT (600/hr, 0
disables) — all range-validated and enforced on-path.
Handler-layer regressions (all mutation-verified RED->GREEN): shed-at-capacity
503 (delete acquire -> falls through), per-source cap sheds same source / admits
another, None-key arm sheds on the global pool, map self-bounds reject-before-insert,
the walk permit is held while the spawn_blocking walk runs (available_permits()==0;
dropping it before the loop -> RED), repos-walked cap (remove break -> 2 walks),
and the route IP rate limit actually fires 429 (drop the Extension -> RED no-op).
The served-git admission permit is acquired, then RepoStore::acquire*/advisory-lock
runs before the bounded git runner starts — with no local deadline. acquire/acquire_fresh
await Tigris HEAD/GET, and acquire_write's advisory-lock loop awaits a per-iteration
pg_try_advisory_lock that can block indefinitely on a hung Postgres pool. Since
GITLAWB_GIT_SERVICE_TIMEOUT_SECS only starts once git spawns, a stalled backend pins
the permit and drains the pool until every later request 503s (P1-2).
Wrap each git-path acquire (git_info_refs read/advert, git_upload_pack, git_receive_pack)
and the /ipfs per-repo acquire loop in tokio::time::timeout(git_acquire_timeout_secs, ..).
On expiry the git paths return AppError::Overloaded (503 + Retry-After); the permit is a
handler-local at that point (moved into the AdmissionGuard only after acquire) so the early
return frees the slot. The /ipfs per-repo acquire keeps its fail-closed `continue` on
timeout (never serves an un-acquired repo). No repo_store.rs signature change: the outer
timeout cancels a mid-sleep/mid-fetch_one future, so no in-loop deadline is needed.
New knob GITLAWB_GIT_ACQUIRE_TIMEOUT_SECS (default 30, range 1..), modeled on
git_service_timeout_secs, kept separate because acquisition and git execution are distinct
cost centers.
Handler-layer regression receive_pack_acquire_deadline_sheds_and_releases_permit: holds the
same pg advisory lock acquire_write derives on a second connection so the real loop must
retry, drives git-receive-pack with the deadline at 2s and write pool size 1, and asserts
503 at ~2s + the permit recovers + a follow-up admits once the lock frees. Removing the
timeout wrapper turns it RED (blocks ~31s to the test ceiling on the ~59s advisory loop).
On the plain (non-path-scoped) info_refs / upload-pack / receive-pack paths the
global + per-source admission permits were handler-locals that dropped the
instant the handler future dropped on a client disconnect, while KillGroupOnDrop
reaps the git process group in a DETACHED task (SIGTERM -> grace -> SIGKILL ->
reap). So a disconnect-spammer admitted replacements while prior groups were
still alive, defeating the process cap and the per-source cap during teardown.
be0cdd6 already fixed this for the path-scoped upload-pack walk; this closes the
residual plain paths (P1-a).
Introduce AdmissionGuard, a move-only holder of the permits, threaded through
info_refs/upload_pack/receive_pack/run_git_service into drive_git_child's
KillGroupOnDrop. On disconnect the guard moves into the detached reaper and drops
only after the group is ESRCH-confirmed reaped; on success/timeout disarm()
returns it for the earliest provably-free drop. The rev-list/pack-objects
visibility-walk callers hold no admission and pass None.
Thread git_bin through upload_pack/receive_pack (they hardcoded "git") so the
plain POST path is drivable by an injected fake git, matching the existing
injectable seam on info_refs/run_git_service/upload_pack_excluding.
Handler-layer regression upload_pack_plain_permit_held_through_group_reap_after_disconnect:
isolates the global read pool (size 1, per-source + rate limiter permissive), a
SIGTERM-ignoring descendant keeps the group alive across the reap window, and
asserts available_permits()==0 held on disconnect + a cross-source replacement
sheds 503, then frees after ESRCH. Reverting the guard-into-reaper move (release
immediately) turns it RED at the held-through-reap assertion. Plus a None-key arm.
A source-scan tripwire that fails when a new site reintroduces the concurrency-cap
class PR #174 fixed, or when one of the five gates (U1-U5) is removed. INV-22: a
permit held per op recovers only if every path is duration-bounded and reaps the
group before releasing admission, and every detached git task carries admission.
Lives in tests/ (a separate crate) on purpose — a guard scanning the file it lives
in would match its own identifier literals and pass vacuously; scanning src/ from
here keeps every check load-bearing. Checks: run_bounded_git confirms child exit via
child_terminated_without_reaping (U1); KillGroupOnDrop launches the reaper via
Handle::try_current (U2); git_receive_pack acquires state.git_write_per_caller (U4);
the encryption walk runs through withheld_recipients_gated over git_encrypt_semaphore
(U5); and the bounded recipients walk is invoked exactly once — only inside the gated
helper — so a new spawn_blocking bypass (count > 1) fails (P1-e non-bypass).
Proven load-bearing: a synthetic second call site turns the count tripwire RED, and
each named gate maps to a fix whose own regression already goes RED on revert.
After a successful path-scoped push the handler released its write permit and
then ran a DETACHED tokio task whose spawn_blocking(withheld_blob_recipients_bounded)
performed another full-history git walk under no admission. N fast completed
pushes spawned N concurrent full-history walks past GITLAWB_MAX_CONCURRENT_GIT_PUSHES
(which bounds only the in-handler phase), and the per-child bounding from 71389e6
caps each walk's duration but not their count (P1-e).
Add git_encrypt_semaphore, a pool of its own (sized from max_concurrent_git_pushes,
no new knob — Q1) so a long background walk never holds a foreground write slot and
a handler holding a write permit can't self-deadlock. Route the walk through a new
withheld_recipients_gated helper that acquires the pool before the walk and DEFERS
(blocks) when full rather than shedding — dropping the walk would lose the
withheld-blob recovery copy, so durability stays fail-closed.
Regression: encrypt_walk_defers_when_pool_exhausted drives the gated helper with the
pool exhausted and asserts the walk blocks and does not run its rev-list, then runs
once a permit frees. RED before (the walk runs regardless of the pool), GREEN after.
The detached push task calls this exact helper.
The authenticated git-receive-pack POST acquired only the global write semaphore
— no per-source sub-cap, unlike the read and anon-advert pools. Owner enforcement
defaults off, so one host minting disposable did:key identities could open
max_concurrent_git_pushes slow POSTs from a single source IP and 503 every other
source's push; the 600/hour push limiter bounds arrival rate, not in-flight
concurrency (P1-d).
Add git_write_per_caller (a PerCallerConcurrency sized like the advert cap,
max_concurrent_git_pushes/8), and acquire it before the global write permit,
keyed on the resolved source IP via read_caller_key — never the signed DID (a DID
farm defeats a DID key). This required adding the PeerAddr + HeaderMap extractors
to git_receive_pack, which it lacked (without them the key is None and the cap is
inert). Neutralized the shared acquire helper's log wording now that it serves
both read and write paths.
Regression: receive_pack_per_source_write_cap_sheds_capped_source_not_others — a
source at its write sub-cap sheds Overloaded/503 (proving the extractors resolve a
key), while a different source is not shed. RED before (the capped source proceeds
past admission to a git error instead of shedding), GREEN after; the existing
receive-pack advert cap test still passes.
On the path-scoped upload-pack path the per-source and global read permits were
handler locals, so a client disconnect dropped the handler future and released
both permits while the awaited spawn_blocking withheld-blob walk kept running
(spawn_blocking can't be cancelled). A permitless caller could disconnect-spam to
exceed the read cap while real git work continued (P1-b).
Move both permits into the blocking task and return them out: on success they
flow back so the serve phase keeps them; on a dropped future the returned tuple
(with the permits) is discarded only when the blocking task completes, so
admission tracks the walk's real duration.
Regression: upload_pack_permit_held_through_walk_after_disconnect drives the
handler to mid-walk (fake git hangs on rev-list), disconnects, and asserts the
global read slot stays held (available_permits == 0). RED before (the slot frees
to 1 the instant the future drops), GREEN after. The test roots the repo store at
its own TempDir so it is isolated per run.
On a client disconnect KillGroupOnDrop sent a lone SIGTERM with no escalation or
reap, so a group member that ignores SIGTERM (a wedged pack-objects, a malicious
helper) survived while the handler released its concurrency permit — letting
disconnect-spam accumulate live git processes and defeat the load shed (P1-c).
The timeout path already ran the full teardown.
Make the disconnect teardown as strong as the timeout path: the guard now owns
the child and, on drop, launches a detached reaper that runs the same
TERM -> grace -> SIGKILL -> reap-the-group sequence (reap_group_on_timeout).
Owning the tokio Child in the reaper keeps it the sole reaper of the leader, so
it never races tokio's own orphan reaper (a raw waitpid(-pgid) would).
Regression: run_git_service_sigkills_a_sigterm_ignoring_child_on_disconnect, RED
before (the descendant survives the lone SIGTERM), GREEN after; all existing
teardown/timeout/disarm tests still pass (the three hand-built-guard tests were
updated to the owned-child shape).
Residual: capacity (the handler's semaphore permit) still releases at disconnect
rather than strictly after the reaper confirms the group dead, so accumulation is
now bounded to ~one teardown window (<=4s) rather than eliminated; gating the
permit on the reap needs threading it through the serve handlers and is left for
a follow-up (it also touches the receive-pack permit lifetime U4 changes).
A group member (or the leader itself) that closes stdout before the deadline made
the stdout drain return EOF early, which stood the watchdog down via done_tx
before it could ever fire; child.wait() then blocked on the still-live child,
pinning the walk thread and its read/write permit past
GITLAWB_GIT_SERVICE_TIMEOUT_SECS. This is distinct from the descendant case
already covered: there the leader sleeps until the deadline so the watchdog does
time out; here the drain-EOF races ahead of it.
Stand the watchdog down only once the child has actually terminated, detected
without reaping (waitid + WNOWAIT) so the leader's pid stays unreaped and its
pgid un-recycled until the watchdog finishes its teardown and is joined. Past the
deadline the watchdog still owns the full SIGTERM -> grace -> SIGKILL sequence.
Regression: run_bounded_git_reaps_a_leader_that_closes_stdout_then_hangs, RED
before (blocks on child.wait past the recv budget), GREEN after; the three
existing teardown tests still pass (no recycle-hazard regression).
build_filtered_pack_times_out_a_hung_rev_list and _a_hung_pack_objects
spawn a freshly-written fake git directly, bypassing the fake_git_run_with_pids
retry the rest of the module uses. Under cargo test fork-storm load a concurrent
worker forks while the fake's write fd is still open, so execve returns ETXTBSY
and the raw io::Error surfaces where the test asserts GitServiceTimeout, failing
test (stable) intermittently.
Route both tests through build_filtered_pack_or_exec_race_retry, which retries
ONLY errno 26 (ETXTBSY) and keeps the outer 10s watchdog per attempt, so a
genuinely-missing async timeout bound still trips the watchdog and a wrong error
type still fails at the assertion. Production paths unchanged.
The finding-2 wiring test covered only list_all_objects (the full-scan fallback).
Add delta_path_exec_fns_time_out_on_a_hung_git covering the common push path:
peeled_object_type (cat-file), rev_list_delta (rev-list), and list_all_objects_with_type
(cat-file, which all_blob_oids delegates to). Each must return within the watchdog
budget on a hung git; reverting any to a bare Command::output() blocks past the recv
budget (RED-verified).
jatmn P2 (INV-24): the config help, .env.example, and README described routing the
handler no longer does.
- The read pool serves upload-pack and the UPLOAD-PACK info/refs advertisement only;
the anon receive-pack info/refs advertisement draws from its own dedicated pool
(sized like the write pool but disjoint), not the read pool and not the write pool.
- The write pool is the authenticated push POST only.
- The per-caller read cap is keyed on the resolved source IP, never the DID: a
signature does not move a caller off it (the docs claimed a per-DID budget, which
would tell a NATed authenticated client it escapes the shared cap when it does not).
- git_service_timeout_secs now also bounds the push-side pin-candidate discovery
(rev-list / cat-file), reaped via process-group teardown, alongside the info/refs
advertisements and the withheld-blob walk.
Route push_delta's candidate-discovery git children (cat-file / rev-list) through the
bounded run_bounded_git (deadline + process-group SIGKILL teardown) instead of bare
Command::output(). A hung scan no longer pins the git-receive-pack write permit past
the deadline or a client disconnect. Threads git_bin + a shared per-scan deadline
(git_service_timeout_secs) through resolve_candidates_for_push and all_blob_oids.
jatmn/CodeRabbit P1: the walk watchdog stood down as soon as the group LEADER was
reaped. A leader that exits cleanly on SIGTERM while a background member ignores
SIGTERM and closes its inherited pipes let the main drain unblock, the leader be
reaped, and the watchdog skip its SIGKILL escalation — so the handler returned while
the descendant kept running. jatmn's suggested "continue until ESRCH" could not work:
the main thread reaping the leader is what frees the pgid, so continuing to kill(-pgid)
after that races a recycled group.
Restructure the coordination instead: the main thread now defers reaping the leader
until the watchdog thread returns. The watchdog always escalates through an
unconditional group SIGKILL (no leader-reap short-circuit), and because the leader is
still unreaped while every kill(-pgid) fires, the pgid cannot have been recycled. The
grace before SIGKILL is a fixed budget (only paid on the exceptional timeout path).
Regression run_bounded_git_sigkills_a_sigterm_ignoring_descendant_after_leader_exits:
a leader that exits 0 on SIGTERM spawns an sh -c descendant (its own $$, not a ( )
subshell's parent pid) that ignores SIGTERM and closes its pipes; the descendant must
be dead when the runner returns. RED on the old teardown (survives), GREEN now. The
existing leader-ignores-SIGTERM and hung-walk teardown tests still pass.
jatmn P1: run_bounded_git was gated #[cfg(unix)], but its callers
(assert_all_refs_are_commits, blob_paths) and the module are compiled on all
targets, so a non-Unix build (the continue-on-error Windows release target) could
not resolve the function.
Add a #[cfg(not(unix))] twin with the same signature and result semantics. It bounds
a single child (no process-group teardown, which is Unix-only): threads feed stdin
and drain stderr, the main thread drains stdout, and a watchdog thread kills the
child at the deadline. The child is shared with the watchdog behind a mutex the main
thread does not hold while draining, so the watchdog can always acquire it to kill,
and killing closes stdout to unblock the drain. Best-effort (reaps only the direct
child), which is why the hardened group-aware path stays Unix-only.
Verified the twin type-checks by compiling its (portable-std) body on the native
target; the full non-Unix link is CI's Windows job.
jatmn P1: the anon-reachable GET info/refs?service=git-receive-pack drew from the
same git_write_semaphore as the authenticated git-receive-pack POST. The per-source
advert cap reserves no global slots, so ~8 sources (each within the per-IP push rate
limit) could hold all 32 write permits across acquire_fresh + the advertisement, and
the next authenticated push then 503s at admission. That is an anonymous caller
shedding an authenticated push across the auth boundary (INV-10).
Add a dedicated git_push_advert_semaphore, disjoint from the write pool. Both the
pre-DB early-shed peek and the held acquire for the receive-pack advertisement now
draw from it; the write pool is left exclusively for the POST. An advert flood can at
worst exhaust the advert pool (each source still capped by git_push_advert_per_caller
and the per-IP push rate limiter); the authenticated push pool is untouched.
Test receive_pack_advertisement_draws_from_dedicated_advert_pool proves at the handler
layer that the advert sheds when its own pool is saturated AND survives when the write
pool is saturated (the reservation). Both flip under revert, verified independently for
the pre-DB peek and the held acquire.
CodeRabbit review: the held git_permit was acquired at the top of git_info_refs
and git_upload_pack, before the per-source cap, so one source could occupy the
global pool during the DB/visibility window before its sub-cap rejected the
excess. Move the held git_permit to after acquire_read_caller_permit (still
before acquire_fresh/git, so INV-10's bound on the fresh Tigris acquire holds),
and add a cheap pre-DB early shed (available_permits() == 0 -> 503, holds no
permit) so the #62 shed-before-DB property is preserved without holding a permit
across the DB work.
The receive-pack POST is unchanged (owner-only; its top-of-handler git_permit is
deliberate for acquire_write bounding).
Tests: the two #62 shed-before-DB tests now exercise the explicit early check
(load-bearing: disable it and they fall through to the DB); three new
*_per_source_cap_sheds_with_global_capacity tests prove the per-source sub-cap
sheds a capped source even with global capacity free (load-bearing on the caller
cap). Full workspace green.
Follow-up to threading the configured timeout into the walk: the walk now honors
GITLAWB_GIT_SERVICE_TIMEOUT_SECS on both the serve and replication paths, so the
README no longer says a fixed internal deadline.
Close the reasoned-not-run gaps from the code review by making the walk's git
binary and timeout injectable, then driving the missing branches with a real
handler and a fake git instead of reasoning about them.
- Add state.git_bin and *_bounded variants of the walk entry points taking
(git_bin, timeout); the served handlers (upload-pack serve, receive-pack
replication and full-scan and encrypt-pin, and the ipfs gate) now pass the
operator-configured GITLAWB_GIT_SERVICE_TIMEOUT_SECS, so the whole walk is bounded
by the same budget as the other served-git ops rather than a fixed constant. The
git_bin-less wrappers stay for the real-git classification tests.
Newly vetted by execution (not reasoning):
- receive-pack replication path is bounded: replication_withheld_set with an injected
hung git returns within the budget and fails closed, so it cannot pin the write
permit git_receive_pack holds across it.
- a hung withheld-blob walk on the upload-pack POST returns 504 (real handler, real
repo on disk, injected hung git), proving the GitServiceTimeout -> git_service_app_error
wiring end to end.
- the watchdog status-gate: a child that exits successfully is not reported as a
timeout even when the watchdog fired (mutation-checked: drop the guard -> RED).
- SIGKILL escalation: a SIGTERM-ignoring child is still reaped via SIGKILL and the
group is gone; a truly uninterruptible D-state child (unreapable by any signal) is
the documented residual, matching the async teardown.
- the advertisement per-source cap sizing never derives 0.
Full gitlawb-node suite 515 green.
U2 moved the receive-pack info/refs advertisement onto git_write_semaphore to keep
an anonymous read flood from starving the push handshake. But the advertisement is
anon-reachable on public repos and holds its write permit across the slow
acquire_fresh Tigris download, and the only per-source brake on it was the push
RATE limiter, not a concurrency cap. So a multi-source flood of receive-pack
advertisements could hold the write pool's slots across those downloads and shed
authenticated pushes (both the advertisement and the owner-gated git-receive-pack
POST draw from the same pool). U2 thus introduced the first anonymous consumer of
the write pool the state doc promised anon could never reach; the plan's residual
note (no worse than the POST) was wrong, because the POST is owner-gated and the
advertisement is not.
Add git_push_advert_per_caller, a per-source concurrency sub-cap on the receive-pack
advertisement keyed on the resolved source IP (the same PerCallerConcurrency
mechanism U1 uses for reads), sized to an eighth of the write pool so a single
source holds at most that share and saturating the pool takes many distinct source
IPs, each also braked by the per-IP push rate limiter. The upload-pack advertisement
keeps its read-pool per-caller cap; the owner-gated POST is unchanged. Correct the
state doc for git_write_semaphore accordingly.
Handler-layer proof: a source at its receive-pack advertisement cap sheds 503 (RED
before the acquisition, 500-not-503), while a different source and the upload-pack
advertisement are unaffected. Full suite 510 green.
Code review found run_bounded_git's watchdog could return a spurious 504 and
signal a recycled process group. The watchdog runs off a wall clock on its own
thread; done_tx.send() only fires after child.wait() reaps the leader, so a walk
that finished within microseconds of the deadline took the watchdog's Timeout
branch, discarded a fully-captured successful result, and returned GitServiceTimeout
(a 504 for a walk that actually completed). Worse, the Timeout branch SIGTERMed
-pgid unconditionally after the leader was reaped, so a recycled pgid could be
signalled, the exact hazard smart_http guards via disarm-after-wait.
Set a reaped AtomicBool the instant the main thread reaps the child; the watchdog
checks it before every kill and stands down if the leader is already reaped. Gate
the timeout verdict on !status.success(), so a child that exited on its own is never
reported as a timeout even if the watchdog fired late. Add the survived-SIGKILL warn
smart_http's reap already emits, for operator visibility on a wedged (D-state) git.
The hung-walk test stays green (a killed child exits by signal, not success, so it
still surfaces GitServiceTimeout and reaps the group) and the 32 real-git
classification tests stay green (a fast walk is never spuriously killed).
GITLAWB_GIT_SERVICE_TIMEOUT_SECS bounds the info/refs advertisement too:
smart_http::info_refs drives it through drive_git_child under this timeout, with a
passing test proving the 504. The old note claimed it does not. It also claimed the
withheld-blob path is unbounded; after the blob_paths seam bound (this PR) the walk
is bounded and reaped, by a fixed internal deadline rather than this env var, so the
line now states that precisely instead of overclaiming this setting covers it.
The withheld-blob classification walk (blob_paths) fanned out blocking git children
with no deadline and no process-group teardown: git for-each-ref, git cat-file, git
rev-list, a git ls-tree per commit, and an uncounted git rev-parse (via
store::head_commit). A hung or pathologically slow child pinned the caller's
served-git permit for the whole hang, and on client disconnect the spawn_blocking
task and its git children ran on, orphaned. blob_paths is the shared core of five
callers: the upload-pack serve path (holds a read permit) AND, inside
git_receive_pack, the post-push replication and encrypt-then-pin walks (hold the
write permit U2 reserves for pushes). So the same unbounded walk could pin either
pool, and leaving the write-side twin unbounded would have made U2's reservation a
claim that does not match behavior.
Bound every git child at the blob_paths spawn seam on the blocking side: each child
runs in its own process group with a watchdog thread that SIGTERMs (then SIGKILLs)
the group on one shared deadline spanning the whole walk, and retains admission
until the group is reaped. This is the blocking-side counterpart of
smart_http::drive_git_child (spawn_blocking cannot be cancelled by an async
timeout). blob_paths stays sync, so all five callers keep their signatures and the
32 classification tests are unchanged; because every caller funnels through
blob_paths, one seam bounds both the serve and replication paths. The previously
unbounded store::head_commit child becomes a bounded git rev-parse inside the walk.
A walk that hits its deadline carries GitServiceTimeout, which the serve handler now
maps to 504 rather than a generic 500.
Proof: a fake git that hangs on rev-list makes blob_paths return GitServiceTimeout
within the watchdog budget (not block on the child) and the recorded process-group
leader is reaped, not orphaned; neutralizing the watchdog kill makes it hang past
the budget (RED). The 32 real-git classification tests stay green through the
refactor, including detached-HEAD, non-standard-ref, and deleted-in-history cases.
git_info_refs acquired git_read_semaphore for BOTH services, so the push handshake
(GET /info/refs?service=git-receive-pack) competed in the global read pool. An
anonymous clone flood could exhaust that pool and shed a legitimate push with 503
during its required advertisement phase, before it ever reached git_write_semaphore
on the POST. The write pool exists precisely so anonymous reads cannot shed an
authenticated push, but only the POST drew from it.
Select the pool by service: the receive-pack advertisement (phase one of a push)
now draws from git_write_semaphore, like the git-receive-pack POST, so a saturated
read pool cannot starve it. The per-IP push_rate_limiter that already brakes the
advertisement stays as the anti-flood control, and the advertisement stays
reader-visible with no new auth requirement. Because the receive-pack branch is now
a write-path op, it no longer consumes a read per-caller slot.
Handler-layer proofs: with the read pool at zero the receive-pack advertisement
survives while the upload-pack advertisement sheds; with the write pool at zero the
receive-pack advertisement sheds while upload-pack is unaffected; and a receive-pack
advertisement from an IP whose read per-caller budget is full still gets through
(mutation-checked, RED when the skip is neutralized).
read_caller_key returned the authenticated DID when a caller signed, dropping the
source-IP key. Public read routes accept any valid did:key via optional_signature
with no admission step, so one host could mint N disposable DIDs and hold
max_concurrent_reads_per_caller slots under each, multiplying its budget N-fold and
filling the global read pool, while the same host unsigned was capped on its IP.
Key the read sub-cap on the resolved source IP for every caller, signed or not,
mirroring the push path's IpRateLimiter which already throttles on source IP for
this exact DID-farm reason. Drops the now-unused caller_did parameter at both call
sites (git_info_refs, git_upload_pack).
Inverts info_refs_per_caller_cap_keys_on_did_not_ip into
info_refs_per_caller_cap_keys_on_ip_not_did: fill one source IP's slot, then two
requests signed under different DIDs from that same IP both shed 503 (farm
defeated), while a signed request from a different IP keeps its own budget. RED on
the DID-keyed tree, GREEN after.
The read-pool knob was referenced by the push and per-caller entries'
comments but had no example line of its own, so operators couldn't
discover it from the template. Add it with the config default (128).
Review of the served-git concurrency cap found no P0/P1; these are the
verified P2 follow-ups.
config: the max_concurrent_git_ops doc overclaimed that "every capped path
is duration-bounded." The rev-list object enumeration in the withheld-blob
path still runs in an uncancellable spawn_blocking, so a stuck rev-list can
hold its slot until git exits. Scope the guarantee to the streaming stages
and name the residual. Also tighten the fairness claim: the receive-pack
advertisement shares the read pool (a shed advertisement is a cheap retryable
GET); only the push POST is on the isolated write pool.
api/repos: add info_refs_per_caller_cap_keys_on_did_not_ip, the missing
handler proof that a signed caller is keyed by its DID, not its source IP.
Filling the DID slot sheds a request from a free IP; collapsing read_caller_key
to its IP arm turns the assertion green-not-503 (mutation-verified RED).
api/repos: extract acquire_read_caller_permit so both read handlers share one
shed path instead of a duplicated match block.
rate_limit: recover from a poisoned PerCallerConcurrency mutex instead of
panicking. The critical section is pure counter arithmetic and cannot poison
the lock, but a panic there would brick the limiter for every caller.
505 tests pass; clippy -D warnings and fmt clean.
Part of #174.
The max_concurrent_git_ops and git_service_timeout_secs doc-comments (and .env.example) described the info/refs and withheld-blob paths as unbounded follow-up gaps. Both are now closed (#174): the comments reflect the read/write pool split, the per-caller sub-cap, and that every capped path is duration-bounded with process-group teardown. Verified the pattern-doc pre-ship checklist: every git_permit / write-permit / per-caller site holds only a timeout+teardown git path, and all three size knobs are range(1..=1_048_576).
Closes the #174 work. No behavior change.
The filtered-pack path ran the whole rev-list + pack-objects build inside a spawn_blocking, so an outer tokio timeout could not cancel the blocking thread and a client disconnect orphaned the git child while the permit freed (jatmn P1b, the second gap path). Split it: rev-list enumeration stays blocking off the runtime (rev_list_keep), but the streaming pack-objects stage now runs under the shared drive_git_child on the async side, so it is duration-bounded (GitServiceTimeout -> 504) and its process group is reaped on disconnect. build_filtered_pack becomes async and takes a git_bin seam + timeout; upload_pack_excluding threads the git_service_timeout through.
A watchdog-bounded test proves a hung pack-objects times out (rev-list fast, pack-objects hangs). The refactor's happy path is covered by the existing filtered-pack correctness and real-git partial-clone/fetch tests, all still green; disconnect/group-teardown is the shared drive_git_child code proven by the run_git_service tests. 504 tests pass.
Part of #174.
info_refs ran a bare Command::output() with no timeout and no process-group teardown, so a hung git pinned its concurrency slot indefinitely and a client disconnect orphaned the child (jatmn P1b). Extract the timeout + process_group(0) + KillGroupOnDrop core from run_git_service into a shared drive_git_child, and route info_refs through it with an injectable git_bin. A hung advertisement now aborts with GitServiceTimeout (mapped to 504); disconnect reaps the group.
run_git_service's teardown tests all pass through the shared core (proving the group teardown info_refs inherits), the real-git filter tests cover the advertisement happy path, and a new watchdog-bounded test proves a hung advertisement times out. 503 tests pass.
Part of #174.
Adds PerCallerConcurrency, a bounded-keyed in-flight limiter (distinct from the request-rate RateLimiter) so no single caller monopolizes the served-git read pool. Each caller (per-DID when signed via optional_signature, else per-source-IP via client_key) may hold at most max_concurrent_reads_per_caller concurrent reads; over that it sheds 503. The key map is self-bounding (a key is dropped when its in-flight count hits zero) with a reject-before-insert max_keys backstop so a key farm can't grow it (INV-15). Applied in git_upload_pack and both info/refs advertisements, acquired after the visibility gate so a denied request never consumes a slot (KTD7).
Primitive unit-tested (cap + self-bounding + reject-before-insert) and mutation-checked. Handler-layer SC2: same-caller sheds while a different caller passes, proven on BOTH git_info_refs and git_upload_pack with independent mutation probes; plus a None-key bypass test. Per-source-IP keying is trust-config dependent, documented on the config knob. 502 tests pass.
Part of #174.
git-receive-pack now draws from a separate git_write_semaphore (max_concurrent_git_pushes) instead of the shared pool, so a flood of anonymous reads can no longer shed an authenticated push at admission (jatmn P1a). The shared field is renamed git_read_semaphore and continues to gate upload-pack and both info/refs advertisements. The write permit stays above acquire_write so it precedes the Tigris fresh-acquire (INV-10).
Handler-layer tests: write-pool shed (503), and a cross-boundary proof that an exhausted read pool does NOT shed a push; both mutation-checked (routing receive-pack back to the read pool flips each RED). 497 tests pass.
Part of #174.
Add max_concurrent_git_pushes (default 32) and max_concurrent_reads_per_caller (default 16), both clap range(1..=1_048_576) so an oversized value is a clean CLI error, not a Semaphore::new boot panic. The per-caller knob documents that per-source-IP keying is only as granular as GITLAWB_TRUSTED_PROXY. Wiring lands in the following commits; these are the config surface for the #174 concurrency-fairness fix.
Resolves jatmn P1a/P1b groundwork on #174.
PR3 of the #62 served-git hardening stack (timeout #165 and teardown
wiring #150 are merged). A bounded semaphore caps how many upload-pack /
receive-pack / info-refs operations run at once; past the cap a request
is shed with a clean 503 + Retry-After before spawning another git
subprocess, instead of exhausting the PID/thread table. A permit is
acquired at the top of each of the three handlers and held for the whole
op, releasing on return.
The cap is a portable backstop: the compose pids_limit is absent on Fly,
whose 500-connection cap is a different axis. Size --max-concurrent-git-ops
(GITLAWB_MAX_CONCURRENT_GIT_OPS, default 128) below the process budget.
Range 1..=1_048_576 so 0 (shed everything) and an oversized value that
would panic tokio's Semaphore at boot are clean CLI errors.
Known gap, tracked separately: info/refs and the withheld-blob
(upload_pack_excluding) path are not duration-bounded and do not reap
their git child on client disconnect, so a hung git on those two paths
holds its slot until it exits and live git can briefly exceed the cap.
The main pack path (run_git_service) tears its group down on drop.
Tests: Overloaded maps to 503 + Retry-After; the config knob defaults
and rejects out-of-range; git_permit sheds at capacity and releases; and
each of the three endpoints sheds with 503 when the semaphore is
exhausted (load-bearing: drop the permit line and the endpoint test goes
red).
2026-07-10 12:10:30 -05:00
62 changed files with 42353 additions and 1181 deletions
This file carries the rules a patch author cannot recover from the code alone, for humans and coding agents alike. For build, test, and PR basics, read `CONTRIBUTING.md` first.
## Authorization invariants (the rules PRs most often break)
Every repo-scoped endpoint must bind the caller to an authorization decision before serving or mutating anything. The `known_ungated` allowlist in `authz_guard::every_repo_scoped_handler_is_gated` is empty on main: `list_webhooks`, `list_replicas`, and `list_protected_branches` are gated (`list_webhooks` with read-visibility then owner; the other two with `authorize_repo_read`). Do not re-open those surfaces as exceptions.
- Owner-only mutations (visibility, webhooks, protected branches, merges) are gated against the repo owner. Three gate forms are in use and all are recognized: webhooks and merges call `require_repo_owner` (`crates/gitlawb-node/src/api/mod.rs`), visibility uses its module-local `require_owner`, and protected branches compare inline with `did_matches`. Use the form the surrounding module already uses.
- Not every write is owner-only, and that is by design: `star_repo`/`unstar_repo` bind to the signer (they still require repo read access), `register_replica`/`unregister_replica` bind to the replica's own DID, the bounty actions (claim, submit, approve, cancel, dispute) have their own multi-party rules, and closing a PR or issue is owner-or-author. Do not add owner gates to these.
- Content-serving reads (blobs, trees, raw files) gate through `authorize_repo_read` with the specific path being served, so a withheld subtree is denied even on an otherwise-public repo; repo-level reads (listings) pass `"/"` by the helper's own contract. The source scans below cannot check this argument's value, so review will.
- A route that reaches repo data through a global id, with no repo in its path, or through an extractor shape the scans do not recognize still needs the same gate; the scans cannot see those.
- Read-surface denials must not reveal existence: a caller who may not read a repo (or a withheld subtree) gets the same 404 as a missing repo, never a 403. (Owner-gated mutations currently return 403 after the repo lookup; do not widen that shape to read paths.)
- Two tests in module `authz_guard` (`crates/gitlawb-node/src/api/mod.rs`) scan handler sources and fail on a repo-scoped handler without a recognized gate: `every_in_scope_mutation_has_its_gate` and `every_repo_scoped_handler_is_gated`. If they fail on your handler, add the gate. The second test's `known_ungated` allowlist is empty; never add an entry to it without an open issue and review.
## Adding or changing routes
- A new gated handler needs tests proving its route-specific gate fires: exercise an unauthorized authenticated caller and an anonymous caller where applicable, assert the exact denial status each way, and assert the body leaks nothing. The non-owner tests in `crates/gitlawb-node/src/test_support.rs` show the shape for owner-gated routes.
- A new mutation handler also gets a row in `every_in_scope_mutation_has_its_gate` naming its gate type; the per-handler row is the point of that test.
- A table-driven deny suite covering every deny-bearing route is in review (#194, #195). If it is present in your checkout, add a probe row there for any new deny-bearing route.
## Verifying signatures and certificates
- Derive the verifying key from something outside the artifact you are checking: the node's own DID, a pinned or configured value, a peers lookup. A key read from a field of the object being verified (`Did::from_str(&cert.node_did)`) only proves the artifact is self-consistent, and identities here are permissionless, so anyone can mint a keypair, name it in the artifact, self-sign, and be told it is valid. A mismatch against that anchor has to fail the result, not log a warning.
- A handler whose correct answer is "valid" needs a positive case driven by a trusted, independently anchored artifact, and a rejection case for a well-formed, fully populated forged artifact. Invalid-only suites miss an implementation that rejects everything; a forged artifact that still returns valid is the regression that matters.
- The fields a signature covers are a versioned format. Adding one invalidates every artifact already signed, so give the payload a version, keep a path that verifies the older form, and add a test that verifies an artifact signed before your change.
## Removing a serving path
When a fix removes a path that used to serve content, the same PR must add a test asserting the removed path now denies. The deletion is what turns that test green; without it the path can quietly come back.
## Database migrations
Schema changes are code-defined in `MIGRATIONS` (`crates/gitlawb-node/src/db/mod.rs`). Always append a new versioned entry; never edit an entry that has merged (operators deploy from main). Test runs build the schema from scratch, so editing an applied version stays green in CI while breaking every existing deployment.
## Client behavior (`gl`, `git-remote-gitlawb`)
When a node denies a request, surface the denial to the user. Never render a denial as an empty list or a silent success; that turns an authorization boundary into invisible data loss.
## Toolchain and CI reality
- MSRV is Rust 1.91 (`rust-version` in the workspace `Cargo.toml`). The MSRV CI job runs `cargo check --workspace --all-targets` on exactly 1.91, so check that an API you rely on is stable there; the full test suite runs on stable.
- The PR workflow runs these jobs on every PR: `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, `cargo test --workspace` (against Postgres), a release build, `cargo audit`, the MSRV check, and a Docker build. The `main` branch ruleset currently requires one approving review but does not require these checks to pass before merge; a failing job should block review approval in practice, but nothing enforces that mechanically today.
- Conventional commit titles (`feat:`, `fix:`, `docs:`, ...) are required by convention and drive releases, but no CI job checks them; a bad title fails review, not the pipeline.
@@ -54,6 +54,7 @@ Smart contracts live in a separate repo: [github.com/Gitlawb/contracts](https://
3.**No breaking changes without discussion.** Open an issue first for protocol-level changes.
4.**Conventional commits.** Use `feat:`, `fix:`, `docs:`, `refactor:`, `test:`, `chore:`. Releases are automated by [release-please](https://github.com/googleapis/release-please) — your commit prefixes drive the next version bump.
5.**Format and lint.** Run `cargo fmt --all` and `cargo clippy --workspace --all-targets -- -D warnings` before submitting. CI will reject anything that fails these.
6.**Security-surface rules.** [`AGENTS.md`](AGENTS.md) carries the authorization invariants PRs most often break; it is written for coding agents and humans alike, so read it before touching endpoints, visibility, or migrations.
Gitlawb Node is the open-source node software behind the Gitlawb network. It lets anyone run a self-hosted node, publish repositories under a DID, sign writes with Ed25519 HTTP signatures, replicate git activity across peers, and move toward a resilient app-delivery network where code and build assets can be served closer to users.
Gitlawb is not trying to be only “another git host.” The long-term direction is:
Gitlawb is not trying to be only "another git host." The long-term direction is:
```txt
Decentralized GitHub
@@ -34,12 +34,12 @@ This is a Rust workspace with four crates:
Most git hosting today depends on a small number of centralized platforms. Gitlawb Nodes are designed for a different model:
- **Own your identity** — every user, agent, and node is an Ed25519 keypair represented as `did:key:z6Mk...`.
- **Signed writes by default** — write requests use RFC 9421 HTTP Signatures instead of passwords.
- **Git-native transport** — repositories are still real git repositories served over smart HTTP.
- **Agent-native workflows** — the `gl` CLI and MCP server expose repo, issue, task, PR, and UCAN flows to AI agents.
- **Peer-aware delivery** — nodes can announce, discover, gossip, and sync with each other.
- **App CDN direction** — the network can evolve from decentralized code storage into code + asset + app delivery.
- **Own your identity**: every user, agent, and node is an Ed25519 keypair represented as `did:key:z6Mk...`.
- **Signed writes by default**: write requests use RFC 9421 HTTP Signatures instead of passwords.
- **Git-native transport**: repositories are still real git repositories served over smart HTTP.
- **Agent-native workflows**: the `gl` CLI and MCP server expose repo, issue, task, PR, and UCAN flows to AI agents.
- **Peer-aware delivery**: nodes can announce, discover, gossip, and sync with each other.
- **App CDN direction**: the network can evolve from decentralized code storage into code + asset + app delivery.
---
@@ -189,14 +189,14 @@ For public-network use, make sure `GITLAWB_NODE` points to the node you want. Th
Public nodes (e.g. `node.gitlawb.com`) require two things on writes:
1.**RFC 9421 HTTP Signatures** — every write is signed by your identity key. `gl`
1.**RFC 9421 HTTP Signatures**: every write is signed by your identity key. `gl`
and the `git-remote-gitlawb` helper do this automatically. An old/unsigned CLI
fails with `401 not_an_agent`; `gl` will tell you to upgrade and register.
2.**An iCaptcha proof** on the spam-gated writes (**repo create, fork, register**).
`gl` solves this for you: on the node's `403 icaptcha_proof_required` it reads the
`x-icaptcha-url` / `x-icaptcha-level` hints, requests a challenge, solves it
locally (arithmetic / algebra / sequence), and **retries the same signed request**
with the `x-icaptcha-proof` header — no manual steps, no env vars.
with the `x-icaptcha-proof` header. No manual steps, no env vars.
```bash
gl identity new # create did:key identity
@@ -215,14 +215,14 @@ Notes:
- **Proofs are short-lived (~5 min TTL) and single-use.** If one expires between
solving and use, the client transparently solves a fresh one and retries.
- **What needs what:** create / fork / register are signed **and** iCaptcha-gated;
`git push` is **signed-only** (owner signature is the gate — no per-push challenge);
`git push` is **signed-only** (owner signature is the gate, no per-push challenge);
reads (clone / fetch / `repo info`) need no proof. A non-existent repo returns a
clear `404`, never a placeholder.
- **API-key iCaptcha deployments:** set `GITLAWB_ICAPTCHA_URL` to your iCaptcha
origin and `GITLAWB_ICAPTCHA_API_KEY` to its key. The client only talks to an
`https` origin whose host is allowlisted (that URL or the public default), and
sends the bearer token **only** to your configured origin — never to a URL a
node advertises — so a hostile node can't capture the key or redirect the solve.
sends the bearer token **only** to your configured origin, never to a URL a
node advertises, so a hostile node can't capture the key or redirect the solve.
---
@@ -309,7 +309,7 @@ POST /api/v1/peers/announce
POST /api/v1/sync/notify
```
When `GITLAWB_REQUIRE_SIGNED_PEER_WRITES=false`, unsigned legacy peers are accepted on those two routes, but signed requests are verified when signature headers are present. Once all live peers upgrade, operators can set:
When `GITLAWB_REQUIRE_SIGNED_PEER_WRITES=false`, unsigned legacy peers are accepted on those two routes, but signed requests are verified when signature headers are present. Staged rollout relaxes who may announce, not who owns a peer row. An unsigned announce may register a previously unseen peer, and it may refresh an existing row whose `http_url` is unchanged, but changing an existing peer's `http_url` requires an RFC 9421 signature from that peer's own DID and is refused with 403 otherwise. An unsigned announce can only register a `did:key`, since that is the only method whose verifying key can be resolved and therefore the only kind of row anyone could ever correct through the signed path; any other DID is refused with 400. Once all live peers upgrade, operators can set:
```bash
GITLAWB_REQUIRE_SIGNED_PEER_WRITES=true
@@ -343,13 +343,28 @@ Important node settings:
| `GITLAWB_REQUIRE_SIGNED_PEER_WRITES` | Require signed peer announce/sync writes. |
| `GITLAWB_AUTO_SYNC` | Enable automatic sync from known peers. |
| `GITLAWB_MAX_PACK_BYTES` | Max git pack body size for smart-HTTP routes. |
| `GITLAWB_GIT_SERVICE_TIMEOUT_SECS` | Max seconds a served git upload-pack/receive-pack may run before it is aborted (504). Default 600. Does not bound `info/refs` or the withheld-blob path. |
| `GITLAWB_GIT_SERVICE_TIMEOUT_SECS` | Max seconds a served git upload-pack, receive-pack, or `info/refs` advertisement may run before it is aborted (504). Default 600. Also bounds the withheld-blob classification walk (on both the upload-pack serve and receive-pack replication paths) and the push-side pin-candidate discovery (`rev-list` / `cat-file`), each reaped via process-group teardown at the deadline. On the path-scoped upload-pack path the classification walk and the pack serve share ONE deadline, so this value bounds their combined duration rather than granting each stage a full budget: a walk that consumes it leaves the serve nothing and the clone gets a 504. Serving large path-scoped repos may therefore need a higher value than they did when each stage was budgeted separately. Accepted range is 1 to 3153600000 (100 years), since the node derives deadlines from this value and a larger one cannot be represented. |
| `GITLAWB_GIT_ACQUIRE_TIMEOUT_SECS` | Max seconds the storage-acquisition phase (Tigris HEAD/GET, push advisory-lock) of a served git op may run before the request is shed with a 503, separate from the git-run timeout. The concurrency permit is released on expiry so a stalled backend cannot pin the pool. Default 30. |
| `GITLAWB_MAX_CONCURRENT_GIT_OPS` | Max concurrent served git READ ops (upload-pack and its `info/refs` advertisement) across all callers; over-cap sheds a 503 + Retry-After. Anonymous reads draw from this pool, so pair it with `GITLAWB_MAX_CONCURRENT_READS_PER_CALLER`. Pushes and the receive-pack advertisement have their own pools, so a read flood cannot shed an authenticated push. Default 128. |
| `GITLAWB_MAX_CONCURRENT_GIT_PUSHES` | Max concurrent `git-receive-pack` POST operations, in a pool separate from the read pool. The anon receive-pack `info/refs` advertisement runs in a third pool of the same size, disjoint from both, so an advertisement flood cannot shed a push either. Two per-source push caps are derived from this value (`/8`, floor 1) and have no env var of their own. Over-cap sheds a 503 + Retry-After. Default 32. |
| `GITLAWB_MAX_CONCURRENT_READS_PER_CALLER` | Max concurrent read ops a single caller may hold, so one caller cannot monopolize the read pool. Keyed on the resolved source IP, never the DID, and only as granular as `GITLAWB_TRUSTED_PROXY`: left unset, a node behind an edge or NAT keys every caller on the edge IP and this collapses to one global cap. Default 16. |
| `GITLAWB_MAX_CONCURRENT_PIN_TASKS` | Max post-push pin loops (IPFS + Pinata) running concurrently across all repos. This caps how many loops RUN at once, not how much object-id list memory the node retains: on the local IPFS path a loop parked waiting for a permit still holds its full list. Do not size memory from this knob alone. A loop over cap waits, never drops a pin. Default 8. |
| `GITLAWB_REPO_LEASE_MAX_WAITERS` | Max pushes parked at once waiting for the same repo's write lease. Each waiter pins its buffered pack body, so this bounds that memory for a hot repo; past the cap the newest push sheds a 503 + Retry-After instead of queueing. Pushes to other repos are unaffected, and the lease holder is not counted. Default 8. |
| `GITLAWB_MAX_CONCURRENT_IPFS_WALKS` | Max concurrent `GET /ipfs/{cid}` visibility walks across all callers (own pool, disjoint from the served-git pools); over-cap sheds 503. Default 32. |
| `GITLAWB_IPFS_WALK_PER_SOURCE` | Max concurrent `/ipfs` walks a single source IP may hold. Default 4. |
| `GITLAWB_IPFS_MAX_LEGACY_PROBES` | Max legacy (NULL-provenance) repos probed per `/ipfs/{cid}` request, bounding the scan-fallback fan-out. A truncated scan returns a retryable 503, not a false 404. Default 256. |
| `GITLAWB_IPFS_MAX_REPOS_WALKED` | Max expensive path-scope visibility walks per `/ipfs/{cid}` request; over-cap repos are skipped and the scan continues, shedding a retryable 503 (not a false 404) if the object is then found nowhere. The effective cap is the tighter of this knob and the node's internal per-request history-walk ceiling of 17 (`MAX_PIN_SOURCES + 1`), so a value above 17 has no effect while a value below it does tighten the cap. Default 64. |
| `GITLAWB_IPFS_MAX_REPO_VISITS` | Ceiling on repos one `/ipfs/{cid}` request may visit (acquire + probe) past the visibility gate. Also the worst-case per-request Tigris fetch count. On exhaustion the scan stops with a retryable 503. Default 1024. |
| `GITLAWB_IPFS_REQUEST_BUDGET_SECS` | Absolute wall-clock budget for one admitted `/ipfs/{cid}` request's acquire+walk lifetime. Per-stage clamps bound the acquire and walk stages to the remaining budget, and no stage starts once it is exhausted; the scan then stops with a retryable 503. The object-type probe and content-read `cat-file` subprocesses are budget-checked before starting and each also run under their own deadline (the lesser of `GITLAWB_GIT_SERVICE_TIMEOUT_SECS` and the remaining budget), reaped via process-group teardown, so a hung `cat-file` cannot hold the request's walk slot past it. One hang path is still unbounded: the probe's object-store readability check is a plain filesystem sweep with nothing to reap, so a wedged filesystem can hold the slot past the deadline. Default 600. Accepted range is 1 to 3153600000 (100 years), since the node derives a deadline from this value and a larger one cannot be represented. |
| `GITLAWB_IPFS_RATE_LIMIT` | Max `/ipfs/{cid}` requests per client IP per hour (route flood brake). 0 disables. Default 600. |
Production note: change the default Postgres password before exposing a node publicly.
Legacy-pin window: releases before the CID-resolver work stored the provider CID (Kubo dag-pb / Pinata) as a pinned object's resolver key. The `/ipfs/{cid}` resolver now recomputes the raw-content CID from the object bytes and refuses to serve a key that does not match, so `GET /api/v1/ipfs/pins` can still advertise an unrepaired legacy CID that 404s. Such a row is repaired opportunistically the next time a push carries the object again (its key is rewritten to the raw CID, the old value kept in `legacy_provider_cid`), but git negotiation omits objects the node already has, so most legacy rows never re-enter a push delta. A deferred one-shot startup sweep, not this opportunistic path, is what fully retires the advertise-then-404 window. Rows whose object bytes are gone stay withheld.
**Node refuses to start with "strict-mode operator check failed"** — either `gl node register` first, or unset `GITLAWB_OPERATOR_STRICT_MODE`.
**Node refuses to start with "GITLAWB_DB_MAX_CONNECTIONS (20) must be at least max_concurrent_git_pushes (32) + 8 headroom"**: raise `GITLAWB_DB_MAX_CONNECTIONS` to at least the push cap plus 8 (40 with the default cap of 32; 48 is the shipped default and the recommended value), or lower `GITLAWB_MAX_CONCURRENT_GIT_PUSHES`. This bites a node upgraded in place that still sets the old pool size of 20. The check is deliberate: each concurrent push pins one pooled connection for its whole receive-pack, so a pool that does not clear the push cap lets a burst of slow pushes starve every other database path.
**Rewards are 0 after a week** — run `gl node onchain-status`. If `currentlyActive: false`, check your heartbeat loop (node logs for `operator heartbeat sent`).
**Want to rotate operator wallet** — requires unstake → re-register with new wallet. No in-place rotation in v1.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
beardthelion