Fix RootMountTotal using filesystem-reserved blocks in retention limit#84
Open
AnIrishDuck wants to merge 9 commits into
Open
Fix RootMountTotal using filesystem-reserved blocks in retention limit#84AnIrishDuck wants to merge 9 commits into
AnIrishDuck wants to merge 9 commits into
Conversation
The buildjet-4vcpu-ubuntu-2204-arm runner is not available on this platform. Switch the build-multi-arm64 job to the 4x16g-arm runner label. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01NJLaAVqqa6kRKwz4W9CRGx
systemstat's `total` includes ~5% of blocks reserved for root (standard ext4 behavior), which are never usable by plateau. This caused the retention limit to be set ~7GB above the actual usable capacity on a 98GB disk, so retention never fired and the storage monitor blocked writes instead. Fix: compute usable capacity as avail + used (= total - reserved_blocks), matching what `df` reports as the true capacity available to non-root processes. Co-Authored-By: Claude <[email protected]>
AnIrishDuck
force-pushed
the
fix/retention-reserved-blocks
branch
from
72424b4 to
afebe9a
Compare
sealed_ix was always initialized to None, so reconcile treated every manifest segment as active (never sealed) until a roll happened in the current process lifetime. On a freshly restarted server, this meant UpdateManifestSizes never fired — all size mismatches were silently skipped. find_starting_index always advances to max_manifest_ix+1 on attach, so every manifest segment including the max is definitionally sealed. Initialize sealed_ix to segment.prev() (= max_manifest_ix, or None for a fresh partition) so reconcile can fix stale sizes on the first pass after startup. Co-Authored-By: Claude <[email protected]>
The active-chunk cache (.arrows) can persist across a crash — it holds rows not yet flushed to the main segment file and is read back by Segment::iter() on the next open. It is only safe to remove after Writer::end() flushes its contents into the main file. Previously size_estimate() excluded the cache, so any segment that survived a crash with a live .arrows file was under-counted in the manifest (and therefore in retention's accounting). On a busy cluster these orphaned cache files accumulated to several GiB of invisible disk usage. Fix: include cache_path() in Segment::size_estimate(). Remove the now- redundant cache.size() addition in Writer::size_estimate() (cache.size() also reads the same .arrows file via fs::metadata, so it would have double-counted). Segment::destroy() already removes cache_path(), so retention cleans it up automatically. Co-Authored-By: Claude <[email protected]>
segment_disk_size() added cache_path() to tracked_files (so orphan detection didn't flag it) but excluded it from the size total via an explicit guard. This meant UpdateManifestSizes never updated manifest entries to reflect the cache bytes, leaving existing rows under-counted even after the size_estimate() fix. Remove the guard so the cache file is counted the same as other parts. For sealed segments this corrects the manifest size; for active segments it gives a more accurate informational delta (process_active_segment never mutates the manifest anyway). Co-Authored-By: Claude <[email protected]>
Ongoing retention removes a segment's manifest entry before destroying its backing data. When the process starts against an already-full disk that order cannot make progress: deleting the manifest row is a SQLite write that itself needs free space to journal, so it panics with "database or disk is full" before any space is reclaimed. Add Catalog::reclaim, a one-shot check run before the steady-state checkpoint/retention loops. When over the limit it removes the oldest segment data-first (destroy backing data, then remove the manifest entry), freeing space so the regular retention loop can take over. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01Jkk2NoeuqoKS8XsxZSu7gq
Introduce `max_active_partitions` config (default 4096) enforced alongside the existing `max_partition_bytes` limit in `prune_topics`. When the number of active (in-memory, writable) partitions across the catalog exceeds the limit, the oldest active partitions are closed until the count is back within bounds, mirroring the byte-based pruning. Also fix the pruning `ages` map, which was keyed solely on segment end time: partitions sharing an end time collided and overwrote each other, hiding them from both the byte and active-count limits. The key now includes the topic and partition names as tiebreakers. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01Dcvvv5f1pihsE6gDaETRid
Reconcile classified segments using the in-memory `sealed_ix` watermark, reading it via `get_topic`/`get_partition` — which lazily *loads* the partition. A freshly loaded partition reports `sealed_ix == None`, so every one of its on-disk segments fell into the informational "active" bucket. This inflated the reported `active_segments` count: each cold or evicted partition contributed all of its segments, even though a closed partition's tail is immutable (a reopen begins a brand-new segment at `max_segment + 1`, so nothing ever appends to the persisted tail again). Classify each partition without loading it: a non-resident partition is quiescent and all its persisted segments are sealed, while a resident partition keeps the conservative watermark semantics so its live active tail (and any in-flight roll) still lands in the active bucket. Add a non-loading `Catalog::resident_sealed_ix` / `Topic::resident_sealed_ix` lookup and a `SealStatus` classification for the pass. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01Dcvvv5f1pihsE6gDaETRid
Collaborator
Author
|
This turned out to be a mirage and totally incorrect, at least when it comes to the issues we were seeing. It is likely still worth investigating briefly in case the theorized issue could actually bite us. |
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
2 participants
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.
I am still verifying this. Claude's explanation so far follows and does make sense from the code I've read, but this has bitten us now so I want to be confident:
systemstat's
totalincludes ~5% of blocks reserved for root (standard ext4 behavior), which are never usable by plateau. This caused the retention limit to be set ~7GB above the actual usable capacity on a 98GB disk, so retention never fired and the storage monitor blocked writes instead.Fix: compute usable capacity as avail + used (= total - reserved_blocks), matching what
dfreports as the true capacity available to non-root processes.