Repository navigation
feat(nodes): Recent Adverts split into flood / zero-hop / mixed with per-class counts - #97
Merged
Merged
Conversation
…per-class counts Port/extension of upstream `Kpa-clawbot#2073`, with the "Recent Adverts" rename and tooltip from upstream `Kpa-clawbot#2071`. Server (GET /api/nodes/{pubkey}): - classifyAdvertRoute is the pure rule behind Relay Airtime Share's advertRouteClass (#89: route_mask, else first-inserted route_type); a SQL CASE mirrors it and a test pins both over every mask x route_type. - recentAdverts is unchanged (20 rows, id DESC) plus route_class on ADVERT rows. New recentAdvertsByRoute holds up to 20 adverts per class, the class filtered in SQL (ROW_NUMBER per class) so zero-hop adverts cannot push flood adverts out; a mixed advert is only listed as mixed. - New advertCounts: distinct adverts per class by first_seen over 24h and 7d, sharing the date floor, parseRelayTS window, hash dedup and row cap with advert_stats.go (extracted, not copied), plus the route_mask backfill status and a truncated flag. - flood_advert_count_7d is unchanged (route_type 1 only); the difference from advertCounts["7d"].flood (route 0, mixed) is documented and tested. - New fields and route_class are omitted for hidden identities (#68). - Per-pubkey cache (30 s TTL, targeted invalidation on a newer transmission debounced to 5 s, 128 entries, singleflight): the scan costs ~135 ms on a node with 50,000 adverts. - OpenAPI and docs/api-spec.md updated; new schemas use a typed struct. Frontend: one shared component (public/node-adverts.js) for the node page and side panel: counts row, All / Flood / Zero-hop / Mixed tabs via initTabBar, route badge per advert, empty states, provisional note while the backfill runs, ?adverts= deep link. Tests: Go unit/handler/cache tests, vm unit test of the real component, Playwright E2E on a seeded fixture (new CI seed step after migration). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…review-blocks-db627a
…nd a single index-ordered scan (#97 review) Cloud review of PR #97 (P2-1, P2-2, P3): - Opt-in: GET /api/nodes/{pubkey} computes recentAdvertsByRoute, advertCounts and route_class only with include=advertRoutes. Without it the response is master's and neither the privacy lookup, the cache nor the scan runs. Only nodes.js fetchNodeDetail (full node page and side panel) sends it, via NodeAdverts.detailPath; packets, live, channels, route view and claimed-node lookups keep the plain URL. Privacy gates run before the cache and fail closed. - The ROW_NUMBER() window query is replaced by one pass over the node's ADVERT rows in idx_transmissions_from_pubkey order; classification, the 20-per-class lists, 24h/7d counts, row cap and truncation are done in Go. A unary + on payload_type keeps the planner off the payload_type index when no ANALYZE statistics exist. The old query stays as a test-only oracle. - The same pass yields flood_advert_count_7d bit-identically to CountFloodAdvertsForNode. It is used only by the request that ran the scan; cache hits and shared scans count it fresh, and it is never cached. - Cache memory comment now gives measured heap per entry. - P3: tests for escaping of decoded text, SNR/RSSI, role colour and the timestamp fallback, and for aria-selected/tabindex on every tab at render. "unknown" vs Relay Airtime Share's "legacy" documented as one bucket in OpenAPI, docs/api-spec.md and code. Tests: 8 new Go tests (opt-in shape, list forms, privacy before cache, fresh flood count, flood only from own scan, stream vs window oracle and flood count vs CountFloodAdvertsForNode over 1,000 random nodes each, query plan); test-node-adverts.js 13 -> 20; 2073 E2E 9 -> 10 steps (node page requests opt in, plain URL master-shaped). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
Author
|
Review feedback addressed (commits
Numbers, hardware and load are in the updated description. Independent re-review nits addressed (commit
Generated by Claude Code |
…lf (#97) CI on df46645 failed test-issue-2027-my-mesh-node-page-e2e.js: its mocked-404 step loads nodes.js without node-adverts.js, so NodeAdverts.detailPath threw before the 404 was handled and the page showed "Failed to load node" instead of "Node not found". The fetch now builds '/nodes/{pk}?include=advertRoutes' itself; NodeAdverts.detailPath is removed. The caller-rule unit test now also fails if fetchNodeDetail depends on NodeAdverts. test-issue-1206-resize-observer-leak-e2e.js mirrors the node-detail request (route glob, api() in-flight key, error text); it now matches the route on the pathname and uses the opted-in path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
N1: add TestNodeDetail_IncludeAdvertRoutesFailsClosedOnLookupError. The existing PrivacyBeforeCache test only exercises isIdentityHidden returning (true, nil) via the blacklist/observer-blacklist, which the fail-open mutant `advertRoutes = !hidden` (dropping the `err == nil` check in handleNodeDetail) happens to agree with. The new test forces the name lookup itself to error (hidden-name prefix configured plus an already-canceled request context) so the two formulas diverge: verified red on the fail-open mutant, green on unmodified code. N2: widen the "only the node page sends include=advertRoutes" caller check in test-node-adverts.js from the literal `include=advertRoutes` to the bare `advertRoutes` token (excluding nodes.js and the comment-only node-adverts.js). Catches the two mutants the review named — a split literal and a URLSearchParams-built query in another file — without touching runtime behaviour; documented in the test as still a lexical check, not a runtime one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9yCWp8MvMrMcW2Zs3EgYW
dborup
marked this pull request as ready for review
September 26, 2026 15:01
This was referenced Sep 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
Port/extension of upstream
Kpa-clawbot/CoreScope#2073(open, unimplemented upstream), including the rename from upstreamKpa-clawbot/CoreScope#2071.The node page's advert list mixed every advert together, so a node that sends many cheap zero-hop adverts buried its few flood adverts — the ones that cost mesh-wide airtime. This PR:
#2071's tooltip (the list isfrom_pubkey, which only ADVERTs carry);?adverts=deep link;recentAdvertsByRoute,advertCountsand a per-rowroute_classtoGET /api/nodes/{pubkey}— only withinclude=advertRoutes(opt-in, see below).Opt-in:
include=advertRoutesThe breakdown scans all of a node's ADVERT rows, so it is computed only when asked for:
GET /api/nodes/{pubkey}?include=advertRoutes(comma-separated list, may repeat; unknown values ignored) adds the three fields.node,recentAdverts), noroute_class, no new fields. Neither the privacy lookup, the cache nor the scan runs (a test counts cache lookups and scans: 0). Measured response bytes are identical to master (table below).fetchNodeDetailinpublic/nodes.js, which serves both the full node page and the side panel. It builds the URL itself, so a missingnode-adverts.jscannot break the fetch or its 404 state.packets.js(sender/ctrl/anon-req lookups),live.js,channels.js,route-view.js(one per hop) and the claimed-nodes lookup innodes.js. A unit test fails if any other file asks for it, iffetchNodeDetailstops asking or starts depending onNodeAdverts, or if the claimed-nodes lookup changes. The E2E asserts that every node-detail request the node page makes carries the opt-in, and that the plain URL stays master-shaped on a live server.api(), keyed by path) keeps the two URLs apart.invalidateApiCache('/nodes…')prefixes still clear both.identityHiddenfor the observer blacklist and observer names) run before the cache and fail closed. A hidden identity never reaches a lookup or a scan.docs/api-spec.md.Design
Classification is the one Relay Airtime Share uses (#89), through the shared pure
classifyAdvertRoute(mask, maskKnown, routeType):floodroute_maskhas only routes 0/1zero_hopMesh::sendZeroHopsends DIRECT / TRANSPORT_DIRECT withpath_len = 0)mixedunknownroute_classcallslegacy(its historical plain ADVERT row). One classifier, two names kept for API compatibility; documented in both endpoints' OpenAPI,docs/api-spec.mdand the code.Without usable mask bits (column absent, row not backfilled yet) it falls back to the first-inserted
route_type, exactly as #89 does.One index-ordered pass.
GetNodeAdvertRoutesreads the node's ADVERT rows once, newest id first, straight offidx_transmissions_from_pubkey(no sort, no window function). Go then classifies each row, keeps the first 20 of every class (so 100 zero-hop adverts cannot push 2 flood adverts out), and feeds every row at or after the 7d date floor into the 24h/7d counts (50,000-row cap,truncated). The listed rows are read afterwards in one batch as lean rows (noobservationsarray). A unary+payload_typekeeps SQLite on thefrom_pubkeyindex. Neither binary runs ANALYZE, and without statistics the choice between thefrom_pubkeyandpayload_typeindexes otherwise depends on index creation order and SQLite version: thesqlite33.54 CLI and a test schema with the indexes in the other order pickpayload_type, which would visit every ADVERT in the database. A plan test covers this. The previousROW_NUMBER() OVER (PARTITION BY class)query is kept as a test-only oracle.Counts. Distinct adverts (by hash) whose
first_seenlies in the last 24h / 7d — the axisflood_advert_count_7duses, so a late buffered upload counts in the window it was sent in. The date floor, exactparseRelayTSwindow and dedup are shared withadvert_stats.go(advertWindow/advertDateFloor).route_mask_backfillsays when classes are still provisional.flood_advert_count_7dis an external contract (ArcScope advisor):route_type = 1only. It is counted fresh on every request and never cached:CountFloodAdvertsForNodeat the same instant. The pass evaluates that query's own SQL predicates per row (first_seen >= floor,route_type = 1), applies its own row cap to the qualifying rows newest first, and runs the samecountFloodAdverts;CountFloodAdvertsForNodeas before;It still differs from
advertCounts["7d"].flood(route_mask based: counts transport flood, never a mixed advert).Cache (unchanged behaviour): per pubkey, 30 s TTL, invalidated when the node has a newer transmission (debounced to one rescan per 5 s), singleflight, at most 128 entries. Measured heap: ~2.5 KB per lean row. That is ~125 KB for a typical entry (~52 rows), ~200 KB at the 80-row maximum, and ~16 MB for 128 typical entries (≤ ~25 MB); the old comment said "a few KB".
Types. New response parts are named structs.
NodeAdvertRowstays map-shaped because the shared transmission scanner is (upstreamKpa-clawbot/CoreScope#1383).map[string]interface{}in touched files:db.go68 → 66, the others unchanged.Frontend. One shared component,
public/node-adverts.js, renders both copies; tabs reuseinitTabBar; colours are existing tokens.Performance
Scratch database (not in the repo): 1,061,500 transmissions (1,487,434 observations) over 10 days, migrated schema, interleaved ids. Heavy nodes: one with 50,000 adverts, one with 10,500, one with 1,000, plus 20 more with 10,500 each. There are also 2,000 nodes with 200 adverts and 390,000 non-advert rows. Adverts are 40 % route 1, 50 % route 2, 5 % route 0, 5 % route 3; 5 % mixed, 2 % NULL mask.
Hardware: Apple M5 (10 cores, 32 GB), Go 1.26.3 locally (CI uses 1.27.1), modernc SQLite 3.46.0.
Load: the machine was shared with other sessions (iOS simulator, Defender scans) and swapped heavily (7.5–10.7 GB swap in use). Load is reported per run, and "quiet" means 1-min load ≤ 8. Both servers ran with
packetStore.retentionHours: 24to keep RSS (~1 GB each) out of swap; node detail is database-bound, so this does not favour either build.In-process (
node_advert_routes_bench_test.go, 5 interleaved rounds × 20 iterations, load 4.0–4.5, median ms/op):CountFloodAdvertsForNodeBefore this change a cold opted-in request ran window + flood count, 159 + 64 = 223 ms on the 50k node. Now it runs the stream alone: 104 ms.
HTTP (
GET /api/nodes/{pubkey}, masterfa5bf4f7and this branch side by side on the same file, requests alternating between the builds; 2 runs × 20–21 rounds of 31.5 s so every "cold" request is past the 30 s TTL). p50 / p95 in ms, from the rotated second run (12 of 21 rounds quiet; the first run agrees within noise):658d8a08): cold opt-in cost +166 ms HTTP (warm connections) and +155 ms in-process on the 50,000-advert node; +34 ms in-process on the 10,500-advert node. Plain (noinclude) matched master on that machine too. This is a direct measurement that supersedes the "roughly +100–150 ms" estimate above for the 50k node; the two are in the same range.Response size (bytes, no gzip):
The review's reference figures (4-vCPU linux/amd64, Go 1.24.7, before this change): cold 616 vs 174 ms p50 on the 50k node; in-process 436 (window) / 151 (flood count) / 335 (stream without window) ms/op; 20 heavy nodes in parallel master 1.0–1.2 s vs PR 2.4–2.8 s; response 29 → 80 KB for every caller. The last two no longer apply to non-node-page callers.
Tests
Test-first; the new opt-in tests were red before the code.
node_advert_routes_optin_test.go):include;hidden = true(a canceled request context with a hidden-name prefix configured, distinct from the existing blacklist/observer-blacklist case — re-review N1, see below);flood_advert_count_7dfresh next to a cached breakdown;node_advert_routes_stream_test.go):route_mask, production schema). They cover mixed, NULL, out-of-range and mistyped masks and route types; everyfirst_seenformat including garbage, empty, date-only and window edges; duplicate and missing hashes; non-advert rows; per-class limits 1–5/20; row caps 1–40/50,000. Identical ids, order, classes, counts and truncation for all 1,000, and identical full responses for every 25th. The run hit 339 truncated nodes, 292 at a per-class limit, 785 with unknown and 704 with mixed;CountFloodAdvertsForNode: bit-identical for 1,000 random nodes. That includes 211 where the row cap binds, 559 with window/parse/dedup skips and 328 with duplicate hashes;node_advert_routes_test.go/ cache tests adapted, and the OpenAPI test now also requires theincludeparameter.test-node-adverts.js, 13 → 19):aria-selectedandtabindexon every tab at render (F14);test-issue-2073-recent-adverts-e2e.js, 9 → 10 steps): every node-page detail request opts in, and the plain URL stays master-shaped on the server.test-issue-1206-resize-observer-leak-e2e.jsmirrors the node-detail request (route glob,api()in-flight key, error text). It now matches the route on the pathname and uses the opted-in path.Follow-up from the independent re-review of
658d8a08handleNodeDetail(cmd/server/routes.go) setsadvertRoutes = err == nil && !hiddenafters.isIdentityHidden. The existingPrivacyBeforeCachetest only exerciseshidden = true, err = nil(blacklist/observer-blacklist), whichisIdentityHiddenresolves without ever running the name lookup that can fail — so the fail-open mutantadvertRoutes = !hidden(dropping theerrcheck) agreed with the correct code there and survived the suite. A new test (TestNodeDetail_IncludeAdvertRoutesFailsClosedOnLookupError) forces the name lookup itself to error (a hidden-name prefix configured so the lookup isn't skipped, plus an already-canceled request context) and asserts the response stays master-shaped (200, noroute_class/advertCounts/recentAdvertsByRoute, 0 cache lookups, 0 scans). Verified red on the fail-open mutant, green on unmodified code.test-node-adverts.js's "only the node page sendsinclude=advertRoutes" check is a regex over source text, not a runtime assertion. It now matches the bareadvertRoutestoken (not just the literalinclude=advertRoutes) in everypublic/*.jsfile exceptnodes.jsandnode-adverts.js(the shared render component, which only names the opt-in in a comment and makes noapi()/fetchcall). That change is enough to catch the two mutants the review named — a split literal ('?include=' + 'advertRoutes') and aURLSearchParams-built query (params.set('include', 'advertRoutes')) elsewhere — both verified red when injected into a scratch copy of another file, green on unmodified code. It remains a lexical check: a caller that assembles the string from smaller pieces ('advert' + 'Routes') or from data outside the source would still slip through; closing that gap needs a runtime check (loading the caller files in avmsandbox with a stubapi()and asserting on the URLs actually built), which was judged too heavy to add here given the files and functions involved (packets.js,live.js,channels.js,route-view.js,nodes.js).CountFloodAdvertsForNodemissing its mutex (issue Decide: port upstream SQLite planner statistics (ANALYZE in the ingestor)? #100) and the row scanner's handling of non-integerroute_type(pre-existing on master) are not changed by this PR; see Limitations.Mutations — each applied temporarily; all compiled, all red, files restored byte-identical (sha256):
TestAdvertRouteClassSQL_MatchesGoClassifier,TestAdvertRouteClassFromMask, stream oracle, …TestNodeAdvertRoutes_MixedOnlyInMixed, stream oracletest-node-adverts.js(3 cases)TestNodeAdvertRoutes_PerClassLimitInSQL, stream oracleTestNodeAdvertRoutes_MixedOnlyInMixed,TestCountAdvertRoutes_DedupAndClassesTestNodeAdvertRoutes_WindowsAndTimestampFormats, stream oracleTestNodeDetail_AdvertRouteFieldsPrivacy,…PrivacyBeforeCacheadvertRoutes = !hidden)TestNodeDetail_IncludeAdvertRoutesFailsClosedOnLookupError(added for re-review N1)aria-selected="true"on every tab…WithoutIncludeIsMasterShaped/…AdvertRouteFields,…ListFormsroute_classread without the opt-in…WithoutIncludeIsMasterShapedfetchNodeDetaildepends onNodeAdverts…PrivacyBeforeCacheincludenot in OpenAPITestOpenAPI_NodeAdvertRouteSchemas…FloodCountOnlyFromOwnScanroute_type = 1…AdvertRouteFields…FloodAdvertCount7dStaysFresh+removed frompayload_typeTestNodeAdvertRoutes_StreamQueryPlanLocal runs (branch tip; Go 1.26.3):
go test -race -timeout 20m ./...incmd/server: ok (228 s);readonly_invariant_test.go: 4/4;go vet, gofmt on touched files andgit diff --check: clean.check-css-vars: 0 undefined;--diff origin/master: clean.test-issue-2073-recent-adverts-e2e.js: 10/10 three times;test-issue-1147-section-order-e2e.js: 2/2;test-e2e-playwright.jswith fail-fast disabled in a scratch copy: 131/135, 3 skipped, 1 failed ("Version info lives on Perf dashboard"). The pass/fail/skip sets are identical on masterfa5bf4f7with its own CI-prepared fixture.test-issue-2027regression fixed in658d8a08. 1206 and 2027 were the only failures specific to this branch; both are fixed (28/28 twice, 25/25). The 10 remaining local failures fail identically on masterfa5bf4f7on this machine (load 25–41: packets/gesture/slide-over timeouts, "Version info"); they passed in CI ond9bb8510.Commits in this round
684c7bb4: merge of masterfa5bf4f7(plain merge commit).df466459: the review fixes.658d8a08: follow-up after CI ondf466459failedtest-issue-2027. The fetch had come to depend onnode-adverts.js; it no longer does, and the 1206 test was adapted to the opted-in URL.7778d262(plain merge commit, treed0c83f5f; brings in feat(channels): shared hashtag channel proposals, with admin revoke #99, fix(server): distance analytics reads a torn snapshot under concurrent ingest/eviction #104, XSS preflight gate: only suggest the DOM-grep test route when it can apply #105, feat(anomaly): experimental offline traffic-anomaly detector library (inactive) #94), plus the N1/N2 fixes and this description update, addressing the independent re-review of658d8a08.Limitations
cmd/serveris read-only, no migration).recentAdvertsfor an identity hidden only via the observer blacklist is unchanged (pre-existing); only the new fields are omitted.fa5bf4f7; master has since gained fix(server): avoid mutating the cached channels slice on includeEncrypted #98 (channels cache append fix,routes.go+8/−2 in another handler). It merges cleanly and is not merged here.CountFloodAdvertsForNodestill lacks the mutex the scan path (nodeAdvertRoutes) added; tracked separately as issue Decide: port upstream SQLite planner statistics (ANALYZE in the ingestor)? #100, not addressed by this PR.route_typevalues (pre-existing behaviour already on master, neither introduced nor fixed here), and a column-level optimization for the advert scan may be possible (e.g. narrowing the columns read per row); both noted as possible future follow-ups, not pursued in this PR.Customizer / config (AGENTS.md rule 8)
Hardcoded for now, candidates for config later: the 24h / 7d windows, the 20-per-class limit, the 50,000-row cap, and the cache TTL (30 s), debounce (5 s) and size (128).
🤖 Generated with Claude Code
https://claude.ai/code/session_01F9yCWp8MvMrMcW2Zs3EgYW