Follow-up: carry PR #701's stranded review fixes (skip the wasted resettle scan, MaintenanceReport symmetry, span visibility, LLP gloss)

[philcunliffe]

Why this exists

PR #701 went through review, a fix worker applied four round-1 items and pushed
cc82d6ffd96e2216f4c219f349c107d06a046ee2, and then #701 was squash-merged at
head 287b67bb - beforecc82d6f landed. #701 is now MERGED and closed,
so its branch is invisible to the reconciler and the four verified fixes would
be silently lost.

Evidence that they never reached master:

• git merge-base --is-ancestor cc82d6f origin/master -> false
• origin/master:src/core/cache/maintenance.js contains zero occurrences of compactionDue
• origin/master:src/core/cache/maintenance.js contains zero occurrences of setAttribute('rebaselined'
• origin/master:src/core/cache/types.d.ts contains zero occurrences of totalRebaselined

All four files cc82d6f touches are byte-identical between 287b67b and
current origin/master, so each fix is carried over unchanged in intent.

The four fixes

1. Skip the wasted re-settle scan (src/core/cache/maintenance.js)

Hoist a cheap const compactionDue = opts.force || (grewSinceCompaction && needsCompaction(liveDir, cfg))
above the hasResettleCandidate row scan, gate the scan on !compactionDue && settle,
and set shouldCompact = compactionDue || hasResettle.

Recognition of a foreign sorted replace outranks the re-settle check, 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 discard the answer. The
boolean is unchanged (compactionDue || hasResettle is the same disjunction as
before); the row scan is skipped when its answer cannot matter. In exchange,
the cheap needsCompaction stat-walk is now evaluated in the re-settle path
where the old || short-circuited past it: needsCompaction only reads
(readdirSync/statSync), so this trades a warm stat-walk for a full
single-column row scan with no semantic change.

Empirical verification (instrumented hasResettleCandidate with a call
counter, then reverted before committing):

suite	scans before	scans after	tests
test/core/cache-retention-maintenance.test.js	1	0	36 pass
test/core/cache-resettle-sweep.test.js	7	4	5 pass

The 4 remaining calls are exactly the cases where the fallback-row scan is the
only possible trigger for compaction, so the sweep keeps working; the 4
eliminated calls are scans whose answer was discarded.

2. LLP 0199 Extended-by gloss (llp/0199-maintenance-compaction-convergence.decision.md)

Give the bare **Extended-by:** LLP 0207 the corpus's usual linked + glossed
form, matching llp/0106:9, llp/0129:9, llp/0158:9, llp/0180:9,
llp/0182:9, llp/0188:9, llp/0190:9, llp/0191:9.

3. MaintenanceReport symmetry (types.d.ts, maintenance.js, commands/query.js)

Add totalRebaselined: number to MaintenanceReport and increment it beside
totalCompacted, so query.js reads the total the report already knows instead
of re-deriving it by filtering report.partitions. maintainCache is the only
producer of a MaintenanceReport, so the new required field has one writer.

4. rebaselined span attribute (src/core/cache/maintenance.js)

The hyp_rebaselines counter was the only in-daemon signal that a re-baseline
happened, and it carries only the dataset, not the partition. Tag the enclosing
maintenance.partition span so a trace query can find which day re-baselined.

Note on PR #698

Open PR #698 (fix/issue-697) rewrites this file heavily (streaming row groups,
descriptor parking) and adds its own span.setAttribute block in
maintainCache, converting the withSpan callback to async (span) =>. To
avoid a needless conflict, item 4 here is deliberately not the original
cc82d6f shape (which restructured that same callback into
const result = ... ; span.setAttribute(...) ; return result). Instead it is a
single line at the re-baseline site itself, using the repo's existing
getActiveSpan() helper (already used in src/core/commands/policy.js):

getActiveSpan()?.setAttribute('rebaselined',true)

withSpan runs its callback under tracer.startActiveSpan, so the enclosing
maintenance.partition span is the active span there; instrumentation confirmed
getActiveSpan() resolves to a real span in all 4 re-baseline paths exercised by
the suite. This leaves the exact block #698 rewrites untouched.

Verified: git merge origin/fix/issue-697 into this branch auto-merges
src/core/cache/maintenance.js, src/core/cache/types.d.ts and
src/core/commands/query.jscleanly. The only conflict is the one-line
**Extended-by:** metadata field in llp/0199, which #698 also rewrites (it
adds a gloss for 0207 and 0209 but no link); resolving it is a union of the two
glosses.

Gate

• npm test: 3901 pass, 0 fail, 6 skipped
• npm run typecheck: clean
• test/core/llp-ref-hygiene.test.js: 11 pass, 0 fail
• hyp smoke cache_lifecycle_maintenance: ok
• hyp smoke incremental_sink_compaction: ok
• hyp smoke cache_roundtrip: ok

Issue #700 was already closed by #701; this PR deliberately carries no
Fixes #... trailer.

🤖 Generated with Claude Code

[philcunliffe]

Review round 1 - head dcae50b

Verdict: approve. No blockers, no majors. Four items raised, all four fixed in abdb17d.

The premise verified first, since it is this PR's whole justification

• git merge-base --is-ancestor cc82d6f origin/master -> false.
• origin/master has 0 occurrences of compactionDue, setAttribute('rebaselined', or totalRebaselined.
• All four files are byte-identical (same blob SHAs) between 287b67b and origin/master, so nothing landed by another route.

The four fixes really were stranded when PR #701 was squash-merged at 287b67b, before cc82d6f was pushed. Not redundant.

Item-by-item verification

Item 1 (scan skip) is boolean-neutral. With D = force || (grew && needsCompaction) and H = settle && grew && candidate: old force || H || (grew && needsCompaction) = D || H; new hasResettle = !D && H, shouldCompact = D || (!D && H) = D || H. Identical across all 8 assignments. hasResettle has no other reader, so redefining its value is unobservable. Reproduced the instrumentation table: retention/maintenance suite 1 scan -> 0, resettle sweep 7 -> 4.

Item 4 (span attribute) lands on the right span.withSpan -> startActiveSpan -> activeContext.run({span}, ...) (AsyncLocalStorage), and getActiveSpan() reads that store. With a capturing TracerProvider the exported span was maintenance.partition {... "status":"ok","rebaselined":true}. The ?. no-op is safe, and the unprefixed key matches its siblings partition/status.

Item 3 (totalRebaselined) has a single producer - only maintainCache returns a MaintenanceReport; format-iceberg's ExportMaintenanceReport is a separate interface, and no test constructs a literal. Value-identical refactor, dry-run behaviour unchanged.

Cross-PR merge claim verified exactly: merging origin/fix/issue-697 (#698) auto-merges maintenance.js and types.d.ts; the only conflict is the one-line **Extended-by:** field in llp/0199, resolvable as a union of the two glosses. The merged maintenance.js was inspected: compactionDue/hasResettle/shouldCompact, getActiveSpan()?.setAttribute and totalRebaselined all survive coherently, so the auto-merge is semantically sound and not merely textually clean.

Findings (all fixed in abdb17d)

1. minor - none of the four fixes was pinned by a test.grep for totalRebaselined|rebaselineNote|'rebaselined' across test/ and the smokes returned nothing. Reverting the scan-skip changed only cost and broke no test; reverting the span attribute was invisible. The scan-skip had been proven by temporary instrumentation, so it would have silently rotted.
Fixed with three tests, each proven to fail on revert: totalRebaselined === 1 on a re-baselining run and === 0 once converged (undefined !== 1 on revert); a capturing TracerProvider asserting the maintenance.partition span carries rebaselined: true (+ undefined - true on revert); and, for the scan-skip, an observation that the resettle scan and the compaction rewrite both read the same data file, so an unskipped scan reads it twice - spying fs.readFileSync and asserting exactly one .parquet read (2 !== 1 on revert). Only those three failed on their respective reverts.

2. nit - the @ref was less precise than it could be:[constrained-by] LLP 0207#foreign-replace pointed at the recognition test while its gloss described what #outranks-resettle actually settles. Fixed by retargeting, with the anchor confirmed to exist.

3. nit - @ref LLP 0207#re-baseline on the span attribute cited a section that says nothing about telemetry.#re-baseline settles the cursor-write shape, not observability, so the ref was close to mechanical by CLAUDE.md's own bar. Fixed by dropping the @ref and keeping the prose comment, which was the genuinely useful part. (The #re-baseline ref on rebaselineCursor itself is untouched and remains a correct [implements].)

4. nit - the PR body slightly overstated item 1. "Only the scan's side effect is skipped" omits that the old || short-circuited past needsCompaction whenever hasResettle was true, whereas compactionDue now evaluates it unconditionally when !force. needsCompaction is pure (readdirSync/statSync), so there is no semantic change and trading a warm stat-walk for a full single-column row scan is a clear win - but the claim implied nothing else moved. Fixed in the body.

Clean

Gates green (npm test 3903 pass / 0 fail, typecheck clean, three cache smokes ok, llp-ref-hygiene 11/11). Both @ref anchors resolve. No em dashes, no semicolons. No Fixes # trailer, deliberately - issue #700 is already closed by #701 and must not be reopened. The llp/0199 edit is a forward-ref field only, a permitted mechanical edit on an Accepted doc.

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

[philcunliffe]

Review round 2 - head abdb17d

Verdict: approve. All four round-1 findings are genuinely fixed, each reproduced independently rather than taken on trust. Two nits, both optional, both taken in 47fd4a4.

Round-1 items, verified by individual revert

Each fix was reverted in a separate worktree and the file re-run, so each test is proven to pin its own change for the stated reason:

• revert the hasResettle/shouldCompact gate: only the scan-skip test fails, expected: 1 / actual: 2.
• delete getActiveSpan()?.setAttribute('rebaselined', true): only the span test fails (expected: true, actual undefined).
• delete totalRebaselined at all three sites plus the types.d.ts field: only the re-baseline test fails.

The scan-skip inference was confirmed by attribution, not just by count. Instrumenting readFileSync with full stacks shows exactly what the test claims: with the fix the single .parquet read comes from scanRowsFromTable under compactGeneration; with the gate reverted, read #1 comes from hasResettleCandidate under maintainGeneration:302 and read #2 is the rewrite. resolver.js:26 reads the whole file with one readFileSync per open on the mutable default fs object, so the mock really intercepts it and no byte cache could hide an unskipped scan. Three consecutive runs, 38/38 each, no flake.

The TracerProvider test does not leak.shutdown() ends with if (globalTracerProvider === this) globalTracerProvider = null, the test calls it in finally, and nothing else in the file registers a provider. scripts/run-tests.js shells to node --test <files>, isolating each file in its own process. Verified both alone and inside the full run.

Items 2-4 all check out: the @ref targets LLP 0207#outranks-resettle (anchor exists, and that section is what actually licenses the skip); the mechanical #re-baseline ref is gone from the span site while the correct [implements] one on rebaselineCursor is untouched; and the body's needsCompaction trade-off is the only behavioural delta, introducing no new throw path (wherever the old code skipped needsCompaction, liveDir provably holds a table, since hasResettleCandidate returning true requires tableExists).

Cross-PR merge re-verified. Merging #698 (05439e6d) into this head: maintenance.js, types.d.tsand the test file all auto-merge; the only conflict is the one-line **Extended-by:** field in llp/0199. Resolving it with #698's side and running the test file on the merged tree gives 38/38, so #698's streaming-row-group rewrite does not disturb the readFileSync accounting the new test depends on.

Item 2 stays unpinned, and that is fine - no hygiene check validates the Extended-by field's form, and a doc-formatting convention is not worth a test.

Nits (both taken)

1. nit - the scan-skip assertion was an exact global count where a targeted attribution is strictly better.dataFileReads === 1 counted every .parquet read in the whole tick, not just the ones the gate governs. Correct today, but any future second legitimate read (a footer-stats probe, a two-pass rewrite) would break it with a message that misdirects the next reader.
Fixed: the test now captures a stack per .parquet read and asserts no stack carries the hasResettleCandidate frame, plus a sanity check that a read happened at all. Verified in both directions - passes at head (3 isolated runs and 3 full-file runs, no flake), and on the reverted gate fails on the intended assertion with the hasResettleCandidate frame visible in the diff output. Immune to unrelated added reads, and it names the culprit. The mock-based technique was already right, since hasResettleCandidate is module-private and scanRowsFromTable is an unpatchable ESM named import.

2. nit - the body's exemplar list for the Extended-by form was mis-cited. Of the five docs cited, only 0191:9 actually carries a link; the other four are gloss-only.
Fixed, and the fix worker went one better than the correction: it verified each candidate itself and excluded llp/0201, which the review had suggested, because 0201 carries a backward Extends rather than an Extended-by. The body now cites eight confirmed docs.

Gate

At 47fd4a4: npm test 3903 pass / 0 fail / 6 skipped, typecheck clean, llp-ref-hygiene 11/11, three cache smokes ok. No em dashes, no added semicolons, no inline import('...') types (the await import(...) hits in query.js are pre-existing dynamic runtime imports, not types), and the added @import { Span } is root-anchored.

The head moved to 47fd4a4, so this PR has used its two review rounds and the next tick triages it. With no residual open findings, that triage should find nothing blocking.

[philcunliffe]

Neutral triage at head 47fd4a4

Independently re-verified the two review rounds rather than trusting them. Result: no true blockers; one non-blocking test-hardening finding deferred to #707.

Reverts re-run (each in isolation, file re-run after each):

• !compactionDue && gate reverted: exactly one failure, the scan-skip test, failing on the attribution assertion ("the resettle scan must not read the data file") with at async hasResettleCandidate present in the captured stack. Not a count failure.
• getActiveSpan()?.setAttribute('rebaselined', true) deleted: exactly one failure, the span test, on "the span, not just the hyp_rebaselines counter, must name which partition re-baselined".
• totalRebaselined increment removed: exactly one failure, the foreign-replace test, on "the report-level rebaseline count mirrors totalCompacted".

Attribution test stress: 8 targeted runs plus 3 full-file runs, all green. node --test isolates files per process and nothing in the repo or icebird mutates Error.stackTraceLimit. Verified the passing-direction stack attributes the single read to at async compactGeneration and confirmed TracerProvider.shutdown() clears the global provider (no leak). One latent hazard found and deferred to #707: the discriminating frame sits at depth 8 of the default 10-frame stack limit, so a deeper dependency call chain could silently blind the test; the stacks.length sanity guard does not cover that.

Commit 47fd4a4: touches only test/core/cache-retention-maintenance.test.js in the tree; the rest of that commit was the PR body citation fix. No behavioural edit.

Cross-PR merge with #698 (05439e6d): re-merged in a scratch worktree. Only conflict is the one-line **Extended-by:** field in llp/0199, resolved as the union of the two glosses; on the merged tree the maintenance test file (38/38), llp-ref-hygiene (11/11), and typecheck are all green.

Gate at head:npm test 3903 pass / 0 fail, typecheck clean, smokes cache_lifecycle_maintenance, incremental_sink_compaction, cache_roundtrip all ok.

[philcunliffe]

Triage (head b86c642): shippable, no new follow-up issue needed

The review fix-loop used its 2 rounds. Triage re-read every finding from both rounds against the current tree rather than against the review comments' own claims that they were fixed, and found one still unresolved. It is a preference, not a blocker, and it is already tracked and already being fixed.

finding	round	state at b86c642
None of the four carried fixes was test-pinned	1	resolved - pins at cache-retention-maintenance.test.js:891/901/913 (totalRebaselined), :1039-1070 (span attribute), :1080-1131 (scan-skip)
@ref targeted #foreign-replace while glossing #outranks-resettle	1	resolved - maintenance.js:314 now carries 0207#outranks-resettle [constrained-by]; both anchors exist in LLP 0207
Mechanical #re-baseline@ref on the span-attribute site	1	resolved - removed; the sole 0207#re-baseline ref is the [implements] on rebaselineCursor, which is correct
PR body overstated item 1	1	resolved - body now names the needsCompaction evaluation change explicitly
Scan-skip assertion was a global read count, not attribution	2	resolved - now captures a stack per .parquet read and asserts no frame contains hasResettleCandidate, with a stacks.length > 0 sanity check
Attribution test's discriminating frame sits near V8's default 10-frame Error.stackTraceLimit, so a deeper call chain could silently blind it	triage note	unresolved, preference

Why the residual finding does not block

Confirmed real: grep stackTraceLimit over the test file at this head returns 0 matches, so the hazard stands. But the worst case is a regression test going blind, not a production defect: no wrong behaviour, no data loss, no crash, no perf regression.

It is also not being dropped. It is recorded in open issue #707, and PR #708 (Fixes #707) is in flight right now and adds precisely the missing guard (Error.stackTraceLimit raised while the mock is installed and restored afterwards, test/core/cache-retention-maintenance.test.js:1094-1112 on that branch). So the deferral has a named owner and a live fix rather than an open-ended promise. No new follow-up issue is warranted, and filing one would duplicate #707.

Shippability, independently checked

The specific risk here was the merge with master: #698 reworked the same cache-maintenance path this PR changes the gate logic around, and the files auto-merged with no conflict, which is exactly where a change disappears silently.

• Both sides coexist.Follow-up: carry PR #701's stranded review fixes (skip the wasted resettle scan, MaintenanceReport symmetry, span visibility, LLP gloss) #706's gate (compactionDue :303, hasResettle :317, shouldCompact :320) feeds the branch where Cache compaction sizes files by bytes written, not the in-memory batch estimate #698's sizing lands (r.compactedBytesWritten = result.bytesWritten :351, openStreamingAppend :20/:657, bytesWritten :752), and Cache compaction sizes files by bytes written, not the in-memory batch estimate #698's bytes_written span attribute (:167-168) tags the same maintenance.partition span as Follow-up: carry PR #701's stranded review fixes (skip the wasted resettle scan, MaintenanceReport symmetry, span visibility, LLP gloss) #706's rebaselined attribute (:337). Neither displaced the other.
• All four carried fixes present: the gate trio; totalRebaselined at all three sites (maintenance.js declared :122, incremented :177, returned :188; types.d.ts:304; commands/query.js:330); the span attribute at maintenance.js:337.
• llp/0199Extended-by: line carries both forward-refs as working links with master's landed gloss text verbatim (diffed against origin/master; only the link wrapping and the ; separator differ). That is the permitted mechanical edit on an Accepted doc, not a rewrite of settled prose.

Gates, as observed on the merged tree

npm test3943 pass / 0 fail / 1 skipped (3944 tests), exit 0 - including master's new cache-compaction-file-size.test.js from #698 alongside this PR's three new pins, so both PRs' suites pass on the combined code. npm run typecheck clean. Cache smokes on the merged tree: cache_lifecycle_maintenance, incremental_sink_compaction, cache_roundtrip all ok.

Not verified

The revert-discrimination claims (each new test failing individually when its target is reverted) were not re-reproduced this round; they were checked twice previously, and this round confirmed only that the tests are present and passing. Issue #707's body was not read beyond its title and open state.

---

Nothing blocks this. The PR is being flipped out of draft state as needed and held for you to merge.