Repository navigation
port(upstream#1821): packets observer filter keeps multi-observer rows in grouped mode - #27
Merged
Conversation
…de (Kpa-clawbot#1748) (Kpa-clawbot#1821) Closes Kpa-clawbot#1748. ## Root cause `renderTableRows()` in `public/packets.js` re-filtered the already server-filtered `/api/packets` (grouped) results client-side, comparing `filters.observer` against each row's `observer_id`. But that field is only the **representative** observer chosen for display — `QueryGroupedPackets` (`cmd/server/db.go:554-606`) picks the observation with the longest observed path via a `LEFT JOIN ... ORDER BY length(path_json) DESC LIMIT 1`, purely for display purposes. The server-side filter (`buildTransmissionWhere`, `cmd/server/db.go:725-790`) is already correct: it uses an `EXISTS` subquery over **all** observations of a transmission, so any row it returns was genuinely seen by at least one selected observer. The client then discarded rows *again*, checking only the representative's `observer_id`, with a fallback to `p._children` — which is `undefined` on initial page load (only fetched lazily on row-expand or when the observer-sort dropdown changes, see the `obsSortSel` change handler). Net effect: a multi-observer transmission stayed visible under an observer filter only when the filtered observer happened to also be the representative (longest-path) observer — which matches the reported "works only for whichever observer logged it first" behavior in dense meshes, where longest-path and earliest-seen correlate. ## Fix Skip the client-side observer re-filter entirely when `groupByHash` is active — the server's `EXISTS` filter is authoritative for grouped rows and needs no client-side correction. The flat/expanded-mode path (single-observation rows, each with its own exact `observer_id` from `buildPacketWhere`) keeps the existing children-aware filter unchanged, which already had test coverage under Kpa-clawbot#537 for the case where `_children` is already populated. ## Tests Added 6 cases in `test-frontend-helpers.js` covering the specific gap Kpa-clawbot#537's tests didn't reach — grouped mode with `_children` still `undefined` (the actual initial-load state that triggers this bug). New tests confirm: - A multi-observer row whose representative doesn't match the filter is kept in grouped mode (the core bug). - Grouped mode never re-filters client-side (trusts the server). - Flat mode behavior is unchanged (matches by own `observer_id`, falls back to already-loaded `_children`). Full run: `test-frontend-helpers.js` 631 passed / 2 failed (same 2 pre-existing `favStar` failures reproduce identically on unmodified `master` — unrelated). `test-packet-filter.js` 92/92. `test-aging.js` 18/18. Operator context: running CoreScope for SaarMesh (SaarLorLux, DE/FR/LU, 800+ nodes, 14 observers) — this was hiding a large share of traffic whenever filtering by a non-primary observer in our dense mesh. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> (cherry picked from commit 02c2338) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 13, 2026
Independent review found that skipping the client-side re-filter in grouped mode made the observer filter a complete no-op in the default view, which is worse than the bug being fixed: the dropdown handler was the only server-side filter handler that did not refetch, so it relied entirely on that client-side pass to narrow an already-loaded page. Reproduced by driving the real renderTableRows(): rows whose sole observer was someone else stayed visible under an observer filter until the next page load. The observer filter is a server-side filter (buildPacketsParams sends 'observer'), so changing it now calls loadPackets(), like hash, time window, group toggle, myNodes, region and channel already do. That makes the grouped early return correct rather than merely harmless, and also fixes the pre-existing converse: unticking back to All Observers now restores the wider set instead of leaving the page narrowed. Also corrects two comments that cited code paths which do not run (buildPacketWhere has no non-test callers; the usual grouped path is store.QueryGroupedPackets, with buildTransmissionWhere only as the older-than-oldestLoaded fallback — both scan all observations, so the conclusion stands), and closes three test gaps the reviewer found by mutation testing: the hashOnly case was asserted in grouped mode where the early return made it vacuous, the children fixture used only string ids so the String() coercion could be dropped unnoticed, and nothing pinned the refetch contract. All three mutants now fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dborup
pushed a commit
that referenced
this pull request
Sep 20, 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
3 participants
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.
Split out of #25 (commit
a3f983a2there). This branch holds exactly one upstream change so it can be reviewed, tested and reverted on its own.Upstream
02c2338d649b11f27b5e2e008438fd25e22b9660git cherry-pick -xonto masterfda24ca5; upstream authorship kept, and the commit message carries the(cherry picked from commit …)line.public/packets.js,test-frontend-helpers.js.Problem
In grouped mode the Packets table re-filtered the already server-filtered rows against each row's
observer_id. That id is only the representative observation (longest path), and_childrenare not loaded yet at that point. So an observer filter hid every multi-observer transmission whose representative was a different observer.Change
The client-side filter is extracted into
applyObserverFilter(displayPackets, filters, groupByHash, hashOnly). In grouped mode it returns the rows unchanged, because the server'sEXISTSfilter over all observations is authoritative. Flat mode keeps the defensive client-side filter. The function is exported for tests, andtest-frontend-helpers.jsgains cases that exercise the real function.Adaptation to this fork
None. The cherry-pick applied without conflicts and the changed lines are identical to upstream.
Dependencies and merge order
fda24ca5and needs no other PR from this split.TestPruneOldNeighborMetricsdeterministic). If test(ingestor): make neighbor metrics pruning deterministic #33 lands first, the expected CI failure named below disappears; nothing in this PR depends on it.Verification
Local run of the same commands as CI's “Go Build & Test” job (server tests with
-race), on this branch and on masterfda24ca5under the same conditions (same machine, run one after another):fda24ca5xss-gate-diffchannel-lib-testdecrypt-cli-build-testdockerfile-copy-invariantsdeclare -A), macOS has 3.2; identical on masterstaging-disk-monitorcss-vars-linttest-issue-1375-scope-stats-fetch.jsExactly oneapi('/scope-stats'call exists (the fixed loader) — found 2test-issue-1648-m4-emoji-scan.jsmap.js has 1 emoji/misc-icon hit(s):test-a11y-axe-routes-coverage.jsaxe ROUTES missing analytics tabs (issue #1706): areas, foreign-traffic, wardrivingtest-frontend-helpers.jsfavStar returns empty star for non-favorite: The expression evaluated to a falsy value:,favStar returns filled star for favorite: The expression evaluated to a falsy value:Baseline failures (fail identically on master; not introduced or changed here): see rows marked baseline failure, unchanged.
Browser validation (local, fixture DB, no staging/production): Local Go server (built from master) on the committed E2E fixture DB (freshened, migrated and seeded exactly like CI), serving this branch's
public/, compared side by side with the same server serving master'spublic/. Seeded one transmission (hash fae0c9e6d357a814) with 3 observations from observers Kennedy (representative), GY889 and DUBLIN — the same seed CI uses for Kpa-clawbot#1486. Opened#/packets?observer=<GY889>&timeWindow=262800in grouped mode./api/packets?groupByHash=true&observer=<GY889>returns 11 rows including the seeded one. master rendered "Latest Packets (10)" — the seeded multi-observer row hidden by the client-side re-filter (the bug). this branch rendered "Latest Packets (11)" with the seeded "Kpa-clawbot#1486 fixture" row visible.Not run:
eslint(not installed locally; CI installs it on the fly).-race/tests for modules this PR does not touch (unchanged code, identical to master).Expected GitHub CI: “Go Build & Test” is expected to fail on
TestPruneOldNeighborMetrics, which already fails on master (see #25's run). Downstream jobs (Playwright, image build) are therefore skipped. “Deploy Staging” and all GHCR publish steps only run onpushtomasterand cannot run for this PR.Two further ingestor tests have failed intermittently in this split's CI on branches whose
cmd/ingestortree is byte-identical to master (#27, #28), so they can also appear here without being caused by this change:TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestamp: also reproduced locally on unmodified master.TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749: the suite flake that upstream test(ingestor): join the watchdog loop goroutine instead of only asking it to stop Kpa-clawbot/CoreScope#2003 (also split out of port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25) addresses.GitHub CI result: run 34749195195 on
72f64e93. Go Build & Test: failure; all downstream jobs incl. Deploy Staging skipped. Failed tests:TestPruneOldNeighborMetrics: fails on master, documented baselineTestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749: intermittent suite flake. It is not reproduced locally on master: 0/20 and 0/100 (-cpu=1/-cpu=2) runs of the watchdog tests, and 3 full ingestor-suite runs. This branch'scmd/ingestorandinternaltrees are byte-identical to master, so the ingestor test binary is the same as master's; the failure has hit two frontend-only split PRs (port(upstream#1821): packets observer filter keeps multi-observer rows in grouped mode #27, port(upstream#1962): naive-clock notice says it clears itself #43). Upstream documents it as flaky on clean master (2/20), and test(ingestor): join the watchdog loop goroutine instead of only asking it to stop Kpa-clawbot/CoreScope#2003 (split PR port(upstream#2003): join the watchdog loop goroutine in tests #48) fixes the test🤖 Generated with Claude Code