Bound the node record table by row count, not just age (GH-3701) - #3734
Merged
Conversation
…H-3701) `wolverine_node_records` is append-only diagnostics -- no foreign keys, and the only reader is `FetchRecentRecordsAsync`, which nothing asks for more than ~100 rows from. Two things left it effectively unbounded: 1. `INodeAgentPersistence.DeleteOldNodeRecordsAsync` was implemented for the relational stores but had no call site anywhere outside tests. 2. The pruning that *did* run, `DeleteOldNodeEventRecords`, bounds the table by age only (`NodeEventRecordExpirationTime`, 5 days). Age is no ceiling at all when the write rate is high: one `AssignmentChanged` row per agent per assignment decision on a cluster with thousands of distributed agents is millions of rows a day, all of them inside the window. The reporting cluster reached 36,135,221 rows / 16 GB in five days -- 15 GB of heap on a table nothing on the hot path reads, plus the insert and WAL load, on the main Wolverine database, while the cluster was already unhealthy for an unrelated reason (GH-3698). Adds `Durability.NodeRecordRetention` (default 10,000 rows; zero or negative keeps the old age-only behavior) applied by a new `TrimNodeRecordsCommand`, and moves both deletes onto a dedicated `Durability.NodeRecordPruningPeriod` timer (default hourly) against the `Main` store. That timer also fixes a second defect found on the way in. The age sweep was appended to `buildOperationBatch` behind an `isTimeToPruneNodeEventRecords()` guard whose backing field `_lastNodeRecordPruneTime` was never assigned -- the `#pragma warning disable CS0649` sitting on it said as much -- so the intended hourly throttle never engaged and the delete went out on *every* recovery cycle, i.e. every 5 seconds by default. On the reporting cluster that was a full scan of a 16 GB table every five seconds. Also: - `MultiTenantedMessageStore` inherited the interface's no-op default for `DeleteOldNodeRecordsAsync`, so a multi-tenanted store would never have trimmed even with a call site. It now delegates to `Main.Nodes`, matching `LogRecordsAsync`/`FetchRecentRecordsAsync` -- and that shape (one main store, hundreds of tenant databases) is exactly where the report came from. - Implements the row cap for Sqlite, MySQL, and Oracle, which had also been falling through to the no-op default. MySQL rejects a LIMIT inside a subquery over the table being deleted, and Oracle pays for a NOT IN over a FETCH FIRST subquery, so both resolve a floor id first and delete as a primary-key range scan. Tests: a Postgres integration test asserts a *running host* drains a seeded table down to the cap with no test-side call to the persistence method (red baseline: 54 rows remain), plus store-level retention tests for Sqlite, MySQL, and Oracle mirroring the existing Postgres and SQL Server ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…GH-3701) Two defects in the new `OracleTests.Agents.delete_old_node_records`, both caught by CIOracle: 1. `ORA-01745: invalid host/bind variable name`. The insert bound `:number` and `:description`; NUMBER is an Oracle reserved word, so a bind variable named after one is rejected outright. Prefixed to `:p_number` / `:p_event` / `:p_description`. 2. `Admin.RebuildAsync()` in `InitializeAsync` took out two sibling tests (`Transport.clear_all_wolverine_storage`). In Oracle a schema IS a user, so every test in the "oracle" collection shares WOLVERINE -- rebuilding it drops the tables out from under whatever runs next. Migrates instead, and clears only the one table this class owns, on the way in and on the way out. Verified against a real Oracle 23 container: the class passes 6/6 and the full OracleTests suite is 113/113, where CI was 108/113. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
This was referenced Jul 30, 2026
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.
Closes #3701.
The report
wolverine_node_recordsreached 36,135,221 rows / 16 GB (15 GB heap + 867 MB index) in five days on a production cluster — a diagnostic table with no foreign keys whose only reader isFetchRecentRecordsAsync, and which nothing in Wolverine ever asks for more than ~100 rows from.Two defects
1. The row cap had no call site.
INodeAgentPersistence.DeleteOldNodeRecordsAsync(int retainCount)was implemented for the relational stores, tested, and never invoked outside those tests — exactly as reported.2. The pruning that did run only bounds by age.
DeleteOldNodeEventRecordsdeletes againstNodeEventRecordExpirationTime(5 days). Age is no ceiling at all when the write rate is high: oneAssignmentChangedrow per agent per assignment decision, on a cluster with thousands of distributed agents, is millions of rows a day — all of them inside the window. Every one of those 36M rows was younger than the cutoff.And a third, found on the way in
The age sweep was appended to
buildOperationBatchbehind this guard:The suppression says it outright — the field is never assigned, so
isTimeToPruneNodeEventRecords()always returnedtrueand the intended hourly throttle never engaged. The delete went out on every recovery cycle, i.e. every 5 seconds by default. On the reporting cluster that is a full scan of a 16 GB table every five seconds, on the main Wolverine database, while it was already unhealthy for an unrelated reason (#3698).The fix
Durability.NodeRecordRetention— a hard row cap, default 10,000, applied by a newTrimNodeRecordsCommand. Zero or negative keeps the old age-only behavior.Durability.NodeRecordPruningPeriod— default hourly. Both deletes now run on a dedicated timer against theMainstore instead of riding the 5-second recovery batch, which is what the dead throttle was supposed to achieve.MultiTenantedMessageStorenow delegatesDeleteOldNodeRecordsAsynctoMain.Nodes, matchingLogRecordsAsync/FetchRecentRecordsAsync. It was inheriting the interface's no-op default, so a multi-tenanted store would never have trimmed even once a call site existed — and one main store with hundreds of tenant databases is precisely the reporter's shape.LIMITinside a subquery over the table being deleted, and Oracle pays for aNOT INover aFETCH FIRSTsubquery, so both resolve a floor id first and delete as a primary-key range scan.On the write volume itself
The reporter offered to split off the "one
AssignmentChangedrow per agent per decision is very fine-grained" point. That is a fair separate issue and this PR does not touch it — but with a row cap in place, that volume is now a bounded cost rather than an unbounded one.Tests
PostgresqlTests/Agents/node_record_retention.cs— seeds 50 rows, starts a real host, and asserts the table drains to the cap with no test-side call toDeleteOldNodeRecordsAsync. Red baseline verified: 54 rows remain without the fix.wolverine.slnxRelease build clean.🤖 Generated with Claude Code