fix(engine): idle-age flush — a small memtable reaches a segment (#873) - #878
Merged
Conversation
`needs_flush` was threshold-only (doc count / bytes), so a dataset below the thresholds NEVER flushed: its documents stayed memtable-resident for the process lifetime, pinned their WAL generations on disk, and replayed on every boot. Measured on the 15-repo autoindex corpus: a 100,001-doc dataset with zero segments 30+ minutes after ingest, its entire content held in RAM on an idle node — the largest single term in the #873 idle-RSS floor and the reason a quiet corpus still pays O(corpus) WAL replay at startup. New `storage.flush_idle_secs` (default 300 s, 0 disables): a non-empty memtable whose contents have not changed for that long flushes regardless of size. Detection is a fingerprint probe piggybacked on the existing periodic flusher (`flush_interval_secs`, 30 s) — the write hot path is untouched, and every write shape (index, bulk, delete, update) re-arms the timer because they all move the (doc_count, bytes) fingerprint. Worst-case flush latency is flush_idle_secs + flush_interval_secs. A fingerprint collision (delete+insert of identical sizes inside one probe window) at worst flushes an active index once, which is always a safe operation. Precedent (approach only; AGPL, no code taken): Elasticsearch flushes a shard idle past `indices.memory.shard_inactive_time` (default 5 m) from a periodic controller — IndexingMemoryController.java:76-78 driving IndexShard.flushOnIdle (IndexShard.java:2787-2790), which compares now - lastWriteNanos >= inactiveTime. Same shape here: a controller probe decides inactivity; 300 s matches their default. The probe clock is process-monotonic, based a day up so the test hook can rewind stamps near process start (a 0-based clock left no room below the first probe and silently pinned the stamp at the saturating floor — caught by the new test's first run). Test `small_idle_memtable_flushes_by_age_not_size`: below-threshold doc does not flush unaged, flushes once aged past flush_idle_secs (crossed via a rewind hook, no sleeps), a new write re-arms the timer, flush drains, an empty memtable never idle-flushes, and flush_idle_secs=0 restores the old behaviour exactly.
`count_user_facing_settings` pins every published settings total, and adding `storage.flush_idle_secs` moved storage 10 -> 11 and the total 116 -> 117. The test names each place the number is quoted; all are updated: the config module header, the sub-config comment block, the StorageConfig doc comment, SETTINGS_BY_SECTION, the in-test assertion, and EXPECTED_SETTINGS in xerj-engine/tests/product_experience.rs. The shipped xerj.default.toml now documents the setting rather than only counting it — a durability knob a user can reach for is worth explaining where they will look for it, so the file sets 55 of 117 and its Storage section header reads 6 of 11.
Review of #878 found the state machine had a stable failure mode, and reproducing it showed the suggested one-line fix was not sufficient. The defect: after a flush drained the memtable, `idle_probe_fingerprint` / `idle_probe_at_ms` kept describing the memtable that no longer existed. A later write reproducing the same (doc_count, bytes) pair — a uniform trickle of fixed-shape records is the ordinary case, not a contrived one — matched the stale fingerprint, so the re-arm was skipped and the comparison ran against a stamp already older than `flush_idle_secs`. `needs_flush` returned true on the very next probe, drained, and returned to the same state: a stable loop flushing every `flush_interval_secs` (30 s) instead of every `flush_idle_secs` (300 s), on a node the operator believes is idle — a `save_hnsw_to_disk`, a `force_wal_maintenance` and a full `query_cache.clear()` each time. That is exactly the background churn the #871-#876 campaign exists to remove, so shipping it would have undercut its own release. The review proposed clearing the pair in the `docs == 0` arm of `needs_flush`. I wrote the regression test first, and it still failed with that in place: the lazy reset only fires if a probe happens to land while the memtable is empty, and nothing schedules a probe between a flush and the next write — a write arriving first inherits the stale pair anyway. The reset has to happen where the drain does, so it now sits at the top of `Index::flush`, the one coordinator every flush path funnels through (user `_flush`, the periodic sweep, a threshold-driven flush, shutdown). The `docs == 0` arm keeps its clear as well: harmless, and correct on its own terms. Also corrects my comment on the arming condition. "Arming needs no at==0 sentinel because fp is never 0" is true only for the FIRST arm; after a flush `seen` holds a stale non-zero fingerprint a later state can coincide with, which is the whole defect. Test proof, both ways: with the flush-site reset removed the new arm fails on "a recurring fingerprint after a flush must arm a FRESH timer"; with it, the suite passes. The earlier version of this test passed on unfixed code — it wrote two docs before the flush, so the post-flush single doc could never collide with that fingerprint. It now uses its own index and one doc, which is what makes the collision reachable.
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
1 participant
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.
needs_flushwas threshold-only (doc count / bytes), so a dataset below thethresholds NEVER flushed: its documents stayed memtable-resident for the
process lifetime, pinned their WAL generations on disk, and replayed on
every boot. Measured on the 15-repo autoindex corpus: a 100,001-doc dataset
with zero segments 30+ minutes after ingest, its entire content held in RAM
on an idle node — the largest single term in the #873 idle-RSS floor and
the reason a quiet corpus still pays O(corpus) WAL replay at startup.
New
storage.flush_idle_secs(default 300 s, 0 disables): a non-emptymemtable whose contents have not changed for that long flushes regardless
of size. Detection is a fingerprint probe piggybacked on the existing
periodic flusher (
flush_interval_secs, 30 s) — the write hot path isuntouched, and every write shape (index, bulk, delete, update) re-arms the
timer because they all move the (doc_count, bytes) fingerprint. Worst-case
flush latency is flush_idle_secs + flush_interval_secs. A fingerprint
collision (delete+insert of identical sizes inside one probe window) at
worst flushes an active index once, which is always a safe operation.
Precedent (approach only; AGPL, no code taken): Elasticsearch flushes a
shard idle past
indices.memory.shard_inactive_time(default 5 m) from aperiodic controller — IndexingMemoryController.java:76-78 driving
IndexShard.flushOnIdle (IndexShard.java:2787-2790), which compares
now - lastWriteNanos >= inactiveTime. Same shape here: a controller probe
decides inactivity; 300 s matches their default.
The probe clock is process-monotonic, based a day up so the test hook can
rewind stamps near process start (a 0-based clock left no room below the
first probe and silently pinned the stamp at the saturating floor — caught
by the new test's first run).
Test
small_idle_memtable_flushes_by_age_not_size: below-threshold docdoes not flush unaged, flushes once aged past flush_idle_secs (crossed via
a rewind hook, no sleeps), a new write re-arms the timer, flush drains, an
empty memtable never idle-flushes, and flush_idle_secs=0 restores the old
behaviour exactly.
Part of #873 (the idle-age lever; adaptive WAL shards and cold-index state remain open on the issue). Part of the #874 budget.
🤖 Generated with Claude Code