Skip to content

Maintenance recognizes a foreign sorted replace and re-baselines instead of shredding it (#700) - #701

Merged
philcunliffe merged 2 commits into
masterfrom
fix/issue-700
Aug 10, 2026
Merged

Maintenance recognizes a foreign sorted replace and re-baselines instead of shredding it (#700)#701
philcunliffe merged 2 commits into
masterfrom
fix/issue-700

Conversation

@philcunliffe

Copy link
Copy Markdown
Contributor

Fixes#700.

What

The central server sorts each day's cache table in place right before its nightly export (hypaware-server LLP 0115/0116): an in-place replace snapshot committed through icebird, without touching the kernel's partition cursor. The LLP 0199 baseline gate only recognizes the kernel's own rewrites, so the foreign replace read as growth, the avg-file-size heuristic flagged the freshly sorted day, and the next hourly maintenance tick rewrote it back into ~0.5MB per-batch files 30-90 minutes after export - every night, for every exported day (prod timeline in #700).

The due-test now recognizes the foreign rewrite: when a compaction-due partition's current snapshot is a replace committed under the table's declared default sort order (the kernel-side mirror of the server day compactor's alreadyCompacted + sortOrderDeclared skip), and no append has landed since (the replace still being current is exactly that test), maintenance records the live data-file count as the new cursor baseline and skips the rewrite. Documented as LLP 0207, extending LLP 0199.

Design points

  • Single metadata load. The metadata read the recognition needs is the one compactGeneration already did for schema/spec/sort-order carry; it is hoisted into loadCompactionTableInfo and shared, loaded once per compaction-due partition. Partitions not due still pay nothing.
  • Recognition outranks the re-settle force. The LLP 0027 sweep only runs inside a rewrite; letting it override would mean one leftover unmatchable fallback row re-shreds the sorted layout every night. Settlement of fallback rows under a foreign sorted replace defers to the next append (LLP 0207#outranks-resettle states the trade).
  • Arbitrary replaces are not blessed. A replace on a table with no declared sort order stays due, and an explicit --force still rewrites.
  • A late append is unchanged behavior: it flips the current snapshot's operation off replace and moves the count off the baseline, so the partition is genuinely due again.
  • Reports gain a rebaselined flag (mutually exclusive with compacted); dry runs predict the recognition without writing the cursor.

Tests

Three new cases in test/core/cache-retention-maintenance.test.js (fixture style of the LLP 0199 tests, foreign rewrite simulated with icebird's icebergRewrite, the same call the server makes):

  1. sorted replace + baseline mismatch: no rewrite, cursor re-baselined to the live count, compactedAt preserved, converged on the next tick, due again after a late append
  2. replace without a declared sort order: still rewritten
  3. --force: still rewritten

Full suite: 3889 pass / 1 pre-existing failure (leave-command#623 case, fails identically on master). Typecheck green.

Coordination

Sibling of #697 / PR #698 (same file, same walk): #698 changes how the kernel's own rewrites size files; this PR stops the kernel destroying the server's rewrites. Kept deliberately independent per #700; the overlap is small (compactGeneration head, types.d.ts, an Extended-by line both PRs add to LLP 0199) and rebases cleanly whichever lands first.

Prod verification (after release + server image rebuild)

After the next 01:00Z export tick, the just-exported day's cache partition should keep its sorted form: cursor epoch unchanged, compaction.resettleBaselineFiles moved to the small live count, no epoch bump 30-90 min post-export, file count stays small. Payoff claim to verify: the whole 7-day cache window serves the sorted layout within a week of shipping.

🤖 Generated with Claude Code

…ead of shredding it (#700)
The central server's export-time day compaction commits an in-place
sorted replace snapshot the LLP 0199 baseline gate could not recognize,
so the next hourly tick rewrote the sorted big-file layout back into
per-batch files 30-90 minutes after every export. The due-test now
recognizes a current replace snapshot committed under the table's
declared default sort order with no append since, records the live
data-file count as the new cursor baseline, and skips the rewrite
(LLP 0207, extending LLP 0199).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Dual-agent review — request_changes (round 1)

  • Verdict:request_changes
  • Risk class:medium
  • Auto-merge advisory: 👎 thumbs down — verdict is request_changes; needs human-gated follow-up

Advisory only: no merge was attempted. All findings are minors; fixes and a full round 2 follow on this PR.

Risk capstone

Cross-reference: reviewer findings vs high-risk surfaces

SourceFinding (severity, evidence)Intersects
ClaudePre-replace data files leak until eviction (minor, maintenance.js:296-300)Risks bullet 1
ClaudeRebaseline invisible in hyp query maintain (minor, query.js:322)Direct callers (CLI printer)
CodexSame CLI/operability gap (minor, cat 11, query.js:317-327)Direct callers (CLI printer)
ClaudeSource-table layout untested on rebaseline path (minor, test:833)Risks bullet 2
ClaudeOutranks-resettle precedence untested (minor, maintenance.js:296)Risks bullet 2
Codex review

Fix Validations

Foreign sorted replace was treated as growth and rewritten

  • Status: correct
  • Evidence:src/core/cache/maintenance.js:273, src/core/cache/maintenance.js:285, src/core/cache/maintenance.js:296, src/core/cache/maintenance.js:299, src/core/cache/maintenance.js:480
  • Assessment: The new path runs only after the existing due-test says compaction would happen, recognizes current replace snapshots on sorted tables, writes only the baseline cursor, and skips compactGeneration.

Existing handling could not already fix this

  • Status: correct
  • Evidence:src/core/cache/maintenance.js:273, src/core/cache/maintenance.js:285
  • Assessment: The prior gate only compared live data-file count to resettleBaselineFiles; a foreign rewrite that changed the count still flowed into shouldCompact.

Unsorted replace and explicit force still rewrite

  • Status: correct
  • Evidence:src/core/cache/maintenance.js:296, src/core/cache/maintenance.js:481, test/core/cache-retention-maintenance.test.js:891, test/core/cache-retention-maintenance.test.js:913
  • Assessment:--force bypasses recognition, and tables without declared sort columns fail foreignSortedReplace.

Findings

11) Debuggability & Operability

  • Severity: minor
  • Confidence: high
  • Evidence:src/core/cache/maintenance.js:297, src/core/cache/maintenance.js:299, src/core/commands/query.js:317, src/core/commands/query.js:321, src/core/commands/query.js:327
  • Why it matters:hyp query maintain can now perform or dry-run a meaningful cursor rebaseline, but the CLI prints no per-partition action and the summary still reports only compacted partitions, making prod verification of this fix look like “nothing happened.”
  • Suggested fix: Teach the CLI formatter to include p.rebaselined, for example rebaselined to N files, and consider adding a total rebaseline count to the human summary.

No Finding

  1. Behavioral Correctness
  2. Contract & Interface Fidelity
  3. Change Impact / Blast Radius
  4. Concurrency, Ordering & State Safety
  5. Error Handling & Resilience
  6. Security Surface
  7. Resource Lifecycle & Cleanup
  8. Release Safety
  9. Test Evidence Quality
  10. Architectural Consistency

Evidence Bundle

  • Changed hot paths:maintainCache report aggregation, maintainGeneration compaction due path, loadCompactionTableInfo, foreignSortedReplace, compactGeneration, cursor baseline handling.
  • Impacted callers:src/core/commands/query.js:301, src/core/commands/query.js:317, src/core/daemon/runtime.js:672, hypaware-core/smoke/flows/cache_lifecycle_maintenance.js:98.
  • Impacted tests:test/core/cache-retention-maintenance.test.js:833, test/core/cache-retention-maintenance.test.js:891, test/core/cache-retention-maintenance.test.js:913, existing baseline test at test/core/cache-retention-maintenance.test.js:783.
  • Unresolved uncertainty: I did not rerun the suite; review is based on the provided diff plus targeted caller/contract tracing.
Claude review

Claude review

Re-baseline permanently leaks the pre-replace data files (disk ~2x per exported day)

  • Severity: minor
  • Confidence: 85
  • Evidence: src/core/cache/maintenance.js:296-300
  • Why it matters: The foreign replace leaves the old per-batch parquet files on disk in the same generation dir (icebird's icebergExpireSnapshots documents that expiry never deletes data files, and walkForRetired only reclaims non-live generation dirs); before this PR the nightly kernel rewrite incidentally reclaimed them by retiring the whole generation, but the new re-baseline keeps that generation live, so every exported day's partition holds both the shredded and the sorted file sets (~2x data) until cache eviction, with no code path that deletes the unreferenced files.
  • Suggested fix: Record the trade explicitly in LLP 0207's Consequences with a follow-up issue for unreferenced-file GC (or delete files unreferenced by the current snapshot's manifests once expiry ages out the pre-replace snapshots).

Rebaseline is invisible in hyp query maintain output

  • Severity: minor
  • Confidence: 85
  • Evidence: src/core/commands/query.js:322
  • Why it matters: The CLI's per-partition line only prints snapshotsExpired/compacted actions and the summary only counts totalCompacted, so a run that recognizes a foreign replace and writes a cursor (or a --dry-run predicting it) prints nothing for that partition and reads as "nothing due", hiding a state-mutating action; there is also no metric counterpart to compactionsCounter.
  • Suggested fix: Push an action such as rebaselined to N files (foreign sorted replace) when p.rebaselined is set, and consider a maintenance.rebaselines counter so prod verification does not require reading cursor JSON by hand.

Foreign-replace recognition untested on the source-table layout

  • Severity: minor
  • Confidence: 82
  • Evidence: test/core/cache-retention-maintenance.test.js:833
  • Why it matters: All three LLP 0207 tests write layout: 'epoch' cursors, but the server's day compactor commits foreign replaces onto both layouts in prod, and the layout-varying pieces the recognition depends on (liveDir = cursor.tableDir ?? 'table' resolution, rebaselineCursor's spread preserving tableDir/retention) never run under the rebaseline path.
  • Suggested fix: Add one source-table variant of the recognition test asserting rebaselined, cursor tableDir unchanged, and resettleBaselineFiles moved.

Recognition-outranks-resettle decision has no test

  • Severity: minor
  • Confidence: 80
  • Evidence: src/core/cache/maintenance.js:296
  • Why it matters: The PR names "recognition outranks the re-settle force" as a design point and mints LLP 0207#outranks-resettle for it (the exact nightly re-shred recurrence the fix exists to stop), but no test threads a settle context plus a resettle candidate under a foreign sorted replace; a future edit adding !hasResettle to the guard would silently regress.
  • Suggested fix: Add a case with a committed fallback row + foreign sorted replace + settle context, asserting rebaselined true and no rewrite.

Reports: /Users/phil/workspace/hypaware/.git/worktrees/issue-700/dual-review/pr-701

…layout and precedence tests
- LLP 0207 consequences state the pre-replace files stay until eviction
(~2x disk per exported day); unreferenced-file GC filed as #704
- hyp query maintain prints the rebaseline action and summary count, and
maintenance emits a hyp_rebaselines counter, so prod verification does
not require reading cursor JSON by hand
- recognition covered on the source-table layout (tableDir and retention
preserved) and against the re-settle force (a committed fallback row
must not undo the sorted layout)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Dual-agent review — approve (round 2, full re-review after round 1 fixes)

  • Verdict:approve
  • Risk class:low
  • Auto-merge advisory: 👍 thumbs up — approve verdict, low blast radius, both reviewer artifacts present, no blocker/major findings

Advisory only: no merge was attempted.

All four round 1 minors are addressed in 287b67b: disk trade documented in LLP 0207 and tracked as #704; rebaselines visible in hyp query maintain output and as a hyp_rebaselines metric; source-table layout and recognition-outranks-resettle now tested.

Risk capstone

Cross-reference: reviewer findings vs high-risk surfaces

SourceFinding (severity, evidence)Intersects
(none)(none)(none)
Codex review

Fix Validations

Foreign sorted replace was being re-compacted after export

  • Status: correct
  • Evidence:src/core/cache/maintenance.js:277, src/core/cache/maintenance.js:289, src/core/cache/maintenance.js:300, src/core/cache/maintenance.js:303, src/core/cache/maintenance.js:485, src/core/cache/maintenance.js:491, test/core/cache-retention-maintenance.test.js:833
  • Assessment: The existing baseline gate only handled equal file counts. The new branch recognizes a current replace snapshot on a table with declared sort columns, writes the live count into the cursor, and skips the rewrite.

Re-settle force must not shred a recognized sorted replace

  • Status: correct
  • Evidence:src/core/cache/maintenance.js:286, src/core/cache/maintenance.js:289, src/core/cache/maintenance.js:300, test/core/cache-retention-maintenance.test.js:927
  • Assessment:hasResettle can make the partition due, but the foreign-replace branch outranks it unless --force is set. The test asserts the sweep hook is not reached.

Unsorted replace and explicit force stay rewrite paths

  • Status: correct
  • Evidence:src/core/cache/maintenance.js:300, src/core/cache/maintenance.js:486, test/core/cache-retention-maintenance.test.js:960, test/core/cache-retention-maintenance.test.js:982
  • Assessment: No declared sort order returns false, and opts.force bypasses recognition. Both edge cases are covered.

Findings

No new findings.

No Finding

  1. Behavioral Correctness
  2. Contract & Interface Fidelity
  3. Change Impact / Blast Radius
  4. Concurrency, Ordering & State Safety
  5. Error Handling & Resilience
  6. Security Surface
  7. Resource Lifecycle & Cleanup
  8. Release Safety
  9. Test Evidence Quality
  10. Architectural Consistency
  11. Debuggability & Operability

Evidence Bundle

  • Changed hot paths:maintainCache partition loop src/core/cache/maintenance.js:91; compaction due-test src/core/cache/maintenance.js:252; foreign replace recognition src/core/cache/maintenance.js:485; compaction metadata carry src/core/cache/maintenance.js:563; CLI reporting src/core/commands/query.js:317.
  • Impacted callers: daemon maintenance src/core/daemon/runtime.js:672; manual CLI maintain src/core/commands/query.js:301; cache lifecycle smoke hypaware-core/smoke/flows/cache_lifecycle_maintenance.js:98; incremental sink compaction smoke hypaware-core/smoke/flows/incremental_sink_compaction.js:157.
  • Impacted tests: existing baseline convergence test/core/cache-retention-maintenance.test.js:783; new sorted replace case test/core/cache-retention-maintenance.test.js:833; source-table layout test/core/cache-retention-maintenance.test.js:891; re-settle precedence test/core/cache-retention-maintenance.test.js:927; unsorted replace test/core/cache-retention-maintenance.test.js:960; force override test/core/cache-retention-maintenance.test.js:982.
  • Unresolved uncertainty: I did not inspect server-side LLP 0115/0116, so I validated the kernel-side recognition against this PR’s stated Icebird replace shape. Test run: npm test -- test/core/cache-retention-maintenance.test.js exercised the suite; LLP 0207 tests passed, with the known pre-existing leave-commandLLP 0063 D4 exclusivity gate fails open on an unparseable central layer: login to a second org is permitted while enrolled #623 failure still failing.
Claude review

Claude review

No issues found.

Round 2 (two independent reviewers, full fresh pass over the complete diff including the round 1 fix commit). Verified: rebaselinesCounter threading and real-run-only increment; the printed rebaseline count equals the value written to the cursor (asserted by test); no consumer parses the changed maintenance: summary format (the smoke flow's totalCompacted check runs under force, which bypasses recognition); the outranks-resettle test is not vacuous (recognition asserted via rebaselined/epoch/totalCompacted independently of the throwing-hook tripwire, and resolveSettleContext genuinely resolves for the fixture); all @ref anchors resolve and the LLP 0207 consequences (dry-run behavior, 2x-disk trade, single metadata load) match the code; guidance clean (no semicolons, no em dashes, JSDoc/@ref conventions followed).

All four round 1 findings confirmed addressed: disk trade documented in LLP 0207 + tracked as #704; rebaseline visible in hyp query maintain output and as a hyp_rebaselines metric; source-table layout covered; recognition-outranks-resettle covered.


Reports: /Users/phil/workspace/hypaware/.git/worktrees/issue-700/dual-review/pr-701

@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

🧭 Decision map — where to spend your attention

Companion to the dual-review verdict. This casts no verdict — it points at the 5 forks where the author made a real choice, so you can skim the rest.

Scanned: 13 hunks across 6 files. Most is mechanical: the metadata-load hoist into loadCompactionTableInfo and its parameter threading, the rebaselined report field, CLI printer lines, and ~200 lines of test fixtures. The decisions worth your eyes, in order:

1. A declared default sort order is the deliberateness signal · contract / shape

src/core/cache/maintenance.js:486

functionforeignSortedReplace(tableInfo){if(!tableInfo?.sortColumns?.length)returnfalse
  • Decision: only a replace on a table whose default sort order is declared (identity transforms) earns convergence credit; the columns and directions themselves are not checked.
  • Alternative not taken: bless any current replace (risks crediting arbitrary foreign rewrites with worse layouts), or pin the exact server sort columns (couples the kernel to a column list the server owns and may evolve).
  • Check: the server's day compactor always declares the order before rewriting (declareSortOrder in hypaware-server's day-compaction.js), so recognition actually fires in prod; this cross-repo convention is the whole contract.

2. Recognition outranks the re-settle force · unhappy-path policy

src/core/cache/maintenance.js:300

if(!opts.force&&foreignSortedReplace(tableInfo)){
  • Decision: a partition due only because it holds committed fallback rows (LLP 0027 sweep trigger) is still skipped under a foreign sorted replace; settlement defers to the next append, indefinitely for closed days.
  • Alternative not taken: let the sweep force the rewrite — which re-shreds the sorted layout every night whenever one unmatchable fallback row exists, reproducing the exact bug being fixed.
  • Check: that indefinitely-provisional fallback rows on closed days are an acceptable cost (LLP 0207#outranks-resettle states the trade).

3. replace-is-current doubles as the no-append-since test · algorithm

src/core/cache/maintenance.js:491

returnsnapshot?.summary?.operation==='replace'
  • Decision: "nothing appended since the sorted rewrite" is inferred entirely from the current snapshot's operation still being replace.
  • Alternative not taken: compare snapshot timestamps or track append counts in the cursor — more state, and the operation check is exactly what the server's own alreadyCompacted skip relies on.
  • Check: every write that should re-open the partition flips the current operation (mover appends commit append, retention commits deletes); a hypothetical future writer committing replace would be silently blessed only if it also declares the sort order.

4. Re-baseline moves only the baseline; compactedAt and the epoch stay · contract / shape

src/core/cache/maintenance.js:872

return{ ...cursor,compaction: { ...compaction,resettleBaselineFiles: liveDataFiles}}
  • Decision: the recognition write is deliberately not a compaction record: no epoch bump, no compactedAt restamp, everything else spread through.
  • Alternative not taken: stamp a rebaselinedAt/refresh compactedAt (would make prod cursors indistinguishable from a kernel rewrite, the exact signal the Maintenance compaction shreds the export-time sorted rewrite: due-test is blind to foreign replace snapshots #700 verification depends on) or bump the epoch (would retire a generation that was never replaced).
  • Check: no consumer assumes resettleBaselineFiles and compactedAt were written by the same run.

5. Pre-replace data files are left on disk until eviction · lifecycle

llp/0207-foreign-sorted-replace-convergence.decision.md:70

- The pre-replace data files stay on disk until day eviction: snapshot
expiry never deletes data files, and the orphan sweep only reclaims
non-live generation dirs, so a recognized partition holds both file
  • Decision: accept ~2x disk per exported day for the cache window; reclamation is deferred to Cache maintenance never reclaims data files unreferenced by the current snapshot inside a live generation #704 rather than done at recognition time.
  • Alternative not taken: delete unreferenced files inside the live generation during recognition — cheaper on disk but a new, riskier deletion path racing in-flight readers, in the same PR as a behavior fix.
  • Check: prod disk headroom tolerates the window (the shredding rewrite previously masked this by retiring whole generations nightly).

Honorable mentions (real but lower-stakes): src/core/cache/maintenance.js:296--force bypasses recognition entirely (operator override wins); src/core/commands/query.js:325 — the CLI prints dataFilesBefore as the rebaseline count, which is exactly the value written to the cursor; src/core/cache/maintenance.js:102 — a new hyp_rebaselines counter rather than overloading hyp_compactions.

Generated by /decision-map. Advisory — directs attention, casts no verdict.

@philcunliffe
philcunliffe merged commit 21b5913 into masterAug 10, 2026
9 checks passed
@philcunliffe
philcunliffe deleted the fix/issue-700 branch August 10, 2026 22:45
philcunliffe pushed a commit that referenced this pull request Aug 10, 2026
Master's highest LLP is 0205. PR #703 (uninstall-detaches-clients) also
mints llp/0206 for an unrelated decision, and PR #701 (fix/issue-700)
already claims 0207, so this PR's compaction-file-size decision moves to
0208, the next free number. Mechanical renumber only (LLP 0156): the
filename, the doc header, the Extended-by forward-ref on LLP 0199, and
every @ref annotation in src/ and test/ move together; no meaning
changes.
Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Review round 1 - head 287b67b

Verdict: approve. The recognition predicate is sound, the baseline bookkeeping cannot reintroduce the LLP 0199 loop, the single-metadata-load claim holds, the LLP edit is a clean forward-ref, and the tests build a real foreign replace via icebergRewrite. Four improvements were raised and all four are fixed in cc82d6f. Gates: npm test 3893 pass / 0 fail, npm run typecheck clean, smokes cache_lifecycle_maintenance / incremental_sink_compaction / cache_roundtrip ok, no em dashes, no semicolons.

False-positive enumeration (the main ask)

Every writer that could put a replace at the head of a cache table was traced:

  • The kernel's own rewrite cannot false-positive.compactGeneration writes into a fresh generation directory via appendRowsToTable, so the new live table's current snapshot is always append. The only replace producer in icebird is write/rewrite.js:220, and its only in-repo caller is src/core/commands/sink.js (export sink tables, not the cache datasets tree).
  • Retention deletes cannot false-positive - they use position-deletes (operation: 'delete'), flipping the current operation away from replace.
  • Snapshot expiry is safe - expireSnapshots explicitly skips the current id, so the replace stays current and stays recognized.
  • A table with no declared sort order is correctly not blessed.sortColumnsFromMetadata reads default-sort-order-id, which is only ever set at icebergCreateTable time; ordinary local ingest never declares one, so on a plain HypAware install foreignSortedReplace is unreachable by construction. This is the strongest part of the guard and it is directly tested.
  • Partial/aborted replace is not reachable as current - icebird commits metadata atomically, so a crash leaves orphan parquet with the prior metadata current.
  • A snapshot id missing from metadata.snapshots yields undefined, predicate false, compaction proceeds. Fails safe.

False-negative direction: the only miss is a replace followed by an append before the tick, which then rewrites. That is exactly the documented "no append since" semantics, correct for a still-live partition, and covered by the third leg of the first test.

The 0199 loop cannot return

rebaselineCursor moves only compaction.resettleBaselineFiles; epoch, rowCount, layout, tableDir, retention and compactedAt are preserved, with an isPlainObject guard against a garbage compaction. On the next tick dataFilesBefore === resettleBaselineFiles, so grewSinceCompaction is false and the partition drops out before any metadata load, while both append paths carry cursor.compaction through unchanged so a later append re-arms it. No path re-arms recognition against itself.

src/core/commands/query.js is germane, not scope creep: 7 lines of rendering, without which a rebaseline run prints nothing for the partition and reads as "nothing due".

Findings

1. minor - the predicate does not verify the replace was written under the declared sort order. It tests two independent facts (the table declares a non-empty default sort order now, and the current snapshot is a replace), never that this replace was committed under that order. icebird stamps sort_order_id per data file, so a replace written under order 0 on a table declaring order 1 would still be blessed. Not reachable from anything in this repo today, and LLP 0207 #foreign-replace states the two-fact test as the decision, so this is a documented narrowing rather than a defect. Deliberately not changed: tightening it costs a manifest read and would break the single-metadata-load property, so it belongs in a separate decision, not a drive-by.

2. minor - a full projected scan whose result was unconditionally discarded.hasResettleCandidate was computed before the recognition test, but recognition outranks it, so on the first tick after each foreign replace a partition with no fallback row paid a complete single-column scan of the day purely to throw the answer away.
Fixed: a cheap compactionDue check is hoisted above the scan and the scan is gated on !compactionDue && settle, with shouldCompact = compactionDue || hasResettle algebraically identical to before. Verified empirically rather than by reasoning: instrumenting hasResettleCandidate gives zero calls across the 36 retention/maintenance tests including every LLP 0207 recognition test, and still 4 calls in cache-resettle-sweep.test.js where the fallback-row scan is the only possible trigger. Instrumentation reverted before commit.

3. nit - Extended-by: lacked the corpus's explanatory gloss (a bare **Extended-by:** LLP 0207, where 0012/0017/0036/0041/0191 all carry a parenthetical and several link the target). Fixed to a linked, glossed form.

4. nit - MaintenanceReport gained no totalRebaselined, so query.js re-derived the count by filtering report.partitions, asymmetric with totalCompacted. Fixed: the aggregate is now on the report and read directly.

5. nit - the maintenance.partition span recorded nothing about a rebaseline, leaving the hyp_rebaselines counter as the only in-daemon signal. Fixed with a single additive span.setAttribute('rebaselined', ...), kept minimal to ease the #698 merge.

Cross-PR flag for whoever merges: #698 (fix/issue-697)

The two are complementary and compose correctly, but three things matter:

  1. Neither obsoletes the other.Cache compaction sizes files by bytes written, not the in-memory batch estimate #698 fixes the consequence (the rewrite produced ~0.5MB files because the file boundary followed the in-memory batch estimate). Maintenance recognizes a foreign sorted replace and re-baselines instead of shredding it (#700) #701 fixes the trigger (do not rewrite a foreign sorted day at all). Even with Cache compaction sizes files by bytes written, not the in-memory batch estimate #698, a kernel rewrite of an exported day would destroy the server's global session sort order (the kernel re-declares the sort order on the new table but writes rows in scan order, never re-sorting) and would still burn a full-partition rewrite plus doubled disk. Maintenance recognizes a foreign sorted replace and re-baselines instead of shredding it (#700) #701 remains necessary after Cache compaction sizes files by bytes written, not the in-memory batch estimate #698.
  2. Cache compaction sizes files by bytes written, not the in-memory batch estimate #698 cannot false-positive Maintenance recognizes a foreign sorted replace and re-baselines instead of shredding it (#700) #701's predicate - its streaming sink still writes into the fresh generation directory via append-shaped commits, so a Cache compaction sizes files by bytes written, not the in-memory batch estimate #698-written generation's current snapshot is still append.
  3. Textual overlap is at two hot spots and the merge is easy to get wrong. Both rewrite the if (shouldCompact) block in maintainGeneration and compactGeneration's preamble (Maintenance recognizes a foreign sorted replace and re-baselines instead of shredding it (#700) #701removes the in-function metadata load and takes tableInfo as a parameter; Cache compaction sizes files by bytes written, not the in-memory batch estimate #698 wraps the body in try/finally and leaves the load in place). A resolution that takes Cache compaction sizes files by bytes written, not the in-memory batch estimate #698's compactGeneration wholesale would silently drop Maintenance recognizes a foreign sorted replace and re-baselines instead of shredding it (#700) #701's tableInfo parameter and reintroduce a second metadata load per compaction. Do not resolve either hunk by "take theirs".

Low priority: once both land, LLP 0207 #context justifies itself partly by the ~0.5MB-files harm that LLP 0208 (#698's renumbered doc) independently fixes, so the standing harm becomes "loses the global session sort, burns a pointless full rewrite, doubles disk". Worth a one-line Related: cross-ref when #698 lands.

The head moved to cc82d6f, so the next tick reviews it as round 2.

philcunliffe pushed a commit that referenced this pull request Aug 10, 2026
… file-size fix
Both changes touch `maintainGeneration` and `compactGeneration`, and they are
complementary: master (#701, LLP 0207) decides *when* a partition is rewritten
at all, this branch decides how the rewrite sizes its output files. Each hunk
is composed rather than taken from one side.
- `maintainCache`: keep this branch's `async (span) =>` callback and its
`compacted` / `data_files_before` / `data_files_after` / `rows` /
`bytes_written` attributes, and pass master's `rebaselinesCounter` through to
`maintainGeneration`.
- `maintainGeneration`: keep master's three-way branch (foreign sorted replace
re-baselines, dry run reports, otherwise compact) with its single
`loadCompactionTableInfo` call, and re-add this branch's
`r.compactedBytesWritten = result.bytesWritten` inside the compact arm.
- `compactGeneration`: keep master's `tableInfo` parameter and the absence of
the in-function metadata load (one metadata load per compaction), and keep
this branch's streaming sink, its try/finally, and its `abort()` path. The
JSDoc keeps master's `@param tableInfo` and this branch's `bytesWritten`
return.
- `cache-retention-maintenance.test.js`: union of both import lists.
A generation written by the streaming sink commits through
`stageSnapshotForAppend`, so its current snapshot is `append`, never `replace`:
`foreignSortedReplace` cannot fire on our own rewrite even though we now carry
the declared sort order forward.
Also renumbers this branch's LLP 0208 to 0209: PR #705 independently took 0208
from master's high-water mark. Filename, header, every `@ref LLP 0208#...` in
src and test, and LLP 0199's `Extended-by:` line (which now names both 0207 and
0209) move with it.
philcunliffe added a commit that referenced this pull request Aug 11, 2026
…h estimate (#698)
* Cache compaction sizes files by bytes written, not the batch estimate
`compactGeneration` wrote one data file per flushed batch, and a batch
flushes at `COMPACT_BATCH_SIZE` rows or `compact_batch_bytes` (32 MB) of
*estimated in-memory* bytes. That byte cap is a real OOM guard, but it
also decided file size. An `ai_gateway_messages` row estimates ~140 KB in
memory and compresses ~70x, so the guard fired after ~230 rows and
produced a ~0.5 MB file: `target_file_bytes` (128 MB) was unreachable by
construction. Production saw a 52,329-row day partition rewritten into
230 files averaging 463 KB, with `compact_avg_file_bytes` ready to
re-flag every partition forever had LLP 0199's baseline gate not landed.
Decouple the two bounds. A flush is now a parquet row group, appended to
a data file that stays open until the bytes actually written reach
`target_file_bytes`. Peak heap is still one batch, plus one row group of
encoded bytes: the local Iceberg writer implements hyparquet-writer's
`flush()` hook, so a large output file is no longer a large allocation.
All of a rewrite's files commit as one snapshot instead of one per batch.
Writing row groups directly needs icebird's private iceberg-to-parquet
schema mapping. Rather than copy it (and drift), the cache writes a
zero-record parquet file in memory with icebird's own `writeParquet` and
reads the schema back out of the footer. Manifest, snapshot, and metadata
commit all remain icebird's, reached through the `icebird/src/*.js` deep
imports the cache already uses. Tables with nested columns fall back to
the previous one-file-per-batch path.
Adds LLP 0206 and a forward-ref on LLP 0199, whose baseline gate is
unchanged and no longer load-bearing against an unsatisfiable heuristic.
Co-Authored-By: Claude <noreply@anthropic.com>
* Bound the row-group metadata a streaming compaction holds open
Round-1 review of the compaction file-size change.
The OOM guard was not preserved. `flush()` drains encoded page bytes, but
`ParquetWriter.write` pushes a `ColumnChunk` per column per row group onto
`row_groups`, and each chunk's `statistics` holds the RAW, untruncated JS
`min_value`/`max_value`; truncation to 16 units happens only in `finish()`.
So an open file pinned two full column values per row group per column for
its whole life. Measured peak retained heap over 100 row groups of one
string column: 4.4 MB at 20 KB values, 27.7 MB at 140 KB, 109.8 MB at
560 KB, and 38.7 MB at issue #697's shape (70 KB values, 278 row groups) -
more than `compact_batch_bytes` itself, times up to `MAX_OPEN_FILES`. A
control where every row shared one string object stayed at 0.1 MB, locating
the growth in the retained bounds.
`openStreamingAppend` now charges each row group an upper bound on what it
pins (widest value per column counted twice, plus a measured ~1 KB per
column chunk) against a global `MAX_OPEN_STATS_BYTES` budget of 32 MiB, and
closes the file holding the most when the budget is reached. A file rolls on
`target_file_bytes` or on the budget, whichever binds first. The budget is
global rather than per-file because a per-file cap must be divided by
`MAX_OPEN_FILES` to bound the aggregate, which for fat rows would force
files back to single-digit megabytes: the defect this change exists to fix.
Same measurements after: 4.5 MB, 16.0 MB, 15.5 MB, 16.3 MB.
`target_file_bytes` is still unreachable for `ai_gateway_messages`, and LLP
0206 said otherwise. That dataset declares identity partitioning on
(session_id, conversation_id, cwd, date), a data file cannot span partition
tuples, and a per-session file never approaches 128 MB. Measured: 600 fat
rows across 10/30/100 distinct sessions compact to 10/30/100 files of 1.7 to
3.0 KB, independent of `target_file_bytes`. The Consequences section now
states the real bound (file count after a rewrite is about the number of
distinct tuples; `target_file_bytes` only binds within a tuple) and drops
the claim that LLP 0199's baseline gate is no longer load-bearing. It is.
Fix an fd and temp-file leak on the error path. The rewrite now holds
writers open across the whole scan, and only `finish()` closes the local
writer's descriptor and unlinks its `.tmp.*` file. `StreamingTableAppend`
gains `abort()`, the local writer gains `abort()`, `closeFile` aborts a
writer whose `finish()` threw, and `compactGeneration` wraps the scan in
`try/finally`. Without it a partition that throws every tick leaked up to 64
descriptors per tick.
The second new test did not discriminate: `dataFilesAfter > 1` is also true
of the pre-fix code. It now asserts at least one output file holds multiple
row groups, which only appending into an already-open file can produce
(verified failing against the pre-PR commit, which yields 21 files of 1 row
group each).
Two comment-accuracy fixes. "The intrinsic cache only ever declares
primitives" was false: `ColumnSpec.type: 'JSON'` maps to iceberg `variant`,
which is a two-leaf parquet group, and `ai_gateway_messages` declares seven.
Variant round-trips correctly, so the comment is corrected rather than the
guard. The guard is widened for a real gap though: iceberg `unknown` maps to
no parquet element while `columnNames` includes every field, which would
misalign positional `columnData`, so the append now falls back unless
icebird's mapping produced exactly one top-level element per field.
`stream_append.js` emitted no telemetry of its own, so a streaming-append
failure was only visible as an aggregate on the partition span. It now logs
file open, close (with the roll reason: target bytes, stats budget, open
file cap, or end of append), the append summary, and abort, with counts and
bytes only.
Co-Authored-By: Claude <noreply@anthropic.com>
* Park the descriptor, keep the file: compaction stopped converging above 64 tuples
`MAX_OPEN_FILES = 64` retired the oldest open output file when a rewrite
fanned out past 64 partition tuples, and that retire was Belady-cyclic
against the access pattern a compaction actually has. The scan walks the
old generation's data files in manifest order, a data file holds exactly
one tuple, so a tuple recurs about every N files for N tuples. Above 64
every file was retired before its tuple came round again, so every output
file closed holding one row group and the rewrite emitted one file per
input row group: the count the pre-streaming code produced. Measured
through `maintainCache` (identity partitioning on one column, 3000 fat
rows, 10 ingest waves, 128 MB target): 30 tuples compacted 300 files to
30, 64 compacted 640 to 64, and 100 compacted 1000 to 1000. That is the
shape of `ai_gateway_messages`, which partitions on identity
(session_id, conversation_id, cwd, date), so the change was very likely a
no-op for issue #697's 52,329-row day partition.
A descriptor and an open file are different resources, and only the
first needs a cap of 64. The local writer gains `park()`: flush, close
the descriptor, drop `ByteWriter`'s never-shrinking buffer, and keep the
temp file and byte offset so the next row group for that tuple reopens
it in append mode. `openStreamingAppend` now caps descriptors, not open
files, so a file closes only on `target_file_bytes`, on the stats
budget, or at the end of the append. Same measurement after: 30, 64,
100, and 200 tuples compact 2000 files to 230, 500 compact 3000 to 720.
Raising the cap instead would only have moved the cliff to the new
number, and no descriptor budget scales with tuples.
`retainedValueBytes` charged every string at the UTF-16 upper bound. V8
stores a string one byte per character unless it holds a character above
U+00FF, and a UTF-8 byte length equal to the character count proves the
string is ASCII, which `ai_gateway_messages` payloads are. The 2x
overcharge halved how much an open file could absorb before the stats
budget rolled it: 200 tuples x 10 batches x 35 K-character ASCII values
went from 811 files (11.4 MB peak retained) to 503 files (18.6 MB),
against a 32 MiB budget. The same shape with two-byte values is
unchanged at 811 files.
`accumulateStats` now runs after `parquet.write` rather than before, so
a write that throws cannot leave the file's metrics counting a row group
it never wrote.
Two regression tests, both verified failing on the previous commit: 100
sessions across 10 ingest waves through `maintainCache` (1000 files ->
1000 before, -> 100 after), and a direct 200-tuple streaming append that
asserts exactly one data file per tuple (2000 before) while the process
descriptor count stays inside the cap.
LLP 0206's Consequences said the file count after a rewrite is
approximately the tuple count. It states the real bound now: one file
per tuple is the floor, plus one for every file the byte target or the
retained-metadata budget rolls early, and the single-wave measurement
that supported the old claim is replaced with a multi-wave one, because
one wave gives each tuple exactly one row group and never exercises
holding a file open across a scan at all.
Co-Authored-By: Claude <noreply@anthropic.com>
* Renumber LLP 0206 to 0208: the number is taken twice across open PRs
Master's highest LLP is 0205. PR #703 (uninstall-detaches-clients) also
mints llp/0206 for an unrelated decision, and PR #701 (fix/issue-700)
already claims 0207, so this PR's compaction-file-size decision moves to
0208, the next free number. Mechanical renumber only (LLP 0156): the
filename, the doc header, the Extended-by forward-ref on LLP 0199, and
every @ref annotation in src/ and test/ move together; no meaning
changes.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: test <test@example.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: test <test@test.com>
Co-authored-by: neutral <neutral@hypaware.local>
philcunliffe added a commit that referenced this pull request Aug 12, 2026
…ettle scan, MaintenanceReport symmetry, span visibility, LLP gloss) (#706)
* Carry PR #701's stranded review fixes: skip the wasted resettle scan, MaintenanceReport symmetry, span visibility, LLP gloss
PR #701 was squash-merged at head 287b67b, before the round-2 review fixes
in cc82d6f were pushed, so four verified fixes never reached master.
- Hoist a cheap `compactionDue` check above the `hasResettleCandidate` row
scan and gate the scan on `!compactionDue`. Recognition of a foreign
sorted `replace` outranks the resettle check, so the first tick after
each foreign replace paid a complete single-column scan of the day
purely to discard the answer.
- Add `totalRebaselined` to `MaintenanceReport` beside `totalCompacted`,
and let `query.js` read it instead of re-deriving the count.
- Tag the enclosing `maintenance.partition` span with `rebaselined`; the
`hyp_rebaselines` counter carries only the dataset, not the partition.
- Give LLP 0199's bare `Extended-by: LLP 0207` the corpus's linked and
glossed form.
Co-Authored-By: Claude <noreply@anthropic.com>
* Pin PR #706's carried fixes with tests; sharpen two @ref citations
Round-1 review of #706 approved the carry but flagged that none of the
four stranded fixes was pinned by a committed test, plus two @ref nits.
- Add three tests to test/core/cache-retention-maintenance.test.js, in
the foreign-sorted-replace block:
- `totalRebaselined === 1` on a re-baselining run, and `=== 0` once
converged (pins the MaintenanceReport symmetry fix).
- a capturing TracerProvider asserting the maintenance.partition span
carries `rebaselined: true` (pins the span-attribute fix).
- a partition already due for compaction: assert the resettle
candidate's data file is read once, not twice, by spying on
`fs.readFileSync` (pins the scan-skip fix). hasResettleCandidate's
return value is otherwise unobservable once compactionDue is true
(it's discarded via `||`), so this is the cheapest honest signal
available; each new test was verified to fail when its
corresponding fix is reverted.
- Retarget the scan-skip gate's @ref from LLP 0207#foreign-replace to
#outranks-resettle: the gloss ("recognition ... still outranks it")
is that anchor's actual subject, not the recognition test's.
- Drop the @ref on the span-attribute comment: #re-baseline settles the
cursor-write shape, not telemetry, so citing it was close to
mechanical. The prose rationale stays; it's the useful part.
- Amend the PR body's claim about item 1: `needsCompaction` (pure,
read-only) is now evaluated unconditionally in the re-settle path
where the old `||` short-circuited past it, so more moved than "only
the scan's side effect is skipped."
Co-Authored-By: Claude <noreply@anthropic.com>
* Sharpen scan-skip test to stack attribution; fix PR #706 body citations
- test/core/cache-retention-maintenance.test.js: the resettle-scan-skip
test asserted a global .parquet readFileSync count of 1 for the whole
maintainCache tick, not just reads the gate governs. Any future second
legitimate read elsewhere in the tick would break it with a message
that misdirects the next reader. Switch to capturing a stack trace per
.parquet read and asserting none pass through hasResettleCandidate,
attributing each read to its caller instead of counting tick-wide.
Verified both directions: passes at this head (3x, no flake), and
fails with the expected message when the !compactionDue && gate in
src/core/cache/maintenance.js is reverted, showing hasResettleCandidate
in the offending stack.
- PR body: item 2's exemplar list cited llp/0012:9, 0017:9, 0036:9,
0041:8, 0191:9 as using the linked-and-glossed Extended-by form,
but only 0191 actually carries a link; the other four are gloss-only.
Replaced with docs confirmed to use the linked form: llp/0106:9,
0129:9, 0158:9, 0180:9, 0182:9, 0188:9, 0190:9, 0191:9. (0201, also
suggested, only has a backward "Extends" pointer, not "Extended-by",
so it was excluded.)
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: test <test@test.com>
Co-authored-by: Claude <noreply@anthropic.com>
philcunliffe added a commit that referenced this pull request Aug 13, 2026
* Carry PR #701's stranded review fixes: skip the wasted resettle scan, MaintenanceReport symmetry, span visibility, LLP gloss
PR #701 was squash-merged at head 287b67b, before the round-2 review fixes
in cc82d6f were pushed, so four verified fixes never reached master.
- Hoist a cheap `compactionDue` check above the `hasResettleCandidate` row
scan and gate the scan on `!compactionDue`. Recognition of a foreign
sorted `replace` outranks the resettle check, so the first tick after
each foreign replace paid a complete single-column scan of the day
purely to discard the answer.
- Add `totalRebaselined` to `MaintenanceReport` beside `totalCompacted`,
and let `query.js` read it instead of re-deriving the count.
- Tag the enclosing `maintenance.partition` span with `rebaselined`; the
`hyp_rebaselines` counter carries only the dataset, not the partition.
- Give LLP 0199's bare `Extended-by: LLP 0207` the corpus's linked and
glossed form.
Co-Authored-By: Claude <noreply@anthropic.com>
* Pin PR #706's carried fixes with tests; sharpen two @ref citations
Round-1 review of #706 approved the carry but flagged that none of the
four stranded fixes was pinned by a committed test, plus two @ref nits.
- Add three tests to test/core/cache-retention-maintenance.test.js, in
the foreign-sorted-replace block:
- `totalRebaselined === 1` on a re-baselining run, and `=== 0` once
converged (pins the MaintenanceReport symmetry fix).
- a capturing TracerProvider asserting the maintenance.partition span
carries `rebaselined: true` (pins the span-attribute fix).
- a partition already due for compaction: assert the resettle
candidate's data file is read once, not twice, by spying on
`fs.readFileSync` (pins the scan-skip fix). hasResettleCandidate's
return value is otherwise unobservable once compactionDue is true
(it's discarded via `||`), so this is the cheapest honest signal
available; each new test was verified to fail when its
corresponding fix is reverted.
- Retarget the scan-skip gate's @ref from LLP 0207#foreign-replace to
#outranks-resettle: the gloss ("recognition ... still outranks it")
is that anchor's actual subject, not the recognition test's.
- Drop the @ref on the span-attribute comment: #re-baseline settles the
cursor-write shape, not telemetry, so citing it was close to
mechanical. The prose rationale stays; it's the useful part.
- Amend the PR body's claim about item 1: `needsCompaction` (pure,
read-only) is now evaluated unconditionally in the re-settle path
where the old `||` short-circuited past it, so more moved than "only
the scan's side effect is skipped."
Co-Authored-By: Claude <noreply@anthropic.com>
* Sharpen scan-skip test to stack attribution; fix PR #706 body citations
- test/core/cache-retention-maintenance.test.js: the resettle-scan-skip
test asserted a global .parquet readFileSync count of 1 for the whole
maintainCache tick, not just reads the gate governs. Any future second
legitimate read elsewhere in the tick would break it with a message
that misdirects the next reader. Switch to capturing a stack trace per
.parquet read and asserting none pass through hasResettleCandidate,
attributing each read to its caller instead of counting tick-wide.
Verified both directions: passes at this head (3x, no flake), and
fails with the expected message when the !compactionDue && gate in
src/core/cache/maintenance.js is reverted, showing hasResettleCandidate
in the offending stack.
- PR body: item 2's exemplar list cited llp/0012:9, 0017:9, 0036:9,
0041:8, 0191:9 as using the linked-and-glossed Extended-by form,
but only 0191 actually carries a link; the other four are gloss-only.
Replaced with docs confirmed to use the linked form: llp/0106:9,
0129:9, 0158:9, 0180:9, 0182:9, 0188:9, 0190:9, 0191:9. (0201, also
suggested, only has a backward "Extends" pointer, not "Extended-by",
so it was excluded.)
Co-Authored-By: Claude <noreply@anthropic.com>
* Scan-skip test goes blind if the call chain grows 3 frames (#707)
`a partition already due for compaction skips the resettle-candidate row
scan` attributes each `.parquet` read to its caller by capturing
`new Error().stack` inside a `readFileSync` mock and asserting no stack
passes through `hasResettleCandidate`. The discriminating frame sits at
frame 8 of 10, and `Error.stackTraceLimit` defaults to 10. Three more
frames anywhere between the mock and the caller (an icebird refactor,
extra node:test mock internals, a wrapper in `resolver.js`) drop that
frame, and a negative "no stack mentions X" assertion then passes
vacuously: the test stays green even with the `!compactionDue &&` gate
reverted. The `stacks.length > 0` sanity check only proved a read
happened, not that the stacks were attributable, and the
`new Error().stack ?? ''` fallback would store an unattributable empty
string without complaint.
Demonstrated by running with `Error.stackTraceLimit = 7` (standing in for
the three extra frames) and the gate reverted: the old test passed.
Two guards, because they cover different routes to the same blindness:
1. Raise `Error.stackTraceLimit` to 50 while the mock is installed,
restoring the previous value in the existing `finally` so it is put
back even if the test throws. This removes the practical hazard.
2. Assert positively that some captured stack names `compactGeneration`,
the legitimate reader. It calls `scanRowsFromTable` from exactly the
same depth as `hasResettleCandidate` does, so any truncation deep
enough to hide the frame the negative assertion hunts for also hides
this one and fails the test loudly. Asserting on `scanRowsFromTable`
itself would not work: it sits one frame shallower and survives
truncation that has already blinded the real check. This guard also
catches the empty-string fallback.
Under the same truncation with the gate still reverted, the hardened
test fails; with the gate restored it passes, truncated or not.
No production code changed.
Co-Authored-By: Claude <noreply@anthropic.com>
* LLP 0199: keep master's landed 0207 gloss, add only the link
The conflict resolution replaced the 0207 gloss that landed on master with
this branch's alternative wording. Both describe LLP 0207 accurately, but
rewording a gloss already on an Accepted doc is a content edit, where adding
the link is the permitted mechanical one. Keeping master's text also matches
how PR #706 resolved the same line, so the two open PRs no longer diverge and
whichever merges second will not re-conflict here.
---------
Co-authored-by: test <test@test.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: neutral <neutral@hyparam.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Maintenance compaction shreds the export-time sorted rewrite: due-test is blind to foreign replace snapshots

1 participant

@philcunliffe