fix(governor): prune mode_transitions, which grew without bound - #905
Open
minion1227 wants to merge 1 commit into
Open
fix(governor): prune mode_transitions, which grew without bound#905minion1227 wants to merge 1 commit into
minion1227 wants to merge 1 commit into
Conversation
`Store` documents "Older rows pruned on startup and every hour", but `prune`
only ever deleted from `tegrastats`. `mode_transitions` is created, indexed and
inserted into and nothing in the crate deletes from it, so a long-running device
accumulated one row per mode transition for the life of the install.
Demonstrated against the current code: insert a transition dated a year back,
call `prune`, and it is still there —
MAIN: prune() deleted=0 rows; 1-YEAR-OLD mode_transitions still present = 1
Typical volume is low (a couple of day/night flips plus media start/stop), but it
is not inherently bounded: `transition` is called from the 5-second tick whenever
the target mode differs, so memory-pressure flapping around `stop_optins_mb` can
record a row per tick — the same worst-case rate as the sample table.
Prune both tables, with the retention windows named rather than inlined.
Transitions keep 30 days instead of 24 hours: they arrive orders of magnitude
more slowly than samples and the history is what makes flapping diagnosable
after the fact, so a longer bounded window costs little.
`prune` now returns per-table counts (`PruneCounts`) instead of a single number,
so the hourly log line says which table shed rows. Both existing callers discard
the value, so nothing else changes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesStore retention pruning
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Store's doc comment says "Older rows pruned on startup and every hour", butpruneonly ever deleted fromtegrastats.mode_transitionsis created, indexed and inserted into — and nothing in the crate deletes from it — so a long-running device accumulates one row per mode transition for the life of the install.Changes
mode_transitionsas well astegrastats.TEGRASTATS_RETENTION_MS,MODE_TRANSITION_RETENTION_MS) instead of inlining24 * 3600 * 1000.prunereturns per-table counts (PruneCounts) rather than one number, so the hourly debug line says which table shed rows. Both existing callers discard the return value, so nothing else changes.Why 30 days for transitions rather than 24 hours
Transitions arrive orders of magnitude more slowly than samples — a couple of day/night flips plus media start/stop, versus ~17,280 samples/day — and the history is exactly what makes mode flapping diagnosable after the fact. A longer bounded window costs very little. This is a judgement call, so it is a named constant: say the word and I'll align it with the 24-hour sample window instead.
The growth is not inherently bounded
transitionis called from the 5-second tick whenever the target mode differs from the current one. Memory-pressure flapping aroundstop_optins_mbcan therefore record a row per tick — the same worst-case rate as the sample table, which is pruned. So this isn't only a slow-accumulation concern.Real Behavior Proof
Tested profile / hardware (check all that apply):
jetsonraspberry_piportable_sbclaptopmacWhat I ran
Linux x86_64 laptop. This is SQLite retention logic with no device dependency, so the laptop exercises it fully. I drove it through a temporary test against a real on-disk SQLite file — first on unmodified
mainto confirm the leak, then on this branch to confirm the fix — then removed the temporary test (see the note at the bottom).What I observed
Against unmodified
main— insert a transition dated one year back, callprune, count what survives:prunereports zero deletions and the row is still there. That is the leak.On this branch — one 25-hour-old sample, one 31-day-old transition, one recent row of each:
The stale transition is removed, the recent one survives, and the sample table behaves as before.
Gates:
cargo clippyclean under-D warningson both the workspace and--no-default-featuresaxes;cargo fmt --all -- --checkclean;cargo test -p genie-governor25 passed;cargo test --workspacegreen.One pre-existing flake worth reporting (unrelated to this change — it lives in
genie-common, which this PR does not touch):tegrastats::tests::mem_available_mb_async_matches_sync_versionreads/proc/meminfotwice and asserts the two readings are equal, so it fails wheneverMemAvailableshifts between them. I measured it failing 1 run in 5 in isolation on both this branch and cleanmain. Happy to fix it separately — comparing two independent reads of live kernel state can't be made reliable, so it probably wants to assert both areOkrather than equal.Test plan
cargo test -p genie-governorTo see the leak on current
main: insert a row intomode_transitionswith an oldts_ms, callStore::prune(), and observe the row survives whileprunereturns 0.Notes for reviewers
genie-governoris bin-only — no[lib]target and notests/directory — so a permanent test would have to be an inline#[cfg(test)]module instore.rs. I verified the behaviour with exactly such a module and then removed it, pasting its output above, rather than leave the crate's layout half-changed. If you'd prefer the regression pinned, I'm happy to either add the inline module or add a[lib]target plustests/store_test.rs— just say which.PruneCountsispubbecauseStoreis; it derivesCopy/Default/Eqso callers can compare or accumulate without ceremony.DELETEs as separate literal statements rather than a table-name-parameterised helper — dynamic SQL here would buy nothing and reads worse.Summary by CodeRabbit
New Features
Bug Fixes