From dcae50bfe01f672c27608a16c538dc8f1c9342f9 Mon Sep 17 00:00:00 2001 From: test Date: Mon, 10 Aug 2026 23:53:18 +0000 Subject: [PATCH 1/3] 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 --- ...tenance-compaction-convergence.decision.md | 2 +- src/core/cache/maintenance.js | 26 ++++++++++++++++--- src/core/cache/types.d.ts | 1 + src/core/commands/query.js | 3 +-- 4 files changed, 26 insertions(+), 6 deletions(-) diff --git a/llp/0199-maintenance-compaction-convergence.decision.md b/llp/0199-maintenance-compaction-convergence.decision.md index a0232ef9..606b4240 100644 --- a/llp/0199-maintenance-compaction-convergence.decision.md +++ b/llp/0199-maintenance-compaction-convergence.decision.md @@ -6,7 +6,7 @@ **Author:** Kenny / Claude **Date:** 2026-08-07 **Related:** LLP 0027 -**Extended-by:** LLP 0207 +**Extended-by:** [LLP 0207](./0207-foreign-sorted-replace-convergence.decision.md) (a second convergence source: a foreign sorted `replace` re-baselines the gate instead of triggering a rewrite) > Maintenance stops re-flagging already-compacted partitions: a partition is > only compaction-due when its live data-file count has moved off the count diff --git a/src/core/cache/maintenance.js b/src/core/cache/maintenance.js index f1d97137..4525ceaa 100644 --- a/src/core/cache/maintenance.js +++ b/src/core/cache/maintenance.js @@ -10,7 +10,7 @@ import { loadLatestFileCatalogMetadata, } from 'icebird' -import { Attr, getMeter, withSpan } from '../observability/index.js' +import { Attr, getActiveSpan, getMeter, withSpan } from '../observability/index.js' import { inferColumnType } from './migrate.js' import { discoverCachePartitions, readCursorSync, tryReadCursorSync, writeCursor } from './partition.js' import { datasetsRoot } from './paths.js' @@ -117,6 +117,7 @@ export async function maintainCache(opts) { const reports = [] let totalSnapshotsExpired = 0 let totalCompacted = 0 + let totalRebaselined = 0 for (const part of partitions) { // Always work one partition before the budget can cut the tick short: @@ -157,6 +158,7 @@ export async function maintainCache(opts) { reports.push(report) totalSnapshotsExpired += report.snapshotsExpired if (report.compacted) totalCompacted++ + if (report.rebaselined) totalRebaselined++ } if (!opts.dryRun) { @@ -167,6 +169,7 @@ export async function maintainCache(opts) { partitions: reports, totalSnapshotsExpired, totalCompacted, + totalRebaselined, dryRun: opts.dryRun ?? false, elapsedMs: Date.now() - startMs, } @@ -275,6 +278,13 @@ async function maintainGeneration(r, cursor, cfg, opts, settle, snapshotsExpired // compact_avg_file_bytes), and the tick budget is burned rewriting the // same partitions while the rest of the walk starves. const grewSinceCompaction = dataFilesBefore !== resettleBaselineFiles(cursor) + // Cheap dueness check first: file-count and byte-size heuristics only, + // no metadata load and no row scan. A foreign sorted replace almost + // always lands here (its baseline mismatch alone doesn't imply the + // size heuristics fire), so gating the expensive re-settle scan behind + // this check means the common "recognized, nothing to scan for" tick + // never pays for one. + const compactionDue = opts.force || (grewSinceCompaction && needsCompaction(liveDir, cfg)) // @ref LLP 0027#re-settle-sweep: a partition holding a committed // fallback row may carry a split twin pair the flush-time settle // never collapsed; force a rewrite so the sweep can re-settle it even @@ -283,10 +293,15 @@ async function maintainGeneration(r, cursor, cfg, opts, settle, snapshotsExpired // line never lands (harness aux, wire-only reminders) - from forcing a // full rewrite every tick, and skips the attributes scan entirely when // nothing new has flushed. - const hasResettle = settle + // @ref LLP 0207#foreign-replace [constrained-by]: when the cheap check + // above already made compaction due, the scan's answer can never + // change the outcome (recognition, tested below, still outranks it), + // so skip it: only run the scan when it might be the sole reason to + // compact. + const hasResettle = !compactionDue && settle ? grewSinceCompaction && await hasResettleCandidate(liveDir) : false - const shouldCompact = opts.force || hasResettle || (grewSinceCompaction && needsCompaction(liveDir, cfg)) + const shouldCompact = compactionDue || hasResettle if (shouldCompact) { const tableInfo = await loadCompactionTableInfo(liveDir) // @ref LLP 0207#foreign-replace [implements]: a baseline mismatch whose @@ -299,6 +314,11 @@ async function maintainGeneration(r, cursor, cfg, opts, settle, snapshotsExpired // the sorted layout every night. An explicit --force still rewrites. if (!opts.force && foreignSortedReplace(tableInfo)) { r.rebaselined = true + // @ref LLP 0207#re-baseline: the counter proves a rebaseline happened + // at all, but it carries only the dataset; tagging the enclosing + // maintenance.partition span names the partition, so a trace query + // finds which day re-baselined without cross-referencing the counter. + getActiveSpan()?.setAttribute('rebaselined', true) if (!opts.dryRun) { await writeCursor(r.path, rebaselineCursor(cursor, dataFilesBefore)) rebaselinesCounter.add(1, { [Attr.DATASET]: r.dataset }) diff --git a/src/core/cache/types.d.ts b/src/core/cache/types.d.ts index 45084d72..00d529d6 100644 --- a/src/core/cache/types.d.ts +++ b/src/core/cache/types.d.ts @@ -232,6 +232,7 @@ export interface MaintenanceReport { partitions: MaintenancePartitionReport[] totalSnapshotsExpired: number totalCompacted: number + totalRebaselined: number dryRun: boolean elapsedMs: number } diff --git a/src/core/commands/query.js b/src/core/commands/query.js index 1cfac29d..b22afa21 100644 --- a/src/core/commands/query.js +++ b/src/core/commands/query.js @@ -327,8 +327,7 @@ export async function runQueryMaintain(argv, ctx) { ctx.stdout.write(` ${label}: ${actions.join(', ')}\n`) } } - const rebaselined = report.partitions.filter((p) => p.rebaselined).length - const rebaselineNote = rebaselined > 0 ? `, ${rebaselined} rebaselined` : '' + const rebaselineNote = report.totalRebaselined > 0 ? `, ${report.totalRebaselined} rebaselined` : '' ctx.stdout.write(`maintenance: ${report.totalSnapshotsExpired} snapshots expired, ${report.totalCompacted} partitions compacted${rebaselineNote} (${report.elapsedMs}ms)\n`) return 0 } From abdb17de486bbe65e1856dcab18b6fb9ddde4727 Mon Sep 17 00:00:00 2001 From: test Date: Tue, 11 Aug 2026 00:44:55 +0000 Subject: [PATCH 2/3] 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 --- src/core/cache/maintenance.js | 10 +- test/core/cache-retention-maintenance.test.js | 91 +++++++++++++++++++ 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/src/core/cache/maintenance.js b/src/core/cache/maintenance.js index 4525ceaa..7080c0fe 100644 --- a/src/core/cache/maintenance.js +++ b/src/core/cache/maintenance.js @@ -293,7 +293,7 @@ async function maintainGeneration(r, cursor, cfg, opts, settle, snapshotsExpired // line never lands (harness aux, wire-only reminders) - from forcing a // full rewrite every tick, and skips the attributes scan entirely when // nothing new has flushed. - // @ref LLP 0207#foreign-replace [constrained-by]: when the cheap check + // @ref LLP 0207#outranks-resettle [constrained-by]: when the cheap check // above already made compaction due, the scan's answer can never // change the outcome (recognition, tested below, still outranks it), // so skip it: only run the scan when it might be the sole reason to @@ -314,10 +314,10 @@ async function maintainGeneration(r, cursor, cfg, opts, settle, snapshotsExpired // the sorted layout every night. An explicit --force still rewrites. if (!opts.force && foreignSortedReplace(tableInfo)) { r.rebaselined = true - // @ref LLP 0207#re-baseline: the counter proves a rebaseline happened - // at all, but it carries only the dataset; tagging the enclosing - // maintenance.partition span names the partition, so a trace query - // finds which day re-baselined without cross-referencing the counter. + // The counter proves a rebaseline happened at all, but it carries + // only the dataset; tagging the enclosing maintenance.partition span + // names the partition, so a trace query finds which day re-baselined + // without cross-referencing the counter. getActiveSpan()?.setAttribute('rebaselined', true) if (!opts.dryRun) { await writeCursor(r.path, rebaselineCursor(cursor, dataFilesBefore)) diff --git a/test/core/cache-retention-maintenance.test.js b/test/core/cache-retention-maintenance.test.js index c8947519..9ca203c2 100644 --- a/test/core/cache-retention-maintenance.test.js +++ b/test/core/cache-retention-maintenance.test.js @@ -3,6 +3,7 @@ import test from 'node:test' import assert from 'node:assert/strict' import fs from 'node:fs/promises' +import fsSync from 'node:fs' import os from 'node:os' import path from 'node:path' @@ -11,11 +12,13 @@ import { maintainCache, cacheStatus, normalizeMaintenanceConfig } from '../../sr import { appendRowsToSourceTable, readCursorSync, writeCursor } from '../../src/core/cache/partition.js' import { appendRowsToTable, currentPartitionSpec, currentSchema, readRowsFromTable, sortColumnsFromMetadata, tableExists } from '../../src/core/cache/iceberg/store.js' import { createLocalIcebergIO, tableUrlForDir } from '../../src/core/cache/iceberg/resolver.js' +import { TracerProvider } from '../../src/core/observability/runtime.js' import { fileCatalog, icebergRewrite, loadLatestFileCatalogMetadata } from 'icebird' /** * @import { ColumnSpec } from '../../hypaware-plugin-kernel-types.js' * @import { CachePartitioningDeclaration } from '../../src/core/cache/types.js' + * @import { Span } from '../../src/core/observability/runtime.js' */ /** @param {string} prefix */ @@ -853,6 +856,7 @@ test('a foreign sorted replace re-baselines the cursor instead of being rewritte // A dry run predicts the recognition without writing anything. const preview = await maintainCache({ cacheRoot, compactOnly: true, dryRun: true }) assert.equal(preview.totalCompacted, 0) + assert.equal(preview.totalRebaselined, 1, 'the report-level rebaseline count mirrors totalCompacted') assert.equal(preview.partitions[0].rebaselined, true) assert.equal( /** @type {{ resettleBaselineFiles: number }} */ (readCursorSync(partDir).compaction).resettleBaselineFiles, @@ -862,6 +866,7 @@ test('a foreign sorted replace re-baselines the cursor instead of being rewritte const first = await maintainCache({ cacheRoot, compactOnly: true }) assert.equal(first.totalCompacted, 0) + assert.equal(first.totalRebaselined, 1, 're-baselining one partition must be reflected in the report total') assert.equal(first.partitions[0].rebaselined, true) const cursor = readCursorSync(partDir) assert.equal(cursor.epoch, 0, 'no rewrite: the generation must not advance') @@ -873,6 +878,7 @@ test('a foreign sorted replace re-baselines the cursor instead of being rewritte // Converged: the baseline gate now blocks before any metadata load. const second = await maintainCache({ cacheRoot, compactOnly: true }) assert.equal(second.totalCompacted, 0) + assert.equal(second.totalRebaselined, 0, 'converged: no rebaseline happened this tick') assert.notEqual(second.partitions[0].rebaselined, true) // A late append flips the current snapshot off `replace` and moves the @@ -998,6 +1004,91 @@ test('force still rewrites a foreign sorted replace', async () => { } }) +test('a foreign sorted replace tags the maintenance.partition span with rebaselined', async () => { + const cacheRoot = await makeTmpDir('maint-foreign-span') + /** @type {Span[]} */ + const captured = [] + const provider = new TracerProvider({ + resource: { attributes: {} }, + exporters: [{ exportBatch(/** @type {Span[]} */ spans) { captured.push(...spans) } }], + }) + provider.register() + try { + const partDir = path.join(cacheRoot, 'datasets', 'ds1', 'date=2026-08-08') + const epoch0 = path.join(partDir, 'epoch=0') + for (let i = 0; i < 3; i++) { + await appendRowsToTable(epoch0, COLUMNS, [ + { id: i, value: `v${i}`, timestamp: new Date().toISOString() }, + ], { sortOrder: [{ column: 'id', direction: 'asc' }] }) + } + await commitForeignReplace(epoch0) + await writeCursor(partDir, { + epoch: 0, + rowCount: 3, + layout: 'epoch', + compaction: { compactedAt: '2026-08-08T00:00:00.000Z', resettleBaselineFiles: 99 }, + }) + + const report = await maintainCache({ cacheRoot, compactOnly: true }) + assert.equal(report.partitions[0].rebaselined, true, 'sanity: this tick recognized the foreign replace') + + const partitionSpan = captured.find((span) => span.name === 'maintenance.partition') + assert.ok(partitionSpan, 'maintenance.partition span must be exported') + assert.equal( + partitionSpan?.attributes.rebaselined, + true, + 'the span, not just the hyp_rebaselines counter, must name which partition re-baselined' + ) + } finally { + await provider.shutdown() + await fs.rm(cacheRoot, { recursive: true, force: true }) + } +}) + +test('a partition already due for compaction skips the resettle-candidate row scan', async (t) => { + // @ref LLP 0207#outranks-resettle [tests]: once the cheap file-count/size + // check alone makes compaction due, the resettle scan's answer cannot + // change `shouldCompact`, so it must not run at all. Observed here by + // counting `readFileSync` calls against the partition's one data file: + // unskipped, the resettle scan and the compaction rewrite each open it + // once (two reads); skipped, only the rewrite does (one read). + const cacheRoot = await makeTmpDir('maint-scan-skip') + try { + const partDir = path.join(cacheRoot, 'datasets', 'ds1', 'date=2026-08-08') + const epoch0 = path.join(partDir, 'epoch=0') + await appendRowsToTable(epoch0, COLUMNS, [ + { id: 1, value: 'v1', timestamp: new Date().toISOString() }, + ]) + // Never compacted: `grewSinceCompaction` is true unconditionally, so the + // only thing standing between the old code and a resettle scan is the + // `compactionDue` gate under test. + await writeCursor(partDir, { epoch: 0, rowCount: 1, compaction: null, layout: 'epoch' }) + + const spy = t.mock.method(fsSync, 'readFileSync') + + const report = await maintainCache({ + cacheRoot, + compactOnly: true, + // compact_file_count: 0 makes `needsCompaction` (and so + // `compactionDue`) true on file count alone, with no size heuristic + // involved: dueness is settled before the resettle scan would run. + config: { compact_file_count: 0 }, + storage: /** @type {any} */ ({}), + getSettleHook: () => async (rows) => rows, + }) + assert.equal(report.totalCompacted, 1, 'sanity: compaction actually ran') + + const dataFileReads = spy.mock.calls.filter((call) => String(call.arguments[0]).endsWith('.parquet')).length + assert.equal( + dataFileReads, + 1, + 'the data file must be read once, by the rewrite; a resettle scan would read it a second time first' + ) + } finally { + await fs.rm(cacheRoot, { recursive: true, force: true }) + } +}) + test('maintenance walks partitions neediest-first, not directory order', async () => { const cacheRoot = await makeTmpDir('maint-order') try { From 47fd4a4e97380c11969dca07ffac1996027ed107 Mon Sep 17 00:00:00 2001 From: test Date: Tue, 11 Aug 2026 01:49:10 +0000 Subject: [PATCH 3/3] 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 --- test/core/cache-retention-maintenance.test.js | 31 +++++++++++++------ 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/test/core/cache-retention-maintenance.test.js b/test/core/cache-retention-maintenance.test.js index 9ca203c2..f55fa62d 100644 --- a/test/core/cache-retention-maintenance.test.js +++ b/test/core/cache-retention-maintenance.test.js @@ -1048,10 +1048,15 @@ test('a foreign sorted replace tags the maintenance.partition span with rebaseli test('a partition already due for compaction skips the resettle-candidate row scan', async (t) => { // @ref LLP 0207#outranks-resettle [tests]: once the cheap file-count/size // check alone makes compaction due, the resettle scan's answer cannot - // change `shouldCompact`, so it must not run at all. Observed here by - // counting `readFileSync` calls against the partition's one data file: - // unskipped, the resettle scan and the compaction rewrite each open it - // once (two reads); skipped, only the rewrite does (one read). + // change `shouldCompact`, so it must not run at all. `hasResettleCandidate` + // is module-private and its `scanRowsFromTable` is an unpatchable ESM named + // import, so there is no direct call-count hook to assert against; instead + // this mocks `readFileSync` and captures a stack trace per `.parquet` read, + // then asserts none of those stacks pass through `hasResettleCandidate`. + // That frame name survives the async boundary between the scan and the + // read, so it attributes each read to its caller instead of only counting + // reads tick-wide (a second legitimate read elsewhere in the tick, e.g. a + // footer-stats probe, would not falsely implicate the scan). const cacheRoot = await makeTmpDir('maint-scan-skip') try { const partDir = path.join(cacheRoot, 'datasets', 'ds1', 'date=2026-08-08') @@ -1064,7 +1069,13 @@ test('a partition already due for compaction skips the resettle-candidate row sc // `compactionDue` gate under test. await writeCursor(partDir, { epoch: 0, rowCount: 1, compaction: null, layout: 'epoch' }) - const spy = t.mock.method(fsSync, 'readFileSync') + /** @type {string[]} */ + const stacks = [] + const original = fsSync.readFileSync + t.mock.method(fsSync, 'readFileSync', function (p, ...rest) { + if (String(p).endsWith('.parquet')) stacks.push(new Error().stack ?? '') + return original.call(this, p, ...rest) + }) const report = await maintainCache({ cacheRoot, @@ -1078,11 +1089,11 @@ test('a partition already due for compaction skips the resettle-candidate row sc }) assert.equal(report.totalCompacted, 1, 'sanity: compaction actually ran') - const dataFileReads = spy.mock.calls.filter((call) => String(call.arguments[0]).endsWith('.parquet')).length - assert.equal( - dataFileReads, - 1, - 'the data file must be read once, by the rewrite; a resettle scan would read it a second time first' + assert.ok(stacks.length > 0, 'sanity: the data file was read at all') + assert.deepEqual( + stacks.filter((s) => s.includes('hasResettleCandidate')), + [], + 'the resettle scan must not read the data file' ) } finally { await fs.rm(cacheRoot, { recursive: true, force: true })