Fix compaction starvation: skip partitions with no new data, do the worst ones first

[platypii]

Cache maintenance was rewriting the same few partitions every tick: the avg-file-size heuristic re-flags a partition forever because compacted output files come out far smaller than 32MB, so nothing ever graduates. On the production server the first four partitions in walk order had been recompacted 29-51 times while every org=hyperparam partition sat at epoch 0 with 700-1250 data files, never reached before the 30s tick budget ran out.

Two changes:

• A partition is only compaction-due when its live data-file count has moved off the count recorded by its last rewrite (generalizing the re-settle baseline from LLP 0027). Closed partitions compact once and converge.
• The maintenance walk ranks partitions by live data-file count, descending, so a budget cutoff postpones the healthiest partitions instead of starving the same directory-order tail.

Design rationale in LLP 0195. Two new tests cover both behaviors; the two failing tests in the suite (blob-store, usage-policy-fold) also fail on master.

[philcunliffe]

Neutral review round 1 of 2 (adopted PR, neutral:adopt) - 13e4c35 - verdict: findings

One blocking item (the LLP number, now fixed by neutral in 33306ec). The scheduling
substance is sound - both predicates were attacked directly and neither broke.

BLOCKING - LLP 0195 collides with PR #658. Renumbered to 0199.

origin/master                             -> tops out at 0194
origin/fix/issue-653                      -> 0195   (PR #658, neutral:approved, held)
origin/maintenance-compaction-starvation  -> 0195   (this PR)
origin/llp/0193-vendor-report-renderer    -> 0196, 0197  (PR #668, approved, held)
origin/wizard-first-ask                   -> 0198   (PR #667)

Filenames differ, so git merges both silently. Neutral pushed the renumber to 0199 with
the three @ref sites in maintenance.js (:105, :262, :775) retargeted. Worth noting
why no tool would have caught it: the refs would still have resolved, to the wrong
document, so no ref checker flags them.

Why CI passed anyway. .github/workflows/llp-check.yml runs
find llp -name '[0-9][0-9][0-9][0-9]-*.md' | cut -c1-4 | sort | uniq -d against the
checked-out tree, which on a PR head is master plus this one branch. It structurally cannot
see sibling branches, and only fails on the push: [master] trigger after the second one
merges - reddening the default branch instead of blocking the PR. The workflow's own header
comment already says this.

This is the third such collision in 24 hours, from three different contributors, all
computing the next number from master. Not a contributor error - the gate cannot see the
conflict. Two mitigations worth considering: have /llp-create enumerate
git for-each-ref refs/remotes/origin rather than the working tree, and/or have the
reconciler re-check at merge time against the held queue.

Substance: both changes verified, and they are load-bearing on each other

Change 1, the baseline gate (maintenance.js:269, :281). Every way a partition could
be wrongly parked was traced, and none exists:

• Never-compacted partitions stay eligible. resettleBaselineFiles() returns undefined
  unless compaction.resettleBaselineFiles is a number, and N !== undefined is always true.
  That is exactly the production case in your report (org=hyperparam, 700-1250 files,
  compaction: null), so the fix does reach the starved partitions.
• Baseline is generation-consistent. compactGeneration (:591-593) computes
  countDataFiles(newDir) and writes it in the same writeCursor that repoints the cursor
  at newDir, so next tick measures against the same directory. No generation skew.
• No path where data accumulates without moving the count. Appends add parquet files;
  retention's delete-commits write -deletes.parquet, which countDataFiles also counts.
  retention.js:214/255 preserves the baseline while the count moves - the safe direction.
• Stale/legacy cursors yield undefined, so they compact once and converge, as the LLP's
  Consequences states.
• Escape hatch exists: opts.force short-circuits the gate, wired to
  hyp query maintain --force.
• The hasResettle rewrite is byte-identical to the LLP 0027 gate it replaces.

Change 2, neediest-first (:104-110). Deterministic (Array.sort is stable per ES2019;
ties keep discovery order). It does not starve small partitions, precisely because change
1 exists: the heavy head converges after one rewrite and thereafter costs only a cheap skip,
so the list drains. On master neither convergence nor ordering existed, which is why the same
four partitions were rewritten 29-51 times. Landing either change alone would be worse than
landing both.

One residual the LLP does not discuss: there is no aging/fairness term, so a permanently-hot
large partition could monopolise the budget. Not a regression (directory order had the same
property, just on an arbitrary partition rather than the worst one), but the convergence
argument quietly assumes partitions eventually close.

Non-blocking, left for you

1. LLP vs code scope mismatch. The doc (L45-49) says the gate covers "the file-count
   heuristics", but the code gates all three clauses of needsCompaction, including
   metadataBytes > 64MB (maintenance.js:406). Bounded in practice - metadata grows via
   commits that also move the file count, and snapshot expiry self-limits at
   min_snapshots_to_keep - so no live bug, but doc and code should agree.
2. Ranking is not budget-checked. It runs before the loop, while
   if (Date.now() - startMs > budgetMs) break sits at the loop top. On a cache large enough
   that ranking alone exceeds max_tick_ms, the loop breaks on iteration 0 and maintenance
   does nothing, forever. Cheap insurance: if (processed > 0 && ...) break.
3. countDataFiles now runs 2-3x per partition per tick (ranking, dataFilesBefore,
   inside needsCompaction), all synchronous readdirSync on the daemon event loop. Thread
   the ranked count through. It is also pure waste under expireOnly, where data-file count
   is the wrong priority signal.
4. No Extended-by: LLP 0199 back-ref on LLP 0027, which this generalizes. 0027 is
   Status: Draft so immutability is loose, but the convention asks for it.
5. PR body claims two suite failures also fail on master. The suite is green here
   (3590 pass / 0 fail), so that note is stale.

Test coverage and mutation testing

Both new tests are well-targeted. The baseline test uses 3 tiny files so
3 > compact_file_count(32) is false, exercising the avg-file-size clause specifically -
the exact self-defeating heuristic in your report - and its three phases genuinely prove the
convergence claim. The order test uses aaa_light (1 file) vs zzz_heavy (3 files), so
directory order and neediest order actually disagree.

9 mutants run, 7 killed. Killed: gate dropped from needsCompaction, gate dropped from
hasResettle, !==→>, grewSinceCompaction = true, sort ascending, sort removed,
liveDataFileCount→0.

The one meaningful survivor: hardcoding liveDataFileCount to ignore the source-table
layout survives. Nothing covers neediest-first ordering for that layout - and it is very
likely the production layout in your report, since generationLayout never advances epoch
for source-table cursors, so "sat at epoch 0" is what a source-table partition looks like by
construction. The shipped code is correct there - verified by hand with two source-table
partitions (1 vs 4 files), maintainCache returned zzz_heavy:4, aaa_light:1. So this is a
coverage gap, not a defect. A third test mirroring the order test with
layout: 'source-table', tableDir: 'table' would close it, and it is the one addition worth
making before merge.

(A second survivor, resettleBaselineFiles(cursor) ?? 0, is effectively equivalent - a
0-file partition also fails needsCompaction. Low signal.)

Gates

• node scripts/run-tests.js → 3590 pass / 0 fail / 1 skipped
• npx tsc -p tsconfig.json --noEmit → exit 0
• CI on 13e4c35 → all 9 green (for the structural reason above)
• No em dashes, no semicolons; all three @ref anchors resolve and honestly describe the code

The engineering here is good: the diagnosis is correct, the fix reaches the actually-starved
partitions, and the two changes are properly interdependent rather than redundant.

[philcunliffe]

Neutral review round 2 of 2 (adopted PR) - dd2f73d - verdict: clean

Round 1 found the substance sound and one blocking item (the LLP number). Neutral healed
that in two commits, and this round verified the heal adversarially rather than trusting it.

The renumber is complete

Unfiltered sweep (grep -rn "0195" . --exclude-dir=.git --exclude-dir=node_modules):
zero hits. That check matters here, because neutral's first heal commit (33306ec) was
incomplete for exactly the reason a scoped check would have hidden: it restricted its replace
to src/ and llp/, leaving a stale LLP 0195 in a section header at
test/core/cache-retention-maintenance.test.js:781. dd2f73d closed that.

All LLP 0199 references resolve: the doc exists with a matching title line, and both
anchors used by code are real <a id> tags - baseline-gate (:43) and neediest-first
(:53). The three @ref sites in maintenance.js (:105, :262, :775) and the test
section header all point at the renamed doc.

0199 is uniquely owned. Confirmed absent from origin/master and from every other
origin/* branch, including the three that hold 0195-0198.

The heal changed numbering only

git diff 13e4c35..HEAD --name-status -M is one R098 rename plus two modified files, and
the full diff is exactly five one-line changes, every one a 0195→0199 substitution: the
doc title, two @ref comments and one JSDoc @ref in maintenance.js, and the test section
header. No logic, no test assertions, no other content touched. Both heal commits were
inspected individually and each is scoped to what its message claims.

Gates

• node scripts/run-tests.js → 3590 pass / 0 fail / 1 skipped
• npx tsc -p tsconfig.json --noEmit → clean, exit 0
• CI on dd2f73d: all 9 green, including llp-check / duplicate-numbers - which now
  passes because the collision is gone, not because it could ever have caught it. PR is
  MERGEABLE.

Fresh pass

No stale forward-refs to the old filename or number anywhere in llp/, no duplicate NNNN
prefixes, nothing extraneous in either commit. No new findings.

All six non-blocking items from round 1 remain open and unaffected by the renumber: the LLP
scope wording vs the three gated needsCompaction clauses, the unbudgeted ranking pass, the
2-3x countDataFiles per partition per tick, the missing Extended-by: back-ref on LLP
0027, the stale PR-body claim about failing tests, and the missing source-table
neediest-first test. That last one is still the single addition most worth making before
merge, since source-table is very likely the production layout in your report.

---

Correction: this record was first posted with an incorrect head SHA in its marker. Neutral knew the short form dd2f73d and wrote out a full SHA it had not verified. The marker now carries the real head, dd2f73d280c848fb7e6b48f3c250c01c5d5008e4. The review content is unchanged; only the marker was wrong.

[philcunliffe]

Verdict: changes requested - but nothing blocking remains

Both review rounds are used, so under the adopted-PR rules the residual findings come back to
you rather than being deferred into a follow-up issue. To be clear about what that means
here: round 2 was clean, the blocking item is fixed, and the engineering is sound. This is
a hand-back, not a rejection.

What was blocking, and is fixed

llp/0195-maintenance-compaction-convergence.decision.md collided with PR #658, which also
claims 0195. Neutral renumbered it to 0199 and retargeted the four citations. Verified by
an unfiltered sweep: zero 0195 references remain, all LLP 0199 refs resolve to real
anchors, 0199 is uniquely owned across every branch, and the diff is numbering-only.

Two notes of neutral's own error, since they are on your branch:

• The first heal commit scoped its replace to src/ and llp/, missing a section header in
  test/core/cache-retention-maintenance.test.js. Fixed in dd2f73d.
• The round-2 review comment was first posted with a fabricated full head SHA in its
  marker. Neutral knew the short form and wrote out 33 characters it had not verified. The
  comment has been corrected and carries a note saying so.

Why this collision keeps happening

Three contributors hit it in 24 hours. .github/workflows/llp-check.yml runs
find llp -name '[0-9][0-9][0-9][0-9]-*.md' | cut -c1-4 | sort | uniq -d against the
checked-out tree, which on a PR head is master plus one branch - it structurally cannot see
sibling branches, and only fails on push: [master] after the second doc merges. The
workflow's own header comment already says this. Master sits at 0194 while four unmerged
branches hold 0195-0199, so anyone computing the next number from master lands on a taken
one, and the @refs still resolve - to the wrong document - so no ref checker flags it.

Worth fixing at the source rather than per-PR: have /llp-create enumerate
git for-each-ref refs/remotes/origin instead of the working tree.

The one thing worth doing before merge

No test covers neediest-first ordering for the source-table layout. That is very likely
the production layout in your report: generationLayout never advances epoch for
source-table cursors, so "sat at epoch 0 with 700-1250 data files" is what a source-table
partition looks like by construction. Neutral verified by hand that your code is correct
there - built two source-table partitions (1 vs 4 files) and maintainCache returned
zzz_heavy:4, aaa_light:1 - so this is a missing test, not a defect. A third case mirroring
the order test with layout: 'source-table', tableDir: 'table' closes it.

Five smaller items, take or leave

1. The LLP says the gate covers "the file-count heuristics", but the code gates all three
   needsCompaction clauses including metadataBytes > 64MB. Bounded in practice, so no
   live bug, but doc and code should agree.
2. Ranking runs before the loop and is not budget-checked, while the budget break sits at the
   loop top. On a cache large enough that ranking alone exceeds max_tick_ms, the loop breaks
   at iteration 0 and maintenance does nothing, forever. if (processed > 0 && ...) break is
   cheap insurance.
3. countDataFiles now runs 2-3x per partition per tick, all synchronous readdirSync on the
   daemon event loop. Threading the ranked count through avoids it. Also pure waste under
   expireOnly.
4. No Extended-by: LLP 0199 back-ref on LLP 0027, which this generalizes.
5. The PR body claims two suite failures also fail on master; the suite is green here.

For the record, on the substance

Round 1 attacked both predicates and neither broke. The baseline gate does reach the starved
partitions (resettleBaselineFiles returns undefined for never-compacted ones, and
N !== undefined is always true - exactly your compaction: null production case). The
baseline is generation-consistent, no path lets data accumulate without moving the count,
stale cursors compact once and converge, and --force remains an operator escape hatch. The
two changes are load-bearing on each other: neediest-first would starve small partitions
without the convergence gate, and the gate alone would not fix walk order.

Push any commit and neutral re-engages with a fresh review at the new head.

[philcunliffe]

Merged as c6f7a0b. Recording what landed unaddressed, so it is findable later - not
re-litigating the merge, which was the right call: round 2 was clean and nothing blocking
remained.

Verified post-merge: no duplicate LLP numbers on master, llp/0199 present with all four
citations resolving, zero stale 0195. Repo-wide numbering is now consistent - master at
0199, and the three unmerged branches holding 0195, 0196/0197, and 0198 with no overlap.

Six non-blocking items from the review are now in master:

1. No test covers neediest-first ordering for the source-table layout - the layout most
   likely in the original bug report, since generationLayout never advances epoch for
   source-table cursors. The code was verified correct by hand; this is a missing test on a
   production path.
2. Ranking is not budget-checked. It runs before the loop while the budget break sits at
   the loop top, so on a cache large enough that ranking alone exceeds max_tick_ms, the
   loop breaks at iteration 0 and maintenance does nothing, indefinitely.
   if (processed > 0 && ...) break is cheap insurance. This is the one with a real, if
   unlikely, production failure mode.
3. countDataFiles runs 2-3x per partition per tick, synchronous readdirSync on the daemon
   event loop; pure waste under expireOnly.
4. The LLP says the gate covers "the file-count heuristics" while the code gates all three
   needsCompaction clauses including metadataBytes > 64MB.
5. No Extended-by: LLP 0199 back-ref on LLP 0027, which this generalizes.
6. The PR body's claim about two failing tests was stale.

Neutral is not filing these as issues - that is the maintainer's call on priority. Label any
of them neutral:fix on a new issue and the loop will pick it up.