mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-11-11 18:18:12 +00:00
af2e96d92a
Follow-up to #2127, from @dborup's review there. The limiter, most of its tests and the docs text are dborup's (dborup/CoreScope@195ec4c3), adapted to master; the commit carries `Co-authored-by: dborup`. ## What changes `GET /api/nodes/{pubkey}/reach` still runs at most 2 uncached reports at once. What changes is the next uncached request: - **Before:** 429 at once, `Retry-After: 5`. - **After:** it waits up to 3 s for a slot. It gets 429 with `Retry-After: 2` and a JSON body only if no slot frees in that time. Unchanged: cached answers never wait, and concurrent requests for the same node and `days` share one computation and one slot. One addition beyond dborup's version: the shared computation runs on the context of the request that started it. If that request disconnects, while queued or mid-scan, the other requests sharing it that are still connected now get 429 instead of 500. The 3 s wait made that case more likely. ## Why dborup's three points on #2127: 1. An instant 429 fails two tabs, or a quick switch between 7/14/30 days, when the database is not under real pressure. 2. The page cannot recover from it, because `api()` retries only 503. The panel retry is a separate PR so that this one stays server-only. 3. No test covered the promised property: a cached request still answers while every slot is held. Waiting parks a goroutine, not a database connection, so the pool is protected exactly as before. ## What I checked - **Tests.** `make test` (all 16 modules), `make vet`, `gofmt`, `sh test-all.sh` and eslint (0 errors) all pass. The reach tests also ran 10 times under `-race`. - **New tests.** 14 tests in `node_reach_cold_limit_test.go`: dborup's 10 plus 4 new. `TestNodeReachConcurrentBuildCap` now drives the new limiter through the production router. Point 3 is `TestNodeReach_WarmHitSkipsColdLimit`. The 4 new tests are: - a queued request is served when a slot frees; - a cache filled just before the flight skips the limiter; - coalesced waiters get 429 when the starter disconnects, queued or mid-scan (two tests). - **Mutations.** I wrote 21 from the code before running the tests, and the tests caught all 21: instant reject, cap +1, release that leaks, slot taken before the in-flight recheck, busy → 500, Retry-After header or value, status 503, dropped context mapping, and others. - **Real binary.** Master `2d09335c` and this branch, run one after the other on a copy of a 10.5M-observation, 11 GB database. Each burst fires its reach requests at once, while `/api/nodes?limit=50` and `/api/packets?limit=50` are polled every 200 ms. | Burst (distinct uncached keys) | master | this branch | |---|---|---| | 3 keys, `days=7`, warm page cache (2 runs) | 2 × 200, 1 × 429 after 0.01 s | 3 × 200; the third answered at 4.1–4.3 s | | 6 keys, three of them `days=14` (2 runs) | 2 × 200, 4 × 429 after ≤ 0.01 s | 2 × 200, 4 × 429 after 3.0 s | | 24 keys, cold page cache | 2 × 200, 22 × 429 | 2 × 200, 22 × 429 after 3.00 s | | `/api/nodes` during every burst | median 1 ms, max 53 ms | median 1 ms, max 66 ms | ## Not checked - **Slow scans.** On a database this size, uncached scans took from about 9 s to over 60 s, so the 3 s wait only helps when page cache is warm or the database is smaller. Otherwise the result is the same 429, 3 s later. A 30-day uncached scan hit the 60 s `WriteTimeout` on both builds; that is the scan #2214's opt-in hop index addresses. - **Pool contention under a live store.** On this copy the in-memory store loaded 0 packets, so `/api/nodes` latency does not show contention under a live load. - **Platforms.** One machine (linux/amd64). No browser check: the server change has no UI, and the panel retry comes with its own PR. **Base.** Rebased on master `7c00a429`, which includes #2214; every test and mutation above was re-run on the rebased commit. The real-binary runs used `2d09335c`, just before #2214, on a database without the hop index. That is the default after #2214 too (`hopIndexDays` unset means off). With the index on, scans are shorter, so more bursts fit in the 3 s wait; I did not measure that. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- ## Update after review (commits `c65f4d3b`, `1d2cd99d`, `04e49854`) efiten asked how many requests may wait at once. With only the 3 s timeout, the number of waiting distinct keys grew with the request rate. n30nex wrote a cap (nullrouten0/Baymc-CoreScope#1, tracked in #2264). Its two commits are now on this branch unchanged, as a fast-forward. - At most 8 distinct cold requests wait for a slot. Any further one gets 429 with `Retry-After: 2` at once, before a timer is set. - A waiter gives its place back on every exit: it gets a slot, disconnects or times out. - Cached answers and same-key sharing are unchanged. Requests for a key that is already queued or building wait on that one place and are not counted. `04e49854` corrects the code comment that said the ceiling was independent of request rate. **What I checked on `04e49854`:** - The two new tests fail on `28da016f` and pass now. - 12 mutations of the cap, all caught: cap 7 and cap 9; the place never returned, returned before waiting, returned only on promotion, leaked on cancel, leaked on timeout; a full queue that blocks, returns the wrong error or skips the counter; a timer armed before the check; a queue sized by the slot count. - `make test`, `make vet`, `make fmt-check`, `sh test-all.sh` and eslint 8.57.1 (0 errors) pass, and the reach tests pass 5 times under `-race`. On a local merge with current master `660cbbf5`, which includes #2277, the reach tests pass 3 times under `-race` and the `cmd/server` package passes. - Real binary: both heads, alternately, on a copy of a 2.5 GB soak database, with 24 distinct uncached `days=7` requests fired at once, two runs each: | | `28da016f` | `04e49854` | |---|---|---| | 200 | 2, 2 | 2, 3 | | 429 at once | 0, 0 | 14, 14 | | 429 after 3 s | 22, 22 | 8, 7 | **Not checked:** a sustained flood, a burst of same-key requests against the cap, `/api/nodes` latency during these bursts, and platforms other than linux/amd64. The real-binary table further up was measured before the cap. --------- Co-authored-by: nullrouten0 <201262769+nullrouten0@users.noreply.github.com> Co-authored-by: dborup <kontakt@meshview.dk> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: n30nex <blushellneon@outlook.com> Co-authored-by: efiten <erwin.fiten@gmail.com>
50 lines
1.7 KiB
Go
50 lines
1.7 KiB
Go
package main
|
|
|
|
import (
|
|
"context"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"testing"
|
|
"time"
|
|
)
|
|
|
|
// GET /api/nodes/{pubkey}/reach is unauthenticated and a cold-cache request
|
|
// starts a scan. Only reachColdBuildLimit scans may run at once; a cold
|
|
// request that finds every slot taken queues for at most the limiter's wait,
|
|
// then gets 429 with Retry-After instead of queueing on the shared SQLite
|
|
// pool. Goes through the production router.
|
|
func TestNodeReachConcurrentBuildCap(t *testing.T) {
|
|
srv, router := setupTestServerWithAPIKey(t, "")
|
|
srv.reach.cold.configure(0, 5*time.Millisecond, nil) // default cap, short wait
|
|
const key = "ab00000000000000000000000000000000000000000000000000000000000001"
|
|
|
|
get := func() *httptest.ResponseRecorder {
|
|
req := httptest.NewRequest("GET", "/api/nodes/"+key+"/reach?days=7", nil)
|
|
w := httptest.NewRecorder()
|
|
router.ServeHTTP(w, req)
|
|
return w
|
|
}
|
|
|
|
// Fill every build slot as if scans were in flight.
|
|
for i := 0; i < reachColdBuildLimit; i++ {
|
|
if err := srv.reach.cold.acquire(context.Background()); err != nil {
|
|
t.Fatalf("slot %d should be free: %v", i, err)
|
|
}
|
|
}
|
|
if w := get(); w.Code != http.StatusTooManyRequests {
|
|
t.Fatalf("expected 429 while all build slots are busy, got %d (body: %s)", w.Code, w.Body.String())
|
|
} else if w.Header().Get("Retry-After") == "" {
|
|
t.Fatalf("429 must carry Retry-After")
|
|
}
|
|
|
|
// Free the slots: the same request now runs the scan. The key is not a
|
|
// known node, so the normal answer is 404 — the point is that it is no
|
|
// longer 429.
|
|
for i := 0; i < reachColdBuildLimit; i++ {
|
|
srv.reach.cold.release()
|
|
}
|
|
if w := get(); w.Code == http.StatusTooManyRequests {
|
|
t.Fatalf("still 429 after slots were released (body: %s)", w.Body.String())
|
|
}
|
|
}
|