From fd825a779ca9bd3d930ee87e8bf9f5955158c469 Mon Sep 17 00:00:00 2001 From: efiten Date: Wed, 9 Sep 2026 17:18:54 +0200 Subject: [PATCH] fix(ingestor): name a region scope deterministically, or not at all (#1988) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `matchScope` returns the first configured region whose derived code equals the packet's `code1` and stops there. Go randomises map iteration order per `range`, so when two configured regions collide on a payload, the stored region name depends on which key the runtime happened to visit first. **The same packet can be named differently on two runs of the same binary**, and neither answer is evidence of anything. ## How often this actually happens `code1` is two bytes, so any two configured regions collide on a given payload with probability 1/65536. That is a curiosity at 5 configured regions and routine at 150. Measured on a live instance carrying 159 configured regions, over 41.7 hours and 147,535 transport-scoped packets: **374 collisions, 0.253% of decisions.** They concentrate on four key pairs rather than scattering, because `code1` is an HMAC over the payload: a payload that collides collides every time it is seen, and a flooded packet is seen by many observers. The existing comment sizes the function for "≤ 50 regions". A live BE/NL instance declares 126 distinct region names across its repeaters, so operators are already past that. ## The rule `matchingRegions` returns every match; `matchScope` applies one rule: - exactly one match names the packet - several matches name nothing The candidates are equally sourced, there is no principled winner between them, and storing a wrong region name is worse than storing none. `""` is already the ingestor's "transport-scoped but unnameable" state (`scopeNameForDB`), so an ambiguous packet lands in a state the rest of the system already understands rather than in a new one. Nothing downstream needs to learn a new value. The collision is logged, because it is otherwise invisible: such a packet is stored exactly like one whose region this instance holds no key for. An operator watching an unnameable count grow deserves to see which of their own configured regions are colliding, since the fix is theirs to make. ## Rule 0 Cost is unchanged: the same single pass over the same keys, it just no longer stops early. The early exit was worth nothing on the common path, where zero or one key matches and the loop runs to the end either way. Worst case is unchanged at one HMAC per key per transport-scoped packet. There is no indexable shortcut to reach for. `code1` is an HMAC over the packet payload, so nothing is payload-independent to index on, and the old comment suggesting a "pre-indexed lookup table" is removed rather than left as a false lead for the next reader. ## Tests Three, and the fixture matters: - an unambiguous packet still gets its region name - a genuinely colliding payload stores the unmatched state instead of a coin flip - the ambiguous case run 50 times, because a first-match matcher passes a single iteration roughly half the time The collision is **found by searching payloads** (~65k tries, fractions of a second) rather than asserting on a hand-picked `code1`. The case only exists when the matcher genuinely finds two names for one packet, and a fabricated code would only prove the test agrees with itself. `cd cmd/ingestor && go test ./...` passes apart from `TestWriteStatsAtomic_SymlinkAtDestIsReplaced`, which needs `SeCreateSymbolicLinkPrivilege` and fails on Windows on master too. `go vet` and `gofmt -l` clean. --- cmd/ingestor/main.go | 49 +++++++++++++++++++--- cmd/ingestor/main_test.go | 88 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 5 deletions(-) diff --git a/cmd/ingestor/main.go b/cmd/ingestor/main.go index ec4ef406..d3ed0dda 100644 --- a/cmd/ingestor/main.go +++ b/cmd/ingestor/main.go @@ -1635,12 +1635,25 @@ func loadRegionKeys(cfg *Config) map[string][]byte { return keys } -// matchScope performs one HMAC-SHA256 per configured region. Expected -// len(regionKeys) ≤ 50; beyond that, consider a pre-indexed lookup table. -func matchScope(regionKeys map[string][]byte, payloadType byte, payloadRaw []byte, code1 string) string { +// matchingRegions returns every configured region whose derived code equals +// the packet's code1, rather than the first one found. +// +// The distinction matters because code1 is two bytes: two configured regions +// collide on a given payload with probability 1/65536, and at 159 keys on a +// live instance that is roughly 0.25% of transport-scoped packets, hundreds a +// week rather than a curiosity. Returning the first match made the stored +// region name depend on Go's randomised map iteration order, so the same +// packet could be named differently on two runs and neither answer was +// evidence of anything. +// +// The cost is unchanged: this is the same single pass over the same keys, it +// just does not stop early. There is no indexable shortcut, because code1 is +// an HMAC over the payload and nothing here is payload-independent. +func matchingRegions(regionKeys map[string][]byte, payloadType byte, payloadRaw []byte, code1 string) []string { if code1 == "0000" || len(regionKeys) == 0 || len(payloadRaw) == 0 { - return "" + return nil } + var matched []string for name, key := range regionKeys { mac := hmac.New(sha256.New, key) mac.Write([]byte{payloadType}) @@ -1654,9 +1667,35 @@ func matchScope(regionKeys map[string][]byte, payloadType byte, payloadRaw []byt } codeBytes := [2]byte{byte(code & 0xFF), byte(code >> 8)} if strings.ToUpper(hex.EncodeToString(codeBytes[:])) == code1 { - return name + matched = append(matched, name) } } + return matched +} + +// matchScope names one packet's region scope, or returns "" when it cannot be +// named with confidence. +// +// Exactly one match names the packet. Several matches name nothing: the two +// candidates are equally sourced, there is no principled winner, and storing +// a wrong region name is worse than storing none. "" is already the ingestor's +// "transport-scoped but unnameable" state (scopeNameForDB), so an ambiguous +// packet lands in a state the rest of the system already understands rather +// than in a new one. +// +// The collision is logged because it is otherwise invisible: the packet is +// stored exactly like one whose region this instance holds no key for, and an +// operator looking at a growing unnameable count deserves to see which of +// their configured regions are colliding. +func matchScope(regionKeys map[string][]byte, payloadType byte, payloadRaw []byte, code1 string) string { + matched := matchingRegions(regionKeys, payloadType, payloadRaw, code1) + switch len(matched) { + case 0: + return "" + case 1: + return matched[0] + } + log.Printf("[regions] ambiguous collision between %v; storing unmatched", matched) return "" } diff --git a/cmd/ingestor/main_test.go b/cmd/ingestor/main_test.go index 9d241f46..cbc6293c 100644 --- a/cmd/ingestor/main_test.go +++ b/cmd/ingestor/main_test.go @@ -2,6 +2,8 @@ package main import ( "bytes" + "crypto/hmac" + "crypto/sha256" "database/sql" "encoding/hex" "encoding/json" @@ -10,6 +12,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" "time" @@ -1186,3 +1189,88 @@ func TestHandleMessageAdvert_MatchedScopeUpdatesDefaultScope(t *testing.T) { t.Errorf("default_scope after matched-scope advert = %q (valid=%v), want #de", got.String, got.Valid) } } + +// codeForRegion derives the on-wire code1 a sender in this region would emit +// for this payload: the forward direction of what matchScope inverts. +func codeForRegion(name string, payloadType byte, payload []byte) string { + if !strings.HasPrefix(name, "#") { + name = "#" + name + } + sum := sha256.Sum256([]byte(name)) + mac := hmac.New(sha256.New, sum[:16]) + mac.Write([]byte{payloadType}) + mac.Write(payload) + h := mac.Sum(nil) + code := uint16(h[0]) | uint16(h[1])<<8 + if code == 0 { + code = 1 + } else if code == 0xFFFF { + code = 0xFFFE + } + return strings.ToUpper(hex.EncodeToString([]byte{byte(code & 0xFF), byte(code >> 8)})) +} + +// findRegionCollision searches for a payload whose code1 is identical under two +// region names. code1 is two bytes, so one turns up after ~65k tries and the +// search costs a fraction of a second. +// +// A hand-built fixture cannot stand in here: the whole point of the case below +// is what happens when the matcher genuinely finds two names for one packet, +// and a fabricated code1 would only prove the test agrees with itself. +func findRegionCollision(t *testing.T, nameA, nameB string, payloadType byte) ([]byte, string) { + t.Helper() + payload := make([]byte, 4) + for i := 0; i < 1<<22; i++ { + payload[0], payload[1] = byte(i), byte(i>>8) + payload[2], payload[3] = byte(i>>16), byte(i>>24) + if a, b := codeForRegion(nameA, payloadType, payload), codeForRegion(nameB, payloadType, payload); a == b { + return append([]byte(nil), payload...), a + } + } + t.Fatalf("no code1 collision between %s and %s in 2^22 payloads", nameA, nameB) + return nil, "" +} + +// TestMatchScopeNamesAnUnambiguousPacket is the ordinary case: one configured +// region derives the packet's code1, so the packet carries that region's name. +func TestMatchScopeNamesAnUnambiguousPacket(t *testing.T) { + keys := loadRegionKeys(&Config{HashRegions: []string{"#be", "#nl"}}) + payload := []byte{0x01, 0x02, 0x03, 0x04} + code1 := codeForRegion("#be", 5, payload) + + if got := matchScope(keys, 5, payload, code1); got != "#be" { + t.Errorf("matchScope = %q, want %q", got, "#be") + } +} + +// TestMatchScopeStoresAmbiguousAsUnmatched pins the reason this changed. Two +// configured regions derive the same code1 for one payload; naming the packet +// after either is a coin flip, and before this the flip was Go's map iteration +// order, so the same packet could be stored under different regions on two +// runs of the same binary. +func TestMatchScopeStoresAmbiguousAsUnmatched(t *testing.T) { + payload, code1 := findRegionCollision(t, "#be", "#zz", 5) + keys := loadRegionKeys(&Config{HashRegions: []string{"#be", "#zz"}}) + + if n := len(matchingRegions(keys, 5, payload, code1)); n != 2 { + t.Fatalf("matchingRegions returned %d names, want 2 — the collision fixture is wrong", n) + } + if got := matchScope(keys, 5, payload, code1); got != "" { + t.Errorf("matchScope = %q, want the unmatched state: two equally-sourced candidates have no principled winner", got) + } +} + +// TestMatchScopeIsOrderIndependent runs the ambiguous case repeatedly. Map +// iteration order is randomised per range in Go, so a first-match matcher +// returns different names across iterations of this loop; the answer must not +// move. +func TestMatchScopeIsOrderIndependent(t *testing.T) { + payload, code1 := findRegionCollision(t, "#be", "#zz", 5) + keys := loadRegionKeys(&Config{HashRegions: []string{"#be", "#zz"}}) + + for i := 0; i < 50; i++ { + if got := matchScope(keys, 5, payload, code1); got != "" { + t.Fatalf("iteration %d: matchScope = %q, want a stable answer across map iteration orders", i, got) + } + } +}