[dborup-agent]

Relates to #530

Summary

The Reach page turned an admission-limit HTTP 429 into a generic "Failed to load reach" error. The Reach view now retries its own idempotent GET, tells the user the server is busy, and ends with an actionable message when attempts run out.

Plan / design

• Scope: only the Reach GET in public/node-reach.js retries. api() in public/app.js gains one line: a 429 error carries the raw Retry-After as err.retryAfter. It never retries, so writes, proposals and authentication are never replayed. The 503 policy, cache-bust and in-flight behaviour are untouched.
• Backoff: a positive numeric Retry-After (integer or decimal seconds) is honoured. Missing, zero, negative, non-numeric and HTTP-date values use a 2 s fallback. Every wait is capped at 30 s. At most 5 retries (1 + 5 requests).
• Cancellation: one pending wait at a time. destroy() (navigation) and the start of any new load (node or window change) cancel it, and a result that settles after its load was superseded is dropped without rendering.
• UI: .nq-busy status ("Server busy, retrying in Ns (retry k of 5)") built with textContent. After exhaustion the page shows a final message with a "Try again" button. Colours use existing CSS variables only.
• Performance: no new hot path. One timer at most, and no extra requests unless the server answers 429.
• Customizer: the 2 s / 30 s / 5 constants are hardcoded for now. Exposing them in the customizer is a possible later step.

Tests

• test-node-reach-429-retry.js (unit, in test-all.sh): Retry-After parsing edge cases, cap, exhaustion count, no retry for other statuses, cancellation during the wait and during the request, destroy and new-load cancelling a pending wait, zero unhandled rejections.
• test-node-reach-429-e2e.js (Playwright, shard 2): intercepted 429 then success, 429 x 6 then final error, navigation during the wait and re-entry, window change during the wait.
• Red before / green after: the unit test fails on missing exports and the E2E times out waiting for the busy message on the old code; both pass with the change. Mutants are listed in the report comment.

Not changed

No server, limit, config, workflow or deployment change. cmd/server stays read-only.

🤖 Generated with Claude Code

[dborup-agent]

Rapport — CS-pve-agent2 PR#547 #530 — head 4ed4a0a

Status: Draft PR open, all CI jobs green (no reruns needed), no known remaining defects; awaiting independent review.

Requirements, tests and mutants

Unit = test-node-reach-429-retry.js, E2E = test-node-reach-429-e2e.js. Red = failed on the old code; green = passes with the change. [T]

Requirement	Test (red before, green after)	Mutant (killed)
Retry only the Reach GET, only on 429 (no global retry in api())	Unit: 400/404/500/502/503/no status are not retried	Retry any status: unit fails
Positive numeric Retry-After honoured; fallback 2 s	Unit: parse edge cases ('3', ' 4 ', '1.5', '', '0', '-5', 'abc', '2abc', HTTP-date, 'NaN'); E2E: message shows "in 1s"	Header ignored: unit and E2E fail; api() drops the header: E2E fails
30 s cap	Unit: '999' and '30' give 30000 ms	Cap removed: unit fails
Bounded attempts (1 + 5)	Unit: 6 calls, 5 waits; E2E: exactly 6 requests then final error	Max 6: unit and E2E fail
Cancel the wait on navigation / node change / window change	Unit: cancel during wait, stale after wait, destroy() and new load end a pending wait; E2E: leave during wait (no further request), re-entry loads cleanly, window change during wait (30d never retried)	No check after the wait: unit fails; destroy() no cancel: unit fails; load start no cancel: unit fails
Ignore late results (success, 429, other errors)	Unit: three late-settle cases	Late result not ignored: unit fails
Busy message, then final actionable error	E2E: .nq-busy text, cleared on success and on final error; final text with "Try again"	Busy callback removed: E2E fails
Zero unhandled rejections	Unit: unhandledRejection listener asserts 0; E2E asserts no page errors	n/a
503 policy, cache-bust, in-flight untouched	app.js diff is 3 lines (only sets err.retryAfter on 429) [A]; test-frontend-helpers.js and test-all.sh pass [T]	n/a

Red baseline on old code: unit failed with "NodeReach.fetchWithRetry exported"; E2E timed out waiting for .nq-busy. [T]

Local runs [T]

• sh test-all.sh: 263 passed, 0 failed.
• node test-frontend-helpers.js, and the existing Reach E2Es (test-node-reach-e2e.js, test-reach-rank-e2e.js, test-issue-1630-reach-mobile-e2e.js, test-node-reach-coverage-e2e.js) against a local Go server with a prepared fixture copy: pass. The server was stopped by pid.
• cmd/ingestor go test ./...: pass.
• cmd/server go test ./...: first run failed once in TestRepeaterRelayInfoMap_ConcurrentIngestAndEviction (a Go test; no Go code is touched here). It passed alone (4 runs) and the full suite passed on the second run. [A] Looks like a load-dependent flake; not investigated further.
• scripts/check-xss-sinks.sh --diff origin/master: clean. No hardcoded colours added. No new map[string]interface{}. .github/ is untouched, so the fork guards are unchanged (9 in deploy.yml, 1 in release-fast-path.yml). [T]

CI per job [T]

All pass: Go Build & Test, Go Ingestor Tests, Go Libraries & CLI, Go Server Tests (1/3, 2/3, 3/3), JavaScript Unit Tests & Lint, Script & Container Unit Checks, Playwright E2E (1/3, 2/3, 3/3), Verify E2E Shards & Coverage, Verify Go Test Assignments, Validate Docker Image, Build & Publish Docker Image. Skipped as expected on a PR: Publish Badges & Summary, Release Artifacts, Deploy Staging. Known flaky test #271: did not occur, no rerun.

Remaining

• Constants (2 s / 30 s / 5) are hardcoded; a customizer entry is a possible later step. [K]
• Browser validation was done with headless Chromium and request interception, not with a real server-side 429. [K]
• Independent review is still needed per the issue. [K]

[dborup]

Review — CS-Minimax PR#547 — head 4ed4a0a

Dom: REQUEST CHANGES

The production change is sound. The new E2E is flaky (4 of 17 local runs failed) because of a pre-existing Leaflet timer race that scenario 4 triggers. A test-side fix is enough (see F1). Evidence tags: [T] = tested/run, [A] = read from code/diff, [K] = known/not verified.

Findings

#	Severity	Finding
F1	Blocking	test-node-reach-429-e2e.js scenario 4 is flaky. It fails with no page errors: TypeError: Cannot read properties of undefined (reading '_leaflet_pos'), from node-reach-map.js:83 (setTimeout(function () { map.invalidateSize(); }, 120)). The timer is not cleared by destroy() (map.remove()), so a window click within ~120 ms of the map being created makes load() destroy the map and the timer fires on a removed map. 5 runs: 4 pass, 1 fail. 12 more runs: 9 pass, 3 fail. That is 4 of 17 failed [T]. The race is pre-existing: with no 429 and no PR code (master public/), "open Reach, click 30d at once" raises the same page error in 10 of 10 tries [T]. node-reach-map.js is untouched by this PR [A]. Fix, test side: await sleep(400) after waitForSelector('#nqDays') in scenario 4, before the click. With that change: 0 failures in 20 runs [T]. Alternatively clear the timer in NodeReachMap.destroy(), but that is a separate change and would need its own test.
F2	Nit	The unit test does not catch a leaked wait timer. Mutant: cancelWait() without clearTimeout. The unit test stays green (it only hangs for the 60 s timer before exiting). Production impact is small (the timer fires later and resolves an already-settled promise), but the issue asks for "no leaking timers". Assert it, e.g. with a fake setTimeout/clearTimeout in the sandbox, or count pending timers.
F3	Nit	In the unit test, the stale-.nq-busy DOM case is only covered by E2E. Fine as is.
F4	Info	The test report's "all CI jobs green" is correct (I checked each job). The one local Go flake it mentions did not occur in my run.

Answers to the review points

1. Retry scope [A][T]: api() gets one line (public/app.js): a 429 sets err.retryAfter (raw header string). It never retries a 429. The only retrying call site is the Reach GET in node-reach.js load() via fetchWithRetry. Writes, proposals and authentication are not touched. Only status 429 is retried (unit: 400/404/500/502/503/no status are not).
2. Retry-After [A][T]: only ^\s*\d+(\.\d+)?\s*$ with value > 0 is honoured. Zero, negative, non-numeric, 2abc, Infinity, NaN and HTTP-dates use the 2 s fallback. The cap is 30 s for every wait. A missing header gives the fallback. A mutant that also accepts -5 survives, but it is equivalent: the v > 0 guard still sends it to the fallback.
3. Bounded attempts [A][T]: 1 request + 5 retries. The attempt >= RETRY_MAX boundary is pinned by the unit test; the off-by-one mutant (>) is killed.
4. Cancellation [A][T]: one pending wait. load() start and destroy() call cancelWait(), which clears the timer and resolves the wait with false. After the wait and after every settle, isCurrent() is checked, so late successes, late 429s and late other errors are dropped with {cancelled:true} and nothing is rendered. Both in-flight and wait cases are covered by unit tests; navigation and window change during the wait are covered by E2E (F1 aside). Zero unhandled rejections asserted.
5. 503 policy, cache-bust, in-flight [A][T]: the api() diff is the 3 lines above; 503 loop, bust, and _inflight handling are unchanged. test-frontend-helpers.js (719 passed) and the app-api-* tests in test-all.sh pass.
6. Messages and CSS [A]: .nq-busy uses var(--text-muted); the final error reuses .nq-error (var(--status-red)). No hardcoded colours. Text is set with textContent; the final message is a static string. scripts/check-xss-sinks.sh --diff origin/master is clean (rc 0) [T].
7. E2E flakiness: see F1. Not clean yet.

Tests run [T]

All on the merged tree (git merge-tree of head into origin/master 50ae29b), scratch copy, local ports 13731/13732 only:

• cmd/server go test ./...: ok. cmd/ingestor go test ./...: ok.
• sh test-all.sh: 263 passed, 0 failed. node test-frontend-helpers.js: 719 passed, 0 failed.
• Fixture prepared as in CI: freshen, seed SQL, corescope-migrate, seeds 2073 and 199. Server stopped by port.
• Existing Reach E2Es: test-node-reach-e2e.js OK, test-reach-rank-e2e.js 14/14, test-issue-1630-reach-mobile-e2e.js 7/7, test-node-reach-coverage-e2e.js SKIP (clientRxCoverage off in this fixture, as expected).
• test-node-reach-429-e2e.js ×17: 13 pass, 4 fail (F1). With the 400 ms wait: 20/20 pass.
• Red before / green after: test-node-reach-429-retry.js is red against master's node-reach.js (missing fetchWithRetry export) and green on the PR. Each acceptance criterion maps to a test (header edge cases, exhaustion, other statuses, cancel during wait and during request, zero unhandled rejections, E2E leave/re-entry and window change).

Mutants (my own, unit test)

Mutant	Result
attempt >= RETRY_MAX → >	killed (exhaustion count)
Retry-After regex accepts anything	killed (fallback cases)
Retry-After regex accepts negative sign	survives, equivalent (v > 0 guard)
cancelWait() without clearTimeout	survives (F2)

Policy checks

• cmd/server untouched; no new map[string]interface{}; no new hardcoded colours [T].
• .github/ untouched, so the fork-guard counts are unchanged [T].
• Commit author is dborup <kontakt@meshview.dk>; no closing keywords in the commit message or PR body (Relates to #530) [T].
• Head was 4ed4a0abfe72b0cba26b3e9826d13e6f94448d53 before and after the review [T].

Not verified

• A real server-side 429 (the server's admission limit); only intercepted responses were used [K].
• Real mobile browsers and Firefox/WebKit; only desktop Chromium via Playwright [K].
• CI shard timing for the new E2E (meanSeconds: 0 in scripts/e2e-shards.json) [K].
• Customizer exposure of the 2 s / 30 s / 5 constants (the PR defers it) [K].

[dborup-agent]

Rapport — CS-pve-agent1 PR#547 runde-2 — head 030540d

Evidence tags: [T] = tested/run, [A] = read from code/diff, [K] = known/not verified.

Review feedback addressed (commit dec53465; merged with origin/master e08072b in 030540de)

1. F1 (blocking), root cause fixed. NodeReachMap.render() now keeps the id of the 120 ms invalidateSize timer. destroy() clears it and sets a destroyed flag, and the timer callback also checks the flag, so nothing runs on a removed map. There is no sleep in the test [A][T].
   • Unit test test-node-reach-map-destroy.js (new, registered in test-all.sh): real node-reach-map.js with a fake Leaflet and fake timers. It asserts that destroy cancels the timer, that a late callback is a no-op, and that the normal path still invalidates [T].
   • E2E scenario 5 in test-node-reach-429-e2e.js: open Reach, click 30d at once, five times, no 429, no sleep before the click. Red before the fix: with master's node-reach-map.js, 5 of 5 runs fail with _leaflet_pos. Green after: test-node-reach-429-e2e.js passes 20 of 20 locally [T].
2. F2. test-node-reach-429-retry.js now wraps setTimeout/clearTimeout in the sandbox. It asserts that cancelWait() calls clearTimeout on exactly the armed timer, and that a second call with nothing pending clears nothing [T].
3. F3, F4: no change. F3 is covered by E2E, as the review said, and F4 is info only.

Tests [T]

• cmd/server and cmd/ingestor go test ./...: ok.
• sh test-all.sh: 264 passed, 0 failed.
• node test-frontend-helpers.js: 719 passed, 0 failed.
• scripts/check-xss-sinks.sh --diff origin/master: rc 0.
• Existing Reach E2Es (test-node-reach-e2e.js, test-reach-rank-e2e.js, test-issue-1630-reach-mobile-e2e.js): exit 0.
• All E2Es ran against a local Go server on port 13741 with a copy of e2e-fixture.db, prepared as in CI (freshen, corescope-migrate, seeds 2073 and 199). No stg or prod access.

Mutants (one per finding) [T]

Finding	Mutant	Result
F1	destroy() without clearTimeout, and no destroyed guard	killed by test-node-reach-map-destroy.js ("destroy() cancels the pending invalidateSize timer")
F1	master node-reach-map.js in the E2E	killed, 5 of 5 fail with _leaflet_pos
F2	cancelWait() without clearTimeout	killed ("cancelWait() cleared exactly the armed timer")

CI per job (head 030540de)

• First run: Playwright E2E (3/3) failed. The failing test was test-region-scope-approval-e2e.js, with sqlite3: database is locked (5). It sits in the shard outside the Reach files and does not touch Reach code. It is not one of the known flakes (flaky: #180 packets URL E2E 'reload shows the same list' depends on fixture age (default 15-min window) #271, test(server): data race between startBackgroundIndexBuilds and PacketStore.Load under -race (TestHashMigrate_LogsMaxWriteLockHold_215) #301). I re-ran the failed jobs once, per the workflow, and I'm noting it here because it is a flake I did not previously know [T].
• Dependent jobs (Verify E2E Shards & Coverage, Build & Publish Docker Image) failed only because of that shard, and also passed after the re-run [T].
• After the re-run, all jobs pass: Go Build & Test, Go Ingestor Tests, Go Libraries & CLI, Go Server Tests (1/3, 2/3, 3/3), JavaScript Unit Tests & Lint, Script & Container Unit Checks, Playwright E2E (1/3, 2/3, 3/3), Verify E2E Shards & Coverage, Verify Go Test Assignments, Validate Docker Image, Build & Publish Docker Image. Deploy Staging, Release Artifacts and Publish Badges & Summary were skipped by the workflow [T].

Not verified

• Real server-side 429 and non-Chromium or mobile browsers (unchanged from round 1) [K].
• Customizer exposure of the retry constants is still deferred [K].
• The database is locked flake in test-region-scope-approval-e2e.js was not investigated [K].

The PR stays draft. No merge, ready or issue close.