Repository navigation
Add opt-in native full-text analytics - #3946
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces opt-in native full-text analytics telemetry, upgrading the @harperfast/fulltext dependency to version 0.9.0 (supporting native ABI 14, query API 8, and telemetry API 1). It implements a FullTextTelemetryCollector to manage per-handle telemetry collection, integrates it with the derived index runtime and reader/writer lifecycles, and updates the analytics aggregation cycle to support cumulative telemetry rows. Feedback on the changes identifies an issue in fullTextTelemetry.ts where the pending telemetry buffer's eviction policy can prematurely evict active groups during overflow because Map insertion order is not updated upon updates; deleting and re-inserting the entry is recommended to ensure correct LRU behavior.
|
Thanks, Kris. The CI investigation and follow-up fixes are included in Add opt-in native full-text analytics, through 7b64407a8995. Unit failure — reproduced and repaired. The Node 24 resource shard encountered unhandled analytics 503 errors during a deliberately wedged native commit. The database-level metric write sits outside the collector’s per-table catch. A real regression reproduced the synchronous refusal on both the original PR head and independently installed/built current main with its exact lockfile. The analytics producer now handles synchronous and asynchronous write failures, logs 503 at debug and other failures at warn, and drops that best-effort row. Native backpressure remains intact; there is no retry or overload bypass. The original failure is tracked in Analytics storeMetric rejection tracking. Aggregate/cursor durability remains separate in Implement full-text generation lifecycle, rebuild, eviction, backup, and status. uWS failure — contract refinement, cause still unproven. The uWS shard first missed GenericMirror events, then reported native index lag. Ordinary subscriptions permit intermediate-version coalescing. The ordered test now waits for the first revision and exact source version before patching, retaining all three application/version/key assertions. A separate ungated PUT/PATCH burst requires the latest source version and progress through both native indexes, stable owner epochs and retained sentinels. Live application also asserts zero source re-fetches; a real missing-mirror HTTP GET proves the source-read observation works. Four cases pass on the final source with CI’s uWS flag. The original four cases also passed against an independently installed/built exact old CI merge and lockfile. These local results do not establish the original Linux lag cause or resolution. Review refinements. A rejected native close now applies recording changes that were deferred while closing, so removing the last consumer/operator cannot leave optional native recording enabled. Six real close cases cover both direct collector and actual backend routes, including failed-close usability and retry. Job-worker persistence checks wait for the final row to reach main storage instead of assuming worker exit guarantees visibility. The benchmark launcher retains child errors and waits for child close/output completion before ending its log and clearing deadlines; this does not promise termination if the OS refuses signals. Verification at runtime commit 9f3a91f (before the final test-only/type cleanup): Node 24 resource shard 2,725 passing/121 pending; Node 26 full nine-file native CI selection 92 passed with --forbid-pending/--fail-zero; real collector/close selection 26 passed; shutdown 7 passed; Node 24/uWS 4 passed. The final source-read test refinement also passed all four uWS cases after the cleanup. Earlier combined native/analytics tests passed 144 and LMDB analytics passed 108 before the close repair. Build, lint, formatting and design checks pass. Short paired launcher smoke passed with qualificationSufficient=false; it is not performance qualification. No native indexes, historical binaries or benchmark output are committed. Fresh CI has started on the pushed head; no all-green claim is made. Review limitations: Composer reviewed the runtime fixes and Codex domain adjudication confirmed the failed-close, shutdown-test and launcher repairs. Claude’s final pass failed expired OAuth authentication after its fresh retry; it needs a human login refresh. Composer and Codex then reviewed the final 28-addition/two-deletion source-read/type cleanup and resolved its new findings; all 12 deferral records now have verified open issues. The delta converged on known concerns, but the closing full review remains owed. The unmeasured fulltext 0.8-versus-0.9 default accounting overhead, enabled storage sizing, internal-consumer and transition-loss boundaries remain tracked in Implement full-text generation lifecycle, rebuild, eviction, backup, and status. Existing off/on benchmarks do not measure the upgrade tax. No exact-final convergence or production-performance guarantee is claimed. Earlier dependency cleanup and broad qualification limitsOrdinary installs contain only fulltext 0.9.0. The integration framework resolves Harper to this checkout through a development link and framework-scoped peer override; unrelated unsatisfied peers still fail. Historical 0.5/0.7/0.8 migration fixtures remain separately integrity-locked outside the checkout. Release staging removes development policy. Both CI producers remove the link before upload and consumers recreate it. Canonicalizing the packed-consumer fixture root fixed macOS /var versus /private/var handling. At historical cleanup head b478966, strict installs on Node 22/24, release packaging, four consumer-install lanes and all three native full-text planes passed. All 83 checks completed; the resource shard, uWS shard and aggregate unit gate failed. Green consumer CI completes the prior fixture-ENOENT functional qualification; no install-performance bound is claimed. Earlier broad local gates remain failing/incomplete: resources 6,348 passing/243 pending/147 failures; integration 3,077 passing/67 failures/51 cancelled/20 skipped. Main stopped advancing in componentPreparationLock after more than 50 minutes without a summary, and only its owned process was stopped. Main began before the packaging-fixture correction. Two integration files affected by ambient npm allow-scripts passed 35 tests with an empty npm user configuration; this does not classify every broad failure. Earlier macOS native-plane failures involved four unchanged five-second waits; historical passing local tests did not prove their cause. No deadlines or oracles were relaxed and no blind CI rerun was made. Whole-process worker-report shutdown and deployed operator-enabled qualification remain unexecuted. Clarify full-text telemetry lifecycle and query-policy guidance remains the product-docs companion. fix(analytics): write telemetry through table transactions overlaps persistence; whichever lands second must preserve awaited final commits and rerun shutdown tests. The core PR remains active for human re-review. No merge or release is performed. Comment generated by kAIle (GPT-6) |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
I think I mentioned in this in another PR, but I'd like to eliminate multiple version copies of the full-text package.
🤖 Reviewed with Muse
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
🤖 Reviewed with Claude
Proposed inline comments (anchors failed):
config/RootConfigWatcher.ts:253: [minor] Two successful read paths skip#markAvailable(): the trailing verify-read (config/RootConfigWatcher.ts:253) and the re-arm read after a watcher reopen (config/RootConfigWatcher.ts:118). If either is the read that sees the recreated file,#unavailablestays latched, noavailablefires, and telemetry stays off with its retired overrides until an unrelated laterchange. Disputed severity: chokidar does not throttleadd, so the trailing-read case needs a droppedadd; the reopen-after-handleErrorsibling is the reachable one. Call#markAvailable()from every#read(...) === truesite.config/RootConfigWatcher.ts:251: [minor] The trailing verify-read and the re-arm read (:118) call#read(true)without#markAvailable(). If either of those reads is the first successful read after anunlink,unavailablestays set, and telemetry keeps the missing-file policy. Scope: in-scope
| } | ||
| } | ||
|
|
||
| export function subscribeFullTextTelemetry( |
There was a problem hiding this comment.
nit — subscribeFullTextTelemetry is still exported with no production caller. resources/analytics/DESIGN.md now documents it as an internal diagnostic/test seam, so this is carried as a known, accepted nit. No comment is proposed.
* Admit writes to a caught-up derived index after an idle period The lag policy's cursor term measured from the last durable transaction to the newest one read. After a quiet period longer than the budget, the first transaction read afterwards made the quiet period itself read as lag, and every worker refused writes to the indexed table with 503 DERIVED_INDEX_LAGGING until the next barrier, on an index with nothing behind. Measure from the first transaction read past the durable one. This is what failed sourcedfrom-log-identity's multi-record test in CI: the Generic test before it waited 30 s for a mirror write that subscription coalescing had folded away (sequenced since #3946), and that wait was the idle period. The suite's failure dump now reaches the report through waitFor's message function instead of a message mutated after the stack was captured. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-ci-sourcedfrom-log-identity-flake * Read the oldest undurable timestamp directly and fit the lag note to the design-doc budget Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-ci-sourcedfrom-log-identity-flake * Say the release note's change is to the cursor distance, not the whole lag policy Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-ci-sourcedfrom-log-identity-flake * File the idle-period release note under Vector Search Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-ci-sourcedfrom-log-identity-flake --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
⊙ Problem
Add opt-in native telemetry for full-text lifecycle tracking, using Harper’s existing analytics pipeline.
🤔 Why this boundary?
Reuse protected analytics; leave native measurements in the wrapper and search execution in Tantivy.
💡 Solution
Adopt published fulltext 0.9.0 (ABI14/query8/telemetry1). Dynamically configure existing handles; collection, histograms and byte observations default off. Companion documentation.
⚖️ Alternatives
Framing-Verdict: chosen-approach-sound (00be9dfdd100)
Reuse analytics’ exit flush and shared watcher retention; avoid new shutdown/availability machinery. Historical engines use isolated test installs; the framework uses this checkout.
🤔 Accepted limits
Accounting stays active with collection off. Transitions/disabling can discard observations; churn adds samples beyond cadence. Tracked limits; no delivery or production-overhead guarantee.
🔧 Changes
Product and architecture tour
Change collection without reopening indexes
Valid effective edits update collection. Missing/invalid files retain the last valid policy and overrides; environment/in-process overlays still apply.
Preserve native ownership and final observations
The wrapper supplies native observations; Harper owns demand, labels and export. Exact cumulative rows survive reader churn. Pending eviction uses last-refresh order, preferring live rows before final rows. Graceful HTTP/job exit flushes after owner close; main awaits raw writes and the store commit barrier. Null observations retain their values without object-typed metric metadata.
Reuse existing reports
Separate test engines from ordinary dependencies
Ordinary installs contain fulltext 0.9 only. The framework uses a local Harper development link; its scoped override and link stay out of releases. CI removes it before upload and relinks after download. Historical 0.5/0.7/0.8 fixtures remain isolated. Benchmark output is ignored.
Implementation, tests and documentation
.github/workflows/integration-tests.yml — provisions 0.5 fixtures; both producers remove/exclude the local link before upload, and consumers relink it.
.github/workflows/unit-test.yml — provisions all historical fixtures and requires the native telemetry and migration tests to run.
.gitignore — excludes generated full-text benchmark results.
AGENTS.md — adds the telemetry benchmark to the repository map.
DESIGN.md — indexes the analytics ownership and configuration design notes.
benchmarks/README.md — documents reproducible runs, artifact handling and interpretation limits.
benchmarks/fulltext-analytics.bench.js — measures real Table/native query and write workloads with optional collection and persistence.
benchmarks/fulltext-analytics.mjs — runs alternating isolated profiles, awaits child close after errors and retains output and provenance.
config-root.schema.json — defines strict optional telemetry configuration.
config/DESIGN.md — records last-valid configuration retention and override ownership.
config/fullTextTelemetryConfiguration.ts — applies validated live policy through the shared watcher and environment overlay.
dependencies.md — explains native ownership, isolated migration engines and the temporary local-peer contract.
integrationTests/resources/full-text-search.test.ts — checks deployed binding versions and genuine two-worker upgrade recovery, failing closed when CI provisioning is absent.
package-lock.json — removes registry Harper and old fulltext, retaining current engine and other locked versions.
package.json — pins fulltext 0.9, isolates migration installs and supplies the framework’s local development peer.
renovate.json — keeps historical fixture manifests fixed.
resources/FULL_TEXT.md — documents operator policy, cumulative interpretation, graceful flushing and upgrade limits.
resources/analytics/DESIGN.md — records collection, pending-row and exit-flush invariants; best-effort aggregate writes drop refused rows without promising durable cursor replay.
resources/analytics/fullTextTelemetry.ts — maintains exact counters, reconciles deferred recording changes after failed close and bounds pending observations.
resources/analytics/fullTextTelemetryRuntime.ts — combines opt-in demand, live policy and existing analytics listeners without opening native handles.
resources/analytics/metadata.ts — names the fulltext analytics metric.
resources/analytics/write.ts — serializes periodic/final reports, folds cumulative rows, and handles refused aggregate writes without bypassing backpressure.
resources/derivedIndexes.ts — supplies stable encoded database/table/index labels.
resources/indexes/DESIGN.md — records native telemetry capability and handle-lifecycle ownership.
resources/indexes/fullTextDerivedIndex.ts — pauses observation during writer close and resumes after a rejected close.
resources/indexes/fullTextNativeBinding.ts — validates ABI/query/telemetry capabilities and uses structural telemetry types.
resources/indexes/fullTextQueryIndex.ts — registers reader telemetry and closes through observation hooks.
resources/indexes/nativeFullTextDerivedIndexLifecycle.ts — registers actual writer handles.
server/jobs/jobProcess.ts — flushes analytics after job database cleanup and before cleanup acknowledgement.
server/threads/threadServer.ts — flushes analytics after HTTP owner cleanup and before worker exit.
unitTests/build-tools/buildDist.test.mjs — checks production imports and emitted declarations without the optional package.
unitTests/config/fullTextTelemetryConfiguration.test.js — verifies boot/live unknown-key rejection, real missing-file reads, revival and override precedence.
unitTests/resources/analytics/aggregationCycle.test.js — checks exact newest-row cumulative folding.
unitTests/resources/derivedIndexRuntimeRocks.test.js — updates native capability fixtures.
unitTests/resources/fixtures/analyticsExitFlush.cjs — drives actual overlapping and failing analytics windows in a managed worker.
unitTests/resources/fixtures/fullTextTelemetryJobExit.cjs — observes native counters through the real job-worker exit path.
unitTests/resources/fixtures/fullTextTelemetryShutdown.cjs — holds real worker owner cleanup to verify final telemetry ordering.
unitTests/resources/fixtures/fullTextTelemetryWorker.cjs — drives cross-worker observation and policy timing.
unitTests/resources/fixtures/fullTextUpgradeHelpers.cjs — launches exact historical engines in explicitly provisioned external roots.
unitTests/resources/fixtures/fullTextUpgradeProcess.cjs — writes and probes genuine old checkpoints with exact version assertions.
unitTests/resources/fixtures/fulltext-upgrade/0.5.0/package-lock.json — integrity-locks the 0.5.0 test dependency tree.
unitTests/resources/fixtures/fulltext-upgrade/0.5.0/package.json — declares the exact released 0.5.0 test engine.
unitTests/resources/fixtures/fulltext-upgrade/0.7.0/package-lock.json — integrity-locks the 0.7.0 test dependency tree.
unitTests/resources/fixtures/fulltext-upgrade/0.7.0/package.json — declares the exact released 0.7.0 test engine.
unitTests/resources/fixtures/fulltext-upgrade/0.8.0/package-lock.json — integrity-locks the 0.8.0 test dependency tree.
unitTests/resources/fixtures/fulltext-upgrade/0.8.0/package.json — declares the exact released 0.8.0 test engine.
unitTests/resources/fixtures/installFullTextUpgradeDependencies.mjs — installs selected supported fixtures from integrity locks outside the checkout.
unitTests/resources/fullTextNativeTestBinding.js — updates structural native capability fixtures.
unitTests/resources/nativeFullTextDerivedIndexLifecycle.test.js — preserves real old-engine rebuild and compatible-checkpoint reuse coverage.
unitTests/resources/nativeFullTextIndexSchemaUpgrade.test.js — uses isolated released-engine fixtures for schema-driven upgrade coverage.
unitTests/resources/nativeFullTextIndexUpgradeCrash.test.js — uses isolated fixtures for interrupted-upgrade recovery.
unitTests/resources/nativeFullTextTelemetry.test.js — checks real snapshots, exact arithmetic, row recency, final-row preference and persisted output.
unitTests/resources/nativeFullTextTelemetryClose.test.js — checks real close gating, rejected-close recording changes through direct/backend paths, retry and final writer observations.
unitTests/resources/nativeFullTextTelemetryShutdown.test.js — waits for actual HTTP/job row persistence after worker exit; checks serialized final flushing and failure ownership.
unitTests/resources/nativeFullTextTelemetryWorkers.test.js — checks managed worker reports, policy timing and activation diagnostics.
utility/environment/environmentManager.ts — validates subtree changes before retiring only matching changed overrides.
utility/hdbTerms.ts — defines the optional configuration keys.
validation/configValidator.ts — rejects unknown telemetry keys and validates strict booleans/interval bounds.
resources/analytics/read.ts — omits null-valued attributes from inferred metric types while retaining stored observations.
unitTests/resources/fixtures/analyticsMainExit.cjs — writes a final report, exits immediately and verifies the real store after reopen.
.github/DESIGN.md — records artifact/relink ownership.
.github/workflows/lockfile-npm-compat.yml — enforces strict peers on the existing compatibility matrix.
build-tools/DESIGN.md — records development-policy removal at release staging.
build-tools/bundleDependencies.ts — removes the scoped development override and rejects it in release validation.
integrationTests/README.md — documents the built local CLI, explicit selection and directory-link safety.
unitTests/build-tools/bundleDependencies.test.mjs — rejects foreign peers and release-policy leaks; canonicalizes macOS fixture paths.
integrationTests/resources/sourcedfrom-log-identity.test.ts — checks ordered delivery, rapid supersession, both native indexes and zero source re-fetches, with a real cache-miss positive control.
unitTests/resources/noAbortAfterTxnLog.test.js — reproduces real metric-write shedding under a wedged native commit without weakening rejection or lock assertions.
integrationTests/resources/sourcedfrom-log-identity/resources.ts — observes genuine source-get calls in the single-worker fixture, preserving the real provider.
✅ Verification
CI repairs: build/lint/design pass; runtime shard 2,725 passing/121 pending; native selection 92, collector/close 26, shutdown 7; final uWS 4. Regressions fail before repair. Short launcher smoke passes, insufficient for performance qualification. Historical gates/limits below.
Broader gate limits and historical measurements
Runtime-suite counts above are from 9f3a91f. The final 28-addition/two-deletion follow-up changes only the source-read test/provider and an unused type export; build/lint and its four uWS cases pass. The broad runtime suites were not repeated for that follow-up.
Native 88/packaging 63/deployed 17 precede the final one-line validator fix; the final rebuilt configuration suite passes 9.
Earlier broad local runs, before this CI repair: resources 6,348 passing/243 pending/147 failures; integration 3,077 passing/67 failures/51 cancelled/20 skipped. Main stopped advancing in componentPreparationLock after more than 50 minutes without a summary; its owned process was stopped. Main began before the test-fixture path correction; the focused packaging suite passed afterward. Remaining broad failures are not all baseline-classified.
Two integration files affected by EALLOWSCRIPTS passed all 35 tests with an empty npm user configuration. This supports that diagnosis for those files, not every integration failure. Initial clean strict installs passed on npm 10/11/12; later repeated/final production-consumer probes were inconclusive. A bounded offline probe also timed out with the unchanged graph. No install-performance qualification is claimed.
Pre-cleanup head
5c568c3macOS native-plane CI passed 84 and failed four five-second waits: managed-worker policy ordering, job flush, deferred-scope exit and Table telemetry activation. Failed macOS job. Linux/Windows native planes passed. Cause is not established; local 88 passing tests do not resolve it.Historical cleanup-head CI (b478966830e7): strict Node 22/24 installs, release-package build, four consumer-install lanes (Linux npm 10/11/12 and Windows npm 12), and native full-text planes on Linux/Windows/macOS pass. Actual archived-build download and dependency relinking also succeeded. This completes the consumer-CI qualification for the historical fixture ENOENT at this head; the managed review record remains historical bookkeeping. All checks completed: Node 24 resources failed on analytics 503; uWS failed GenericMirror progress then native lag; the aggregate unit gate also failed. Analytics reproduced on independently built current main with its exact lockfile; its best-effort producer now catches synchronous/asynchronous failures. See Analytics storeMetric rejection tracking. The test now respects subscription coalescing and separately requires latest-version and both-index progress after rapid supersession. Original CI native lag remains unattributed; local passing runs do not establish its cause. Fresh CI is running on the repaired head; no all-green claim.
Earlier paired 70-second bounded-lag qualification validated 8,003,174 queries; enabled throughput was 0.7%/2.1% lower. Strict-freshness variation remains unexplained. These historical measurements do not qualify final-code production overhead or sustained memory.
🤔 Review depth
Final delta review resolved the new findings. Accounting-upgrade baseline and storage sizing remain; closing full review is owed. Claude authentication failed; Composer and Codex adjudicated the follow-up. No completed closing-review claim.
Coordinate with fix(analytics): write telemetry through table transactions, then rerun shutdown tests.
PR QA: skipped @ 7b64407 — dispatch CLI unavailable
Comment generated by kAIle (GPT-6)
Release target: v5.4 — new opt-in development-line surface
Complexity: complicated
Related PRs: #2170 overlaps (raw analytics writes), #3847 overlaps (dependency/binding), #3846 overlaps (reader lifecycle), 4 others independent
Deferred findings
0c5c09fdd7dfee94minor performance —resources/analytics/fullTextTelemetry.ts— enabled collection observes and exports on every reader retirement; rate follows churn, not interval — filed Implement full-text generation lifecycle, rebuild, eviction, backup, and status #2510c986c0e301871907minor reliability —.github/workflows/npm-package-app-e2e.yml— packed-consumer npm ci reportedly fails with ENOENT on npm 11/12 — filed Implement full-text generation lifecycle, rebuild, eviction, backup, and status #251076eee034ee4212bcminor data-integrity —resources/analytics/write.ts— Aggregation cursor can advance past failed aggregate writes — filed Implement full-text generation lifecycle, rebuild, eviction, backup, and status #2510e00b38df16d81c48nit reliability —server/threads/threadServer.ts— main with threads.count 0 never sets componentsLoading, so a scope created after the deferred sweep stays open at exit — filed Graceful shutdown doesn't await scope 'close' cleanup — app teardown races process.exit #1912667f9e0a6b041345nit performance —resources/analytics/fullTextTelemetryRuntime.ts— Disabled telemetry installs configuration and flush listeners on every thread — filed Implement full-text generation lifecycle, rebuild, eviction, backup, and status #2510ce7cd0c4cd6a8fcbnit correctness —resources/analytics/fullTextTelemetry.ts— Stored telemetry rows retain null-valued leaves — filed Implement full-text generation lifecycle, rebuild, eviction, backup, and status #251004874ab2c4f55188nit reliability —resources/analytics/fullTextTelemetryRuntime.ts— main receive-time policy filter drops worker rows incl. final rows uncounted during transitions — filed Implement full-text generation lifecycle, rebuild, eviction, backup, and status #2510ee4ef8e5d12360a6nit reliability —resources/analytics/write.ts— Receive-time policy changes can discard worker telemetry rows — filed Implement full-text generation lifecycle, rebuild, eviction, backup, and status #251010197948fc9e8249nit reliability —build-tools/bundleDependencies.ts— Bundle validation recognizes only the current dev override — filed Implement full-text generation lifecycle, rebuild, eviction, backup, and status #2510dee1eab820047bf4nit maintainability —resources/analytics/fullTextTelemetryRuntime.ts— Internal telemetry subscriber has no production caller and can alter global collection policy — filed Implement full-text generation lifecycle, rebuild, eviction, backup, and status #2510Review-Coverage: authored=codex; ran=cursor-composer,claude; adjudicated=domain(claude),domain; declined=gemini,cursor-grok,muse; rounds=21; full=11 @ 7b64407
Review-Attention: deep ~70m (critical: write.ts, derivedIndexes.ts +4; sensitive: package-lock.json; hot: threadServer.ts; decisions: do-less-alternative, disabled-accounting-boundary, transition-loss-semantics, historical-engine-provisioning; raised: degraded review) @ 7b64407