Overview planner charges for the sections it will actually run - #666
Overview planner charges for the sections it will actually run#666philcunliffe wants to merge 1 commit into
Conversation
`rowsAffordable` divided the time budget by `OVERVIEW_SECTIONS.length`,
always four, while `collectOverview` runs only the sections it was asked
for. A subset caller was therefore charged for work nobody would do and
got a window narrowed in proportion, with nothing to show for it: a
window bound by time reports no reason, so the shortfall is invisible.
LLP 0135 #window already states the divisor as the sections planned.
`missingSections` had the same seam in reverse: it filtered the full
section list, so a subset run reported the sections nobody asked for as
sections that did not finish. The wizard prints that list verbatim
("the repos and tools sections did not finish"), which would have been a
false claim about work never started. `collectOverview` now stamps the
plan on the result, before any await, so an abandoned run still carries
what it meant to do.
Co-Authored-By: Claude <noreply@anthropic.com>
Review round 1 -
|
| Check | Result |
|---|---|
node scripts/run-tests.js |
3592 tests / 3591 pass / 0 fail / 1 skipped |
npx tsc -p tsconfig.json --noEmit |
exit 0 |
| Em dashes (U+2014) in diff | 0 |
LLP anchors 0135#window, 0135#overrun |
both exist (:808, :864) |
The regression tests genuinely prove the fix
Applied the PR's test file verbatim onto a clean origin/master worktree: exactly the
three claimed tests fail, and only those (59 pass / 3 fail). The arithmetic was re-derived
by hand and holds: 200k rows, probeMs 2000 → perRowMs 0.01, sectionBudgetMs 3000;
four sections afford 39,473 rows → 3 days, two sections afford 78,947 → 7 days.
Test 36 reaches the same result through the real collectOverview seam rather than
restating test 30, and test 37's third assert pins the emptyOverview() fallback. That is
the right shape.
The out.sections stamp holds up under attack
This was the part most likely to be subtly wrong, so it was attacked directly:
- Set before every abandonment path? Yes. The stamp is synchronous, before
const probeStart = clock()and the firstawait, with no early return or throw site
ahead of it. BecausewithDeadline(collectOverview(...))evaluates the call at the
argument position,partial.sectionsexists before the promise reacheswithDeadline.
There is no window where an abandoned run holds apartialwithout its plan. - Hand-built
OverviewRows?emptyOverview()omitssections, so
?? OVERVIEW_SECTIONSreproduces master exactly - which test 37 pins. first_look.js? Requests all four, somissingSectionsfilters all four, identical
to master. Both call sites (:186span attr,:203the user-facing sentence) unaffected.- Empty section list?
budgetMs / 0→Infinity→boundBy: 'rows', as the new
comment claims. The0/0 → NaNdegenerate case is unreachable because
sectionBudgetMs >= MIN_SECTION_BUDGET_MS(400). The comment is honest.
Only sharp edge left is contrived: reusing one into object across two collectOverview
calls with different subsets (last write wins). No such caller exists.
Finding 1 (non-blocking) - FIXED in the PR body
hyp query overview --json gains a sections key. query.js:221 serialises the whole
OverviewRows, and out.sections is now an enumerable own property, so the machine-readable
payload changes additively. No test pins the JSON key set, and the field is arguably useful,
but the PR body claimed no shipped caller's behaviour changes - and this is the surface most
likely to have an out-of-repo consumer. The body has been corrected to state the additive key
explicitly. No code change: suppressing it via a non-enumerable property would trade a
readable field for an obscure one.
Two nits, deliberately left
- LLP 0164:193 has a descriptive aside, "
OVERVIEW_SECTIONS.lengthfeeds
rowsAffordable", which after this PR is true only of the parameter default. The
decision's actual claim on that line still holds and 0164 is Accepted, so the
immutability rule argues for leaving it. Recorded so it is a choice, not an oversight. - The section-name union is now repeated six times (
overview.js:163,241,391,484,515
andtypes.d.ts:167); this PR adds three. Anexport type OverviewSectionin
types.d.tswould collapse them. Pre-existing pattern, out of scope for a fix PR, but
it is now at the point where drift is plausible.
Deliberate omissions, confirmed correct
SECTION_COST_VS_PROBE left at 1.9, no ratio redesign, no time-budget change, the issue's
open question left open. Each needs measurement neutral cannot perform, and #665 stays open
after this merges.
Review round 2 -
|
philcunliffe
added
the
neutral:approved
1 participant
Partial fix for #665. It addresses the issue's item 2 only (the hardcoded
section count), plus a sibling defect with the same root cause. It deliberately
does not touch the calibration constant, the ratio's design, or the open
question - see "What this PR does NOT fix" below.
What was wrong
rowsAffordablecomputedconst sectionCount = OVERVIEW_SECTIONS.length-always 4 - while
collectOverviewrunsopts.sections ?? OVERVIEW_SECTIONSand executes only the requested subset. A caller asking for two sections was
charged for four, so its time cap came out half what the work costs, and the
window narrowed in proportion. The shortfall is invisible to the caller:
describeWindowreports the scope and the lever, never the reason, so atime-bound window looks the same whether the estimate was right or double.
LLP 0135 #window already states the model as
remaining / (perRowMs x 1.9 x sections). "sections" there is the count planned; the code read it as thecount that exists.
Sibling defect, same seam, opposite direction.
missingSectionsfilteredOVERVIEW_SECTIONSunconditionally, though its own doc comment says "which ofthe requested sections". A subset run therefore reported every unrequested
section as missing, and
first_look.jsprints that list verbatim:Stopped here to keep setup moving - the repos and tools sections did not finish.Thosesections were never started, so the sentence is false in exactly the way
LLP 0135 #overrun says it must not be ("'no repos' and 'the repos section did
not finish' are different claims and only one is true"). Fixed in the same PR
because it is the same missing thread, and it has its own regression test.
The fix
rowsAffordabletakessections(defaulting to all four) and divides bysections.length.chooseOverviewWindowacceptssectionsin its opts and passes it through.collectOverviewpasses the section list it is about to run.collectOverviewstampsout.sectionson the result before any await, so anabandoned run still carries the plan;
missingSectionsfiltersrows.sections ?? OVERVIEW_SECTIONS.OverviewRows.sectionsadded tosrc/core/query/types.d.ts.No behaviour change for either shipped caller:
hyp query overviewand thewizard both request all four sections today, so both still divide by 4. What
changes is that the documented subset seam now plans correctly instead of
silently halving the window and mis-naming the sections.
Evidence: fails before, passes after
Three tests in
test/core/query-overview.test.js. On unmodifiedmaster(withonly the test file added):
The
3 !== 7is the whole bug in one number: 20 days x 10k rows, a 2000msprobe against a 5000ms budget leaves 3000ms at 0.01ms/row, which buys 39,473
rows (3 days) for four sections and 78,947 (7 days) for two. Master returns the
four-section answer to both. The
['repos', 'tools']is the false claim thewizard would have printed.
After the fix, same file:
Full suite
node scripts/run-tests.js: green both ways - 3588 pass / 0 fail onmaster, 3591 pass / 0 fail here (the three new tests).
npx tsc -p tsconfig.json --noEmit: clean, exit 0.What this PR does NOT fix
The issue is broader than this change. It should not be read as closed.
SECTION_COST_VS_PROBEis untouched at 1.9. The issue's item 1 asks fora re-measurement across several cache sizes; what exists is one data point
(~3.4x at 91k rows) from a machine this branch cannot reach. Re-calibrating a
constant on a single unreproducible measurement is the kind of unverified
change that has no regression test to sit behind, and over-correcting to 3.4
would narrow the window on every smaller cache where 1.9 is right. The
constant and its comment still record the 48k-row measurement, which is the
honest state of what is known.
floor on the estimate) is a design change: the constant's own comment argues
for a ratio over an absolute rate, and replacing it is a decision for a
maintainer and an LLP, not a bugfix.
the arithmetic predicts ~4.7-5.7s against an 8s deadline is unresolved, and
cannot be resolved from here: it needs a
wizard.first_lookspan from a real91k-row cache. It may well indicate a cost the model does not account for at
all, in which case this PR's arithmetic correction is necessary but not
sufficient.
OVERVIEW_TIME_BUDGET_MSandFIRST_LOOK_BUDGET_MSare unchanged, perthe issue and
first_look.js: the deadline is a backstop, not a mechanism.One incidental staleness left alone: LLP 0164 notes in passing that
"
OVERVIEW_SECTIONS.lengthfeedsrowsAffordable". That is still true of thedefault path, and 0164 is an accepted decision record whose point (that it left
the overview budget untouched) is unaffected, so it was not edited.
Fixes #665