mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-09-25 21:23:45 +00:00
efitenandClaude Opus 5
7c6b95ea53
Fixes #2024. `s.packets` is declared "sorted by first_seen ASC (oldest first; newest at tail)" (`cmd/server/store.go:177`), and retention eviction depends on it: `evictStaleInternal` walks from the head and stops at the first transmission inside the window. A slice out of order is therefore **under-evicted silently** rather than failing loudly. ## What breaks it The background chunk loader. Chunks are windowed on `last_seen` (#1690), so a transmission first heard weeks ago and heard again recently arrives in a *recent* chunk carrying its old `first_seen`. The chunk was then put in front of the slice: ```go s.packets = append(localPackets, s.packets...) ``` and never re-sorted, so the next chunk, which covers an older window, was prepended in front of it and left that ancient row sitting behind newer ones. `LoadChunked` re-sorts after its own load; the background merge did not. That asymmetry is the whole bug. It is not a corner case. On a production database, of the **236080** transmissions in a 14 day window, **2071** have a `first_seen` more than a day older than their `last_seen`, and **1848** more than a week. This matters more since #2035: with the accounting fixed, `maxMemoryMB` actually triggers, and a walk that stops early works against it. ## The fix `mergeChunkIntoPackets` merges the two sorted runs linearly. Re-sorting the whole slice was not an option: this runs under `s.mu` once per chunk, so it would sort hundreds of thousands of packets while ingest waits for the lock. The chunk already arrives sorted, since the chunk query ends in `ORDER BY t.first_seen ASC`, so the `sort.SliceIsSorted` guard is a contract check costing one linear pass that never sorts in production. ## Covered - `TestMergeChunkIntoPackets_KeepsFirstSeenOrder` pins the merge against an interleaving, deliberately unsorted chunk. - `BenchmarkMergeChunkIntoPackets` guards the linear cost, against a future simplification back into a sort. The server suite runs under `-race` in CI and is green. ## Not covered, and I would rather say it than let the PR imply otherwise There is **no integration test driving `loadChunk` end to end**. I wrote one and dropped it: a faithful seed database for that path needs more of the schema and more of the loader's preconditions than the fix itself is worth. Two CI rounds in, the seed was still loading zero packets (the first attempt failed at `OpenDB` on a missing `nodes` table, the second on the window). Both attempts are in this branch's history rather than rewritten away. So the end-to-end claim rests on the code path quoted above and on the production measurement, not on a test that exercises it. The unit test covers the function where the logic now lives, which is the part that can regress. Also not verified locally: `cmd/server` needs cgo for the #1992 driver and this machine has no C toolchain, so CI is the check. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
83 lines
2.8 KiB
Go
83 lines
2.8 KiB
Go
package main
|
|
|
|
import (
|
|
"fmt"
|
|
"testing"
|
|
"time"
|
|
)
|
|
|
|
// s.packets is declared "sorted by first_seen ASC (oldest first; newest at
|
|
// tail)" (store.go:177), and retention eviction depends on it: it walks from
|
|
// the head and stops at the first transmission inside the window, so a slice
|
|
// out of order is under-evicted silently rather than noisily wrong.
|
|
//
|
|
// The background chunk loader broke that. Chunks are windowed on last_seen, so
|
|
// a transmission first heard weeks ago and heard again recently rides in with
|
|
// a recent chunk carrying its old first_seen; the chunk was put in front with
|
|
// `append(localPackets, s.packets...)` and never re-sorted, so a later, older
|
|
// chunk prepended in front of it left that ancient row behind newer ones.
|
|
//
|
|
// Not a corner case: on a production database 2071 of the 236080 transmissions
|
|
// in a 14 day window have a first_seen more than a day older than their
|
|
// last_seen, 1848 of them more than a week.
|
|
|
|
// mergeChunkIntoPackets is what keeps that invariant true, so pin it directly.
|
|
func TestMergeChunkIntoPackets_KeepsFirstSeenOrder(t *testing.T) {
|
|
at := func(h int) string { return time.Now().UTC().Add(-time.Duration(h) * time.Hour).Format(time.RFC3339) }
|
|
|
|
existing := []*StoreTx{
|
|
{ID: 2, Hash: "b", FirstSeen: at(30)},
|
|
{ID: 4, Hash: "d", FirstSeen: at(10)},
|
|
}
|
|
// Interleaves with what is already loaded, and is deliberately not in
|
|
// order: the chunk query sorts, but the merge must not depend on it.
|
|
chunk := []*StoreTx{
|
|
{ID: 3, Hash: "c", FirstSeen: at(20)},
|
|
{ID: 1, Hash: "a", FirstSeen: at(40)},
|
|
{ID: 5, Hash: "e", FirstSeen: at(5)},
|
|
}
|
|
|
|
merged := mergeChunkIntoPackets(chunk, existing)
|
|
|
|
if len(merged) != 5 {
|
|
t.Fatalf("merged %d packets, want 5", len(merged))
|
|
}
|
|
got := ""
|
|
for _, tx := range merged {
|
|
got += tx.Hash
|
|
}
|
|
if got != "abcde" {
|
|
t.Fatalf("merge order = %q, want %q", got, "abcde")
|
|
}
|
|
for i := 1; i < len(merged); i++ {
|
|
if merged[i-1].FirstSeen > merged[i].FirstSeen {
|
|
t.Fatalf("merge left the slice out of order at index %d", i)
|
|
}
|
|
}
|
|
}
|
|
|
|
// The merge runs under s.mu once per chunk, so it must stay linear. This
|
|
// guards against it being "simplified" back into a sort of the whole slice,
|
|
// which on a loaded instance means sorting hundreds of thousands of packets
|
|
// while ingest waits for the lock.
|
|
func BenchmarkMergeChunkIntoPackets(b *testing.B) {
|
|
base := time.Now().UTC().Add(-14 * 24 * time.Hour)
|
|
mk := func(n, stride, off int) []*StoreTx {
|
|
out := make([]*StoreTx, n)
|
|
for i := 0; i < n; i++ {
|
|
out[i] = &StoreTx{
|
|
ID: i,
|
|
Hash: fmt.Sprintf("h%06d", i),
|
|
FirstSeen: base.Add(time.Duration(i*stride+off) * time.Second).Format(time.RFC3339),
|
|
}
|
|
}
|
|
return out
|
|
}
|
|
existing := mk(200000, 6, 0)
|
|
chunk := mk(20000, 60, 3)
|
|
b.ResetTimer()
|
|
for i := 0; i < b.N; i++ {
|
|
mergeChunkIntoPackets(chunk, existing)
|
|
}
|
|
}
|