Skip to content

port(upstream#1886): opt-in observerPurgeDays hard-delete for long-inactive observers - #34

Closed
adminopenclaw8-sketch wants to merge 2 commits into
masterfrom
codex/port-upstream-1886-observer-purge-days
Closed

adminopenclaw8-sketch wants to merge 2 commits into
masterfrom
codex/port-upstream-1886-observer-purge-days

Conversation

@adminopenclaw8-sketch

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

Copy link
Copy Markdown
Collaborator

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

Upstream

Problem

RemoveStaleObservers only soft-deletes (inactive = 1), so observer rows accumulate forever with no way to reclaim them.

Change

New retention.observerPurgeDays (default 0 = disabled). When set, PurgeStaleObservers hard-deletes observers that are already inactive = 1, not seen for N days, and not referenced by any observations, observer_metrics or dropped_packets row. The reference guards matter because observations.observer_idx has no foreign key. It runs after the soft-delete at startup and on the observer retention ticker. The server's read-only invariant test now also asserts the server's *DB has no PurgeStaleObservers.

Adaptation to this fork

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

Notes for review

Opt-in; nothing happens until an operator sets the value (it should be above both observerDays and packetDays).

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 TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestamp, TestPruneOldNeighborMetrics baseline failure, unchanged
go-server-build-vet PASS PASS
go-server-test-race PASS PASS
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 34750395843 on d33dd6fe. Go Build & Test: failure; all downstream jobs incl. Deploy Staging skipped. Failed tests:

  • TestPruneOldNeighborMetrics: fails on master, documented baseline

🤖 Generated with Claude Code

efiten and others added 2 commits September 13, 2026 10:42
…observers (Kpa-clawbot#1886)

## Problem

`RemoveStaleObservers` only soft-deletes — it sets `inactive = 1` and
the row stays forever. On a long-running deployment those rows just
accumulate: on a two-year-old instance roughly 25% of the `observers`
table was rows nobody can ever see again.

There is currently no way to reclaim them.

## Fix

A second retention stage. `PurgeStaleObservers` hard-deletes rows that
are:

- already `inactive = 1` (so the soft-delete stage owns the decision of
*when* an observer goes stale), **and**
- older than `retention.observerPurgeDays`, **and**
- referenced by nothing.

New config field `retention.observerPurgeDays`, default `0` = disabled.
Existing deployments are unaffected until they opt in. Set it above both
`observerDays` and `packetDays` — below those the reference guards keep
every candidate row anyway.

## Why the reference guards are the point

`observations.observer_idx` is a bare rowid with no foreign key.
Deleting a still-referenced observer silently orphans history —
`packets_v` stops resolving the observer and those packets get
mis-attributed. Nothing errors; the data just quietly goes wrong.

So the statement guards on all three referencing tables:

```sql
AND NOT EXISTS (SELECT 1 FROM observations o     WHERE o.observer_idx = observers.rowid)
AND NOT EXISTS (SELECT 1 FROM observer_metrics m WHERE m.observer_id  = observers.id)
AND NOT EXISTS (SELECT 1 FROM dropped_packets d  WHERE d.observer_id  = observers.id)
```

This is correctness, not defensive padding — it was found the hard way,
by orphaning 280 observation rows during a manual purge that skipped one
of these checks. Each guard has its own test.

## Performance

Each `NOT EXISTS` is an index seek per candidate row
(`idx_observations_observer_idx`, `idx_dropped_observer`, the
`observer_metrics` PK), and `observers` is O(100). It runs on the
existing daily retention tick alongside `RemoveStaleObservers`, never on
the ingest path.

## Tests

Eight tests in `cmd/ingestor/observer_purge_test.go`, written before the
implementation:

- deletes an unreferenced stale row
- keeps a row referenced by `observations` — and asserts zero orphans
afterwards
- keeps a row referenced by `observer_metrics`
- keeps a row referenced by `dropped_packets`
- keeps a row that is old enough but still `inactive = 0`
- keeps a row inside the retention window
- no-ops when disabled (`0` and `-1`)
- config accessor table test

## Invariant

Writes stay in `cmd/ingestor` per Kpa-clawbot#1283.
`cmd/server/readonly_invariant_test.go` now also forbids
`PurgeStaleObservers` as a method on the server's `*DB`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit d821d9a)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dborup

dborup commented Sep 21, 2026

Copy link
Copy Markdown
Owner

Verificeret lokalt — og et hul i slettekontrakten er bekræftet ved eksekvering

Branchen er synkroniseret med master 96319acc (almindelig merge-commit 84e6aef1, ingen rebase/squash/force-push). go build, go vet og pakkens tests er grønne i både cmd/ingestor og cmd/server.

Slettekontrakten, som den faktisk er

PurgeStaleObservers garderer mod tre tabeller:

AND NOT EXISTS (SELECT 1 FROM observations       o WHERE o.observer_idx = observers.rowid)
AND NOT EXISTS (SELECT 1 FROM observer_metrics   m WHERE m.observer_id  = observers.id)
AND NOT EXISTS (SELECT 1 FROM dropped_packets    d WHERE d.observer_id  = observers.id)

Men der findes to tabeller mere på observer_id, som ikke er dækket: observer_neighbors og observer_neighbor_metrics. Jeg testede det direkte — såede en soft-deleted observer med én række i hver, kørte purgen:

PURGE RESULT: purged=1  observers_left=0  observer_neighbors_left=1  observer_neighbor_metrics_left=1
GAP CONFIRMED: observer row deleted but 1 neighbor + 1 metric rows are now ORPHANED

Konsekvensen er synlig i API'et, ikke kun i databasen:

  • GetObserverNeighbors (cmd/server/db.go:1541) er FROM observer_neighbors on2 LEFT JOIN nodes … WHERE on2.observer_id = ? — den joiner slet ikke til observers. "Direct Neighbors"-panelet vil derfor servere en purget observers naboer i al evighed.
  • GetAllObserverNeighbors (cmd/server/db.go:1610) bruger LEFT JOIN observers — rækken forsvinder altså ikke, den vises bare med tomme observer-felter.

Og rækkerne ryddes ikke op ad anden vej: observer_neighbors er en current-only snapshot, som ReplaceObserverNeighbors sletter og genindsætter når observeren rapporterer igen — og en purget, død observer rapporterer aldrig igen. observer_neighbor_metrics beskæres kun efter alder (30 dage), ikke efter observer-eksistens.

Docstringen siger, at de tre garder "are what make the delete safe; they are correctness, not defensive padding". Det er rigtigt for de tre tabeller, de dækker — men sætningen læses let som at kontrakten er komplet, og det er den ikke.

Dette kræver en produktbeslutning før merge, og det er derfor PR'en ikke bare venter på staging:

  1. udvid garderne til også at kræve NOT EXISTS på de to nabotabeller (konservativt — så bliver observers med naborapporter aldrig purget), eller
  2. slet de to tabellers rækker sammen med observeren i samme transaktion (fuldfører oprydningen), eller
  3. accepter og dokumentér adfærden eksplicit i docstringen, så den næste læser ikke tror kontrakten er udtømmende.

Jeg har ikke valgt for dig — det ændrer, hvad "hard-delete" betyder.

Andre forhold, jeg har kontrolleret

  • Default off er intakt. observerPurgeDays er 0 i config.example.json, og PurgeStaleObservers returnerer 0, nil med det samme ved purgeDays <= 0. Hele sikkerhedsmodellen hviler på det, og det skal verificeres igen på staging før funktionen slås til.
  • rowid-genbrug er en reel, men afgrænset risiko. observers har id TEXT PRIMARY KEY, så rowid er implicit og ikke AUTOINCREMENT — SQLite tildeler max(rowid)+1, og en slettet observers rowid kan derfor genbruges. Garderne sikrer, at ingen observations peger på rowid'en på slettetidspunktet, så der er ingen eksisterende historik at fejltilskrive; men det bør verificeres eksplicit på staging (slet observeren med højeste rowid, indsæt en ny, bekræft at ingen historik skifter ejer).
  • observer_idx caches ikke. Den slås op friskt pr. insert (db.go:1177 via stmtGetObserverRowid), og både opslag og insert sker under writerMu i samme funktion — så der er ikke et vindue mellem opslag og brug.

Hvorfor PARKERET

Ud over produktbeslutningen ovenfor kræver opgavens regler staging før merge for retention-/sletteændringer, og udtrykkeligt med syntetiske data, aldrig produktionsdata, plus verificeret database-backup og isoleret mutationstest. Staging kan ikke nås herfra: docker-dæmonen kører ikke, ~/meshcore-staging-data findes ikke, ingen ~/.ssh/config, ingen remote docker-context, og deploy-jobbet kører på den self-hostede runner [self-hosted, meshcore-runner-2], som er fork-guarded fra. Docker blev bevidst ikke startet.

Konkret testplan ligger i STAGING-TESTPLAN.md under "#34" — inklusive det nu bekræftede hul som eksplicit afklaringspunkt.

🤖 Generated with Claude Code

@dborup

dborup commented Oct 7, 2026

Copy link
Copy Markdown
Owner

Keeping this open as input for #329 (opt-in retention for soft-deleted observers and the other never-pruned tables). #329 will build on this port against current master. This PR will be closed when #329 lands.

@dborup

dborup commented Oct 7, 2026

Copy link
Copy Markdown
Owner

Closing as superseded: the soft-deleted observer retention from this port landed via #350 (Relates to #329), rebuilt on current master.

@dborup dborup closed this Oct 7, 2026
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