[dborup]

Relates to #557

Summary

Reuse the existing queue-admission correction rather than redesigning the Reach limiter:

• Keep two active cold report builds and the existing three-second wait.
• Admit at most eight waiting distinct-key build leaders per server.
• Reject further leaders immediately with the existing HTTP 429 and Retry-After: 2, before allocating a wait timer.
• Return each queue admission on promotion, cancellation or timeout.
• Preserve warm-cache bypass, same-key singleflight coalescing, visibility, ranking, response semantics and the frontend's bounded 429 retry behavior.

Draft only. The independent static review described below completed after PR creation. Auto-merge remains off. No merge, deployment, host configuration or schema change is part of this PR.

Reuse and scope

The buffered-channel admission algorithm and its regression starting point come from the existing correction authored by n30nex. Attribution is retained in the commit message and here; the adaptation commits use the fork's requested dborup identity.

Existing correction commit: 1d2cd99ddb550570aa2a7c34e79eee30c9db5f8d
Source repository: nullrouten0/Baymc-CoreScope
Pull request number: 1
Original author: n30nex
Related comparison repository: Kpa-clawbot/CoreScope
Issue number: 2264
Related pull request number: 2249

Only four files differ from master:

• cmd/server/node_reach_cold_limit.go
• cmd/server/node_reach_cold_limit_test.go
• cmd/server/node_reach_waiter_cap_test.go
• docs/api-spec.md

Fork-specific adaptations retain the original fixture and assertions, issue pre-existing "everyone succeeds" tests in bounded bursts, bound cleanup waits, prime our separate rank snapshot for the no-queued-DB-work fixture, and add a 64-round cancellation-versus-promotion regression. No frontend, dependency, configuration, workflow or publication/deployment guard changes.

Commits and master synchronization

• 772b7f48: one logical implementation/test/documentation commit, initially based on ff1acc02.
• e9bacf22: ordinary merge of master 5af6129c after the separate LRU follow-up landed. No rebase, amend or force push.
• Final tree: 34fcb7d880d363ee7c287c6434cdc9213eabc290.

Local validation

Environment: macOS arm64, Go 1.26.3, Node 26 and real cached Chromium. Tests use disposable local fixtures, not staging or production.

Check	Result
Test-first regression on unchanged production code	Four intended overflow assertion failures: three admission-release subcases plus HTTP integration
Focused limiter/handler tests	PASS; driver also ran 13 top-level tests three times under -race, including 64 cancellation/promotion races per run
Parent Reach/rank/limiter race checks	PASS, -race -count=3, before master synchronization
Mutation removing the queue-admission block in a separate scratch copy	Same four intended assertion failures; mutation is not committed
Full server suite before master synchronization	PASS: 2,754 top-level Tests; 4,074 passing results including subtests, 12 existing skips; 103.929 s
Full unchanged ingestor suite	PASS: 1,075 top-level Tests; 1,398 passing results, 6 existing skips; 119.646 s
Full JS/script suite before synchronization	266 test files PASS, zero failing files; platform-specific backup-tool cases retain their existing skips
Required focused frontend checks	Packet filter 92, aging 19 and frontend helpers 719 checks PASS
Vet and whitespace/formatting	PASS; server vet rerun after synchronization
Go shard assignment guard after synchronization	PASS: 2,772 runnable entries, each selected by exactly one shard
Unchanged per-node Reach browser suite on synchronized head	PASS, no suite skip
Unchanged Reach-rank browser suite on synchronized head	14/14 PASS, including keyboard navigation, rank consistency, legacy-edge filtering and mobile layout
Unchanged Reach 429/retry browser suite on synchronized head	17 checks PASS, no suite skip

Initial sandbox-only Go attempts could not bind local fake-server ports. Their logs are retained separately; loopback-capable runs above actually executed those tests.

Verification gaps — not a fully green final-head claim

• The full server rerun on the synchronized tree hit its unchanged 10-minute timeout in the existing disk-backed ObsDedup411 fixtures. No earlier assertion failure was observed. Two unchanged fixture cases grew from approximately 0.87/1.04 seconds on the earlier baseline to 132.08/359.20 seconds and did eventually pass; ordinary local file/build operations also became slow. This supports an environment/I/O explanation but does not prove it. The timeout and stack are retained, not counted as success. Final-head CI must still establish the complete Linux result.
• The general browser smoke was interrupted by the scratch network-policy shim's own TargetClosed handling during browser teardown. After a narrow shim correction, a post-push attempt reused the exact already-built final source artifact and a fresh fixture copy to avoid another Go compilation. That attempt reached the existing per-node Reach suite's outer time limit on the locally stalled host; no new browser success or broad-smoke completion is claimed. The earlier completed stock Reach checks above remain their own evidence. Repository suites were not edited.
• An additional strict scratch navigation journey exposed _leaflet_pos errors on both the candidate and pristine ff1acc02. The stock rank test already documents that pre-existing teardown issue. A separate extra mouse attempt was intercepted by the fixed navigation; that pointer failure was not runtime-baseline-verified. Neither is silently presented as passing coverage or fixed by this PR.
• The three completed stock Reach suites used a scratch policy blocking non-loopback HTTP/WebSocket traffic and external tiles. Those checks validate local UI integration, not production traffic capacity. HTTP queue capacity itself is covered by the real Go handler regression.

Performance and limits

Admission is O(1), with one fixed eight-entry buffered channel per limiter. Once initialized, a full queue rejects with zero allocations, enforced by testing.AllocsPerRun; rejected overflow does not allocate another wait timer. Tests pin two active builds, eight admitted waiters, admission reuse and unchanged pool wait count while the controlled leaders are queued.

This is not global HTTP/database admission control. Same-key singleflight followers are outside the eight-leader count. Visibility and ranking checks may issue SQL outside the cold-build limiter, including on cached responses. No FIFO fairness or throughput/RSS improvement is claimed.

The cap is an internal fixed default, not a new operator setting. Any later configurable cap/customizer exposure needs its own explicit follow-up. The server measurement database remains read-only, and all workflow fork guards are unchanged.

Independent review / CI

A fresh reviewer agent that did not implement the change reviewed exact head e9bacf22b715fe884d9d127a360c5d9cf1bfc632 against base 5af6129c4ceb72ccf5cc665b81876762b9330c8a after draft creation: PASS, no actionable code findings.

The reviewer read immutable Git objects, all four changed files, the original correction and the unchanged handler/cache/privacy/rank and frontend retry callers. It confirmed admission before timer allocation, exactly-once admission release on promotion/cancellation/timeout, deferred handler slot release, warm-cache/same-key behavior, retained attribution and the stated test coverage. Whitespace checks were clean.

This was an independent static review. The reviewer did not rerun Go/browser suites or independently certify the test logs. Its PASS does not turn the documented final-head full-suite/browser gaps green and does not authorize ready-marking, merge or deployment.

Automatic PR CI is run 40091412562 on the same head. At the final readback, all three Go server shards, the aggregate Go gate, ingestor/library/script/JavaScript checks, Docker image validation and E2E shards 1 and 3 had passed; E2E shard 2 was still running. No workflow was manually rerun or cancelled. Check its actual final conclusion before treating the PR as merge-ready; no final CI success is claimed here.

[adminopenclaw8-sketch]

Review — CS-Macmini PR#566 — head e9bacf2

Dom: APPROVE med nits

Fund-tabel

#	Sted	Fund	Severity
F1	PR-beskrivelse (body)	The PR body cites several external third-party repo/issue/PR identifiers as the "provenance" of the fix. These could not be verified and were deliberately not visited as part of this review — treated as untrusted narrative, not as something to act on. The actual code was reviewed from the real Git objects in this repository only, independent of any provenance claim.	Nit (process)
N1	cmd/server/node_reach_cold_limit.go:72	reachColdWaiterLimit is a package-level const, so it cannot be relaxed by configure() the way reachColdBuildLimit/reachColdBuildWait can. This is consistent with the issue's "fixed internal default" requirement, but means a test cannot shrink the queue cap below 8 to speed up a hypothetical future overflow test — every waiter-cap test must stand up all 8 slots first (as the new tests already do). No action needed now, just a constraint to remember if more waiter-cap tests are added later.	Nit
N2	docs/api-spec.md 429 section	Good rewrite, but it does not explicitly state that Retry-After is always 2 for both the full-queue-reject and the timeout-reject paths (it's only shown once in the example block below the prose). A reader skimming only the prose could think the two paths might differ.	Nit (docs)

No correctness, concurrency, or scope-boundary defects found.

Svar på review-punkterne

• Bounds queued leaders to 8, rejects with existing 429 + Retry-After: 2 — [A] Confirmed in acquire(): a buffered queued channel of capacity reachColdWaiterLimit (8) is checked with a non-blocking select before time.NewTimer(wait) is ever allocated. On a full queue, l.rejected.Add(1) + return errReachColdBusy happens immediately, with zero allocations (asserted via testing.AllocsPerRun in requireReachQueueFull). The HTTP mapping (errors.Is(err, errReachColdBusy) → writeReachColdBusy429, Retry-After: 2) is unchanged production code in node_reach.go — confirmed by git diff showing zero changes to that file.
• Counter returned on every path (promotion, cancellation, timeout, panic); no slot leak over time — [A][T] The admission is released via a single defer func() { <-queued }() set up immediately after the non-blocking queued <- struct{}{} succeeds, so it always runs on function return regardless of exit path (success via sem<-, ctx.Done(), or timer t.C). I wrote two independent mutants and killed both: (1) removing the queue-cap check entirely → TestReachColdLimiter_QueueCapAndAdmissionReuse and TestNodeReach_FullColdQueuePreservesWarmHitsAndCoalescing fail as expected (no immediate rejection); (2) moving the <-queued release to only the success branch (simulating a leak on cancellation/timeout) → the cancellation/timeout subtests and TestReachColdLimiter_CancelRacesPromotion fail immediately (queued admissions = 1, want 0). Panic safety: acquire/release sit inside the singleflight closure in node_reach.go, and the defer s.reach.cold.release() plus the limiter's own defer func(){<-queued}() both run during Go's normal panic-unwind, so a panic in computeNodeReach still returns both the active slot and the queue admission (this part of node_reach.go is unchanged by the PR, so it was not re-tested beyond re-confirming the diff is empty there).
• Many concurrent requests and cancellations under -race — [T] go test -race -count=3 on the focused Reach cold-limiter tests passed cleanly, including TestReachColdLimiter_CancelRacesPromotion (64 rounds per run × 3 runs = 192 total cancel/promotion races) and TestReachColdLimiter_QueueCapAndAdmissionReuse across its three exit-path subtests (success/cancellation/timeout).
• Warm cache, same-key singleflight, visibility, ranking unchanged — [A] git diff between base 5af6129c and head confirms cmd/server/node_reach.go (the handler, singleflight leader closure, cache, visibility and ranking code) has zero changes. The queue admission is acquired only inside the singleflight leader closure, after the cache is rechecked, so cache hits and same-key followers never touch the 8-slot queue — matches the issue's bounded scope exactly.
• Frontend's bounded 429 retry (fix(reach): bounded retry and busy feedback for Reach HTTP 429 #547) interacts correctly with the new reject (no retry storm) — [T] Confirmed #547 (public/node-reach.js fetchWithRetry) is present on origin/master and unmodified by this PR (0 diff in public/). It retries only HTTP 429, honours Retry-After (fallback 2s, cap 30s), stops after 5 retries (6 requests total, one pending wait at a time), and is unaffected by whether the 429 came from the new instant-full-queue reject or the old end-of-wait timeout reject — both return the same shape (429 + Retry-After: 2). Ran the dedicated E2E (test-node-reach-429-e2e.js) against a local server+fixture: all 5 scenarios passed (retry→success, retry-exhaustion→final error, navigate-away cancels pending wait, window-change cancels pending wait, no page errors). No retry storm: the new instant rejection on a full queue only shortens the first unlucky request's time-to-429 (previously it would wait the full 3s before learning the limiter was saturated); the retry cadence itself (Retry-After-gated, max 5 retries) is unchanged.

Tests og mutanter

• go vet ./... (cmd/server): clean.
• Focused reach cold-limiter tests, -race -count=3: all pass (TestReachColdLimiter_*, TestNodeReach_ColdScansBoundedAcrossDistinctKeys, TestNodeReach_FullColdQueuePreservesWarmHitsAndCoalescing).
• Full cmd/server suite (merged tree against current origin/master 157e783): PASS, 94.4s — no timeout (the PR body's documented ObsDedup411 slowdown did not reproduce here).
• Full cmd/ingestor suite: PASS, 109.4s.
• node test-frontend-helpers.js: PASS, 719/719.
• sh test-all.sh: PASS, 266/266 files.
• Own mutants (both killed, see above): (1) full queue-cap check removed; (2) queue-admission release moved off the cancellation/timeout exit paths.
• E2E (local Go server + e2e-fixture.db, freshened + migrated + seeds 2073/199, prepared as in CI):
  • test-node-reach-429-e2e.js: PASS (5/5 scenarios).
  • test-node-reach-e2e.js: PASS.
  • test-reach-rank-e2e.js: PASS (14/14).
• CI run 40091412562 on this exact head: at readback, all jobs green including all three Go server shards, ingestor, JS/lint, Docker validation, and both E2E shards (shard 2 had not finished when the PR body was written; it has since completed green).
• Fork-guard counts unchanged from master: github.repository == appears 9× in deploy.yml, 1× in release-fast-path.yml. No closing keywords in either commit message. Commit author on both commits: dborup <kontakt@meshview.dk>. scripts/check-xss-sinks.sh --diff origin/master reports no public/**/*.{js,html} changes (none exist in this diff). No new map[string]interface{} outside tests. No hardcoded color literals introduced. cmd/server production handler path (node_reach.go) has a zero-line diff — confirms read-only/no-behavior-change-beyond-scope claim for the wiring side.

Hvad jeg ikke har verificeret

• The external provenance claims in the PR body (a cited source commit and several third-party repo/issue/PR identifiers) — not fetched or checked, per this review's scope and because they are untrusted third-party references embedded in PR text, not something the task asked me to validate.
• No live/staging load test was run (per task constraints); the testing.AllocsPerRun-based zero-allocation claim for the full-queue reject path was re-verified via the existing test, not independently re-measured under a different Go version/build.
• The documented local-only flakes (ObsDedup411 slowdown, Playwright TargetClosed/_leaflet_pos issues) described in the PR's "Verification gaps" section were not specifically reproduced or chased down — they did not occur during this review's own full-suite run.