Sidecar builds at maintenance: compaction finalizes a file, the index follows (LLP 0265 T6) - #953
Sidecar builds at maintenance: compaction finalizes a file, the index follows (LLP 0265 T6)#953bgmcmullen wants to merge 4 commits into
Conversation
… follows (LLP 0265 T6) Compaction is the moment a data file stops changing, so it is the one point where a hypgrep index can be built once and stay valid against its rows. maintainCache now follows every committed rewrite of the grep dataset with a sidecar-build pass over the new generation's files, in a worker thread (createIndex is seconds of straight-line CPU and the daemon is single-threaded), one file at a time. The worker handle and thread are ports of the server's index-worker pair, with one behavioral fix: the worker holds an event-loop ref exactly while a build is in flight, because an always-unref'd worker deadlocks any process whose loop would otherwise drain while awaiting the build. Sidecar existence is the completion marker, no ledger: the publish is a write-then-rename, a killed daemon leaves nothing half-claimed, and the next pass rebuilds whatever is missing. A file whose build keeps failing is quarantined after three attempts (in-memory, process-lifetime; a restart is the retry) and the scan tier serves it forever after: index presence is purely a performance property. The build pass can never fail the partition's own maintenance verdict. Two hazards found and closed on the way: - countDataFiles and measureDataDir counted sidecars (*.parquet in data/), which would have made every just-indexed partition read as "grew since compaction" and rewrite itself every tick through the LLP 0199 baseline gate. Both now exclude .index.parquet; a test pins that a second unforced tick stays converged. - A corrupt sidecar used to fail the whole search; the indexed tier now runs into local buffers and commits only on success, so an unreadable sidecar degrades that one file to the brute scan with no double count. GREP_DATASET joins the shared searchable-columns module so the search service and the build pass cannot disagree about which dataset carries indexes. Tests: per-file build and existence-marker idempotency, the quarantine budget with the scan tier still serving, the corrupt-sidecar fallback, maintenance building indexes for exactly the grep dataset, sidecars not re-triggering compaction, and a retired generation dying whole with its sidecars inside (the no-GC-code guarantee). The compaction-effectiveness tests' liveDataFiles helper learns the same sidecar exclusion the production counters did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bgmcmullen
added
the
neutral:adopt
philcunliffe
added
the
neutral:adopted
…sion-safe publish, and failures that name their file - Hoist `sidecarPathFor` beside `GREP_DATASET` in searchable_columns.js. The build pass and the search service each carried their own copy of the `.index.parquet` rule; two copies of a path contract drift into a build that writes an index nobody probes for. - Give the publish scratch file a random token. A fixed `<sidecar>.tmp` is only atomic for a single writer: the daemon tick and a hand-run `hyp` over the same cache would interleave into one scratch file and rename the mixture into place as a finished sidecar. The scratch file is now also removed on the failure path. - Name the data file on `grep_index.build_failed` / `grep_index.file_quarantined` / `grep_search.sidecar_unreadable`, and add the component/operation attributes, so three warnings can be told apart as one poisoned file or three. - Append rather than spread the indexed tier's buffered hits: `limit` reaches the service unvalidated and one file can fill the budget. - Correct the module docs: the pass runs only behind a committed compaction, which always publishes a fresh generation, so it never re-attempts a file it skipped or failed on, and a daemon restart is not a retry. Add the LLP 0264#lifecycle ref the module realizes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Verdict: changes requestedThe T6 machinery is well built (the worker handle, the ref-while-pending discipline, the transfer-list guard, the manifest-driven file walk, and the retired-generation reclaim all hold up under scrutiny). One finding blocks: the indexed tier silently answers a different question than the scan tier, which is exactly the invariant HIGH (not fixed, needs a design decision)1.
Not a string leaf, so Reproduced against this branch with one row whose only occurrence of the pattern is inside So Not fixed here because every sound repair is an LLP-level change, not a code fix: drop MEDIUM (not fixed, design-shaped)2. Nothing ever rebuilds a missing sidecar. The pass is gated on 3. A corrupt sidecar is permanent.
LOW (not fixed)4. The build pass sits outside 5. Sidecar bytes are invisible to every disk-accounting surface. Fixed and pushed (07cd466)
|
philcunliffe
added
the
neutral:stuck
Stuck: the indexed tier cannot cover
|
Decision: option 1. Drop
|
philcunliffe
removed
the
neutral:stuck
|
Re-engaging with your guidance - option 1 taken: drop |
…: nine columns, tool_args moves to the not-searched list The maintainer's unstick on PR #954 settled that #953 drops tool_args from SEARCHABLE_COLUMNS. The verb's coverage clause interpolates the constant so it follows on its own, and the query_grep_roundtrip smoke never seeds or asserts a tool_args hit, so the SKILL.md enumeration (both host copies, one shared line) was the only surface stating the old set. The two deliberate host-only lines are untouched, so the divergence fixture needs no re-record. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lowlist Merge of the base branch plus the maintainer's decision on #953. ## Conflict: src/core/search/grep_service.js Both branches independently fixed "a poisoned sidecar fails the whole query", so the conflict is two spellings of one fix, not two intents. - t4 (`b590b110`) hoisted a `searchIndexed` helper that collects through the shared `collect`/`trimHits` machinery (its `8b694e5b` sort-order truncation) and degrades ONLY when the read failed before the first row; a later failure propagates, because the rows already pushed to the shared buffer could not be taken back. - t6 buffered the attempt into a local array and committed on success, so any failure degrades, and logged `grep_search.sidecar_unreadable` naming the file that needs deleting. Taken: t4's structure with t6's buffering and log folded in. The helper keeps its name, the 2-argument `isAbort(err, signal)` t4 introduced (t6 still called the 1-argument form, which would have turned a timeout into a thrown error), the day-descending early break, and sort-order truncation; it gains the local buffer, so a sidecar that tears mid-read degrades that one file instead of failing the query, and it gains the warning. `trimHits` is generalized to `trimBuffer(list)` so the per-file buffer is trimmed by the same rule as the shared one: buffering must not trade the walk's memory bound away, and cutting the buffer in walk order would reintroduce the bug `8b694e5b` fixed. ## Decision on #953: tool_args leaves SEARCHABLE_COLUMNS `tool_args` is the dataset's one VARIANT column. The index worker only indexes STRING leaves in the allowlist and the server's row predicate gates on `typeof value === 'string'`, so the column has never produced a hit on any tier in either repository: T3's `cellText` coercion made the client's scan tier uniquely able to match it, on a premise ("the indexed tier reads the column's own text") that was false on both sides. Rather than add coverage neither repo has ever had, the column is dropped and the loss is recorded. Follow-up: #977. - `SEARCHABLE_COLUMNS` loses `'tool_args'`; `SCAN_COLUMNS` derives from it, so the brute scan stops decoding the column too. - The module comment now gives the VARIANT reason and points at #977, in the spirit of server LLP 0157 #identifier-columns. - `cellText` keeps its coercion (it is what keeps `rowTest`, `test` and `locate` answering identically on any cell shape, and #977 needs it in place), but its comment stops claiming the indexed tier reads the column's text. Its `@ref` gloss is corrected to match. - `toHit` and `GrepSearchMatcher` lose the same false claim. - Tests: the pinned allowlist drops the column, a new test pins its absence from both the allowlist and the scan projection, and the matcher tests exercise the coercion through a column that is actually searchable while pinning that `tool_args` no longer matches. - New in `search-grep-service.test.js`: a row matching only in `tool_args` returns zero hits from BOTH tiers, scan and indexed, with the tier counters proving each one really served the file. The invariant is tier agreement, not coverage. No LLP change: LLP 0264 never enumerates the columns (it points at this module and defers to server LLP 0157), so nothing it settled is contradicted. Amending server LLP 0157 and the now-wrong `grep_search` tool blurb is hypaware-server work, tracked separately. `sidecarPathFor` stays the single contract owner in `searchable_columns.js`; no re-export from `sidecar_build.js` is restored (#954 takes the import fix on its side). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Merged Conflict resolutionOne conflicted file,
Chosen: the base's structure with the head's two improvements folded in. The helper keeps its name, the day-descending early break, sort-order truncation, and the 2-argument
Guidance items
Two extra spots carried the same now-false claim and were corrected for honesty: Explicitly not touched, per the guidance: nothing rebuilds a missing sidecar; a corrupt sidecar is permanent; the build pass sits outside Local verification
|
… scratch is not data bytes The merge resolution's per-file buffer degraded one file instead of failing the query, but it also made a deadline throw away everything the index had already produced for the file it landed in. hypgrep checks the signal at every coalesced range boundary, so a deadline lands inside a file, and on a newest-first walk that is the newest file the caller most wants. Committed before the abort propagates: safe because an abort ends the walk, so the file is never rescanned and no row can be counted twice. grep_search.sidecar_unreadable named only the sidecar, but parquetFind opens the source data file through the same factory and runs the row filter per row, so a torn source parquet lands in that catch too and points the operator at a healthy index. Renamed to grep_search.indexed_read_failed and both files are named. measureDataDir excluded `*.index.parquet` but not the build's publish scratch, `<file>.index.parquet.<uuid>.tmp`, which survives a kill between write and rename with no reaper until the generation retires. countDataFiles already skips it, so counting its bytes broke the shared-file-set invariant in the dangerous direction: needsCompaction compacts on a LOW average, so a large orphan makes a fragmented partition read as healthy. Test pins it. Also recorded, not fixed: a sidecar freezes the allowlist it was built over (hypgrep stores hypgrep.text_columns in the index and prunes to it, and nothing compares that stamp to today's SEARCHABLE_COLUMNS), so #977 has to invalidate existing sidecars rather than only build new ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 2 verdict: approve once CI is green. Four findings, all fixed and pushed as
|
| item | verified |
|---|---|
'tool_args' removed from SEARCHABLE_COLUMNS |
yes, searchable_columns.js:36-46. SCAN_COLUMNS is [...SEARCHABLE_COLUMNS, ...] at :128, so the brute scan stops decoding it, as intended |
| module comment rewritten with the VARIANT reason and a #977 pointer | yes, searchable_columns.js:16-27. Gives the iceberg-variant reason, states that neither repo's tier has ever produced a hit from it, keeps the "worse than absent" argument but pointed the other way, and names #977 |
matcher.js:112-119 premise corrected, coercion KEPT |
yes. cellText's body is byte-identical; only the doc changed (matcher.js:111-131). It no longer claims the indexed tier reads the column's text, and the false @ref LLP 0264#shared gloss was rewritten rather than left |
| tests that pin the set updated | yes. search-searchable-columns.test.js:18 drops the column; :39 is a new both-lists absence assertion. :49 correctly needed no edit (derived, not a second literal). search-exports.test.js genuinely does not pin the set. search-matcher.test.js did: three tests were rerouted through content_text and now additionally pin that tool_args does not match |
| both-tiers-zero-hits assertion | yes, search-grep-service.test.js:241. Asserts zero hits on the scan tier with scannedFiles >= 1, zero on the indexed tier with indexedFiles >= 1, plus a control query proving the row is still reachable. Framed as tier agreement, as asked |
| no new LLP | correct. Nothing under llp/ enumerates the columns; LLP 0264 #shared points at the module. Every @ref anchor in the diff (0264#shared, 0264#lifecycle, 0265#out-of-scope) resolves |
| base merged in | yes. git merge-base --is-ancestor origin/grep/t4-grep-service HEAD passes; c00072b1 is a real merge of 07cd4661 and b590b110 |
sidecarPathFor stays in searchable_columns.js |
yes, :70, single owner, no re-export from sidecar_build.js |
The conflict resolution was checked against both parents. git diff b590b110 c00072b1 shows nothing from the base side dropped, and git diff 07cd4661 c00072b1 touches no T6 file. The searchIndexed helper keeps the base's name, the day-descending early break (grep_service.js:388), sort-order truncation through the generalized trimBuffer, and the 2-argument isAbort(err, signal) at all three call sites. The head's per-file local buffer and its warning are folded in.
Findings
1. MEDIUM (fixed). An aborted indexed read threw away the file's already-matched rows. src/core/search/grep_service.js:310 (at c00072b1)
The pre-merge base collected into the shared hits as it went, so an abort kept what it had. The merged version buffers into found and commits at :322-327, after the loop, but the catch rethrows on isAbort before reaching it, so found and withheldHere are both discarded.
hypgrep calls signal?.throwIfAborted() at every coalesced range boundary, so a deadline lands inside a file, not between files, and that is the normal case, not the rare one. On a newest-first walk the interrupted file is the newest and most valuable one, so an AbortSignal.timeout query answered exhausted: false with zero hits from exactly the file the caller cared most about, and that file's withheld-row count vanished from the usage_policy.query_withhold report. That contradicts what 8b694e5b established on the base: an abort returns the partial answer.
Committing before the rethrow is safe, and the reason is specific: an abort ends the walk (grep_service.js:392-397 breaks out), so unlike a degrade there is no rescan to double-count against. The file still does not count toward indexedFiles, matching the base's counter semantics, since it was not served whole.
Not covered by a new test: reaching this path deterministically requires an abort between hypgrep's internal range checkpoints, which the service cannot inject without pinning hypgrep's read chunking. The invariant is stated in the code instead.
2. MEDIUM (recorded, not fixed - needs the same decision the deferred residuals need). A sidecar freezes the allowlist it was built over. src/core/search/index_worker_thread.js:101, src/core/search/grep_service.js:283
searchableStringColumns() intersects the file's string columns with SEARCHABLE_COLUMNS at build time. hypgrep stores that list in the index (hypgrep.text_columns, createIndex.js:245) and parquetFind prunes candidate blocks to it (parquetFind.js:36-45). The index carries a hypgrep format version that queryIndex hard-fails on, but no allowlist stamp, and the service never compares the returned textColumns against today's constant.
Combined with the intended no-rebuild lifecycle, the first release that adds a column to the allowlist makes every already-indexed file answer that column with zero while uncompacted files answer correctly: silent, and permanent for the life of the generation. This is not the tool_args coverage decision, which is settled; it is that the mechanism cannot notice when the allowlist moves. #977 is exactly this scenario and needs to invalidate existing sidecars, not merely start building new ones.
Not raised as a blocker because it is the same shape as the residuals you deferred (nothing rebuilds a sidecar) and needs the same call. Recorded in sidecar_build.js:36-47 in the record-the-loss spirit you asked for on the column, so it is on the record rather than rediscovered when #977 lands.
3. LOW (fixed). The build's publish scratch was counted as data bytes. src/core/cache/maintenance.js:1573 (at c00072b1)
measureDataDir's new filter excluded *.index.parquet but not <file>.index.parquet.<uuid>.tmp, which sidecar_build.js:124 writes into the same data/ dir. countDataFiles already skips it (no .parquet suffix), so the new comment's own "the two must see the same file set" invariant was broken, and broken in the dangerous direction: needsCompaction compacts when the average is low (maintenance.js:688), so an inflated byte total makes a genuinely fragmented partition read as healthy and go unrewritten. The rm at sidecar_build.js:133 only covers a thrown build; a SIGKILL or OOM between write and rename leaves an orphan with no reaper until the generation retires.
Fixed by testing includes('.index.parquet') rather than endsWith, which covers both halves of the pair. Pinned by a new test (search-sidecar-build.test.js:171) that writes an orphaned scratch and asserts a due compaction still runs; the test was confirmed to fail against the old endsWith predicate.
4. LOW (fixed). grep_search.sidecar_unreadable named the wrong file. src/core/search/grep_service.js:311 (at c00072b1)
parquetFind opens the source data file through the same asyncBufferFactory and runs rowFilter: accept per row, so a torn source parquet, or a throw out of the matcher, lands in that catch and was logged with sidecar_file naming a perfectly healthy index. The comment above the field claimed the line was the only notice that a sidecar needs deleting, which made the misattribution actionable in the wrong direction: in the corrupt-source case the fallback then reads the same file, throws, and fails the whole query anyway, after pointing the operator at the wrong path.
Renamed to grep_search.indexed_read_failed (the event was introduced by 07cd4661 in this same stack and nothing else references it) and it now carries both sidecar_file and data_file.
Also folded in: the append-not-spread comment at :324 claimed limit "reaches this service unvalidated", which the base merge falsified - 8b694e5b added the Number.isSafeInteger(limit) && limit > 0 gate at :113. The conclusion still holds (validated is not bounded, so one file can fill a buffer of millions), so only the premise was corrected.
Checked and found sound
Worker respawn and failAll map ownership across a crash; ref-while-pending so an awaited build completes and an idle worker never holds a shutting-down daemon; transferable()'s pooled-Buffer guard; ByteWriter.getBuffer() returning a real ArrayBuffer, so the transfer list is valid; searchableStringColumns being functionally identical to hypgrep's own getTextColumnsFromSchema; listLiveDataFiles being manifest-driven, so a sidecar can never be mistaken for a data file; the liveDir the build targets matching resolveIcebergDir for both layouts; report.compacted being set only after the cursor commit, so readCursorSync really names the new generation; and both retention eviction paths being whole-directory rm -r, so sidecars die with their partition (LLP 0264 #lifecycle asks for a test pinning that too - only the retired-generation half is pinned, search-sidecar-build.test.js:212; the retention half is correct by construction).
Dismissed after checking: index_worker_thread.js's static import { ByteWriter } from 'hyparquet-writer' is not an undeclared dependency - it is a declared optionalDependency, and src/core/cache/iceberg/stream_append.js:4 already statically imports the same package on the core cache write path, which is what LLP 0264 #dependency means by "exactly as the cache write path does". src/ ships whole, so the worker module is in the published file set.
Still open, per your decision
Nothing rebuilds a missing sidecar; a corrupt sidecar is permanent; the build pass sits outside max_tick_ms; sidecar bytes are invisible to disk accounting. Not re-raised, not re-stuck. Finding 2 above joins that family.
Local verification on bf79490e
npm run typecheck clean. npm test: 4634 pass, 1 fail, 1 skipped - the fail is test/core/hyparquet-floor-pin.test.js, the known sandbox dedupe failure, which reproduces identically on unmodified origin/grep/t4-grep-service. The six search suites on their own: 59/59. Smokes core_boot_noop, cache_query_debounce, gateway_claude_capture, local_parquet_export, status_diagnostics, cli_bundled_plugins_activated, hypignore_capture_drop, otel_loopback_capture, claude_telemetry_capture all ok. local_only_query_withhold FAILs on its hyp ignore --local-only output assertion, and was confirmed to fail identically in a clean origin/grep/t4-grep-service worktree, so it is pre-existing and unrelated. CI on the PR is the authority.
Triage verdict at
|
philcunliffe
added
the
neutral:approved
…: nine columns, tool_args moves to the not-searched list The maintainer's unstick on PR #954 settled that #953 drops tool_args from SEARCHABLE_COLUMNS. The verb's coverage clause interpolates the constant so it follows on its own, and the query_grep_roundtrip smoke never seeds or asserts a tool_args hit, so the SKILL.md enumeration (both host copies, one shared line) was the only surface stating the old set. The two deliberate host-only lines are untouched, so the divergence fixture needs no re-record. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The T7 review commit that stopped restating the sidecar name imported sidecarPathFor from sidecar_build.js, which is where it lived when T7 was branched. #953's own review round moved it beside GREP_DATASET in searchable_columns.js, so re-stacking T7 on the current T6 tip left the import naming an export that is no longer there: a typecheck error and every maintenance test red. Point it at the module that exports it now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Superseded by #984, which carries this work (and every review fix from this PR) on a single branch cut from current master. The stack could not be merged in sequence: this repo squash-merges, so merging one PR put a commit on master sharing no history with the branches behind it, and each remaining PR then re-proposed its parent's changes. Verified by simulation: merging #951 and then #953 produced add/add conflicts in Integrating on current master also surfaced two problems invisible here: a The branch is retained, so this can be reopened if needed. This PR's task: T6, sidecar builds at maintenance. |
2 participants
What this is
LLP 0265 T6, the wave-4 task, stacked on T4 (#951) like T5 is - retarget to
masterwhen that merges. After this PR the indexed tier is real: compaction finalizes a generation's data files and maintenance immediately gives each one a hypgrep.index.parquetbeside it, which T4's walk then serves throughparquetFind. No user-visible change except speed.The shape
maintainCachefollows every committed rewrite of the grep dataset (GREP_DATASET, now exported beside the allowlist so the search service and the build pass cannot disagree) withbuildSidecarsForTableover the new generation, in its ownmaintenance.grep_indexspan, reported per partition (sidecarsBuilt/sidecarsFailed). The pass can never fail the partition's own verdict: an index that cannot be built costs speed, never the tick.index_worker.js/index_worker_thread.jsare the server pair's shape: bytes in, bytes out by transfer, no IO in the thread, only searchable string columns indexed, a zero-byte answer treated as a protocol breach. One behavioral fix over the port: the worker holds an event-loop ref exactly while a build is in flight. The server's always-unref()worker never bit because a daemon's loop always has other handles; in any process whose loop would otherwise drain (every test, a CLI one-shot), awaiting an unref'd worker's reply deadlocks - the loop empties before the message arrives..index.parquet.tmp, then rename. A killed daemon leaves nothing that lists as a finished index, and the next pass rebuilds whatever is missing. No ledger.Two hazards found and closed
countDataFilesandmeasureDataDircount*.parquetunderdata/- which is where sidecars live. Three data files plus three sidecars reads as six, the LLP 0199 baseline gate sees "grew since compaction", and the partition rewrites itself every tick, generating new sidecars, forever. Both counters now exclude.index.parquet, and a test pins that a second unforced tick stays converged. (The test helpers' ownliveDataFilesglob needed the same fix: it was truncating a sidecar to simulate a torn data file, and the rewrite shrugged.)Tests
test/core/search-sidecar-build.test.js, 7 tests over real caches with the real worker thread: build one sidecar per live file and prove grep serves the indexed tier with identical hits; existence-marker idempotency; the three-attempt quarantine with the scan tier still answering; the corrupt-sidecar fallback; maintenance indexing exactly the grep dataset (a control dataset compacts with no sidecars); sidecars not reading as growth; and a retired generation dying whole with its sidecars inside - the no-GC-code guarantee the plan asked to pin.npm run typecheckclean;npm testgreen modulo the two known environment-only failures noted on #951.🤖 Generated with Claude Code