Files
meshcore-analyzer/cmd/server/node_reach_cold_limit.go
T
af2e96d92a fix(reach): queue a cold request up to 3 s for a build slot before answering 429 (#2249)
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>
2026-10-29 22:50:59 +01:00

192 lines
6.7 KiB
Go

package main
import (
"context"
"errors"
"net/http"
"strconv"
"sync"
"sync/atomic"
"time"
)
// Bounded concurrency for cold-cache reach reports.
//
// /api/nodes/{pubkey}/reach collapses a thundering herd on one cache key with
// singleflight, but distinct (pubkey, days) keys are independent: N cold keys
// arriving together would run N windowed observation scans at once. The read
// pool is 4 connections (OpenDB), so a handful of concurrent cold scans could
// hold every connection and leave /api/nodes and /api/packets queueing behind
// them. At most reachColdBuildLimit cold scans run at once, which keeps half
// of the pool free for every other endpoint.
//
// At most reachColdWaiterLimit distinct cold build leaders may queue for up
// to reachColdBuildWait; excess leaders are shed immediately with 429 +
// Retry-After. The wait costs a parked goroutine and timer, not a database
// connection. For distinct (pubkey, days) keys the ceiling is two active
// scans plus eight queued leaders per Server, whatever the request rate.
// Requests for a key that is already queued or building are singleflight
// followers: they wait in sf.Do on their leader's admission and are not
// counted, so their number still grows with the request rate.
//
// Design and tests by dborup (dborup/CoreScope@195ec4c3), reviewing #2127.
//
// Vars (not consts) so tests can tighten the cap and the queue wait without
// standing up slow scans, the same pattern as reachScanRowLimit.
var (
// reachColdBuildLimit is how many cold reach reports may compute at once.
reachColdBuildLimit = 2
// reachColdBuildWait is how long a cold request queues for a slot before
// it is shed with 429.
reachColdBuildWait = 3 * time.Second
)
// reachColdRetryAfterSeconds is the Retry-After sent with a shed request.
// Short on purpose: a slot frees as soon as one in-flight scan finishes.
const reachColdRetryAfterSeconds = 2
// reachColdWaiterLimit is a fixed internal default, not operator/customizer
// configuration. It bounds distinct leaders waiting for a build slot.
const reachColdWaiterLimit = 8
// errReachColdBusy is returned by reachColdLimiter.acquire when the limiter
// has a full queue or stayed saturated for the whole queue wait. The handler
// maps it to 429.
// singleflight does not keep errors, so the next request recomputes.
var errReachColdBusy = errors.New("reach: cold-scan concurrency limit reached")
// reachColdLimiter is a counting semaphore with a bounded queue count/wait, plus
// counters read by the tests. Usable as a zero value: the slot channel is
// built on first use, so every Server (including the ones tests assemble by
// hand) gets the limit without a constructor.
type reachColdLimiter struct {
mu sync.Mutex
sem chan struct{} // buffered to the limit; one slot per in-flight scan
queued chan struct{} // one admission per distinct leader waiting for a slot
lim int // <=0 → reachColdBuildLimit
wait time.Duration // <=0 → reachColdBuildWait
hook func() // test injection, run just after a slot is taken
inflight atomic.Int64 // slots currently held
peak atomic.Int64 // high-water mark of inflight
acquired atomic.Uint64 // slots handed out
rejected atomic.Uint64 // requests shed with errReachColdBusy
canceled atomic.Uint64 // waiters whose context ended while queued
}
// configure replaces the limiter's capacity, queue wait and acquire hook.
// Test setup only: call it before any request can acquire, since holders
// release into the channel it installs. limit <= 0 and wait <= 0 fall back to
// the package defaults; hook is nil in production.
func (l *reachColdLimiter) configure(limit int, wait time.Duration, hook func()) {
l.mu.Lock()
defer l.mu.Unlock()
l.lim, l.wait, l.hook, l.sem, l.queued = limit, wait, hook, nil, nil
}
// slots returns the lazily built slot and queue channels, wait and hook.
func (l *reachColdLimiter) slots() (chan struct{}, chan struct{}, time.Duration, func()) {
l.mu.Lock()
defer l.mu.Unlock()
if l.sem == nil {
n := l.lim
if n <= 0 {
n = reachColdBuildLimit
}
if n <= 0 {
n = 1
}
l.sem = make(chan struct{}, n)
l.queued = make(chan struct{}, reachColdWaiterLimit)
}
w := l.wait
if w <= 0 {
w = reachColdBuildWait
}
return l.sem, l.queued, w, l.hook
}
// acquire takes a slot for one cold reach build. It returns nil when a slot
// was taken (the caller MUST release it), ctx.Err() when the request went
// away first, or errReachColdBusy when the queue is full or the limiter stayed
// saturated for the whole queue wait.
func (l *reachColdLimiter) acquire(ctx context.Context) error {
if err := ctx.Err(); err != nil {
return err
}
sem, queued, wait, hook := l.slots()
// Fast path: a free slot is taken without arming a timer.
select {
case sem <- struct{}{}:
l.took(hook)
return nil
default:
}
// Bound waiting leaders before allocating a timer. Every exit below gives
// back the admission, including promotion to an active scan.
select {
case queued <- struct{}{}:
defer func() { <-queued }()
default:
l.rejected.Add(1)
return errReachColdBusy
}
t := time.NewTimer(wait)
defer t.Stop()
select {
case sem <- struct{}{}:
l.took(hook)
return nil
case <-ctx.Done():
l.canceled.Add(1)
return ctx.Err()
case <-t.C:
l.rejected.Add(1)
return errReachColdBusy
}
}
// took records a handed-out slot and runs the test hook, if any, outside the
// limiter's mutex.
func (l *reachColdLimiter) took(hook func()) {
l.acquired.Add(1)
n := l.inflight.Add(1)
for {
p := l.peak.Load()
if n <= p || l.peak.CompareAndSwap(p, n) {
break
}
}
if hook != nil {
hook()
}
}
// release hands a slot back. Call it exactly once per successful acquire (the
// handler defers it). Never blocks, so an unbalanced release cannot wedge a
// request.
func (l *reachColdLimiter) release() {
l.inflight.Add(-1)
sem, _, _, _ := l.slots()
select {
case <-sem:
default:
}
}
func (l *reachColdLimiter) peakInflight() int64 { return l.peak.Load() }
func (l *reachColdLimiter) acquiredCount() uint64 { return l.acquired.Load() }
func (l *reachColdLimiter) rejectedCount() uint64 { return l.rejected.Load() }
func (l *reachColdLimiter) canceledCount() uint64 { return l.canceled.Load() }
// writeReachColdBusy429 sheds a cold reach request that could not get a slot.
// 429 (not 503) because the server is healthy and the work is rate-limited;
// Retry-After tells a well-behaved client how long to back off.
func writeReachColdBusy429(w http.ResponseWriter) {
ra := strconv.Itoa(reachColdRetryAfterSeconds)
w.Header().Set("Retry-After", ra)
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusTooManyRequests)
_, _ = w.Write([]byte(`{"error":"reach is busy computing other reports","retryAfter":` + ra + `}`))
}