Repository navigation
test(ingestor): detect a dropped minimised client RX packet (#305) - #317
Conversation
Relates to #305, the follow-up review of #298. 1. TestHandleClientPacketMinimisedAndWholeAgree asserted only a row count, with both packets at the same rx_at, so the UNIQUE(rx_pubkey, heard_key, rx_at) index collapsed them and the whole packet alone satisfied the assertion. Every "payload required" mutant stayed green. The minimised packet is now sent first, at its own rx_at, and its row is asserted on its own before the whole twin is sent; agreement and idempotency are then separate steps. 2. The three claims docs/client-rx-coverage.md makes get a test each: a minimised 0-hop advert is dropped (both the DIRECT sendZeroHop form and a flooded advert with no hops yet, plus a pubkey-only truncation), a minimised relayed advert survives via path[last], and a partially kept payload is tolerated. Each was re-checked against the firmware source first. The third needed correcting: a group payload is channel hash + cipher MAC + ciphertext, so only a remnant shorter than that 3-byte envelope is reported as undecodable — keep three bytes or more and the channel hash and MAC are published, which the doc now says. 3. The PAYLOAD_TYPE_* defines are on firmware/src/Packet.h:19-32, not 20-33. TestFirmwareLineCitationsAreAccurate keeps that honest: the citations in the test file must match a table of ranges, which runs everywhere, and the cited lines are matched against the real firmware when the (gitignored) firmware/ checkout is present. Tests only; no production file changes.
Rapport — CS-Minimax PR#317 #305 — head 5dfebafStatus: All three points of #305 done, tests only; one of the three documented claims did not hold as written and the doc was corrected; 7 mutants run (at least one per point), the old test shape shown to survive the point-1 mutant; CI per job below. Evidence markers: [T] = I ran it, [A] = code/firmware reading, [K] = known/pre-existing. Requirement 1 —
|
| What | Test | Mutant |
|---|---|---|
The minimised packet is asserted on its own, at its own rx_at, before the whole twin is sent at all. Step 1 asserts the full row it wrote (heard_key=ccdd, keylen 2, src=rxlog, rx_at); step 2 asserts the whole twin adds a second row at its own rx_at with the same key; step 3 asserts idempotency separately (the whole packet re-sent at the minimised packet's rx_at does not add a row) [T] |
TestHandleClientPacketMinimisedAndWholeAgree (rewritten) |
M1 handleClientPacket returns early on decoded.Payload.Error != "" → the rewritten test fails at step 1 with 0 rows [T]. The old shape of the same test passes under M1 — the exact gap #305 reports, reproduced and then closed [T] |
New helper receptionsFor reads the rows (key, keylen, src, rx_at, ordered by rx_at) instead of counting them; counting is what let the whole packet stand in for the minimised one, because UNIQUE(rx_pubkey, heard_key, rx_at) collapsed two same-rx_at calls into one row [A].
Requirement 2 — the three documented claims, each verified against firmware then pinned
Firmware read at a366955 (the commit the #298 review cites), cloned locally per AGENTS.md [A].
| Claim | Verified against firmware | Test | Mutant |
|---|---|---|---|
| (a) a minimised 0-hop advert is dropped | Holds. Mesh::createAdvert writes the advertiser's 32-byte pub_key as the first payload field, then a 4-byte timestamp and a 64-byte signature (docs/payloads.md, "Node advertisement"), so a path-less advert carries its heard key only in the payload [A] |
TestHandleClientPacketMinimisedZeroHopAdvertDropped — both on-wire 0-hop forms (ROUTE_TYPE_DIRECT via Mesh::sendZeroHop, and ROUTE_TYPE_FLOOD before any relay appended a hop), plus a pubkey-only truncation, plus a whole control that is accepted [T] |
M2 deriveHeardKey's advert branch drops the advertPubkey != "" guard → all three minimised subtests get a row [T] |
| (b) a minimised relayed advert survives | Holds. Mesh::routeRecvPacket APPENDS the forwarder's own hash at path[hash_count * hash_size] and bumps the count, so path[last] is the node that physically transmitted — readable with no payload [A] |
TestHandleClientPacketMinimisedRelayedAdvertSurvives — 2-byte, 3-byte and behind-transport-codes forms, each one row keyed on path[last] with src=rxlog (not advert) [T] |
M3 deriveHeardKey skips the path branch for adverts (len(hops) > 0 && !isAdvert) → both minimised relayed adverts get 0 rows [T] |
| (c) a partially kept payload is tolerated | Holds, but the stated reason does not. The doc said "(it is simply reported as undecodable)". A group payload is channel hash (1) + cipher MAC (2) + ciphertext (docs/payloads.md, "Group text message"), so a remnant of 3 bytes or more decodes that envelope without an error and publishes the channel hash and the MAC; only a shorter remnant is reported as undecodable [A]/[T]. Doc corrected to say what actually leaks |
TestHandleClientPacketPartialPayloadTolerated — both sub-cases: the row is identical to the fully minimised one in each, and the decoder's view is asserted per case (Payload.Error set for 7f12; ChannelHashHex=7F, MAC=1234, no error for 7f1234) [T] |
M4 handleClientPacket drops a packet whose payload is present but undecodable → kills the short-remnant subtest only, the fully minimised tests stay green [T]. M5 decodeGrpTxt requires 4 bytes instead of 3 → the 3-byte remnant reports too short and loses the channel hash and MAC [T] |
Two smaller corrections in the same pass: truncating an advert payload to just the 32-byte pubkey does not rescue a 0-hop advert either (the payload is pubkey 32 + timestamp 4 + signature 64, and all 100 bytes must be present before the pubkey is read back) [A]/[T], and the fwHeader comment now names the PH_* mask/shift defines that actually sit on Packet.h:8-12 rather than the getters, which are at :62, :72 and :77 [A].
Requirement 3 — the line reference, and a test that keeps it honest
| What | Test | Mutant |
|---|---|---|
firmware/src/Packet.h:20-33 → :19-32. At a366955 the PAYLOAD_TYPE_* defines run from PAYLOAD_TYPE_REQ on 19 to PAYLOAD_TYPE_RAW_CUSTOM on 32 [A] |
TestFirmwareLineCitationsAreAccurate, in two halves: (1) every firmware/...:N-M citation in the test file must appear in the fwLineRefs table and every table row must still be cited — source-only, so it runs everywhere, CI included; (2) each row also carries a pattern the first and last line of the cited range must match, checked against the real firmware [T] |
M6 the comment reverts to :20-33 → fails with and without the firmware checkout present [T]. M7 an fwLineRefs row claims the wrong first line for 19-32 → the firmware-reading half fails [T] |
Limitation, stated plainly: firmware/ is a gitignored local clone (AGENTS.md), so half (2) skips in CI — verified that it skips cleanly rather than failing when the checkout is absent [T]. Half (1), which is the half that turns the wrong line number into a failure, runs in CI.
All mutants were applied to the working tree, run, and reverted from a scratchpad snapshot (not via git checkout --). git diff origin/master afterwards shows only the test file and the doc; no production file is touched [T].
No bug found
The three claims and the point-1 gap are all test/doc issues. No production behaviour needed changing, and none was changed. Because of that the new tests are green on master too — the red-before signal for each point is the mutant (and for point 1 also the old test shape, which survives M1 where the new one dies).
CI per job
| Job | Result |
|---|---|
| Go Build & Test | ✅ success [T] |
| Playwright E2E Tests | ✅ success [T] |
| Build & Publish Docker Image | ✅ success [T] |
| Release Artifacts | skipped (not a release) |
| Deploy Staging | skipped (draft PR) |
| Publish Badges & Summary | skipped |
Run 37452077644, attempt 1 — no re-runs needed. The known flaky E2E (#271) did not fire [K].
Local runs
cmd/ingestorgo test -count=1 -timeout 20m ./...:ok 108.5s[T].cmd/servergo test -count=1 -timeout 20m ./...:ok 46.4s[T] (untouched by this PR; the test(ingestor): pin minimised raw packets on the client RX topic (#284) #298 review'sTestHandleNodePaths_FallbackUnresolvableHop_1352flake did not fire [K]).cmd/migrate: pass [T]. All 14internal/*modules: pass [T].- The client-RX tests under
-race: pass [T]. go vet ./...clean incmd/ingestor,cmd/server,cmd/migrate[T].gofmt -lreports no diff for the changed file (other files in the package were already unformatted on master [K]).sh test-all.sh: 222 files, 222 passed [T].node test-frontend-helpers.js: 709 passed [T].- E2E: nothing frontend or server-side changed, so none is affected. Ran the coverage-adjacent ones against a local Go server on
e2e-fixture.db, prepared as in CI (freshen, the Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486/"Group Data" message type missing from packet view window's "message type" filter Kpa-clawbot/CoreScope#1791 seed SQL,corescope-migrate, seeds 2073/199/245):test-home-coverage-e2e.js12/12,test-path-inspector-coverage-e2e.js10/10,test-issue-1274-legend-coverage-e2e.js10/10,test-issue-124-rx-coverage-viewport.js16/16,test-node-reach-e2e.jsOK,test-node-reach-coverage-e2e.jsskips with coverage disabled exactly as in CI [T]. Server stopped by the pid on its port, port confirmed released [T].
Invariants
cmd/serveruntouched: the diff undercmdis one test file [T]. No newmap[string]interface{}outside tests [T]. No hardcoded colours [T].bash scripts/check-xss-sinks.sh --diff origin/master: "no public/**/*.{js,html} changes to scan", exit 0 [T]. (The script still needsbash; undershit fails at line 58 — predates this PR [K].)- Fork guards unchanged: 9 in
deploy.yml, 1 inrelease-fast-path.yml;.github/untouched [T]. - Single commit, committed under the expected author identity; no closing keywords in the title, body or commit message ("Relates to Follow-up to #298: tests that can actually detect a dropped minimised client RX packet #305") [T].
Remaining / not done
- Left as a draft; no merge, no ready-for-review, Follow-up to #298: tests that can actually detect a dropped minimised client RX packet #305 not closed.
- The firmware-reading half of
TestFirmwareLineCitationsAreAccurateskips in CI (see Requirement 3). Making it run would mean cloning the firmware in CI, which is a workflow change and out of scope here. - Nothing else from the test(ingestor): pin minimised raw packets on the client RX topic (#284) #298 review is outstanding: findings 1, 2 and 4 are this PR; finding 3 (no production change, so the tests are green on master) is informational and still accurate.
Review — CS-pve-agent2 PR#317 — head 5dfebafDom: APPROVE with nits Independent read-only review. Evidence markers: [T] = I ran it, [A] = code/firmware reading, [K] = known/pre-existing. Findings
No blocking findings. No production change, no behaviour change. Point 1 —
|
| Claim | Firmware | Verdict | Test | My mutant |
|---|---|---|---|---|
| (a) minimised 0-hop advert is dropped | Mesh::createAdvert writes pub_key first (Mesh.cpp:416). sendZeroHop sets ROUTE_TYPE_DIRECT, path_len = 0 (:717-721). sendFlood sets ROUTE_TYPE_FLOOD with hash count 0 (:647-649). Advert layout 32+4+64+appdata (docs/payloads.md:29-34) [A] |
Holds | TestHandleClientPacketMinimisedZeroHopAdvertDropped |
MB: a 0-hop advert without a pubkey gets a placeholder key, which kills all 3 minimised subtests. MB2: decodeAdvert returns the pubkey from a 32-byte truncation, which kills only the truncation subtest [T] |
| (b) minimised relayed advert survives | Mesh::routeRecvPacket appends self_id hash at path[n * hash_size], then setPathHashCount(n + 1) (Mesh.cpp:344-350) [A] |
Holds | TestHandleClientPacketMinimisedRelayedAdvertSurvives |
MC: adverts keyed on path[0] instead of path[last], which kills all 3 subtests [T] |
| (c) partial payload tolerated | Group text = channel hash 1 + MAC 2 + ciphertext (docs/payloads.md:229-235) [A] |
Outer claim holds. The "simply reported as undecodable" correction is right. Wording nit, see finding 2 | TestHandleClientPacketPartialPayloadTolerated |
MD: drop an envelope-only remnant (MAC set, no ciphertext), which kills the 7f1234 subtest only [T] |
Each claim has its own test. The extra "100 bytes before the pubkey is read" note matches decodeAdvert (len(buf) < 100) [A]. All Packet.h citations in the file are correct at a366955: 8-12 (PH_*), 9, 14-17 (ROUTE_TYPE_*), 19-32 (PAYLOAD_TYPE_REQ … RAW_CUSTOM) and 24 (GRP_TXT) [A].
Point 3 — TestFirmwareLineCitationsAreAccurate in CI
firmware/is not cloned in CI (no workflow step) [A]. The test reads two local files, with no network access and no time or ordering dependence, so it adds no flakiness [A]/[T].- Without firmware, half 1 runs and half 2 skips with a clear message [T]. The CI Go job ran ingestor tests green: run 37452077644, attempt 1 [T]. The skip is documented in the test doc comment and the PR body, so it is not hidden. See finding 3 for how it reports.
- ME, the comment reverted to
:20-33, is killed with and without firmware [T]. - MF2, where comment, table and example all move consistently to 20-33, survives without firmware and is killed with it [T]. That is the documented limitation.
- MG is the masking from finding 1.
Scope
- The diff is
cmd/ingestor/client_rx_minimised_test.goanddocs/client-rx-coverage.mdonly. It is test-only, andcmd/serverand.github/are untouched [T]. - No new
map[string]interface{}in non-test code. The only#hexmatches are#305issue references, so there are no hardcoded colours [T]. scripts/check-xss-sinks.sh --diff origin/masterexits 0 ("no public/** changes"), run in a scratch clone with HEAD = PR head [T].- Fork guards on the merged tree: 9 in
deploy.yml, 1 inrelease-fast-path.yml[T]. - No closing keywords in the title, body or commit ("Relates to Follow-up to #298: tests that can actually detect a dropped minimised client RX packet #305") [T].
- Commit author and committer are
dborup <kontakt@meshview.dk>[T].
Tests and mutants
Merged tree (origin/master 30c7de46 + head) [T]:
cmd/ingestorgo test -count=1 ./...:ok 363.2scmd/servergo test -count=1 ./...:ok 38.6s- My first attempt ran all four suites in parallel on a 4-core box with slow disk I/O, so both Go packages hit the 20m package timeout; no test was hung, the running test was 0–1s old. In that run
TestHandleNodePaths_FallbackPreconfirmed_1352failed once with503 index loading, the startup race in the same_1352family the author notes from the test(ingestor): pin minimised raw packets on the client RX topic (#284) #298 review [K]. It passed 10/10 in isolation (-count=10 -run TestHandleNodePaths_Fallback). Both packages were green when re-run sequentially withTMPDIRon tmpfs (results above).cmd/serveris untouched by this PR. sh test-all.sh: 224 passed, 0 failed (224 files)node test-frontend-helpers.js: 709 passed, 0 failed
Head, targeted with -v, all green [T]: the 3 new tests, the rewritten Agree test and the existing TestHandleClientPacket* / TestDecodePacketMinimised*. TestFirmwareLineCitationsAreAccurate reports SKIP without firmware and PASS with firmware/ at a366955 linked in.
E2E against a local Go server on e2e-fixture.db [T]. The fixture was prepared as in CI:
freshen-fixture.sh;- the Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486/"Group Data" message type missing from packet view window's "message type" filter Kpa-clawbot/CoreScope#1791 seed SQL taken from the workflow;
corescope-migrate;- seeds 2073, 199 and 245.
No E2E touches this change (Go test file + doc). I ran these as regression checks:
test-e2e-playwright.js: 6 passed, then failed at test 7 (Version info lives on Perf dashboard:waitForFunctionon#navStatstimed out). It failed in both runs, including on an idle box. The merged tree differs fromorigin/masteronly in the PR's two files, so server andpublic/are byte-identical to master and the failure cannot come from this PR. The same suite passed in CI on this head. A fresh headless page against the same server renders#navStatscorrectly (500 pkts · 204 nodes · 31 obs). I did not investigate further. This is local-environment or pre-existing, not a blocker here [T]/[K].test-home-coverage-e2e.js12/12test-issue-1274-legend-coverage-e2e.js10/10test-node-reach-e2e.jsOK
The server was stopped via the pid on its port, and the port was confirmed released.
Mutants. Each was applied to a scratch copy and restored from the archived head before the next one [T]:
| Mutant | Target | Result |
|---|---|---|
| MA | payload must be non-empty (point 1) | killed by new Agree test; survives old Agree test |
| MB | 0-hop advert w/o pubkey keyed anyway | killed (3/3 minimised subtests) |
| MB2 | decodeAdvert reads pubkey from 32-byte truncation |
killed (truncation subtest) |
| MC | relayed advert keyed on path[0] |
killed (3/3) |
| MD | drop envelope-only GRP_TXT remnant | killed (7f1234 subtest) |
| ME | comment back to :20-33 |
killed, with and without firmware |
| MF / MF2 | comment+table (+example) to 20-33 | MF killed both ways (via the example — finding 1); MF2 survives w/o firmware, killed with |
| MG / MG2 | drop the 19-32 citation (/ and the example) | MG survives both ways; MG2 killed — finding 1 |
Not verified
- I did not rerun the author's M1–M7 verbatim. My MA–MG cover the same points with different edits.
- I did not run
-race,go vetor the full Playwright E2E list. I did not do browser or visual validation, since nothing in it is visual. - I did not find the cause of the local
test-e2e-playwright.jsfailure at test 7. See above; it is independent of this PR. - Firmware was checked only at
a366955, which is also current upstreammain. Other commits were not checked. - CI was not re-run. I only read the existing attempt-1 results.
Relates to #305, the follow-up review of #298 (which was itself the #284 investigation).
Plan (what this PR does)
TestHandleClientPacketMinimisedAndWholeAgreesent the minimised and the whole packet at the same
rx_atand then only counted rows. TheUNIQUE(rx_pubkey, heard_key, rx_at)index collapses those two calls into one row, so the wholepacket alone produced the single expected row and every "payload required" mutant stayed green.
Rewritten as three ordered steps: send the minimised packet first at its own
rx_atand assertits row (
heard_key, keylen,src,rx_at); then send the whole twin at a secondrx_atandassert it adds a second row with the same key; then assert idempotency separately, by re-sending
the whole packet at the minimised packet's
rx_atand checking the row count does not grow.A new
receptionsForhelper reads rows rather than counting them, which is what makes "the rowthis call wrote" assertable at all.
with the firmware reason in the comment:
Mesh::createAdvertwrites the advertiser's 32-bytepub_keyas the first payload field, so a path-less advert carries its heard key only in thepayload. Covers both on-wire 0-hop forms (
ROUTE_TYPE_DIRECTviasendZeroHop, andROUTE_TYPE_FLOODbefore any relay has appended a hop), a pubkey-only truncation, and a wholecontrol so the drop cannot be a broken fixture.
Mesh::routeRecvPacketAPPENDS the forwarder's hash atpath[hash_count * hash_size], sopath[last]is the node that transmitted and needs nopayload. 2-byte, 3-byte and behind-transport-codes forms;
srcisrxlog, notadvert.minimised one, plus what the remnant decodes to.
firmware/src/Packet.h:20-33→:19-32), and keep every firmwarecitation in the file honest with
TestFirmwareLineCitationsAreAccurate.One claim did not hold and was corrected
Claim 3 was documented as "A partially kept payload is tolerated too (it is simply reported as
undecodable)". The parenthetical is only true for a very short remnant. A group payload is
channel hash (1 byte) + cipher MAC (2 bytes) + ciphertext (firmware
docs/payloads.md, "Group textmessage"), so a remnant of 3 bytes or more decodes that envelope without an error and publishes
the channel hash and the MAC. The outer claim (tolerated, same coverage row) holds; the doc now says
what actually leaks, which is the point an uploader needs. Both sub-cases are pinned.
Two smaller doc/comment corrections in the same spirit: truncating an advert payload to just the
32-byte pubkey does not rescue a 0-hop advert either (the payload is pubkey 32 + timestamp 4 +
signature 64, and all 100 bytes must be present before the pubkey is read back), and the comment on
fwHeadernow names thePH_*mask/shift defines that actually sit onPacket.h:8-12rather thanthe getters, which are further down the file.
How the line reference stays fixed
A line number in a comment has nothing to fail against, so
TestFirmwareLineCitationsAreAccurategives it one, in two halves:
firmware/...:N-Mcitation in the test file must appear in anfwLineRefstable (and everytable row must still be cited). This needs only the source file, so it runs everywhere, CI
included — and it is what turns
:20-33into a test failure.is checked against the real firmware.
firmware/is a gitignored local clone (AGENTS.md), so thishalf skips when the checkout is absent, as it is in CI. Verified locally against firmware
a366955, where thePAYLOAD_TYPE_*defines are indeed on 19-32.Changes
cmd/ingestor/client_rx_minimised_test.go— tests onlydocs/client-rx-coverage.md— the corrected claim 3, the advert-truncation note, and the testpointer
No production file is touched (
git diff origin/master -- cmd internal --statshows only the testfile).
Mutants
Applied to the working tree, run, reverted. One per issue point, plus the ones that show the gap is
real.
handleClientPacketreturns early whendecoded.Payload.Error != ""— a client-path "payload required" checkderiveHeardKey's advert branch drops theadvertPubkey != ""guardderiveHeardKeyskips the path branch for adverts (len(hops) > 0 && !isAdvert)handleClientPacketdrops a packet whose payload is present but undecodabledecodeGrpTxtrequires 4 bytes instead of 3too shortand loses the channel hash and MACfirmware/src/Packet.h:20-33fwLineRefsrow claims the wrong first line for 19-32Invariants
cmd/serveruntouched; no newmap[string]interface{}outside tests; no hardcoded colours.bash scripts/check-xss-sinks.sh --diff origin/masterclean (nopublic/**changes).deploy.yml, 1 inrelease-fast-path.yml.