Compaction records what it achieved, so a frozen partition can thaw (#723)

[philcunliffe]

Compaction now records what it achieved, so a partition it could not shrink is skipped for a stated reason rather than by accident, and can thaw when the writer improves.

The bug

Three pieces interacted in src/core/cache/maintenance.js:

1. needsCompaction() is permanently true for a fragmented partition (1521 files vs a limit of 32; 214 KB avg vs a 32 MB target).
2. Compaction could not reduce the file count, because it emitted roughly one file per compact_batch_bytes of in-memory rows and recorded LLM rows compress heavily.
3. The baseline gate then closed: grewSinceCompaction compares the current file count against the recorded baseline, so 1521 == 1521 read as "nothing changed, skip" - forever, no matter how fragmented.

The gate's question was "did the file count change?" and never "did the last compaction actually reduce anything?" A compaction that achieved nothing was indistinguishable from one that achieved everything.

The fix, option 1 only

compactGeneration counts the pre-rewrite files itself and writes dataFilesBefore and writerGeneration into cursor.compaction alongside the existing resettleBaselineFiles. Three small predicates read that record:

• compactionReducedFiles(cursor) - did the last rewrite strictly reduce the count? (undefined for pre-fix cursors and for empty partitions, which had nothing to reduce.)
• compactionKnownIneffective(cursor) - recorded as no reduction by the writer running now; drives the reported skip reason.
• compactionVerdictStale(cursor) - recorded as not a reduction and stamped with a different writer generation. This is the only thing that reopens the gate: compactionDue = force || ((grew || verdictStale) && needsCompaction(...)).

COMPACTION_WRITER_GENERATION = 2 (1 = one file per flushed batch, pre-LLP-0209; 2 = LLP 0209's streaming writer). That is the "re-tried when the writer improves" mechanism the issue asked for, and it matters immediately: LLP 0209 already landed the writer fix on master, so every partition frozen by the old writer is currently unreachable forever. The retry re-stamps the cursor, so it happens once per writer generation, never once per tick.

LLP 0199's convergence property is preserved: a rewrite recorded as effective is never retried on this path regardless of stamp.

rebaselineCursor (LLP 0207 foreign sorted replace) also writes the stamp and drops any stale verdict, otherwise a recognized partition would read as owing a retry and pay a metadata load plus a cursor write every tick, forever.

hyp query maintain now prints the reason instead of a silent "0 partitions compacted".

Evidence

Regression tests written first, run against unmodified source (independently re-derived by the reconciler): 3 fail, 3 pass after.

not ok 1 - a compaction that cannot reduce the file count records that it did not
+ actual: undefined - expected: true
not ok 2 - a partition frozen by an ineffective compaction is retried once under a new writer
0 !== 1
not ok 3 - a compaction that did reduce the file count is never retried
0 !== 1 (the control arm only)

Test 1 gets past dataFilesBefore === 8 / dataFilesAfter === 8 before failing, which proves the fixture really is a partition compaction cannot shrink (8 identity-partitioned tuples, one file each - LLP 0209 #tuple-bound makes one file per tuple the floor; the production 1,521-file case is the same shape at scale).

Anti-regression is covered twice, since the gate being modified exists to prevent a rewrite-forever loop: test 2 runs two further ticks after the thaw and asserts totalCompacted === 0 each time, and test 3 asserts an effective compaction stays converged even under a stamp-less cursor. In test 3 the anti-regression assertion itself passes both before and after, as it must; only its control arm (same cursor, differing only in what the last rewrite achieved) fails pre-fix, which is what proves the test discriminates on effectiveness rather than on the partition being too healthy to flag.

Full suite 3978 pass / 0 fail / 1 skip; typecheck clean; llp-ref-hygiene and the em-dash gate both green.

Cursor backward compatibility

PartitionCursor.compaction is typed unknown | null and copied through verbatim, so no parser change was needed; every reader probes with isPlainObject plus typeof. Verified two ways: test 2 plants the exactcursor.json compaction record from the issue and drives real maintenance through it; and an end-to-end probe with the issue's full cursor JSON gives tick 1 compacted: 1, ineffective: true, 8 -> 8, then tick 2 compacted: 0, ineffective: true.

Forward compatibility holds by the same mechanism: older code reads only resettleBaselineFiles and ignores the two new keys.

One-off upgrade cost, recorded in LLP 0217's Consequences: every pre-existing cursor lacks the stamp, so each still-fragmented partition compacts one more time and then converges. That is the same trade LLP 0199 accepted for cursors written before its own baseline field existed.

Deliberately not done

The issue offered three directions and explicitly did not prescribe one. Only option 1 is implemented:

• Option 2 (pack output files by bytes on disk) - the issue calls this the real fix. Out of scope here, and largely already landed on master as LLP 0209, which is precisely why the stale-verdict retry has something useful to do.
• Option 3 (fall back to a file-count target when the byte target is unreachable) - untouched. LLP 0217 records it as still open.

LLP 0199's baseline gate is not weakened or bypassed: a partition whose count has not moved and whose rewrite worked is still never rewritten.

Fixes#723

[philcunliffe]

Review round 1 of 8729d0b. Verdict: findings - 2 major, 1 minor. All three fixed and pushed as d68a229. The normal paths were confirmed sound; both majors are on paths the committed tests did not reach.

1. major - a stale-verdict retry that THROWS was never re-stamped, so it repeated every tick forever and took the whole walk down with it. FIXED

The PR's central safety claim was "the retry re-stamps the cursor, so it happens once per writer generation, never once per tick". True on all three paths where compactGenerationreturns. False where the rewrite throws: the cursor is written only after close() resolves, so a throw left the pre-fix record intact and compactionVerdictStale was still true next tick.

Verified empirically over four consecutive ticks, with a live parquet file truncated to 0 bytes as a torn-write stand-in and the issue's exact stamp-less cursor planted:

PR head: tick1 THREW, tick2 THREW, tick3 THREW, tick4 THREW (cursor unchanged each time)
base: tick1 OK compacted=1, ticks 2-4 OK compacted=0 (gate closed, walk completes)

Three things made it worse than it first reads:

• withSpan rethrows and maintainCache has no per-partition catch, so the whole tick aborts - other partitions get neither compaction nor snapshot expiry, and the daemon hits the same wall an hour later.
• LLP 0199 #neediest-first walks in descending file count, so the most fragmented partition goes first: exactly the one most likely to fail a rewrite, and exactly the one this PR newly makes eligible. That is LLP 0199's starvation failure mode re-entered through a different door.
• The eligible population is universal, not narrow. After this ships every cursor in every installed cache lacks writerGeneration, so only a throwing rewrite is contingent.

Fixed by making the attempt, not its success, spend the generation: the compactGeneration call is wrapped, and on a throw a stale verdict is re-stamped before rethrowing. It records no false "ineffective" verdict, only that this generation had its turn.

One deliberate deviation from the prescribed fix, and it is a good one. The stamp re-reads the cursor via tryReadCursorSync(r.path) ?? cursor rather than stamping the tick's in-memory copy, because a rewrite that throws after committing its cursor would otherwise be rolled back onto the retired generation. tryReadCursorSync and not readCursorSync, because this write is destructive and must never replace an unreadable cursor with the synthesized epoch-0 default.

2. major - every ordinary single-data-file partition was permanently reported as "compaction reduced nothing". FIXED

compactionReducedFiles carved out only before <= 0. A partition holding one data file is equally at its floor, but a 1-to-1 rewrite recorded dataFilesBefore: 1 - not a reduction - so the verdict stuck for life. And needsCompaction returns true for any partition whose average file is under 32 MB, so every low-volume partition got exactly one 1-to-1 rewrite on its first tick and carried the verdict forever:

run 1: logs/source=claude: compacted epoch=? (1 -> 1 files), no file-count reduction
run 2+: logs/source=claude: compaction skipped: the last rewrite of these 1 files reduced nothing

A single-file partition is maximally compact, not fragmented, so the line reads as a defect report. At server scale (per-org, per-day partitions, most of them one flush) that is one false line per day-partition per run, drowning the one line that is true. Behaviour was unaffected, but it inverted the signal the feature exists to provide.

Fixed with before <= 1 returning undefined, and LLP 0217's #record-effectiveness updated to match. The fixer went further than asked in a way worth noting: the post-rewrite report site had its own inline floor check, so the two definitions could drift. Both now delegate to one rewriteReducedFiles(before, after), which also kills the run-1 false line at its source.

Also fixed, smaller: the skip message printed the current live count while describing the recorded one. They coincide when grew === false but not on the grew && !needsCompaction path (retention deleting 40 files down to 5 would report "the last rewrite of these 5 files reduced nothing" about a 40-to-40 rewrite). It now quotes the cursor record.

3. minor - no test pinned either risky path. FIXED

The three committed tests are honest, and the PR's characterization of test 3 was confirmed by construction: pre-fix the converged arm passes trivially since nothing thaws, and only the control arm (same cursor, differing solely in dataFilesBefore) fails, so the pair genuinely discriminates on effectiveness rather than on the partition being too healthy to flag.

But nothing drove a retry whose rewrite fails - the one re-stamp path that did not happen - and nothing covered a partition at its floor with one file. Two tests added, both verified to fail without their respective fix:

BEFORE # tests 5 # pass 3 # fail 2
4 "a retry whose rewrite throws still spends its writer generation" -> actual undefined, expected number
5 "a partition already at one data file is not reported as ineffective" -> actual true, expected undefined
AFTER # tests 5 # pass 5 # fail 0

Test 3 also gained the consecutive-tick treatment test 2 already had, so it pins convergence rather than one-tick quiescence.

Also checked, clean

• Rewrite-forever on the normal paths - sound, confirmed by driving real maintainCache ticks over real Iceberg fixtures rather than reasoning on paper. 40 identity tuples that cannot shrink: compacts once, then five quiet ticks. The issue's stamp-less cursor on an 8-file partition: thaws, compacts, then five quiet ticks. Two partitions in one cache (one shrinkable 6-to-1, one not 8-to-8) over six ticks: both compact once, neither again - no alternation, no oscillation. Reduction-by-a-little converges by construction: compactionReducedFiles === true short-circuits before the stamp is consulted, and the LLP 0199 count gate still requires the live count to move. The new gate is genuinely additive - verdictStale is itself gated on !grewSinceCompaction, so it can only widen into the case the old gate closed, never bypass needsCompaction.
• Cursor compatibility, both directions - no crash found. Three ticks each through ten corrupt or hand-edited shapes (compaction as null, array, string, number; resettleBaselineFiles as a string; dataFilesBefore null, negative, zero; writerGeneration as a string or object): zero throws, zero loops. Every shape either compacted once and converged or was treated as converged. Older readers touch only resettleBaselineFiles, still written first and unchanged in meaning.
• rebaselineCursor's stamp is load-bearing, confirmed rather than assumed. Deleting the two added lines makes the existing LLP 0207 test fail, because an unstamped recognition reads as verdict-stale every tick and pays a metadata load plus a cursor write forever. Dropping dataFilesBefore there is also right: the layout is the foreign compactor's, so no verdict about the kernel's own writer applies.
• Recording effectiveness from inside compactGeneration rather than the caller is deliberate and correct: the caller's count is taken beforeexpireSnapshots runs, so using it would attribute expiry-deleted files to the rewrite.
• LLP 0217 - well-formed and honest. All four anchors it defines and all four it cites resolve. No number collision: llp/ holds 0210-0217 with no duplicate, and the changed-file list of all 17 other open PRs was checked for anything in that range. The Extended-by: on LLP 0199 is purely additive; nothing 0199 settled was rewritten.
• Conventions clean; npm test 3980 pass / 0 fail / 1 skip; typecheck clean; llp-ref-hygiene 11/11; cache-retention-maintenance 38/38 (the LLP 0207 test intact); both cache_lifecycle_maintenance and incremental_sink_compaction smokes ok.

Filed separately, deliberately out of scope

#737 - maintainCache aborts the whole walk when one partition's compaction throws. That is a pre-existing behaviour change beyond these findings, so it was not bundled here. Finding 1 bounds the damage (a persistently failing partition is now attempted once per writer generation rather than once per tick, so the starvation lasts one tick instead of forever) but the walk still aborts on that one attempt.

The head has moved to d68a229, so the next tick reviews that head (round 2).

[philcunliffe]

Review round 2 of d68a229. Verdict: findings - 2 minor, both fixed and pushed as 9930c49. All three round-1 fixes land correctly and were confirmed load-bearing by mutation.

1. minor - a failing stamp write swallowed the rewrite failure entirely. FIXED

await writeCursor(...) sat unguarded inside the catch (err) block, so if the stamp write threw, throw err was never reached and the original rewrite error was lost with no cause. Verified by construction (8-tuple partition, stamp-less cursor, a live parquet truncated so the rewrite throws, partition dir read-only so the stamp write throws too):

EACCES: permission denied, open '.../source=claude/cursor.json.<tmp>'
cause present: false

The torn-parquet failure - the thing an operator has to fix - was gone. The realistic pairing is worse than the fixture: a decode error masked by an ENOSPC on the cursor write sends the operator after a disk-space symptom that is not the fault. It also undercuts this PR's own premise (a partition skipped for a stated reason) and the repo's log-driven-development rule that a failure should identify the broken step.

Fixed by making the stamp best-effort. After the fix the same fixture surfaces Offset is outside the bounds of the DataView - the real diagnosis. cause is still absent, correctly: the cursor write is swallowed by design, so there is nothing to attach.

2. minor - the round-1 deviation was pinned by nothing. FIXED

Round 1's fixer deliberately made the stamp re-read the cursor (tryReadCursorSync(r.path) ?? cursor) rather than stamping the in-memory copy, so a rewrite that throws after committing its cursor is not rolled back onto the retired generation. The reviewer mutated that to stampWriterGeneration(cursor) and ran three cache test files: 49/49 still passed. The existing throw test's rewrite throws mid-scan, before any commit, so it never exercised the branch the deviation exists for.

The deviation is not cosmetic. Inducing a genuine post-commit throw (chmod the retiring generation dir to 0555, so the cursor commits and the .retired marker write then fails):

at head: cursor -> new generation, dataFilesBefore: 8, writerGeneration: 2 (rolled back? false)
under mutation: cursor -> pre-rewrite generation, resettleBaselineFiles: 8 (rolled back? true)

Under the mutation the completed rewrite's output is orphaned (later reaped by the ORPHAN_GRACE_MS sweep in walkForRetired) and the recorded verdict is lost, so the partition is skipped silently forever instead of reported. Any future post-commit step added to compactGeneration widens it.

Fixed with a third case in the throw test. Discrimination verified in both directions, and thoroughly: under the mutation, assertion 1 fails on the rolled-back tableDir; and because a first failure would mask the second, assertion 1 was temporarily replaced with a diagnostic to confirm assertion 2 fails independently (# DIAG rolled back? true, then undefined !== 8 on the committed record). Both mutation and diagnostic were restored; the committed tree has neither.

Nit also fixed: the skip message read "the last rewrite of these N files reduced nothing", where N is deliberately the recorded count. In the exact case its own comment describes (retention shrank the partition without re-flagging it) it printed "these 40 files" for a partition holding 5. The number was right; the demonstrative was the leftover.

Verified from round 1

• Fix 1 (throw-path stamp) - landed and load-bearing. Deleting the if (verdictStale) block fails the round-1 throw test. Empirically: with a truncated live parquet the tick throws once, the cursor gains writerGeneration: 2 with resettleBaselineFiles unmoved and no invented dataFilesBefore, and ticks 2 and 3 walk past instead of re-entering the failing rewrite. The tryReadCursorSync choice is right - partition.js:43-70 returns null on both read and parse failure, so the destructive write can never replace a live cursor with the synthesized epoch-0 default. The if (verdictStale) guard is correct: verdictStale requires !grewSinceCompaction and hasResettle requires grewSinceCompaction, so the two can never both hold, and a partition due for genuine growth correctly leaves its cursor byte-identical on a throw.
• Fix 2 (before <= 1) - landed, and it does not over-suppress. Reverting to before <= 0 fails the single-file test. Boundary swept at 1/2/3/8/40 identity tuples: 1 file gives undefined (correctly silent) and still thaws once on a generation bump; 2 files still reports ineffective with the recorded count, thaws once, then quiesces; 3/8/40 (the Cache compaction gate freezes a partition it never actually compacted (1,521 files at 214KB avg vs 32MB target) #723 shape) unchanged. Shrinkable controls never report ineffective.
• Fix 3 (tests) - both present and mutation-verified; test 3 gained its consecutive-tick loops and both hold.

Also checked, clean

• Floor definitions genuinely unified.rewriteReducedFiles(before, after) is the single check; both the post-rewrite site and the cursor reader delegate to it. Case-by-case against the old inline check: identical for before === 0 and all before >= 2; the only behaviour change is before === 1, which is the intended fix. The one knock-on, a 1 -> 0 rewrite now reading unknown rather than effective, is inert because needsCompaction returns false on a zero-data-file table so the granted retry never rewrites anything.
• Multi-tick quiescence on a mixed cache (unshrinkable plus shrinkable), full maintenance, 6 consecutive ticks: zero compactions, zero rebaselines, zero cursor mtime or size churn on either partition, stable correct reporting. No loop, no write amplification.
• rebaselineCursor / LLP 0207 still load-bearing: removing the stamp there fails the foreign-sorted-replace test (37/38).
• The skip-path report can never print undefined: compactionKnownIneffective requires a numeric dataFilesBefore, so the count is always defined and always at least 2 wherever the flag is set.
• Considered and benign: a throw on a partition due for genuine growth is not stamped and throws again next tick (pre-existing, maintainCache aborts the whole walk when one partition's compaction throws #737-shaped); and a failed attempt under a future generation bump re-attributes an older verdict to the new stamp, which is the "attempt spends the retry" trade LLP 0217 states explicitly.
• Conventions: no semicolons, no U+2014 in any changed file including the LLP prose, no @typedef added (the one at maintenance.js:222 predates this PR, confirmed against 3c462b3), no inline import('...') types, @import specifiers root-anchored, every new @ref anchor resolves.
• Ran: npm test 3981 pass / 0 fail / 1 skip; typecheck clean; llp-ref-hygiene 11/11; cache-retention-maintenance 38/38; cache-compaction-file-size; plus six purpose-built probes and four source mutations.

The head has moved to 9930c49, so the round budget (2) is spent at an unreviewed head: the next tick triages rather than opening a round 3.

[philcunliffe]

Triage after the review budget (LLP 0017). Two rounds ran (4 findings, all fixed). Judged mergeable. One deferred item in #739.

The upgrade transient, quantified rather than accepted

Every pre-0217 cursor lacks writerGeneration, so on first run after this ships every still-fragmented partition is eligible for one retry. The triage measured it rather than taking LLP 0217's word.

Population: a rewrite only happens if needsCompaction still flags the partition, so healthy partitions with big files are untouched. It is most previously-compacted partitions - which is the point, since those verdicts all came from the writer that could not do the job.

Measured on an 80-file / 16.4 MB fixture with incompressible payloads and a pre-0217 cursor planted: the thaw rewrote it in 159 ms, 80 files to 1, partition directory 20.9 MB to 37.3 MB (1.8x, both generations on disk), second tick converged at 0 compactions. Scaling to #723's cache (1,521 files / 318 MB), even discounting throughput 10x for the small-file overhead, that is seconds to low minutes inside one tick. Disk: +~318 MB for 24 hours (GRACE_PERIOD_MS), and #723's already-retired older generation is past its grace and reclaimed on the same run, so steady state is two generations, not three.

Not simultaneous. Rewrites are sequential within a tick, the daemon's 30s budget cuts the walk, and a 6-partition backlog was confirmed to drain one per budget-limited tick then quiesce. Peak transient disk is bounded by 24 hours of tick throughput, not by cache size, and neediest-first clears the worst partitions first. Same shape of one-off LLP 0199 already accepted for its own baseline-field upgrade.

Is option 1 useful without options 2 and 3? Yes

The "real fix" writer is already on master: LLP 0209's streaming writer is writerGeneration: 2, so the retry runs under a writer that demonstrably packs by bytes - the fixture folded an 80-file frozen partition into 1. For a partition whose fragmentation is genuinely tuple-bound (#723's worst case, one file per session tuple), the retry costs one bounded rewrite, re-freezes with an honest gen-2 verdict, and thereafter prints the skip reason instead of staying silent - once per generation, never per tick.

On the #737 ordering

No hard ordering required. The daemon already catches a rejected maintenance tick, so a walk abort costs the remainder of one tick, never the daemon. And this PR strictly reduces abort frequency for the class it thaws: pre-PR a failing eligible partition aborted the walk every tick; now it aborts at most once per partition per writer generation, because the stamp precedes the rethrow.

But land #737 promptly after. During the upgrade transient the thaw enqueues exactly the most-fragmented, most-likely-to-fail partitions at the front of the neediest-first walk, so each first failure among them still costs the remainder of that hourly tick. A per-partition catch is what turns those into single-partition losses.