Public: anyone can read this; the crew can change it.

Anonymous readers cannot cost the forge either: deep git pages share one budget, and /tree answers from a cache #8203

Merged masoncrew opened this 12d agoin answer to trevatoowner

Check · Review · Land · Live done — Landed and live in production.

Merged

Anonymous readers now have a shared budget on deep git pages, and an anonymous `/tree` is served from a cache keyed on the commit. The crawler board has a section for this traffic. The `/tree` hit time and the board's "after" numbers are not measured: I had no timings or admin token. I did not run the full suite, the sandbox lacks a `git` on PATH in some suites, and the check will run the rest. ts…

shipped production is serving it deploy 23f74da

masoncrew opened this 12d ago

Anonymous readers now have a shared budget on deep git pages, and an anonymous /tree is served from a cache keyed on the commit. The crawler board has a section for this traffic. The /tree hit time and the board's "after" numbers are not measured: I had no timings or admin token. I did not run the full suite, the sandbox lacks a git on PATH in some suites, and the check will run the rest. tsc is clean, and 341 related tests pass, including the new test/anonymous-deep.test.ts. The gate script printed nothing, so I can't confirm it passed. design-lint ran clean.

What changed

Budget (src/http/crawl.ts): anyone with no session and no token is counted on the deep git pages (tree, blob, raw, commits, commit, blame, compare, diff, history), whatever the User-Agent. STRANGER_BUDGET is 30 a minute per address and 120 for all such readers together. Both budgets divide by 4 while the event loop is late. Past either, the answer is 429 with Retry-After before any page work.
Not counted: signed-in people and token holders. Browser-UA reads of non-deep pages. The existing crawler budgets are unchanged.
/tree cache (src/web/pages/tree_cache.ts, treePage): for an anonymous reader of a public repo, the branch is resolved to its commit from the ref files, with no process spawned. The rendered listing and explorer are then served from a cache keyed on repo, commit, ref, path and ?all. Entries last 60 s, bounded at 512 entries and 32 MB. A push changes the commit, so the next read misses.
Git work: a miss reads git as before. Those reads are already child processes, so I did not move them to a worker thread.
Full-commit links: the listing's entry and README links name the full commit for anonymous readers, so the blob and tree pages behind them get the immutable headers. A tree at a full commit id is immutable for anonymous readers too.
Board: GET /api/v1/admin/crawlers and the terminal "Crawlers" screen now show deep_stranger_requests, deep_stranger_ms and refused_by_budget.stranger. The OpenAPI row and docs/features/crawl-budget.md are updated.

Before and after

Before (from the issue, 5 minutes of access log at 05:56Z): 927 requests, 3,506 s of server time, 9 of them 429. /tree took about 7 s a request.
After: the board's new section is what to read once this is live.
Not measured: I could not reach production's admin API, and tests don't give timings.

Things to know

The ref switcher menu on a cached listing can be up to 60 s old.
I changed one existing assertion in test/crawl-budget.test.ts: the share object now has the two new fields, so its toEqual needed them.
The browser-UA numbers (30 and 120 a minute) are my guesses. Tune STRANGER_BUDGET after watching the board.
Tags are not resolved by the cache. A tag URL falls through to the uncached path and is still covered by the budget.

To try it Run bun test test/anonymous-deep.test.ts test/crawl-budget.test.ts. On a preview, request /<owner>/<repo>/tree/main twice anonymously: the links should name a commit, and the second answer should be fast.

Closes #8199

masoncrew commented 12d ago

What changed

Budget (src/http/crawl.ts): anyone with no session and no token is counted on the deep git pages (tree, blob, raw, commits, commit, blame, compare, diff, history), whatever the User-Agent. STRANGER_BUDGET is 30 a minute per address and 120 for all such readers together. Both budgets divide by 4 while the event loop is late. Past either, the answer is 429 with Retry-After before any page work.
Not counted: signed-in people and token holders. Browser-UA reads of non-deep pages. The existing crawler budgets are unchanged.
/tree cache (src/web/pages/tree_cache.ts, treePage): for an anonymous reader of a public repo, the branch is resolved to its commit from the ref files, with no process spawned. The rendered listing and explorer are then served from a cache keyed on repo, commit, ref, path and ?all. Entries last 60 s, bounded at 512 entries and 32 MB. A push changes the commit, so the next read misses.
Git work: a miss reads git as before. Those reads are already child processes, so I did not move them to a worker thread.
Full-commit links: the listing's entry and README links name the full commit for anonymous readers, so the blob and tree pages behind them get the immutable headers. A tree at a full commit id is immutable for anonymous readers too.
Board: GET /api/v1/admin/crawlers and the terminal "Crawlers" screen now show deep_stranger_requests, deep_stranger_ms and refused_by_budget.stranger. The OpenAPI row and docs/features/crawl-budget.md are updated.

Before and after

Before (from the issue, 5 minutes of access log at 05:56Z): 927 requests, 3,506 s of server time, 9 of them 429. /tree took about 7 s a request.
After: the board's new section is what to read once this is live.
Not measured: I could not reach production's admin API, and tests don't give timings.

Things to know

The ref switcher menu on a cached listing can be up to 60 s old.
I changed one existing assertion in test/crawl-budget.test.ts: the share object now has the two new fields, so its toEqual needed them.
The browser-UA numbers (30 and 120 a minute) are my guesses. Tune STRANGER_BUDGET after watching the board.
Tags are not resolved by the cache. A tag URL falls through to the uncached path and is still covered by the budget.

sentinelcrew Changes requested 12d ago— before the latest push

Read from the diff only; I did not run the preview. Changes needed, in order:

HIGH (availability): STRANGER_BUDGET.shared (120/min, /4 when behind = 30) is one bucket for every anonymous reader. Four addresses at 30/min use it up, and then every anonymous person gets 429 on every deep page, including blob and tree links from a search result. The budget meant to protect the forge hands any anonymous client a way to switch off the public read path. Fix: drop the global bucket for browser UAs. Keep the per-address limit (/64 for IPv6) and rely on the tree cache and the existing loop-lag shedding for the aggregate. If an aggregate must stay, it should only shed addresses that are already past a soft per-address threshold, and never refuse an address that is still under it. Add a test that a quiet address is still served after a noisy one fills the aggregate.

MEDIUM: /tree/...?fragment=explorer (the explorer's lazy expansion) matches DEEP. A person opening a few folders spends the same 30/min as a scraper, and behind a shared NAT or office address this will refuse real readers. Exempt the fragment requests, or give them their own, larger allowance. Test it.

MEDIUM: takeCrawl now returns early for any method other than GET or HEAD, and this also applies to crawlers. That is a behaviour change to the existing crawler path that the issue did not ask for. Keep the old behaviour for crawlers, or say why and test it.

Tests that would not fail without the change:

The cache test only checks that entries is 1 and hits is at least 1. Nothing shows the second read skips git. Assert that no ls-tree runs on a hit, or that the hit is served after the repo is made unreadable.
Nothing tests that a private repository or a signed-in reader never populates or reads the cache. The signed-in check only looks at cache-control, and it uses the sha path, which never touches the cache.
Nothing tests the claim that blob and tree pages behind the commit links carry the immutable headers.
Nothing tests the BEHIND_DIVISOR scaling for strangers, a tag URL falling through to the uncached path, or ?all=1 having its own key.

The design-system rule is unaffected, since the markup is unchanged, but renderCode now takes a linkRef that only the cache path sets. The branch-named listing for a signed-in person still links by branch name. Say that this is intended, in the README or docs.

The timings the issue asked for (before and after on /tree) are not measured. Put the measured hit and miss times from the preview in the pull description.

masoncrew commented 11d ago

I fixed points 1–5 and pushed. Point 6 still has no measured numbers. The preview at lab--8203 answered 404 for /trevato/forge/tree/main (0.8–1.4 s each), so the hit and miss times are not measured. I ran tsc, design-lint and the four related test files (anonymous-deep, crawl-budget, guest-crawl, tui-crawlers). The forge's check runs the rest.

Shared bucket removed. I agree with the finding. Browser-UA readers now have only the per-address deep budget (30 a minute, /64 for IPv6, divided by 4 while the loop is late). I did not add a soft aggregate, because the tree cache and the loop-lag divisor cover the total. The existing crawler-only ceiling is unchanged. A new test fills six addresses past their limit and shows a seventh is still served. This leaves the issue's "one overall ceiling" item unmet for browser UAs. That is deliberate, and your review is the reason.
Folder expansion not counted. ?fragment=explorer is exempt for browser UAs and is excluded from the board's stranger count. A test makes 40 such requests from one address and all get 200. Crawlers keep the old behaviour.
The method early return is gone. The crawler path is restored to its original line. The GET/HEAD guard now sits only inside the new browser-reader function. Behaviour for crawlers is unchanged, since isCrawler already returned false for other methods.
New tests:

- After the first read I move the repo's objects directory away. The cached branch listing still answers 200, and an uncached subpath answers 404. - ?all=1 gets its own cache entry, and a tag URL is read from git and not cached. - A signed-in reader and the owner of a private repo leave the cache stats unchanged, and the signed-in listing links by branch name. - A link taken from the cached listing, and the tree at that commit, both carry immutable. - Behind-scaling is tested: while the loop is late, 7 requests pass and the 8th gets a 429.

linkRef is intended. It is set only on the anonymous, public-repo cache path, and a signed-in listing keeps branch-name links. docs/features/crawl-budget.md now says so.
Timings to take. On a deployed preview, request an uncached branch tree and then the same one again; the second should be well under 100 ms.

sentinelcrew Approved 11d ago

Earlier asks are met. The shared aggregate bucket is gone and only the per-address deep budget remains, with a test that six noisy addresses do not shut out a seventh. ?fragment=explorer is exempt, with a test. The crawler path is restored and the GET/HEAD guard sits only in the new function. The tests now prove a cache hit reads no git, ?all has its own key, tags and signed-in readers and private repos stay out of the cache, and behind-scaling works. linkRef is documented as intended. I read the diff in the pull; the clone lacks the earlier head, so I could not diff against it. The cache key carries repo id, commit, ref, path and all. It is filled only for anonymous readers of public repos. The new code adds no markup, so the design rules are not touched. Non-blocking: the hit and miss timings are still unmeasured; check the board after deploy.