On a resumed request whose visit budget was already spent by the provenance
phase, the scan's top-of-loop visit arm sealed pager.cursor, which at that
moment is the position the caller just sent. The node returned the caller's own
token, verbatim, rung after rung. Three rungs were observed returning an
identical position.
A token looks like progress, so the client keeps going: gl retries to its
resume cap, and every one of those requests re-runs the full provenance phase,
up to seventeen repo acquires and cat-file subprocesses, advancing nothing
before it errors. That is roughly nine anonymous requests worth of work for
none, and it is worse than shedding nothing, because a caller who is told the
ladder is over stops immediately.
A seal now has to be strictly ahead of where the request itself started: a
different candidate is ahead by construction, since only the gated advance can
name one, and the same candidate needs a row past the start row. A request that
started at the front is before everything, so its seals pass untouched.
When the proposer settled at least one row this rung, the existing ceiling arm
already seals that row, and it is strictly ahead because a resumed scan only
walks rows past its cursor. Only a rung that settled nothing sheds without a
token, and that is honest: the spender is the provenance phase, which runs the
same way every rung, so no retry can do better.
The filter sits at the single mint site, where a future call site cannot bypass
it, and it logs the drop as a boolean. record_scan_truncation has already
logged that a position was sealed by then, and a 503 carrying no token next to
that line is the confusion that log exists to prevent.
A CID that maps to several oids shared one resume slot across every candidate,
and the slot kept the maximum position. An earlier candidate could spend the
probe budget walking past a repo that holds the object for a later one, seal a
position beyond it, and the next rung would resume past a row that candidate
never examined, wrap, and shed tokenless. The client reads an absent token as
the ladder being over, so the object became permanently unretrievable, at stock
config, deterministically on every retry.
The token now names which candidate it is resuming, and the rules that keep
that sound are narrower than they first look:
Only one candidate per request may seal, and which one depends on where the
REQUEST started. On a resumed request it is the resumed candidate alone, since
the shared pager holds only the table suffix from the caller's cursor, so a
later candidate walked a suffix and never saw the front. On a front-started
request it is the first unfinished candidate, since there every candidate walks
from the front and a later candidate's stop is honest coverage. Silencing later
candidates unconditionally would remove the only thing that mints rung 1 when
the first candidate wraps untruncated.
A candidate is finished when its row loop walked every fetched row, or when it
owed no scan at all. Both matter: a properly provenanced candidate never wraps,
so without the second arm the ladder dies every rung. The wrap is witnessed per
candidate at the row loop's own two exits, never by reading the shared pager
flag at the tail, which any short page sets and which would let a candidate that
truncated mid-page look finished and strand the rows it refused.
Finishing a non-final candidate advances the seal to the next one at a front
sentinel and taints, because the tail emits a continuation only when something
tainted; sealing without tainting would suppress the taint and return a
definitive 404 while discarding the token it had just minted.
The keep-the-maximum comparison is gone. With one proposer per request the slot
is written at most once, so an assertion states that directly instead.
The pager stays shared per request. A per-candidate pager would restore the
fan-out the paging exists to remove.
A CID can map to several git oids, and the ladder needs to name which one it is
resuming. The sealed position gains the candidate's oid hex so a rung resumes
that candidate rather than a position in a list: oids_for_cid is a sorted set,
so an ordinal silently repoints at a different candidate when a pin that sorts
earlier arrives between rungs, while an identity degrades safely to "not found,
restart at the front".
The field is length-prefixed and padded to 64, matching the framing the row
fields already use, because production oids are 40 hex, not 64: repos are
created with --object-format=sha1 and only the test fixtures are sha256. A
fixed 64-byte field would fail every seal on a real deployment and shed a
tokenless 503, which the client reads as the ladder being over. Both widths are
exercised, and a zero-length candidate is rejected at decode so it cannot be
confused with the front-of-table sentinel, which is empty row fields with a
real candidate.
VERSION goes to 3 and the plaintext to 527 bytes, so a token minted under the
old layout opens to None and the caller restarts at the front. Nothing has
minted one outside tests.
The slot carrying the position is a struct rather than a widened tuple on
purpose: a 3-tuple would have pulled the hex into the existing keep-the-maximum
comparison, changing behavior this commit is meant to leave alone.
Token length stays invariant across both oid widths, since length would
otherwise be a side channel for the withheld row it names. That is asserted on
a real seal in gitlawb-core, not on the gl fixtures: nothing in gl seals or
opens a token, so its width constant cannot detect a wrong layout.
Code review of the previous commit turned up three things worth fixing in place.
record_scan_truncation centralized the taint and the seal but logged nothing, so
centralizing actually made a truncation less visible than the scattered inline
taints it replaced: only the visit ceiling logged, and it logs from inside the
gate, before the caller decides whether a position gets sealed. Two identical log
lines could therefore mean "the ladder continues" or "the caller is stranded".
One debug line now carries the reason and whether a position was sealed. It logs
only whether one exists, never its value, since the position names a withheld
row's created_at and id.
The doc comment claimed to be "the one site that records a scan truncation" while
eight other taint sites bypass it. The distinction is real but it is not the one
the comment drew: a ceiling stops the scan and owes the caller a position, while
a transient skip refuses one row and the rows behind it are still walked. Says
that now.
The forward-only rule's stated justification was wrong. A later candidate's
position is still ahead of the token the caller echoed, so letting it win would
not move the ladder backwards; it would shrink each rung toward a single row.
Also records that the comparison is Rust byte order while the pager's keyset runs
under the database collation, which can disagree on a non-C collation, and why
that costs a replay rather than a skipped row.
The probe and visit ceilings taint inside gate_and_serve and returned Skip, so
the row they refused and every row behind it were walked past without a verdict
while the resume position was sealed from pager.cursor, the end of the fetched
page. Two ways that stranded content:
- On the final page, `pager.exhausted` breaks ahead of every mint arm, so the
shed carried no continuation at all. A tokenless search_incomplete is the
wrapped-scan answer, which tells `gl ipfs get` its ladder is over, so a holder
on that page was unreachable on every retry.
- Mid-page, the sealed cursor sat past the refused rows, so the resume skipped
them. The shipped ladder tests all set page_rows == ceiling, which puts the
break exactly on a page boundary and hides both.
The ceiling now returns GateOutcome::CeilingStop rather than tainting on its way
out, and the scan loop stops there and seals the row in front of the one that was
refused. record_scan_truncation is the single site that records a truncation:
it taints and seals together, and only ever moves the position forward, so a
later oid candidate re-walking the same rows on a spent budget cannot hand back a
token the caller already echoed. The wrapped-scan tail no longer clears a seal,
since a ceiling can stop a resumed scan part way through the last page.
All four are in the deployment half added last round; none is in the Rust.
The AWS guide told operators two different things
--------------------------------------------------
"Changing configuration" still said editing `image_tag` does not affect a running
instance and routed to `-replace`, while the upgrade section thirty lines above
now reinstalls the rendered compose, which carries `image_tag`. The same
paragraph told operators to edit `.env` and restart — which the new "Why the
upgrade rewrites the compose file" section explains is silently ineffective for
any key the installed compose does not already name.
Split by which file carries the change rather than deleted, because the
distinction is real: values rendered into `compose.yaml` (`image_tag`, ports,
`domain_name`, `db_host`, `icaptcha_*`) roll out with `apply` + `upgrade_command`;
values user-data writes into `.env` at first boot (`public_url`,
`bootstrap_peers`, integrations) do not propagate from tfvars at all and need an
edit on the instance or a replacement. The third case is the one that bites: a
setting the installed compose does not yet name is read from `.env` and dropped,
so it needs the upgrade first.
`--remove-orphans` stopped being inert
---------------------------------------
The flag is not new, but until the upgrade rewrote `compose.yaml` it reconciled
against the instance's own file, so the only orphans were hand-started
containers. It now reconciles against a fresh rendering whose service set is
conditional — `postgres` only when `db_host` is empty, `caddy` only when
`domain_name` is set. Setting `use_rds` or clearing `domain_name` and then
running what the README calls a pull-and-restart stops and removes the local
postgres or the TLS terminator. That is what those variables mean, but it happens
on the upgrade rather than on apply, so it is now stated next to the
Terraform-owned warning. Data survives either way; both bind-mount under
/mnt/data.
The macOS setting had no writer
--------------------------------
`enforceOwnerPush` was persisted, loaded, emitted into `.env`, and forwarded by
the generated compose — and nothing set it. `SettingsWindow` had no state and no
assignment, so the only way to reach the opt-out on that launch path was hand
editing config.json in Application Support, which no document mentions. Editing
`.env` does not work there either: `writeEnvFile()` rebuilds it from `Config` on
every start. It now has a Toggle beside "Sync repos from peers", following the
pattern already in that view.
Three lines the change made untrue
-----------------------------------
The handler test asserted "with no configuration at all" while setting
`enforce_owner_push` explicitly — the right thing for the test, the wrong claim
for the message. The cross-tenant docstring still described three distinct pusher
DIDs after A and B were given `n1`'s owner. And OSS-READINESS-AUDIT listed
unprotected branches not enforcing owner checks as a live blocker; owner checks
now cover every branch, and only the UCAN half of that sentence still stands.
Also dropped the `#[allow(dead_code)]` on `test_state_with`, redundant since the
same commit added its call site.
Code review found the doc comments carrying claims the code does not
support and two coupling points documented from one side only.
The floor and the resolver's walk_cap are not textual twins: the floor
reads the constant, the resolver reads the AppState seam, and they agree
only because every construction seeds one from the other. Say that, and
give walk_cap the back-reference it lacked. The lifted node_verifies
helper no longer named the middleware it mirrors, so an edit to
require_signature would not find either copy.
The request-target clause pins @path, not @method or content-digest: a
301, 302 or 303 still rewrites a signed POST to a bodyless GET while the
signature headers ride along. Record that rather than implying the hop is
safe. Add the query-removed and fragment-only matrix rows, and a mutant
pinning that scan-phase walks are not charged to the work bucket, which
nothing covered.
The floor reserved one complete legacy search per window, probes plus page
tolls, but the provenance visibility walk debits the same bucket before the
fallback runs and its cap is charged per phase. With a route limit set below
the floor the provenance phase spent from the budget the search was promised,
the fallback 429d short of its configured reach, and the retry re-paid the
same walk charges, so a readable holder past that point stayed unreachable.
Add the walk term, min(MAX_HISTORY_WALKS_PER_REQUEST, ipfs_max_repos_walked),
which is textually the resolver's own walk_cap so the two move together. The
ladder fixture pins repos-walked to 1 to keep the page toll, not the new term,
as the thing binding it.
Every item below was verified against the code before changing it; two of them
were confirmed by running the probe the reviewer described.
A test the fixture sweep missed, and that could not fail
--------------------------------------------------------
`f1_write_cap_is_inert_without_a_resolvable_source_key` seeded `z6f1none` and
pushed as `z6MkF1NoKeyPusher…`, so the owner gate rejected it before it reached
the per-caller cap the test exists to measure. Its assertion was
`!matches!(r, Err(Overloaded(_)))`, which a `Forbidden` satisfies, so it reported
ok while measuring nothing — and it is not `#[cfg(unix)]`, so it did that inside
every green run. Confirmed by inverting the assertion to expect `Forbidden`: it
passed, proving the request stopped at the gate.
The pusher is now the owner, and the assertion is positive: reaching git on the
missing on-disk repo (`Err(Git(_))`) is proof the request cleared both the gate
and the cap. The sibling `f1_write_cap_key_is_per_source_not_per_repo` carried
the same weak negative in its second assertion and is tightened the same way.
The default was barely tested, and what tested it read the environment
---------------------------------------------------------------------
`Config::parse_from` consults the process environment, so on a host exporting
`GITLAWB_ENFORCE_OWNER_PUSH=false` the old `enforce_owner_push_defaults_to_true`
reported the operator's setting rather than what this crate declares — it failed
outright, which would also break any contributor who sets the variable. It is
replaced by two tests that are env-independent and behavioural:
* `enforce_owner_push_is_declared_true_independent_of_the_environment` reads
the declared default off `Config::command()`.
* `default_config_refuses_a_non_owner_push` drives the handler on an untouched
`test_state` config and asserts 403 — authenticated by a real RFC 9421
signature, so 403 rather than 401 is what makes it a statement about
authorization.
Both were checked by reverting `default_value_t` to `false`: the handler test
reports 500 where it wants 403, and the declared test reports `["false"]`.
The cross-tenant test lost its second tenant
--------------------------------------------
The sweep collapsed `u1_parked_push_does_not_shed_another_pusher_behind_the_same
_ip` onto one identity, leaving the test named for cross-tenant behaviour with a
single tenant and two docstrings describing a scenario it no longer ran. `n2` is
now owned by `z6u1nat2` and its push signs as that owner, so two distinct tenants
share one edge IP again — which is the property under test, since the per-source
key is the resolved peer address and never the DID.
Four comments outlived their premise
-------------------------------------
`repos.rs` (3 sites) and `.env.example` still said owner enforcement defaults
off. The IP-keying rationale they support is unchanged and still correct — a DID
farm defeats a DID key — so only the dead premise is rewritten: the cap is
load-bearing because the rate limiter bounds arrival rate rather than occupancy,
because the gate can be turned off for a rolling upgrade, and because one owner
can open many concurrent pushes to their own repo.
Operators on the container path got none of this
-------------------------------------------------
`GITLAWB_ENFORCE_OWNER_PUSH` is now forwarded in `docker-compose.yml`, the macOS
app's bundled compose, and `infra/aws/compose.yaml.tftpl`, defaulting to `true`
to match the node. Without it the rolling-upgrade opt-out this PR documents in
three places was unreachable for anyone running compose.
A cutover this project never promised
--------------------------------------
The README paragraph announcing a 15 September 2026 flip of
`GITLAWB_REQUIRE_SIGNED_PEER_WRITES` is removed. That flag is not changed here,
the date appears nowhere on main, and a PR about a different flag is not where
that commitment would be made. The table row for `GITLAWB_ENFORCE_OWNER_PUSH`
carries the part that belongs.
A docstring that credited the wrong thing
------------------------------------------
The field and test docs claimed the value-taking action is what makes
`GITLAWB_ENFORCE_OWNER_PUSH=false` resolve to `false`. It is not: the env form
resolved under the presence-only declaration too. What the change buys is
`--enforce-owner-push false` parsing at all, rather than erroring as an
unexpected argument. Both docstrings now say so.
`p2_push` signed every push as one shared `P2_PUSHER` constant, but its two
callers own different repos — `z6p2tail` and `z6p2fail`. Pinning the constant to
either owner leaves the other rejected by the owner-push gate, so the identity
now follows the `owner` argument the helper already takes, the same way the
burst test derives it.
The rejection surfaced as `async fn resumed after completion` rather than a 403:
a denied push completes on the first poll, `p2_logged` never observes
`receive-pack`, and the polling loop then polls the finished future again.
Fourteen concurrency tests seed a repo through `f4_state_with_repo` or
`p2_parked_release_state` and then push under an unrelated DID, which the
owner-push gate now rejects before the machinery each test asserts on is ever
reached. They are transport and lease tests, not authorization tests, so each
now signs as the owner of the repo it targets.
Two needed more than a swapped constant:
- `receive_pack_burst_scans_serialized_and_both_pushes_succeed` pushes to two
repos with different owners, so the identity is derived per push from the
owner already threaded through the closure.
- `u1_parked_push_does_not_shed_another_pusher_behind_the_same_ip` used three
distinct pusher DIDs, which owner-only push makes impossible. The distinction
was narrative: the per-source key is the resolved peer IP and never the DID,
so three concurrent pushes from one edge IP contend exactly as before. The
comment now says so rather than leaving the reader to infer it.
The gate keeps its own coverage in `enforced_rejects_non_owner_with_forbidden`
and `enforced_rejects_missing_did_with_forbidden`, so nothing here removes the
assertion that a non-owner push is refused.
These tests are `#[cfg(unix)]`, so a Windows checkout cannot run them; they were
missed in the previous round because the Windows suite runs 717 of the 825 tests
Linux runs.
A `did:key` signature is authentication, not authorization: the method is
self-certifying, so any party can generate a keypair, derive its DID and sign.
With `GITLAWB_ENFORCE_OWNER_PUSH` defaulting to `false`, `owner_push_rejection`
short-circuited and every `git-receive-pack` carrying any valid signature was
accepted — including pushes to a repository the signer does not own, and
including private ones, on every branch not explicitly protected.
The gate itself already worked when switched on. What was missing is that no
reachable default configuration switched it on, so a node started with nothing
configured accepted a push from anyone. That default is what this changes.
The argument now takes a value so there is a way back: `--enforce-owner-push
false` and `GITLAWB_ENFORCE_OWNER_PUSH=false` both disable it, while the bare
`--enforce-owner-push` form still means `true`. Without this the flag would be
presence-only (`ArgAction::SetTrue`), which with a `true` default would leave
operators no way to opt out during a rolling upgrade.
Three concurrency tests pushed as non-owners in order to reach the shedding
logic they actually assert on. They now push as the repo owner, so they keep
exercising the shipped default rather than opting out of the gate.
Docs updated in README.md, .env.example and docs/RUN-A-NODE.md, including the
delegated-key caveat below. README also carries a dated cutover for
`GITLAWB_REQUIRE_SIGNED_PEER_WRITES`, which stays `false` for now so live peers
can finish upgrading.
BREAKING CHANGE: `git-receive-pack` now rejects a push whose authenticated DID
is not the repo owner, returning 403 before any ref update is applied.
Delegated and CI keys count as non-owners: a UCAN `git/push` capability is
verified but not yet honored for authorization, so an agent pushing under its
own DID cannot push while this is on. Set `GITLAWB_ENFORCE_OWNER_PUSH=false`
during a rolling upgrade, or have automation push as the repo owner, until
scoped collaborator / UCAN-delegated push rights land.
Two tests build a fake git as a `/bin/sh` script, mark it executable through
`std::os::unix::fs::PermissionsExt`, and reap the hung `rev-list` with
`libc::kill(SIGKILL)`. None of that exists on Windows, and neither test was
cfg-gated, so `cargo test -p gitlawb-node` failed to compile there with six
errors before running anything. A Windows checkout could not run a single test
in the crate, including the ones that are platform-independent.
Gate both with `#[cfg(unix)]`. The attribute is a no-op on Linux, so CI keeps
running them exactly as before; only the Windows build changes, from "does not
compile" to "runs the platform-independent tests".
Refs #228
The node client followed redirects with no policy, and reqwest strips only the
classic credential headers when a redirect crosses origins. RFC 9421 signature
headers are not on that list, so a node answering 302 could hand a caller's
signature to another host, which then served whatever bytes it liked for a
content address that never hashed to them. Following continuations made it
worse by turning one signed request per invocation into as many as nine. The
signature covers the method, the path and the body digest but not the
authority, and the node accepts any signature inside a five minute skew with no
nonce ledger, so one harvested header reads a caller's private paths at any
node in the fleet.
Redirects are now scoped to the origin that issued them. Same-origin ones still
follow, so an http to https upgrade or a trailing-slash normalization keeps
working, and a cross-origin one stops rather than errors, so the caller sees
the 3xx through the status path it already has. A downgrade back to http on the
same host is refused too: the origin matches, but the signature would leave in
cleartext, which is the same leak by a slower route. A custom policy replaces
reqwest's built-in chain limit, so the limit is restated rather than lost.
The recovery path in clone had the same silence the CLI just lost: it skipped a
blob on any non-success without saying why, so a truncated scan, a permission
denial and a dead gateway were one indistinguishable outcome. It now names the
status or the transport error, capped and stripped of control characters since
the text comes from a caller-chosen gateway, and continues as before.
Reading a response body no longer conflates three endings. The cap cutting the
body, the body finishing, and the read failing partway were all one silent
break, so a failed read surfaced as an error message that stopped after the
colon. Each is now reported, and a body that could not be read says so.
Coverage for three paths that were previously argued rather than run: a
transport failure mid-ladder still hands back the resume token, a scan that
ends before any ceiling still answers a definitive not-found, and a response
whose body stalls after its headers is still ended by the client's own
deadline.
Review of the scan-continuation work found the ladder surfacing its token on
three terminals and dropping it on three others: a mid-ladder 5xx that is not
an overload, a truncation whose offered continuation fails validation while a
good token is still held, and a transport error. In each case the user is told
to start over, and a bare re-run restarts at the first row and re-spends the
node's per-IP budget, which is the failure the token exists to prevent. It is
surfaced on those paths now, still never on a definitive 404 or on the node's
deliberate scan-wrapped signal, and a rejected token is still never echoed
back.
A body cut at the read cap used to change the answer. Truncated mid-JSON it
failed to parse, lost its error code, and fell to the transient-overload arm,
which retried the old position for every remaining attempt and then reported
that the object might lie beyond the rows scanned when nothing past the first
page had been. The cap now reports that it cut, and a cut body is terminal.
Three guards were doing nothing. Mutation testing showed the Retry-After clamp
could be deleted with every test still green, so a node advertising a day-long
wait would have parked the client inside a single sleep that neither the
attempt cap nor the deadline can interrupt. The rejection of a malformed
caller-supplied token was equally untested, though the same property is proven
on the node-offered side. The withheld-blob ladder asserted only that each rung
was not a success, which any early exit satisfies without the rule ever being
evaluated; it now witnesses that the scan reached the end of the table.
The inter-attempt wait is bounded by the time left on the deadline, so a run
can no longer overshoot the bound it advertises. That drops the wait out of the
worst case entirely rather than shrinking it: the last attempt starts before
the deadline and the client's total timeout covers that request through the end
of its body, so a run is over by the deadline plus that timeout. The docs said
about 95 seconds and now say about 90.
They also said a large download already streaming is never cut off part way.
That was wrong. The scan deadline does not cover the download, but the client's
timeout is a total request deadline running until the body finishes, so a slow
transfer is cut off. Both prose sites now say what the code does.
Also: stdout is flushed before the command returns, so a closed pipe fails
loudly instead of yielding a truncated object with a success status.
GITLAWB_IPFS_MAX_LEGACY_SCAN_ROWS accepts any value from one upward, but the
pager always asked the database for a full page and the ceiling was only
checked afterwards, in the scan loop. With the ceiling below the page size an
anonymous request still selected and rule-loaded a whole page while holding
walk admission, and any ceiling that is not a page multiple overshot by up to
a page. Compute the remaining budget first and ask for min(page, remaining),
so the page size stays a batching detail rather than an implicit floor under an
operator ceiling.
The exhaustion inference moves with it. A short page used to prove the table
was exhausted; once the limit can be shortened by the budget it proves nothing,
and marking the pager exhausted breaks at the top-of-loop arm that sits ahead
of every ceiling arm, taints nothing, and mints no continuation. That path ends
in a definitive 404 for content that exists, so the predicate now compares the
returned rows against the limit actually sent.
The budget computation sits above the timeout opener so the deadline-wrapping
guard's lookback window is unaffected, and the limit is read from the AppState
ceiling the row-ceiling arm reads, not the config field, so the arm and the
query cannot disagree.
Full suite 1059 green, fmt, clippy, and --locked clean.
The row ceiling is enforced in the scan loop, after a full page has already
been fetched and rule-loaded, so a cap below the page size is ignored for the
first fetch and any cap that is not a page multiple overshoots by up to a page.
The existing coverage could not catch that. Both row-ceiling tests asserted
rows <= ceiling + one page, which passes whether or not the query is bounded,
and rows-returned cannot distinguish a bounded query from a full fetch that is
trimmed afterwards. Add a scan_limit seam recording the limit actually sent to
the database, tighten both tolerant assertions to exact counts, and add two
tests that pin the limit: a ceiling below the page size and a ceiling that is
not a page multiple.
Also add the fail-open fixture. A repo evaluated with a partial rule set is
served as if it had no rules, so the boundary repo is seeded public at root
with a path-scoped rule and a real blob on disk, pinned with NULL provenance so
the scan path's rule map is the one consulted, and asserted never served across
its own ladder. Its positive control serves the same blob with the rule absent,
so a failure there is attributable to the rules rather than to a probe or a
budget skip.
Both new tests fail against current code on the scan_limit assertion.
Both write-lock tests hand-copied advisory_lock_key's derivation. The copy
still used DefaultHasher, so once the key moved to SHA-256 (#210) the probe
checked a key nobody held. The disconnect test then failed asserting the
lock was held, and the success test passed asserting a release it never
actually observed.
Import the production function, as the acquire-deadline test already does,
so the two cannot diverge again.
An external review found three costs that outlive the request or the boot
that paid for them.
The scan token was sealed with a key minted per boot, so a restart or a
rolling deploy made every outstanding continuation undecryptable and the
handler reads an unopenable token as absent. A caller walking a large
inventory could be sent back to the front indefinitely. The key is now
derived from the node's Ed25519 seed with HKDF, so it is the same after a
reload, under a distinct salt and info string so it is not the signing
key and cannot be turned against signatures. Rotation is a version bump in
the info string. An earlier comment rejected derivation on the grounds
that surviving restarts bought nothing; that is exactly what it buys, and
the comment now says so.
The rule byte ceiling was summed after the page had already been fetched,
so it truncated the request without bounding the work: the allocation and
the transfer had happened. The budget now bounds the query itself. The cut
lands on a repo boundary, which is not cosmetic: a mid-repo cut leaves a
partial rule set, and a partial rule set at the gate is indistinguishable
from a repo with no rules, so it would fail open and serve an object those
rules withheld. Only fully loaded repos are gated. The query always admits
the first rule-carrying repo of a page, so the bound is the budget plus
one repo rather than the budget; without that a single oversized repo
would put the cut at the cursor and every retry would reproduce the same
page.
The old check carried a not-exhausted condition so a short final page
could not turn a complete scan into a permanent shed. That condition is
gone because the taint no longer keys on page length; it keys on the query
having left repos unloaded. A short final page that fits produces no cut
and still 404s, and one that is cut is honestly incomplete and resumable.
Discovery paged the whole repo table and stat'd every warm repo to pick a
sixteen candidate window. Its comment called that acceptable because the
cost was paid once, which was true when the sweep ran once per boot. This
round made the sweep periodic, so an upgraded node with one unrepairable
row paid a full table scan on every run, forever. The load now stops once
the window is full. It collects one candidate past the cap so that running
out still distinguishes a warm set that fits from one that does not, which
is what the reset arm needs now that the total count is no longer known.
Ordering is unchanged, so an attacker who mints repos still cannot move
the boundary.
Verified by mutation: a per-boot key strands a continuation across a
restart, using the signing seed directly defeats the domain separation,
summing bytes after the fetch materializes the whole page, paging to
exhaustion buys pages the window cannot use, and reading the fit under cap
from the window length loses the reset.
Code review found two ways the scan ladder still dead-ends, both the same
shape the round has been fixing: a truncation that sheds without a token,
which this design reads as "the ladder is over", so the caller restarts at
the front forever and everything past that point is unreachable.
The continuation was wired into the row and rules ceilings but not the
probe and visit ceilings, and those two bind first on any inventory with
root-readable repos, long before the row ceiling. Every fixture seeded
root-denying repos, so no probe was ever charged and no test could reach
the arms that were missing it. All four breaks stop at the same cursor, so
all four now carry it.
The token's field width was 128 bytes against a repo id the write path
admits at 356 (255 owner, a slash, 100 name); a plain full DID with a long
name already passes 128. A repo over the width at a truncation boundary
failed the seal and shed tokenless, and repo names are peer-controlled, so
the boundary was steerable rather than unlucky. Fields are now sized
separately, 64 for the timestamp key and 384 for the id, and the version
byte moves so older tokens fail open to absent instead of mis-parsing.
Tokens stay constant length at 668 characters.
The sweep no longer exits for good on a failed pass. Making it periodic
was the point of the round, and returning on the first error meant one
deadlock disabled repair until the next boot, silently. It re-arms on a
longer interval, escalates the log once failures are consecutive, and
backs off again when a run repairs nothing so a table that will never
repair stops costing 64 object reads every five minutes.
The rule ceiling counts retained bytes instead of rule rows and is checked
where the rules are actually fetched, since a page can carry unboundedly
many rules and rule count is the wrong unit for a memory bound. It skips a
short final page: without that, a complete scan of an absent object turns
into a permanent 503.
Two docs contradicted the code and are corrected: the walk cap is charged
per phase now, so a request can spend twice the knob, and the README's
retry bound holds when the row ceiling binds.
Verified by mutation: reverting either ceiling's continuation strands the
holder, narrowing the token width fails the seal at the boundary,
restoring the terminal return stops the sweep, counting rules instead of
bytes lets a page overshoot, and removing the idle backoff keeps a
fruitless table under the base interval.
get_by_cid takes the global walk permit and the per-source permit before
resolving the CID, and that lookup was clamped only by the 600s request
budget. A well-formed CID with no pinned row does no probe and no walk,
but against a stalled pool it held a scarce walk slot for the full
window, so requests from enough distinct sources could reject every real
retrieval at admission while nothing was walking.
The lookup now runs under its own budget, defaulting to ten seconds and
clamped by the request budget, so a larger value degrades rather than
extends. Admission still comes first. Resolving before admission was the
other way to fix this and it trades one amplification for another: taking
the read out from behind the gate lets unadmitted anonymous callers stack
concurrent queries, and shedding a flood cheaply before it reaches the
database is what admission-first is for.
Only that one await moves. Everything after it stays on the request
budget because it runs interleaved with work the caller has already been
admitted for: the per-oid source lookup runs after real probing from the
second candidate on, the marker pair only on a provenance miss, and the
pager fetches between pages that cost probes. A short clock anchored at
admission would be long spent by then, so widening it would shed
requests that are making progress. The region comment now carries that
assignment per await.
The shed names the knob an operator can actually turn, separately from
the request-budget shed, so the two are distinguishable in the body and
the log.
Verified by mutation: reverting the clamp overruns the short budget, and
anchoring one short clock across the whole permit-held region sheds a
slow but progressing walk. The second leaves the first test green, which
is what makes it a boundary case rather than a second removal.
MAX_HISTORY_WALKS_PER_REQUEST is MAX_PIN_SOURCES + 1, which is exactly
enough for the first pinner plus every recorded additional source. Those
seventeen sources each spend a walk reaching their deny when they are
root-readable but path-scoped, so the walk budget is gone by the time the
at-cap fallback reaches a public source that record_pin_source dropped at
its sixteen-row cap. If that source needs a walk too it is skipped at the
ceiling and the request returns 503 search_incomplete on every retry: a
public object nobody can ever fetch.
The counter is now per phase. gate_and_serve already knows which phase it
is in, so provenance and fallback each get their own budget against the
same cap, and the fallback's capacity no longer depends on what the
provenance phase spent. Total walk work per request stays bounded at
twice the cap, a constant, so the amplification story holds.
Raising the shared ceiling was the obvious alternative and does not work:
the adversary picks how many provenance slots exist, since they are
grindable repo ids filling pin_repo_sources, so cap + N denials recreate
the exhaustion for any constant N.
The test uses real repos with real path-scoped rules that each spend a
real walk, proven by the walk log. The existing buried-source test seeds
repos that do not exist, which never enter the walk block at all, and
that is why it could not see this.
The must-not assertion needed changing to mean anything. A CID is
content-addressed, so every holder of it has byte-identical content and
comparing the served bytes against the public repo's object cannot show
which repo served. The gate is pinned instead by a second object held
only by a path-denying repo, with the fallback armed for it, asserted to
404 for an anonymous caller.
Verified by mutation: re-merging the counters restores the deterministic
503; capping the fallback at what the provenance phase left over is a
reserve that still couples them and fails the same way; and dropping the
fallback cap entirely unbounds the total. Each reddens exactly one of the
two tests, so neither is standing in for the other.
A denial-only inventory paged through the whole repo table. gate_and_serve
returns Skip on quarantine and on the root visibility deny before either
walk.probes or walk.visits increments, and those two counters were the
pager's only stop conditions, so an anonymous caller holding a CID from
the public pins index could read every row and its rules while the global
IPFS walk admission stayed held for the request budget.
Three parts, and the shape of each is set by a way an earlier attempt was
wrong.
A row ceiling, with a rules companion. Rows scanned bounds the DB reads
and the pager's row retention; it does not bound rules, since the rules
query has no LIMIT and an owner picks how many rules their repos carry, so
that gets its own ceiling. Both taint into the existing retryable 503 and
never a 404.
A ceiling alone would stop there and strand content. The scan starts from
a NULL cursor over (created_at, id) ASC, so a bare ceiling cuts the same
oldest prefix on every request and a holder past it is unservable for
good, which is the defect this PR already carries a finding about. So the
503 hands back the position. Server-side alternatives were tried and
dropped: a node-global cursor lets concurrent callers skip each other's
windows, and a per-caller map lets a key farm evict an honest caller
mid-ladder. State the caller carries has neither problem.
The token is sealed, not encoded. It names a row the caller may not be
allowed to see, and id derives from the owner DID, so plaintext would leak
a withheld repo's existence, creation time and owner. XChaCha20-Poly1305
with a fresh nonce per seal, the canonical CID as associated data, an
expiry inside the sealed bytes, and a fixed-width plaintext so token
length cannot vary with the row it names. Undecryptable, expired,
malformed and foreign-CID tokens are one indistinguishable case: absent,
start at the front.
The wrap taint keys on the pager reaching exhaustion while resumed, not
on any break site, so a token pointing at the last row still sheds a
retryable 503 instead of falling through to the 404 tail.
Pages are charged to the per-IP work bucket. Without that the ceiling
bounds one request and nothing across requests, and the denial path pays
nothing, which is the per-request-free-allowance shape already rejected
once on this branch. The derived work-budget floor grows by the same page
term so a deep scan cannot self-throttle mid-ladder.
A toll shed renders 429 rather than 503, which reverses the tail's
documented precedence for this one case. The tail comment names the
exception: the caller's own bucket stopped the scan, not the node's
search, and a 503 would invite an immediate retry into an empty bucket.
Four existing tests hand-size the work limiter to their exact probe count
and now also pay for a page; each gained one token and a comment showing
the arithmetic. Production is unaffected because the floor grew by the
same term.
Verified by mutation: removing the ceiling unbounds the scan; truncating
without tainting turns it into a 404; emitting a token but never resuming
never reaches the holder; dropping the toll makes pages free; encoding
the token as base64 plaintext leaks the withheld fields; dropping the CID
from the associated data honors a cross-CID resume; a variable-width
encoding makes token length vary with the row; and a constant nonce makes
two seals identical.
GITLAWB_IPFS_MAX_LEGACY_PROBES bounded the probe fan-out, but nothing bounded
the load that ran before the first probe. On a legacy (NULL-provenance) or
capped/incomplete provenance set, get_by_cid called list_all_repos, cloned every
id, fetched visibility rules for the whole set, and fetched the node's entire
quarantine list, then retained all three for the request. /ipfs/{cid} is
anon-reachable and CIDs are enumerable from the unsigned pins index, and an
object pinned from MAX_PIN_SOURCES repos is permanently at cap, so every later
anonymous GET for it paid that whole preload with the scarce walk permits held,
even at a probe budget of 1.
The selection is now keyset-paged on (created_at, id) ASC, with the quarantine
flag returned per row so the separate whole-node quarantine query is gone. Both
key columns are immutable, so paging is exact: a repo touched mid-scan cannot
cross a page boundary and go unvisited, and updated_at is attacker-bumpable,
which would have let a caller sort their own repos ahead of the true holder.
The pager lives outside the oid loop, so its cursor and its accounting are per
request rather than per candidate. Before fetching another page the scan checks
that a row on it could still reach a verdict; a spent probe or visit budget
stops the read and TAINTS, so a truncated search still sheds the retryable 503
and never a false 404. A page of pure denials is deliberately not a stop
condition, since a cheap denial costs no probe and a public object buried behind
private repos must still be reached. Both page queries stay clamped to the
remaining request budget, and the rules query still fails closed.
A new v25 migration adds idx_repos_created_at_id. Bounding what the handler
materializes is only half the property: repos carried no index in the new keyset
order, so Postgres seq-scanned the table and top-N sorted it to return every
page while the walk admission was held, which moves the same O(rows) cost onto
the database rather than removing it. Measured on 50k rows: 951 shared buffers
and ~44ms per page without the index, versus an Index Only Scan at 7 buffers and
~0.09ms with it, and the keyset predicate pushed down as an Index Cond instead
of filtering after a scan. Nothing names the index in any query text, so its
entry says why it exists and a test asserts its presence structurally.
Two order-dependent tests were reseeded rather than left green: the buried-row
and acquire-taint cases encoded their ordering through updated_at, which no
longer determines iteration. Both were re-confirmed RED under their documented
mutations after the reseed. Three regressions land with the fix. One drives a
one-probe, one-row-per-page request against three candidate repos and asserts it
materializes one row and runs one query, which a query counter alone could not
have caught. One seeds two distinct source-less oids under a single CID and pins
the per-request property directly: the second candidate re-reads the pages the
first paid for, so the whole request costs one pass rather than one per
candidate. The third covers the index, including the INV-7 upgrade path.
Refs Gitlawb/node#173.
pinned_cids.pin_sources_incomplete was one boolean per object, so any
successful source record cleared it, including a genuine record from a repo
unrelated to the failure. The resolver reads a cleared marker as "every source
is recorded", drops the scan fallback, and 404s an anonymous caller whose only
servable copy is the one whose record failed. The missing source is a property
of an (object, repo) pair, so v24 stores it as one and only a later record from
that same repo clears it.
Pre-upgrade markers carry over with an empty sentinel repo id that no real
record matches, so they keep their fallback instead of being cleared by the
next unrelated record. pinned_cids.pin_sources_incomplete is left in place,
unread, so a rollback to the previous release still finds its markers.
Also adds the handler-level coverage the size-probe commit could not: a test
seam drops the object between the type probe and the size probe, and the
request tails to a retryable 503 rather than a definitive 404. The seam is
keyed on (repo path, oid) because fixture oids are content-derived and shared
across tests, and an oid-only key deleted objects out from under two unrelated
tests running in parallel.
object_size_bounded mapped every non-timeout failure to Ok(None), which
gate_and_serve reads as a verified absence and does not taint the search for.
A corrupt object, an unreadable pack, or a failed spawn therefore ended the
search cleanly and handed an authorized caller a definitive 404 instead of the
retryable 503 tail, even though the type probe had already reported the object
present. Confirmed by execution against the old code: a corrupt loose object
returned Ok(None) where the healthy read returned Ok(Some(n)).
The probe now returns Result<u64, ProbeError>, the vocabulary the type and
content stages either side of it already use: a reaped child is Transient, and
everything else is classified by store readability, Transient when the store
cannot be read and Deterministic when it can. There is no absence value left,
so the collapse is unrepresentable rather than fixed at one call site, and the
gc-between-stages case is classified like any other fault.
ServedRead::Gone lost its only producer and is removed with it.
Covered by two store tests, each asserting a healthy read first so the fault
case cannot pass on a broken fixture. Not covered: the handler path that turns
the fault into a 503, because that call site hardcodes "git" rather than
state.git_bin, so no shim can be injected there.
The deadline test hand-copied the advisory-lock key derivation, and that
copy drifted once the key moved to SHA-256, silently breaking the lock
collision the test is built around. Import the production function
instead so the key cannot diverge again.
#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.
The test reproduces acquire_write's advisory-lock key derivation locally so
a held session lock can stall the handler. repo_store's advisory_lock_key
now uses SHA-256, so the DefaultHasher copy no longer collided and the
stall never happened. Update the local copy to the SHA-256 derivation.
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.
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.
#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.
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)
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.
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.
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.
beardthelion