Skip to content

port(upstream#2000): prune aged packets in bounded batches - #49

Merged
dborup merged 3 commits into
masterfrom
codex/port-upstream-2000-chunked-packet-prune
Sep 22, 2026
Merged

dborup merged 3 commits into
masterfrom
codex/port-upstream-2000-chunked-packet-prune

Conversation

@adminopenclaw8-sketch

@adminopenclaw8-sketch adminopenclaw8-sketch commented Sep 13, 2026 •

Copy link
Copy Markdown
Collaborator

Split out of #25 (commit dcac010e there). This branch holds exactly one upstream change so it can be reviewed, tested and reverted on its own.

Upstream

Problem

PruneOldPackets deleted a whole retention day inside a single WriterTx. writerMu serialises every writer call, so MQTT ingest was blocked for the whole delete. Upstream measured about 35 s on an instance with ~260k observations/day.

Change

Deletes in bounded batches of 250 transmissions (child observations first, same selection in both statements), committing and releasing the writer lock between batches. Batches are selected with ORDER BY first_seen, id, which idx_transmissions_first_seen satisfies, so the empty steady-state pass does not scan the table. A test pins that query plan. On error, the count already deleted by committed batches is returned.

Adaptation to this fork

None. The cherry-pick applied without conflicts and the changed lines are identical to upstream.

Notes for review

idx_transmissions_first_seen exists in this fork's schema (internal/dbschema/dbschema.go, cmd/ingestor/db.go).

Dependencies and merge order

Verification

Local run of the same commands as CI's “Go Build & Test” job (server tests with -race), on this branch and on master fda24ca5 under the same conditions (same machine, run one after another):

Check master fda24ca5 this branch verdict
go-ingestor-build-vet PASS PASS
go-ingestor-test FAIL FAIL TestPruneOldNeighborMetrics baseline failure, unchanged
channel-lib-test PASS PASS
decrypt-cli-build-test PASS PASS
dockerfile-copy-invariants FAIL FAIL not runnable locally: script needs bash ≥4 (declare -A), macOS has 3.2; identical on master
staging-disk-monitor PASS PASS
css-vars-lint PASS PASS

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): Not applicable (no frontend change).

Not run:

  • Playwright E2E suites (no local Playwright install); CI's E2E job will also be skipped, see below.
  • eslint (not installed locally; CI installs it on the fly).
  • Frontend JS suites (no frontend change).
  • Browser tests against staging/production (deliberately none).

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 on push to master and cannot run for this PR.
Two further ingestor tests have failed intermittently in this split's CI on branches whose cmd/ingestor tree is byte-identical to master (#27, #28), so they can also appear here without being caused by this change:

GitHub CI result: run 34752488840 on 76f42816. Go Build & Test: failure; all downstream jobs incl. Deploy Staging skipped. Failed tests:

  • TestPruneOldNeighborMetrics: fails on master, documented baseline

🤖 Generated with Claude Code

liquidraver and others added 3 commits September 13, 2026 10:43
…stalling ingest (Kpa-clawbot#2000)

Reviewed at ecf0b37: query plans dumped and confirmed index-driven for all three statements, termination proven against concurrent ingest (first_seen is always time.Now()), FK child-first ordering required and correct, writer-stats assertions non-racy. Two low findings noted on the PR for follow-up: the dropped RowsAffected error now gates the loop, and ~0.53s batches will trip defaultSlowWriterMs=500.

(cherry picked from commit fe37f10)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dborup

dborup commented Sep 20, 2026

Copy link
Copy Markdown
Owner

Uafhængig review + målt verifikation — VERDICT: ingen blockers

Gennemgået af en reviewer, der ikke skrev ændringen, med krav om at måle frem for at ræsonnere. Branchen er synkroniseret med master 16ffe105 (almindelig merge-commit 14a72998, ingen rebase/squash/force-push).

PR'ens kernepåstand holder. På en produktionsformet datamængde (16.000 forældede transmissions × 16 observationer = 256.000 observation-rækker — PR'ens egen "~260k observationer/dag"):

ny (chunked) gammel (én transaktion) forbedring
prune-transaktioner 65 1
writerMu hold max / p50 24,4 / 18,65 ms 1089,1 ms 44,6×
maks. ingest-blokering (wall) 43,7 ms 1089,3 ms 24,9×
ingest-skrivninger fuldført under prune 32 1 32×
samlet prune-wall 1198 ms 1089 ms +10 %

Miljø: darwin/arm64, Go 1.26.0, modernc.org/sqlite v1.34.5, journal_mode=wal, synchronous=FULL, MaxOpenConns(1).

Batchgrænsen er bevist, ikke antaget. På 2037 forældede rækker: total == 2037 og præcis 9 transaktioner tvinger opdelingen til 8×250 + 1×37. En transaktion med <250 før den sidste ville have ramt if batch < pruneBatchTransmissions { break } og efterladt total < 2037; en med >250 ville have givet færre end 9 transaktioner.

Utilsigtet gevinst: chunking skærer peak -wal 2,4× (11,5 MB → 4,7 MB). Pris: 32 ekstra fsyncs.

Indeksplanen — påstanden VERIFICERET

ANALYZE køres aldrig i produktion, så no-stats-planen er den, der sendes. Ved 1M rækker, intet forældet:

ordering plan bedste af 9
ORDER BY first_seen, id SEARCH … USING COVERING INDEX idx_transmissions_first_seen 0,007 ms
ORDER BY id SCAN transmissions 70,809 ms (10.115×)

Kommentarens "~10 µs til ~73 ms" er præcis. Revieweren rapporterede ærligt, at dens første forsøg modsagde kommentaren — det var dens eget harness: ved 200k rækker beholder SQLite det dækkende indeks for ORDER BY id (tilføjer USE TEMP B-TREE), og planlæggeren skifter først til SCAN ved større tabeller. Den pinnede test er derfor dækkende i begge størrelsesordener.

Differential mod implementeringen før PR'en

14 datasæt: tom tabel, intet forældet, alt forældet på et eksakt multiplum af 250, 600 rækker med samme first_seen hen over batchgrænser, rækker præcis på cutoff og ±1 s, ikke-UTC-offsets, samt 8 randomiserede blandinger på 266–1122 rækker. Hver gang: identisk returneret antal, identisk overlevende transmission-id-sæt, identisk overlevende (transmission_id, observer_idx)-sæt. Tidsstempel-sammenfald giver hverken spring eller dobbeltsletning, fordi transmissions.id er INTEGER PRIMARY KEY AUTOINCREMENT (= rowid), som indekset bærer som tiebreaker.

Desuden målt: de to statements i en batch materialiseres som LIST SUBQUERY (ikke korreleret), så der er ingen delete-while-scanning-risiko — nul forældreløse observations i alle differentialtilfælde. Og en fejl midt i løkken spinner ikke: den blokerede batch rullede helt tilbage, og funktionen returnerede straks.

SHOULD-FIX (dokumenteret, ikke rettet her — se begrundelse nederst)

  1. maintenance.go:61-62 overdriver grænsen ~2×. Kommentaren siger "ingest is never blocked for longer than a single batch". Målt: batch-hold p50 18,65 ms, men ingest-probens writerMu-ventetid p50 36,61 ms — konsekvent ~2 batches. Starvation-mode-overdragelsen virker, men prune-goroutinen genanskaffer låsen ofte nok til, at ingest typisk betjenes ved anden grænse. Forbedringen er reel og stor; sætningen er bare ikke bogstaveligt sand. (målt)
  2. maintenance.go:76 — løkken har intet iterationsloft og ingen deadline. cutoff er fast, men rækker indsat under den efter løkkestart samles op af senere batches. Demonstreret: med forældede rækker injiceret samtidig kørte PruneOldPackets stadig efter 3,0 s og havde slettet 355.100 mod 300 seedede. Det kræver ~14.000 forældede transmissions/sekund, hvilket MQTT-ingest ikke kan nå — derfor ikke en blocker. Et loft (ceil(count_at_start/250)+2) eller en deadline ville lukke det. Den gamle implementering kunne ikke udvise dette. (målt)
  3. maintenance.go:66-68 dokumenterer en kontrakt uden aftager. Returværdien er korrekt (målt: total=500 med præcis 500 rækker væk og den fejlede batch rullet helt tilbage), men begge kaldere kasserer den — main.go:278 og main.go:350 gør if n, err := …; err != nil { log… } else if n > 0 { … }, så n er uopnåelig i fejlgrenen. Værre: RunIncrementalVacuum springes over ved fejl, selv om committede batches frigav sider. (tracet + målt)
  4. Begrundelsen gælder ikke startup-kalderen. main.go:278 kører før ingestBuffer.Ready(), og bufferen drænes først efter Ready(). Under startup-prunen forbruger intet bufferen, så det køber intet der — og +10 % wall-tid øger marginalt drop-presset. Gevinsten er reel for den daglige ticker (main.go:350). (tracet)

Værste tilfælde: observation-sletningen er ikke bundet af 250

scenarie obs slettet i én tx writerMu-hold
250 tx × 16 obs (kommentarens eksempel) 4.000 11,9 ms
250 tx × 200 obs 50.000 164,6 ms
250-tx batch med ÉN tx med 60.000 obs 60.249 218,1 ms

Lineært ved ~3,3–3,6 µs pr. slettet række, så hold ≈ 3,4 µs × 250 × obs-pr-transmission. Et mesh med 1000 obs/transmission ville se ~850 ms hold ved samme batchstørrelse. Kommentaren på maintenance.go:24-28 siger dette korrekt — flagget her som måling, ikke som fejl.

Pre-eksisterende -race-fejl i cmd/ingestor — root-cause fundet

go test -race ./... i cmd/ingestor fejler på denne branch, men fejlen er pre-eksisterende og urelateret: diffen er præcis maintenance.go + prune_chunked_test.go. Revieweren isolerede årsagen:

StartStatsFileWriter (stats_file.go:239) starter en goroutine med for range t.C og ingen stop-mekanisme. To tests starter den (stats_file_test.go:59, stats_file_timestamp_test.go:72). Den lækkede goroutine læser readProcSelfIOFn (stats_file.go:263) hvert 50. ms for evigt og kolliderer med t.Cleanup-gendannelsen i stats_file_timestamp_test.go:52 — hvorefter racen tilskrives den test, der tilfældigvis kører bagefter. Det forklarer, hvorfor den har vist sig som TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestamp på tværs af #27, #28 og #39.

Reproduceret isoleret med go test -race -run TestStatsFileWriter uden prune-test involveret. Jeg laver en separat minimal test-fix-PR til dette, da den blokerer flere PR'er i køen.

Målrettet kørsel her: go test -race -run 'TestPruneOldPackets|TestPruneAgedTransmissionIDs' — alle 6 af PR'ens tests plus 9 af reviewerens concurrency-/differentialtests består, nul race-advarsler. Ingen eksisterende assertion er ændret.

Hvorfor denne PR er PARKERET, ikke merget

Opgavens egne regler kræver staging før merge for "database/pruning/retention-ændringer", og netop denne PR's berettigelse er en påstand om live drift. Staging kan ikke nås fra den maskine, arbejdet kører på: docker-dæmonen kører ikke, ~/meshcore-staging-data findes ikke, der er ingen ~/.ssh/config og ingen remote docker-context. docker-compose.staging.yml beskriver "the internal staging VM" med en mqtt-broker provisioneret out-of-band på meshcore-net, og deploy-jobbet kører på den self-hostede runner [self-hosted, meshcore-runner-2], som er fork-guarded fra her. Docker blev bevidst ikke startet: containerne har restart: unless-stopped, så en dæmonstart kunne rejse en ingestor, der forbinder til en live broker.

Alt andet er gjort. Branchen er synkroniseret, bygger, vetter og har grønne målrettede -race-tests. Det udestående er staging-observationen — konkret testplan ligger i STAGING-TESTPLAN.md i repoet under "#49".

De fire SHOULD-FIX-punkter er bevidst ikke rettet: de ændrer porten væk fra upstream #2000 og koster paritet for fremtidige ports. Punkt 1 og 3 er rene kommentar-/logrettelser, der kan tages i en selvstændig opfølgning.

🤖 Generated with Claude Code

@dborup
dborup merged commit 3345b6b into master Sep 22, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants