Scan-skip test goes blind if the call chain grows 3 frames (#707)

[philcunliffe]

Root cause

test/core/cache-retention-maintenance.test.js, test a partition already due for compaction skips the resettle-candidate row scan, attributes each .parquetreadFileSync to its caller by capturing new Error().stack inside a mock and asserting no stack passes through hasResettleCandidate.

The discriminating frame sits at frame 8 of 10, and Error.stackTraceLimit defaults to 10:

Error
at Object.<anonymous> (test/core/cache-retention-maintenance.test.js:1076:55)
at Object.apply (node:internal/test_runner/mock/mock:765:20)
at Object.reader (src/core/cache/iceberg/resolver.js:26:26)
at readDataFile (node_modules/icebird/src/read.js:182:35)
at readDataFile.next (<anonymous>)
at Object.rows (node_modules/icebird/src/sql/icebergDataSource.js:170:30)
at async scanRowsFromTable (src/core/cache/iceberg/store.js:509:20)
at async hasResettleCandidate (src/core/cache/maintenance.js:829:22) <-- frame 8
at async maintainGeneration (src/core/cache/maintenance.js:302:32)
at async withSpan.component (src/core/cache/maintenance.js:154:16)

Two frames of headroom. If the chain between the mock and hasResettleCandidate gains 3 or more frames (an icebird refactor, extra node:test mock internals, a wrapper in resolver.js), the frame silently falls off and the negative assertion stacks.filter(s => s.includes('hasResettleCandidate')) passes vacuously - the test stays green even with the !compactionDue && gate reverted.

The existing stacks.length > 0 sanity check only proved a read happened, not that stacks were attributable, and the new Error().stack ?? '' fallback would store an unattributable empty string without failing.

The fix

Both hardenings suggested on the issue, 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 outright.
2. Positive attribution assertion: some captured stack must name 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 the test fails loudly instead of going quiet. 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.

The existing comment's explanation of why a mock-based observation is the only option (hasResettleCandidate is module-private, scanRowsFromTable is an unpatchable ESM named import) is kept, and extended to describe the two new guards.

No production code changed.

Demonstration

Truncation is simulated with a preload module (Error.stackTraceLimit = 7, standing in for three extra frames) passed via NODE_OPTIONS="--import ...".

Step 0 - the test is sensitive today at the default limit

Gate reverted (const hasResettle = settle), no truncation:

not ok 1 - a partition already due for compaction skips the resettle-candidate row scan
error: 'the resettle scan must not read the data file'
...
at async hasResettleCandidate (src/core/cache/maintenance.js:829:22)
# pass 0
# fail 1

Step 1 - blindness reproduced

Gate still reverted, plus truncation. Old test:

TAP version 13
# Subtest: a partition already due for compaction skips the resettle-candidate row scan
ok 1 - a partition already due for compaction skips the resettle-candidate row scan
# pass 1
# fail 0

Green with the gate reverted. Exactly the defect the issue describes.

Step 2 - hardening applied

(the diff in this PR)

Step 3 - hardened test fails under the same truncation, gate still reverted

Guard 1 (the limit raise) defeats the external truncation, so the real check sees the frame again and fires:

not ok 1 - a partition already due for compaction skips the resettle-candidate row scan
error: |-
the resettle scan must not read the data file
# pass 0
# fail 1

Step 3b - guard 2, checked independently

Simulating truncation that the raise cannot fix (in-test limit forced to 7, as if the chain had outgrown even 50), gate still reverted. The positive assertion fires rather than the test going quiet:

not ok 1 - a partition already due for compaction skips the resettle-candidate row scan
error: 'sanity: captured stacks must be deep enough to name the reader, or the assertion below passes vacuously'
code: 'ERR_ASSERTION'
# pass 0
# fail 1

Step 4 - gate restored, hardened test passes

=== normal ===
ok 1 - a partition already due for compaction skips the resettle-candidate row scan
# pass 1
# fail 0
=== under external truncation (guard 1 defeats it) ===
ok 1 - a partition already due for compaction skips the resettle-candidate row scan
# pass 1
# fail 0

Stacking

This stacks on #706 and should merge after it. The test being hardened was added by #706 (fix/issue-700-followup) and is not on master; this branch is based on #706's head 47fd4a4, so the fix applies to the code it targets. The diff here is one commit touching only the test file.

Gate

• npm test: 3907 tests, # pass 3903, # fail 0 (6 pre-existing skips)
• npm run typecheck: clean
• npm run smoke -- cache_lifecycle_maintenance: ok
• npm run smoke -- incremental_sink_compaction: ok
• npm run smoke -- cache_roundtrip: ok

Fixes#707

[philcunliffe]

Review round 1 - head 4045b63

Verdict: approve. No findings. Every claim in the PR description was reproduced exactly rather than accepted: the defect was real, both guards work, and nothing leaks.

Scope

git diff 47fd4a4..4045b632 -- src/ is empty. The sole commit touches only test/core/cache-retention-maintenance.test.js (+26 lines, no deletions). The large diff against master is entirely the stacked #706 chain, as stated. The existing @ref LLP 0207#outranks-resettle still resolves and no new refs were added.

The four-step demonstration, re-derived

With the gate reverted (maintenance.js:301, !compactionDue && settle back to settle):

step	test	truncation	gate	result
0	old (47fd4a4)	none	reverted	fails, at async hasResettleCandidate present
1	old	stackTraceLimit = 7 via NODE_OPTIONS --import	reverted	passes - blindness reproduced
3	new	external 7	reverted	fails on the attribution assertion
3b	new, in-test limit forced to 7	n/a	reverted	fails on the new sanity assertion
3b'	new, in-test limit forced to 7	n/a	restored	fails loudly - guard 2 is gate-independent, as intended
4	new	none, then external 7, then real --stack-trace-limit=7	restored	passes in all three

Step 0's stack is exactly 10 frames with hasResettleCandidate at frame 8, so the comment's "2 frames of headroom, 3 extra frames drops it" arithmetic is exact rather than approximate. Step 1 is the load-bearing one: the old test really does go green under truncation with the gate reverted, so the defect this PR fixes was genuine.

The scanRowsFromTable call, verified independently

Dumping the good-path stacks gives 15 frames with scanRowsFromTable at 7 and compactGeneration at 8 - exactly the depth hasResettleCandidate occupies on the bad path, with scanRowsFromTable one frame shallower in both. So the author's non-obvious call is right: asserting on scanRowsFromTable would survive truncation to 7 that has already blinded the negative check, reproducing the very blindness the fix removes. Choosing compactGeneration is load-bearing, not stylistic.

Restoration, vacuity and cost

• originalStackTraceLimit is captured before the try, the raise is inside it, and the restore is the first statement of finally, ahead of the await fs.rm - so a failing cleanup cannot skip it. Verified empirically by injecting a forced assertion failure mid-test and probing from a later test: the limit was back to 10.
• node:test runs top-level tests in a file with concurrency 1, so the raised global is never visible to a sibling even while in effect, and nothing else in the file touches Error.stackTraceLimit or .stack.
• stacks.some(...) on an empty array is false, so the new assertion cannot pass vacuously; with the existing stacks.length > 0 ahead of it, the ?? '' empty-string hole really is closed.
• No measurable cost: 5 runs each, old 23.4-26.6 ms vs new 22.7-23.5 ms. The gate means one .parquet read per run and the full stack is 15 frames, well under 50.

Gates

npm test 3903 pass / 0 fail / 6 skipped, typecheck clean, smokes cache_lifecycle_maintenance, incremental_sink_compaction, cache_roundtrip all ok. Target file run 5 times consecutively: 38/38 each, no flake. No em dashes, no code semicolons, and the comment prose matches both guards and the measured frame depths.

Note for whoever merges: this PR is stacked on #706 and should land after it.

[philcunliffe]

Review (head 8e9a2b7): clean

This PR was already reviewed clean at 4045b63. Since then the head moved twice for reasons unrelated to its own subject: a merge of origin/master (which had advanced through #705, #711, #698, #710), and a one-line follow-up commit by the reconciler. So this round reviews the delta, not the PR again.

Nothing actionable. One nit is recorded below and deliberately not acted on.

The merge resolution (llp/0199, line 9)

Both forward-refs are present as working links, both targets exist at this head, and master's landed gloss text is preserved verbatim for both LLP 0207 and LLP 0209.

That last point is what the follow-up commit 8e9a2b7 fixed. The merge at 31e450c had kept this branch's shorter, differently-worded 0207 gloss, which would have reworded settled prose on an Accepted doc. Adding or linking a forward-ref is the permitted mechanical edit; rewording a gloss already landed is not. The follow-up restores master's text and keeps only the link addition, and touches nothing else.

Worth being plain about why that mattered beyond this PR: PR #706 hit the same conflicted line and resolved it the other way, keeping master's gloss. Two independent resolutions of one line would have left the two open PRs disagreeing, so whichever merged second would have re-conflicted there. They are now byte-identical on that line.

Semantic merge safety in maintenance.js: both sides survived

Nothing else conflicted textually, but master's #698 and this branch both edited src/core/cache/maintenance.js. A clean auto-merge is exactly where a change vanishes with no marker, so this was checked by two-way parent comparison rather than by eye:

• Branch side intact - the branch's contribution is textually identical pre- and post-merge (same 5 files, same 154 insertions(+), 6 deletions(-); only blob hashes, hunk offsets, and the conflicted line differ).
• Master side intact - Cache compaction sizes files by bytes written, not the in-memory batch estimate #698 arrives whole: openStreamingAppend, the StreamingTableAppend@import, the flushBatch/sink streaming rewrite, the try/finally + abort() descriptor-leak guard, bytesWritten on the return, r.compactedBytesWritten, and the bytes_written span attribute.
• Coexistence at the one interaction point - Cache compaction sizes files by bytes written, not the in-memory batch estimate #698 sets compacted, data_files_before, data_files_after, rows, bytes_written on the maintenance.partition span aftermaintainGeneration returns; this branch sets rebaselined from inside it via getActiveSpan(). withSpan uses tracer.startActiveSpan, so the nested call resolves to the same span. Disjoint attribute keys, no overwrite, both signals on one span.
• Gate rewrite is boolean-equivalent - compactionDue || (!compactionDue && settle && ...) is equivalent to master's force || hasResettle || (grew && needsCompaction(...)). hasResettleCandidate is a read-only scan with no side effects, so short-circuiting past it changes cost, not outcome, and the foreignSortedReplace recognition path still outranks it per LLP 0207#outranks-resettle.

@ref annotations

All 14 in the merged maintenance.js resolve. Anchors verified for every ref the merge touched or introduced: 0199#baseline-gate, 0027#re-settle-sweep, 0207#outranks-resettle, 0207#foreign-replace, 0209#decision, 0209#retained-metadata. Each still describes the code above it accurately.

Nit, recorded and not acted on

The merged Extended-by: line joins its two entries with ; where master landed , and the corpus generally uses , . Normalization showed this is the only character differing from master's line (same length, no gloss reworded). Both glosses embed commas, so the semicolon is a defensible readability call and does not change meaning, which keeps it inside the mechanical-edit carve-out. Acting on it would move the head, re-run CI, and delay this PR and #706 for no substantive gain, so it stays. Flagged here so the choice is on the record rather than invisible.

Gates, as observed

npm test3944 tests, 3943 pass, 0 fail, 1 skipped. npm run typecheck clean. Zero U+2014 in the delta; no semicolons added in JS; no @typedef or inline import('...') types.

Not verified

Hermetic and acceptance smokes were not run, only the two gates above. Note also that origin/master has since advanced one commit (a14ed1f, #712) that this branch does not contain; the branch touches none of #712's files, so that is simply being one commit behind, not a revert, consistent with the reported MERGEABLE.

[philcunliffe]

Triage after the review budget (LLP 0017). The head is the conflict-resolution merge commit, which no review round covers, and the round budget is spent. Judged mergeable with no residual findings - nothing to defer, so no follow-up issue was opened.

The conflict resolution was verified against the tree, not taken on trust.git diff ead72e8 8e9a2b7 on the test file produces a diff whose only - line is the header itself: every content line is additive, the promised 26-line pure-addition superset. The other four files were byte-identical between the two sides, confirming they only conflicted because both branches independently carried PR #701's content via #706, not because of divergent edits. git diff 8e9a2b7 dff381c on the test file is empty, so the merge kept the branch tip byte-for-byte with nothing silently dropped.

Two things examined and cleared:

• The finally restore.originalStackTraceLimit is captured before the outer try, and the finally at :1155-1158 restores it on both the success and throw paths. node --test isolates per file, and this suite sets no concurrency, so tests run sequentially and the restore always completes before the next one starts. No leakage observed across the full suite.
• Whether the positive assertion tracks the right depth.compactGeneration (maintenance.js:693) and hasResettleCandidate (:890) both call scanRowsFromTable as the direct next frame in their own body, so the icebird-internal frames beneath them are identical in depth. The dueness gate at :317 short-circuits hasResettleCandidate entirely when compactionDue is true, which is what the test forces. The positive assertion is checked before the vacuous-pass-prone negative one, so truncation deep enough to blind the real check fails loudly first.

Test-only change, so the blast radius is bounded to test-guard strength even in the worst case - and no case was found where it is weak. Suite green at the merge head: 3957 pass / 0 fail / 1 skipped.