refactor: remove Elasticsearch and the BullMQ legacy stack#4547
Merged
Conversation
Contributor
|
Important Review skippedToo many files! This PR contains 354 files, which is 204 over the limit of 150. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (354)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
0xdeafcafe
force-pushed
the
refactor/remove-elasticsearch-and-background
branch
from
June 3, 2026 18:53
65d0623 to
663ac10
Compare
rogeriochaves
left a comment
rogeriochaves
left a comment
Contributor
There was a problem hiding this comment.
3 findings: 1 CI/install blocker and 2 stale references left by the ES/BullMQ/topic-clustering removal. Details inline.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
0xdeafcafe
force-pushed
the
refactor/remove-elasticsearch-and-background
branch
from
June 4, 2026 09:32
3b98914 to
e671306
Compare
0xdeafcafe
force-pushed
the
refactor/remove-elasticsearch-and-background
branch
from
June 20, 2026 05:07
e35be4a to
f8d8cde
Compare
0xdeafcafe
added a commit
that referenced
this pull request
Jun 20, 2026
Verified P0 + behavioral P1s from the PR #4547 audit (the blocker set). Each path regressed because logic was dropped while extracting code out of the deleted BullMQ worker. - workers.ts: re-wire worker-role boot — scenario execution pool (P0: simulations were silently skipped on workers), worker /metrics listener, anomaly detector, ClickHouse storage-stats collection; add graceful SIGINT/SIGTERM shutdown with handle cleanup. - runEvaluation.ts: restore code-evaluator routing (code/<id> -> runCodeEvaluator) and the embeddings_model env setup + openai/moderation guard the extracted module lost vs the canonical service. - evaluation-execution.service.ts: enrich trace.evaluations in executeForTrace before building mapped data (parity with runEvaluationForTrace; the monitor path fed an empty evaluations source). - routes/collector.ts: route REST ingestion through ingestNormalizedSpan so it shares the (tenant,trace,span) dedup gate + ADR-022 spool with OTLP, instead of calling recordSpan directly. - tracer/collector: restore unit coverage for the moved span-tree/text-extraction helpers (deleted common.test.ts had no committed replacement). typecheck clean; 235 evaluation unit tests green.
0xdeafcafe
added a commit
that referenced
this pull request
Jun 20, 2026
…nd mapper builders Audit §B6 follow-up to PR #4547. No live callers remain after the ES backends were dropped: generateTracesPivotQueryConditions/generateFilterConditions (analytics/common.ts) and mapEsRunToExperimentRun/mapEsBatchEvaluationToRunWithItems + ESRunAggregationBucket (experiments-v3/mappers.ts). Keeps the still-live mapEsTargetsToTargets (used by routes/evaluations-legacy.ts). Drops the now-orphaned tests and a stale filter-conditions comment.
0xdeafcafe
force-pushed
the
refactor/remove-elasticsearch-and-background
branch
from
June 23, 2026 13:39
10cdf4c to
6c77ab3
Compare
0xdeafcafe
added a commit
that referenced
this pull request
Jun 23, 2026
Verified P0 + behavioral P1s from the PR #4547 audit (the blocker set). Each path regressed because logic was dropped while extracting code out of the deleted BullMQ worker. - workers.ts: re-wire worker-role boot — scenario execution pool (P0: simulations were silently skipped on workers), worker /metrics listener, anomaly detector, ClickHouse storage-stats collection; add graceful SIGINT/SIGTERM shutdown with handle cleanup. - runEvaluation.ts: restore code-evaluator routing (code/<id> -> runCodeEvaluator) and the embeddings_model env setup + openai/moderation guard the extracted module lost vs the canonical service. - evaluation-execution.service.ts: enrich trace.evaluations in executeForTrace before building mapped data (parity with runEvaluationForTrace; the monitor path fed an empty evaluations source). - routes/collector.ts: route REST ingestion through ingestNormalizedSpan so it shares the (tenant,trace,span) dedup gate + ADR-022 spool with OTLP, instead of calling recordSpan directly. - tracer/collector: restore unit coverage for the moved span-tree/text-extraction helpers (deleted common.test.ts had no committed replacement). typecheck clean; 235 evaluation unit tests green.
0xdeafcafe
added a commit
that referenced
this pull request
Jun 23, 2026
…nd mapper builders Audit §B6 follow-up to PR #4547. No live callers remain after the ES backends were dropped: generateTracesPivotQueryConditions/generateFilterConditions (analytics/common.ts) and mapEsRunToExperimentRun/mapEsBatchEvaluationToRunWithItems + ESRunAggregationBucket (experiments-v3/mappers.ts). Keeps the still-live mapEsTargetsToTargets (used by routes/evaluations-legacy.ts). Drops the now-orphaned tests and a stale filter-conditions comment.
Aryansharma28
left a comment
Aryansharma28
left a comment
Contributor
There was a problem hiding this comment.
Review summary — goated-review pass
Verdict: 1 blocker, 12 non-blockers. All net-new — every existing reviewer thread is already resolved.
Methodology: pulled the room first (existing reviewers, CodeRabbit/Copilot, CI), bucketed the 262 files by risk (lockfile, mass deletions, behavior changes, workers/queues, EE admin, docs), grilled the PR description's own "Anything surprising?" claims, adversarially verified each finding before posting, and deduped against existing reviews.
🛑 BLOCKER
B1 — Worker memory-leak safety restart removed without replacement (see inline at src/workers.ts)
Pre-PR src/workers.ts on origin/main had an env-gated 15-minute setTimeout that exited the worker, with a comment that explicitly named the case it protected: "memory-leak safety pattern... In environments without that supervisor (e.g. npx @langwatch/server, where supervise() in spawn.ts doesn't restart-on-exit), set LANGWATCH_WORKERS_MAX_RUNTIME_MS=0 to disable the timer." The PR description argues the timer is fine to drop because "helm/docker already restart-on-exit," but that's exactly backward — those supervisors only fire on exit; the timer was what caused the periodic exit so that a slow leak couldn't grow unbounded.
The post-PR file has no setTimeout/setInterval/RSS check/liveness mechanism. For the npx @langwatch/server path the PR comment specifically called out, there's now nothing.
Fix options (any one): (a) preserve the timer behind the same env switch (default-on, ~5 lines); (b) prove the underlying leak has been independently fixed (link the PR); (c) explicitly retire the npx @langwatch/server path and document it in this PR's description.
NON-BLOCKERS — code
NB5 — ExperimentRunService.getRun inconsistency (inline at experiment-run.service.ts:714)
NB6 — Worker getEvaluationsMultiple callers now hard-fail (inline at evaluation-execution.service.ts:273)
NON-BLOCKERS — EE admin + Prisma footprint (no diff context, summary only)
NB1 — PR description claim 1 is partially wrong: the EE admin layer still encrypts and writes the ES Prisma columns.
The "Anything surprising?" section says "only the application layer that reads/writes them" was removed. In tracked code on this PR head, the three columns are still read/written end-to-end via the EE admin path:
langwatch/ee/admin/backoffice/resources/OrganizationsView.tsx:192-518— full superadmin form foruseCustomElasticsearch/elasticsearchNodeUrl/elasticsearchApiKey, with submit handler that posts these back to the org update.langwatch/ee/admin/safeSelects.ts:29—useCustomElasticsearch: truestill in the orgselectshape.langwatch/src/server/app-layer/organizations/repositories/organization.repository.ts:173-174— repository interface still declares both fields optional inUpdateOrganizationInput.langwatch/src/server/app-layer/organizations/repositories/organization.prisma.repository.ts:445-449—prisma.updatestill encrypts and writeselasticsearchNodeUrl/elasticsearchApiKeywhen the caller passes them.langwatch/src/factories/organization.factory.ts:17-19— test factory still defaults all three.
User-facing settings page was correctly stripped; the EE admin path is intact. Fix: either amend the description (so a future drop-column migration scopes the EE admin too) or remove the EE write path now. None of these files are in the PR's diff, hence summary-only.
NON-BLOCKERS — other code paths (no diff context)
NB7 — Third unmentioned BullMQ importer. langwatch/bullboard/src/server.ts:17 imports Queue from bullmq. PR description's commit-2 list names 6 importers (puller×2, topic-clustering×2, queues×2); the actual set is 7 with bullboard. Worth adding so any future BullMQ removal scopes it.
NB8 — Stale vi.mock("ElasticsearchTraceService", ...) in 2 unit tests. langwatch/src/server/traces/__tests__/trace-service-blob-resolution.unit.test.ts:54 and trace-service-4888-full-flag.unit.test.ts:82. Mock factory now silently no-ops since the module shape is gone. Either drop the mock or replace with the CH-only shape. (Only the ~/server/elasticsearch/protections → ~/server/traces/protections import is in the PR's diff for these files; the mock line at 54 / 82 is unchanged context, so no inline anchor.)
NB9 — Stale references to deleted routes in docs + a file-header comment.
langwatch/dev/docs/security/hono-api-rbac-audit.md:50-51— lists/api/rerun_checks+/api/start_workersas live unauthenticated routes; both deleted.langwatch/src/server/routes/misc.ts:11-12— file-header comment still points to deletedsrc/pages/api/rerun_checks.ts/start_workers.ts.langwatch/dev/docs/adr/006-redis-cluster-bullmq-hash-tags.md:28andadr/023-orphan-sweep-reactor-chain.md:175-181— ADRs reference deletedsrc/server/background/. ADRs are historical, but a one-line "superseded by #4547" prevents future readers chasing dead paths.
NB10 — makeQueueName import path inconsistency (bundle-protection nit). langwatch/src/server/event-sourcing/eventSourcing.ts:9 and langwatch/ee/governance/services/pullers/pullerWorker.ts:35 still import makeQueueName via the ~/server/redis re-export instead of the standalone ~/server/queues/makeQueueName module. Server-only files, so no immediate bundle harm — but the inconsistency invites a future "copy this import" mistake that does pull ioredis into a client chunk. Route everything through the standalone module.
NON-BLOCKERS — PR description fixes
NB2 — Lockfile bumps paragraph is stale. After rebase (03be9869), the hono / qs / ws / mermaid / liquidjs / turbo / @opentelemetry/* bumps the description warns about are gone (git diff origin/main -- langwatch/pnpm-lock.yaml is clean of those). Remove the paragraph.
NB3 — Wrong env-var name in the reproduce note. Description says SKIP_HOST_REDIS_CHECK=1 to skip the host Redis check; the actual variable at langwatch/src/server/redis.ts:27 is SKIP_REDIS. Reproducers will be confused.
NB4 — withJobContext location wrong. Commit-2 section says the helper moved to ee/governance/services/pullers/withJobContext.ts; the actual location is langwatch/src/server/queues/withJobContext.ts (the later corrected note mentions src/server/queues/ but the wording could be tightened).
NON-BLOCKER — CI infra (not this PR's defect)
NB11 — The evaluate workflow can't review a PR this size. The job's gh pr diff call returns HTTP 406 for diffs >20k lines, exiting the step with code 1 before the workflow's own oversized=true size guard runs. Pre-existing brittleness in .github/workflows/evaluate.yml (the call should be gh pr diff ... > /tmp/pr_diff.txt 2>/dev/null || true so the size check can decide). Worth a separate one-line PR against the workflow.
What I could NOT verify (worth flagging)
- Whether the underlying memory-leak the 15-min timer protected against has been independently fixed in another PR. If yes, B1 demotes to a description nit ("timer removed because leak X was fixed in #YYYY"). If no, B1 stays as a regression risk for long-uptime envs.
- Whether
/cron/schedule_topic_clusteringis still triggered post-PR end-to-end (handler is kept, but I didn't trace the cron-scheduler → handler chain to confirm it fires).
Existing reviewer findings I am NOT re-raising (already resolved)
- @rogeriochaves's P1 (lockfile importer /
hasown@2.0.4/tinyexec@1.2.3) — resolved in03be9869. - @rogeriochaves's P2 (topic-clustering left in settings sidebar after route deletion) — resolved in
03be9869(full topic-clustering restore: route, MenuLink, command-bar, worker). - @rogeriochaves's P2 (stale
~/server/background/...mock paths in three tests) — resolved in03be9869. github-code-quality[bot]×5 (unusedCostReferenceType/CostType/nanoid/getProtectionsForProject/captureException/withScope/loggerinrunEvaluation.ts; unusedprismainhttpProxyTracing.ts) — resolved in54cea3de+10cdf4cb.
Reviewed with the goated-review skill: read-the-room → slice-by-risk → find → adversarial-verify → dedupe → classify → show-the-plan → post.
0xdeafcafe
added a commit
that referenced
this pull request
Jun 24, 2026
…-CH (#4547 review) Per Aryansharma28's review (thread PRRT_kwDOKRXhvM6L6N-6): getRun's null-on-no-CH was a deliberate UX call (PR #3483 dogfood — fold-window polling race) but the class-level JSDoc still claimed all methods return null, and getRun's own JSDoc didn't explain the divergence from listRuns / getRunAggregates / listRunsForExperimentPaginated (which throw). Update the class JSDoc to call out the exception, expand getRun's JSDoc with the rationale + the route caller's intentional 404 mapping, and add an inline comment at the no-CH guard.
0xdeafcafe
force-pushed
the
refactor/remove-elasticsearch-and-background
branch
from
June 24, 2026 13:39
b5e34fd to
5c03f72
Compare
0xdeafcafe
added a commit
that referenced
this pull request
Jun 24, 2026
Verified P0 + behavioral P1s from the PR #4547 audit (the blocker set). Each path regressed because logic was dropped while extracting code out of the deleted BullMQ worker. - workers.ts: re-wire worker-role boot — scenario execution pool (P0: simulations were silently skipped on workers), worker /metrics listener, anomaly detector, ClickHouse storage-stats collection; add graceful SIGINT/SIGTERM shutdown with handle cleanup. - runEvaluation.ts: restore code-evaluator routing (code/<id> -> runCodeEvaluator) and the embeddings_model env setup + openai/moderation guard the extracted module lost vs the canonical service. - evaluation-execution.service.ts: enrich trace.evaluations in executeForTrace before building mapped data (parity with runEvaluationForTrace; the monitor path fed an empty evaluations source). - routes/collector.ts: route REST ingestion through ingestNormalizedSpan so it shares the (tenant,trace,span) dedup gate + ADR-022 spool with OTLP, instead of calling recordSpan directly. - tracer/collector: restore unit coverage for the moved span-tree/text-extraction helpers (deleted common.test.ts had no committed replacement). typecheck clean; 235 evaluation unit tests green.
0xdeafcafe
added a commit
that referenced
this pull request
Jun 24, 2026
…nd mapper builders Audit §B6 follow-up to PR #4547. No live callers remain after the ES backends were dropped: generateTracesPivotQueryConditions/generateFilterConditions (analytics/common.ts) and mapEsRunToExperimentRun/mapEsBatchEvaluationToRunWithItems + ESRunAggregationBucket (experiments-v3/mappers.ts). Keeps the still-live mapEsTargetsToTargets (used by routes/evaluations-legacy.ts). Drops the now-orphaned tests and a stale filter-conditions comment.
0xdeafcafe
added a commit
that referenced
this pull request
Jun 24, 2026
…-CH (#4547 review) Per Aryansharma28's review (thread PRRT_kwDOKRXhvM6L6N-6): getRun's null-on-no-CH was a deliberate UX call (PR #3483 dogfood — fold-window polling race) but the class-level JSDoc still claimed all methods return null, and getRun's own JSDoc didn't explain the divergence from listRuns / getRunAggregates / listRunsForExperimentPaginated (which throw). Update the class JSDoc to call out the exception, expand getRun's JSDoc with the rationale + the route caller's intentional 404 mapping, and add an inline comment at the no-CH guard.
0xdeafcafe
force-pushed
the
refactor/remove-elasticsearch-and-background
branch
from
July 3, 2026 11:34
5c03f72 to
fced137
Compare
0xdeafcafe
added a commit
that referenced
this pull request
Jul 3, 2026
Verified P0 + behavioral P1s from the PR #4547 audit (the blocker set). Each path regressed because logic was dropped while extracting code out of the deleted BullMQ worker. - workers.ts: re-wire worker-role boot — scenario execution pool (P0: simulations were silently skipped on workers), worker /metrics listener, anomaly detector, ClickHouse storage-stats collection; add graceful SIGINT/SIGTERM shutdown with handle cleanup. - runEvaluation.ts: restore code-evaluator routing (code/<id> -> runCodeEvaluator) and the embeddings_model env setup + openai/moderation guard the extracted module lost vs the canonical service. - evaluation-execution.service.ts: enrich trace.evaluations in executeForTrace before building mapped data (parity with runEvaluationForTrace; the monitor path fed an empty evaluations source). - routes/collector.ts: route REST ingestion through ingestNormalizedSpan so it shares the (tenant,trace,span) dedup gate + ADR-022 spool with OTLP, instead of calling recordSpan directly. - tracer/collector: restore unit coverage for the moved span-tree/text-extraction helpers (deleted common.test.ts had no committed replacement). typecheck clean; 235 evaluation unit tests green.
0xdeafcafe
added a commit
that referenced
this pull request
Jul 3, 2026
…nd mapper builders Audit §B6 follow-up to PR #4547. No live callers remain after the ES backends were dropped: generateTracesPivotQueryConditions/generateFilterConditions (analytics/common.ts) and mapEsRunToExperimentRun/mapEsBatchEvaluationToRunWithItems + ESRunAggregationBucket (experiments-v3/mappers.ts). Keeps the still-live mapEsTargetsToTargets (used by routes/evaluations-legacy.ts). Drops the now-orphaned tests and a stale filter-conditions comment.
0xdeafcafe
added a commit
that referenced
this pull request
Jul 3, 2026
…-CH (#4547 review) Per Aryansharma28's review (thread PRRT_kwDOKRXhvM6L6N-6): getRun's null-on-no-CH was a deliberate UX call (PR #3483 dogfood — fold-window polling race) but the class-level JSDoc still claimed all methods return null, and getRun's own JSDoc didn't explain the divergence from listRuns / getRunAggregates / listRunsForExperimentPaginated (which throw). Update the class JSDoc to call out the exception, expand getRun's JSDoc with the rationale + the route caller's intentional 404 mapping, and add an inline comment at the no-CH guard.
0xdeafcafe
added a commit
that referenced
this pull request
Jul 3, 2026
…asticsearch/protections (rebase fixup) PR #4547 moves src/server/background/workers/collector/cost -> src/server/tracer/collector/cost and src/server/elasticsearch/protections -> src/server/traces/protections. Upstream commits landed 3 files that import from the old paths; the rebase onto current main surfaced the dangling imports. Retarget to the new locations.
Aryansharma28
left a comment
Aryansharma28
left a comment
Contributor
There was a problem hiding this comment.
Review summary — goated-review follow-up pass
Verdict: COMMENT. P0: 0 | P1: 1 | P2: 1 | P3: 0 | P4: 1 | P5: 2
This is a re-verification of my prior review pass (2026-06-24) plus a fresh sweep. Methodology: re-checked every previously-flagged finding against current code (not against the author's reply text), ran two independent audits (collector dedup semantics, throw-vs-null call-site safety across all 3 collapsed facades — 13 production call sites checked), and grepped the whole branch for orphaned references to every deleted route/file/package.
What's genuinely fixed since the last pass
- Lockfile — clean, identical to
main. - Topic-clustering route/sidebar entry — restored correctly.
- PII redaction test mocks — updated to the new module paths.
ExperimentRunService.getRunnull-vs-throw exception — properly documented with a regression test backing the rationale.- Facade throw-vs-null semantics — audited every caller of every newly-throwing method on
EvaluationService,ExperimentRunService,FilterService. Zero new at-risk workers/crons beyond the two the PR description already discloses. - Orphaned references to every deleted route/file/package (
start_workers,rerun_checks,scenario_analytics,traces_retention_period_cleanup,SimulationFacade,@elastic/elasticsearch, helm/CI configs) — clean, nothing dangling except one harmless stale doc-comment (P4 below).
🟠 P1 — re-raised, not actually addressed
See inline comment on langwatch/src/workers.ts. My prior BLOCKER finding on the removed memory-leak self-restart got a reply ("no longer needed, workers are safe now") but no code change. I re-grepped the whole PR branch for any replacement liveness/restart mechanism — none exists.
🟡 P2 — new finding this pass
See inline comment on langwatch/src/server/routes/collector.ts. The REST collector's dedup mechanism changed semantics, not just location, and that's understated in the PR description.
P4 — nit
langwatch/src/server/routes/misc.ts lines 11-12 (
langwatch/langwatch/src/server/routes/misc.ts
Lines 11 to 12 in fced137
Replaces: JSDoc list still names src/pages/api/rerun_checks.ts and src/pages/api/start_workers.ts, both deleted by this PR. Harmless (doc-only, and outside the diff's hunk window so it can't be anchored as an inline comment), but misleading to a future reader tracing where those routes went. Suggested fix: drop those two lines from the list.
P5 — praise / observations (non-blocking)
- Praise: the collapsed-facade throw/null rule is applied consistently. I traced all 13 production call sites across the 3 collapsed services and found zero surprises beyond what the PR description already calls out — that's a clean, disciplined refactor of a genuinely risky semantic change.
- Observation: the
evaluateCI check is failing, but it's the automated-eval bot hitting the same 20k-line GitHub diff API cap that CodeRabbit and Copilot both hit on this PR (could not find pull request diff: HTTP 406... too_large). Not a defect in this PR —alls-green, the actual required aggregate gate, passes. Flagging for awareness only: no automated tool has been able to review this diff end-to-end; human review is the only real gate here.
Existing findings NOT re-raised (verified fixed, not just claimed)
- @rogeriochaves' lockfile P1 — confirmed clean vs
main. - @rogeriochaves' topic-clustering route P2 — confirmed restored.
- @rogeriochaves' PII test mock P2 — confirmed updated.
- My own prior
getRunnull-vs-throw non-blocker — confirmed properly resolved with a regression test.
Things I couldn't verify from the diff alone
- Whether SaaS-deployed workers (k8s/helm) actually get restarted promptly on OOM via the platform's restart policy — I'm inferring this from general k8s behavior, not from this repo's helm chart. Worth confirming with whoever owns the workers deployment manifest before treating the P1 as low-risk for the hosted product; it's not low-risk for the
npx @langwatch/serverself-hosted path either way. - Actual production traffic patterns for delayed/replayed span ingestion (backfills, SDK retry queues) that would hit the new 1-hour dedup TTL — I can only confirm the mechanism changed, not how often the old unbounded window was actually relied upon in practice.
…eview-pr audit) Re-homes the deleted suites' assertions against the surviving subjects: TraceUsageService (incl. the CH-unavailable ?? 0 branch), makeQueueName hash-tags, metrics auth fail-closed + getWorkerMetricsPort, REST collector result handling + 429 span cap, googleDLP redaction, stall derivation, RUN_STARTED metadata pass-through, extensible-metadata schemas, evaluator-extras allowlist, and monitor settings resolution.
Restores behaviours silently dropped with the BullMQ stack: the 5-minute spend-spike anomaly evaluation tick, daily self-hosted usage telemetry, and OTel instrumentation in the worker process. Forwards the bearer token on the /workers/metrics proxy, returns 500 when every span in an ingest request fails, fixes googleDLPClearPII discarding its own redactions, and gates decrypted s3SecretAccessKey behind manage permission. Deduplicates the track_event route against the shared service, extracts member-classification helpers, honours traceIds in CH timeseries, and deletes the dead ES-era helpers, builders, and modules the audit flagged.
…nciliation - Retarget five scenario-run-utils annotations to the renamed scenarios in unified-run-table.feature (ES/BullMQ wording was rewritten) - Drop the vacuous 'does not import Elasticsearch' source check; the module no longer exists and the spec scenario was removed - Restore the missing binding for 'Uses cached count within TTL' on the trace-usage cache test - Mark the event_details aggregation-keys scenario @unimplemented: its ES binding died with the ES backend and events.event_details is not yet supported by the ClickHouse metric translator
concurrently (pnpm dev) sets FORCE_COLOR, which makes console.log wrap numbers in ANSI colour codes even when piped — the coloured start timestamp then got interpolated into the elapsed-time eval and killed the whole prepare chain with a SyntaxError. Write raw strings instead.
The playground proxy's provider inference only knew the six original providers, so a default model like xai/grok-4.3 hard-400'd with missing_provider on /go/proxy/v1/chat/completions. Map the remaining TS-registry providers on both nlpgo paths (x-litellm headers for the playground proxy, inline credentials for llmexecutor/Studio): - xai / groq / cerebras are Bifrost-native and ride the ProviderID passthrough with a plain API key - deepseek is absent from Bifrost's enum; route it through the vLLM (openai-compat) adapter defaulting to https://api.deepseek.com - plain api-key providers share one Generic inline-credentials slot instead of four copy-pasted per-provider slots; the duplicated provider switches in gatewayproxy are merged into providerForPrefix Spec: specs/nlp-go/proxy.feature (provider-prefix routing scenarios)
nlpgo's handled-error envelope (herr) was flattened to a 500 with a bare string by the scenario-generate route, leaving the browser with "bad_request" and no way to react. Now: - nlpgoHandledErrorFrom() parses the envelope off the AI SDK's APICallError (unwrapping RetryError) into a DomainError keyed by meta.reason, forwarded as domainError on the response - the browser service throws ScenarioGenerationError carrying kind + meta; classifyGenerationError matches typed kinds before string sniffing (missing_provider → config tier + configure CTA) - unknown kinds expand kind/message/meta into the raw-message surface instead of showing an opaque status word
Both the scenario editor panel and the AI create modal now render "Uses <model> · Change" next to their Generate buttons (resolved via the scenarios.generator feature key), linking to model provider settings. Config-tier generation failures get a "Model settings" toast action. AICreateModal grows an optional footerHint slot.
- CurrentDrawer: the lazy-drawer Suspense fallback rendered a 200px block in page flow, shoving the whole page down while the chunk loaded; it is now a fixed overlay where the drawer appears - SimulationCard: reserve clearance for the floating status pill so it no longer covers the last preview lines - SaveAndRunMenu, Stop buttons (RunRow, ScenarioGridCard), and the variable-mapping editor swapped hard-coded light-palette literals (blue.50, gray.100, orange.50, bg white, raw gray-500 vars) for semantic tokens so dark mode is readable; missing-mapping error uses fg.error
0xdeafcafe
force-pushed
the
refactor/remove-elasticsearch-and-background
branch
from
July 7, 2026 11:50
298d40d to
7c10221
Compare
rogeriochaves
approved these changes
Jul 7, 2026
drewdrewthis
added a commit
that referenced
this pull request
Jul 7, 2026
- Update two #4991 test files to import Protections from ~/server/traces/protections (was ~/server/elasticsearch/protections, deleted in #4547 Elasticsearch removal) - Narrow ClickHouseTraceService.getTracesByThreadId return type from Trace[] | null to Trace[] — the method never returns null (always returns [] or throws); fixes TS2719 in trace.service.ts Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SufnT5rv6hGhd2jJ1Z5yeA
langwatch-agent
pushed a commit
that referenced
this pull request
Jul 8, 2026
…urredAt window Rebased onto main after the Elasticsearch/BullMQ refactor (#4547) moved enrichItemsWithTraceCosts into experiment-run.service.ts. enrichItemsWithTraceCosts backfills experiment target-item costs from `SELECT TraceId, TotalCost FROM trace_summaries WHERE TraceId IN (...)` with no OccurredAt bound. trace_summaries is partitioned by toYearWeek(OccurredAt), so a TraceId-only filter cannot prune and opens every weekly partition (including cold storage) to read two light columns. A target's trace runs during its run, so its OccurredAt falls inside the buffered run window getRun already computes (computeOccurredAtRangeForRuns); thread it in and bound the read. The bound is applied on the outer read only, not inside the max(UpdatedAt) dedup subquery: OccurredAt can shift across ReplacingMergeTree versions (a late span can move a trace's min start time), so filtering versions by OccurredAt before resolving the latest could pick the wrong version. EXPLAIN INDEXES on a large tenant: 271/271 -> 12/269 parts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
0xdeafcafe
pushed a commit
that referenced
this pull request
Jul 8, 2026
…t window (#5327) perf(clickhouse): bound experiment trace-cost lookup to the run's OccurredAt window Rebased onto main after the Elasticsearch/BullMQ refactor (#4547) moved enrichItemsWithTraceCosts into experiment-run.service.ts. enrichItemsWithTraceCosts backfills experiment target-item costs from `SELECT TraceId, TotalCost FROM trace_summaries WHERE TraceId IN (...)` with no OccurredAt bound. trace_summaries is partitioned by toYearWeek(OccurredAt), so a TraceId-only filter cannot prune and opens every weekly partition (including cold storage) to read two light columns. A target's trace runs during its run, so its OccurredAt falls inside the buffered run window getRun already computes (computeOccurredAtRangeForRuns); thread it in and bound the read. The bound is applied on the outer read only, not inside the max(UpdatedAt) dedup subquery: OccurredAt can shift across ReplacingMergeTree versions (a late span can move a trace's min start time), so filtering versions by OccurredAt before resolving the latest could pick the wrong version. EXPLAIN INDEXES on a large tenant: 271/271 -> 12/269 parts. Co-authored-by: Ubuntu <ubuntu@ip-10-0-3-183.eu-central-1.compute.internal> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
drewdrewthis
added a commit
that referenced
this pull request
Jul 9, 2026
… statuses (#4805) The trigger filter dropdown for `evaluations.state` offered whatever status values happened to sit in storage. Non-canonical values (e.g. "Error_Message") were offered but never matched at runtime, because the trigger matcher compares against the canonical EvaluationStatus enum — so alerts configured with those options never fired. Derive the option list from `evaluationRunDataSchema.shape.status.options` instead. The link is compile-time, so a future enum change fails CI here rather than silently reintroducing phantom options. Observed ClickHouse counts are folded in, and a status with no rows yet reports 0 so a trigger can be configured before its first occurrence. Rebased onto #4547 (Elasticsearch removal), which deleted the ES `listMatch` aggregation this fix originally patched in `registry.ts`. The fix now lives in the ClickHouse filter layer, which is the current source of filter options; `registry.ts` is unchanged from main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
drewdrewthis
added a commit
that referenced
this pull request
Jul 9, 2026
main removed src/server/elasticsearch/ wholesale in #4547 (remove Elasticsearch and the BullMQ legacy stack), so the noRestrictedImports override exempting transformers.ts now points at a file that does not exist. Drop it and keep the preconditions.ts exemption, which is still load-bearing: TryItOut.tsx value-imports preconditions.ts into the client bundle, so it must stay on the browser-safe logger. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
drewdrewthis
added a commit
that referenced
this pull request
Jul 10, 2026
Scenario editor/execution: drive the CriteriaInput add-flow correctly — reveal the entry textarea via the "Add … criteria" button, commit with Enter, and match committed criteria as text within the criteria-list container. Assertions use .last() to select the visible dialog (Chakra renders duplicates). These 4 tests now pass in CI. Member-invitation tests stay skipped (test.fixme): free-tier self-hosted CI has no license, so the org resolves to FREE_PLAN (maxMembers=1) and the owner alone is already at the cap — createInviteRequest 403s "maximum number of team members" (verified via licenseEnforcement.checkLimit: current=1, max=1). Re-enabling needs a license/subscription provisioned for the CI org (tracked in #1811). Also: drop dead ELASTICSEARCH_NODE_URL (Elasticsearch removed in #4547; nothing reads it) and hoist the describe-level test.slow() to the top of the Scenario Execution suite so its whole-suite scope is explicit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 10, 2026
drewdrewthis
added a commit
that referenced
this pull request
Jul 13, 2026
- Update two #4991 test files to import Protections from ~/server/traces/protections (was ~/server/elasticsearch/protections, deleted in #4547 Elasticsearch removal) - Narrow ClickHouseTraceService.getTracesByThreadId return type from Trace[] | null to Trace[] — the method never returns null (always returns [] or throws); fixes TS2719 in trace.service.ts Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SufnT5rv6hGhd2jJ1Z5yeA
drewdrewthis
added a commit
that referenced
this pull request
Jul 13, 2026
… statuses, not stale ES (#4805) (#5395) fix(triggers): source evaluations.state filter options from canonical statuses (#4805) The trigger filter dropdown for `evaluations.state` offered whatever status values happened to sit in storage. Non-canonical values (e.g. "Error_Message") were offered but never matched at runtime, because the trigger matcher compares against the canonical EvaluationStatus enum — so alerts configured with those options never fired. Derive the option list from `evaluationRunDataSchema.shape.status.options` instead. The link is compile-time, so a future enum change fails CI here rather than silently reintroducing phantom options. Observed ClickHouse counts are folded in, and a status with no rows yet reports 0 so a trigger can be configured before its first occurrence. Rebased onto #4547 (Elasticsearch removal), which deleted the ES `listMatch` aggregation this fix originally patched in `registry.ts`. The fix now lives in the ClickHouse filter layer, which is the current source of filter options; `registry.ts` is unchanged from main. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
drewdrewthis
added a commit
that referenced
this pull request
Jul 13, 2026
…aths (2 of 2 — #4888) (#5082) * fix(traces): batched/streamed bulk blob-resolution mechanism (#4991 AC6/AC7) The >64KB IO offload (#4215/ADR-022) stores a 64KB preview + an eventref pointer; the full value lives in event_log. #4984 wired single-trace detail reads. Bulk reads (export, thread, annotation, sample builders) read whole result sets, where resolving each trace independently fans out an unbounded N×M burst of event_log SELECTs. Add resolveOffloadedTracesBatch: resolve a WHOLE result set in one pass — decode eventrefs across every span/trace, dedupe identical (aggregateId, eventId, field) refs, and stream the event_log reads through a bounded concurrency pool (EVENT_LOG_RESOLVE_CONCURRENCY) so peak in-flight CH reads is a constant regardless of result-set size (AC6). A failed/missing row keeps the field's preview and logs a warn — no silent truncation (AC7). Extract the eventref decoder (hasEventRefs/parseSpanEventRefs) into a shared offloaded-eventref-parsing module used by both the per-trace and batch resolvers (one definition). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(traces): wire batch resolver into bulk CH reads; gate list/aggregations preview-only (#4991 AC1/AC2/AC5) ClickHouseTraceService now accepts a resolveTraceSpansBatch callback (built by TraceService from the same buildTraceBlobResolutionDeps as the per-trace one). getTracesWithSpans and the download-path enrichTracesWithSpans resolve via the batch callback in one bounded pass; getTracesByThreadId forwards resolveBlobs so thread-detail reads resolve full. When only the per-trace callback is wired (merged #4984 unit tests), the bulk paths fall back to it — those tests stay green. Preview-only protection (AC5): resolution is gated by an EXPLICIT resolveBlobs option on getAllTracesForProject, deliberately separate from includeSpans — app.v1's public list passes includeSpans=true but must stay preview. The list grid / aggregations never set resolveBlobs, so they issue ZERO event_log reads (ADR-022 heavy-read protection preserved). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(traces): resolve full IO on export / thread / annotation / sample read paths (#4991 AC1-AC4) Wire blob-resolution deps + opt into full resolution at every content-consuming bulk read site (mirrors #4984's detail-read wiring): - AC1 export: ExportService.create + getAllForDownload build deps; the full export opts resolveBlobs into the getAllTracesForProject options. - AC2 thread: legacy GET /api/thread/:id + tRPC getTracesByThreadId / getTracesWithSpansByThreadIds build deps + full:true. - AC3 annotation: both queue-read sites (getQueueItems + the shared enrich helper behind getOptimizedAnnotationQueues) build deps + full:true. - AC4 dataset/sample: getSampleTraces + getSampleTracesDataset build deps + full:true on the span fetch (the ID-only list read stays preview). TDD: per-site call-site tests (createCaller / Hono handler / ExportService) assert each constructs TraceService WITH deps and requests full resolution; the list grid + getFormattedSpansDigest assert preview-only. RED with the wiring reverted, GREEN with it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style(traces): satisfy biome lint on #4991 added lines CI biome (reviewdog, filter_mode=added) flagged added-line issues local typecheck doesn't catch: noEmptyBlockStatements (() => {} in test mocks → () => undefined), noForIn (hasEventRefs → Object.keys().some()), and formatter diffs on added lines (collapse single-fit import; split an over-80 object literal). Scoped to the lines this PR added — pre-existing non-conformant lines in touched files are left alone (reviewdog only checks added lines), keeping the diff minimal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style(traces): organize imports in export/annotation for #4991 added-line lint biome assist/source/organizeImports flagged the import blocks where this PR added a buildTraceBlobResolutionDeps import (reviewdog filter_mode=added: the added import line falls in the organizeImports range). Re-sorted only those two import blocks (assist-only, formatter left off) so pre-existing non-import lines stay untouched. traces-legacy.ts is intentionally not re-sorted: its organizeImports range (line 12) is the pre-existing #4984 import, not an added line. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style(traces): organize traces-legacy imports for #4991 added-line lint reviewdog flagged biome assist/source/organizeImports on traces-legacy.ts (it treats the whole-file import assist as failing on a changed file, not just changed lines). Sorted the import block and merged the two duplicate trace-formatting imports — biome-exact import region only; the handlers are left untouched (no whole-file reformat). Full rdjson sweep over every changed file now returns zero diagnostics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style(traces): apply biome formatter+assist to #4991 touched files for CI lint CI runs biome with organizeImports/assist+formatter (reviewdog filter_mode=added); my plain local 'biome check' missed it and my earlier formatter-disabled organize left format violations on added lines (e.g. an over-80 single-line import biome wants multi-line). Ran full 'biome check --write' on the touched files so they are whole-file biome-conformant — guarantees zero diagnostics on added lines. Behavior unchanged (import ordering + whitespace only); typecheck + trace suites green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test+docs(traces): address #5082 review — falsifiable preview asserts + stale doc fixes From the /review pass on #5082: - clickhouse-trace.service-4991-bulk: AC5 + AC2 preview assertions now assert the EXACT preview value (toBe PREVIEW_OUTPUT) instead of not.toBe(FULL_OUTPUT) — a broken mapper returning empty/null would have slipped past the negative check. - trace.service.ts: getTracesWithSpansByThreadIds JSDoc no longer claims customer thread views stay on the preview (the thread tRPC now opts into full:true). - resolve-offloaded-traces.ts: drop the stale @param eventId (each span reads its own embedded eventref; no caller-supplied eventId). Reviewer's WarnLogger 'error'-is-dead finding overruled: removing it breaks typecheck (test fixtures pass {warn,error} object literals — excess-prop check). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(types): reconcile blob-resolution types after rebase - Update two #4991 test files to import Protections from ~/server/traces/protections (was ~/server/elasticsearch/protections, deleted in #4547 Elasticsearch removal) - Narrow ClickHouseTraceService.getTracesByThreadId return type from Trace[] | null to Trace[] — the method never returns null (always returns [] or throws); fixes TS2719 in trace.service.ts Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SufnT5rv6hGhd2jJ1Z5yeA * fix(traces): authorize public-share thread reads before resolving blobs (#5082) getTracesByThreadId is a publicProcedure. It resolved { full: true } for every trace in the thread and only then filtered the response down to the publicly shared trace(s). So one valid share link into a busy thread let an anonymous caller force full event_log de-offloading of every other trace in that thread — traces they are not authorized to read the content of. Not a data leak (the response was still filtered), but exactly the connection-pool amplification the ADR-022 offload exists to prevent, now reachable unauthenticated. The public branch now reads PREVIEW-only (zero event_log resolution), authorizes down to the shared trace ids, then resolves full IO for JUST those ids. The authorized (non-public) branch is unchanged: the whole thread is theirs to read, so it still resolves full up front. Also fixes a genuinely failing test this PR shipped: getAllForDownload is a tRPC mutation, so the auditLogMutations middleware reaches for the prisma SINGLETON in ~/server/db (not ctx.prisma) and tried to open a Postgres connection the unit shard has no server for. It failed the assertion AND left a pending socket that wedged the shard — which the test-unit globalSetup hard-floor then masked with process.exit(0), so the shard reported success. Stubbing the singleton fixes both. Threads: PRRT_kwDOKRXhvM6PMGtM * fix(export): resolve blobs for summary exports too, not just full (#5082) resolveBlobs was gated on `includeSpans`, so it was false for every summary-mode export. But summary mode is a content-consuming read: buildSummaryRow emits trace-level `trace.input.value` / `trace.output.value` (serializers/csv-serializer .ts:91-92), and the summary JSON serializer does the same. For an offloaded (>64 KB) trace, a summary export therefore shipped the truncated 64 KB preview silently — no error, no indication data was cut. That is the same data-loss bug this PR opens with ("a full CSV/JSONL export lost data"), just on the other mode. Resolve blobs for every export mode. Exports are already a bounded/streamed bulk read via this PR's batch resolver, so the extra event_log reads stay pool-safe. The old test asserted the buggy contract (resolveBlobs === false on summary); it is replaced by one asserting the fixed contract, plus a payload test grounding WHY it must hold — proving the summary CSV really does carry the trace-level IO value that truncation would have eaten. Threads: PRRT_kwDOKRXhvM6PMHCA * fix(traces): named params for fetchKeyOf + enforce batch-resolver cardinality (#5082) fetchKeyOf(aggregateId, eventId, field) took three positional strings, so a caller that transposed two of them compiled cleanly and silently deduped/fetched the wrong event_log row onto the wrong span. Now takes named params, per the repo's TypeScript standard (CLAUDE.md). ResolveTraceSpansBatchFn is INJECTED, so "exactly one resolution per input trace, in input order" is a convention its type cannot enforce — yet `resolutions[i]!` relied on it on the hot bulk-read path shared by export, thread, and the dataset/sample builders. A resolver (or test double) that dropped or reordered entries would crash with zero context, or silently pair the wrong resolved spans with the wrong trace summary. Guard it at the boundary where the untrusted value enters (resolveSpansBatch), not at the two downstream index sites: that catches the violation while the offending resolver is still nameable, and covers both `resolutions[i]!` and `merged[i]!` with one assertion. (`merged` comes from `entries.map(...)`, so its length already equals `enrichable.length` by construction — a second assert there would be unreachable.) The error is a named class, following the ClickHouseClientUnavailableError precedent, and both read paths' generic try/catch re-throw it unwrapped — a resolver-contract violation is a code bug, not a fetch failure, and flattening it into "Failed to fetch traces from ClickHouse" would lose the very context the guard exists to provide. Covered by a test that drives a genuinely non-conforming resolver through the real service and observes the throw. Also folds in biome's import ordering for export.service.ts (formatter ran over the files touched here). Threads: PRRT_kwDOKRXhvM6PMHE7, PRRT_kwDOKRXhvM6PMHVR * test(traces): mock the hint-less OccurredAt resolve in the thread query sequence Rebasing onto main picked up #5231, which resolves the OccurredAt partition window itself when a batch trace read gets no `occurredAt` hint. The thread-view paths know only trace ids, so they take exactly that hint-less branch — and the resolve is a real ClickHouse query that consumes a mock. setupThreadMocks() didn't have one, so every later mock shifted by one and the spans read silently received the summary's rows, failing the two AC2 thread tests (and masking the new batch-resolver cardinality assertion behind the generic catch). Semantic conflict, not a textual one: the rebase merged clean and only surfaced here when the suite ran. * fix(traces): set cost/nonBilledCost on the NormalizedSpan test fixture Rebasing onto main picked up #5012 (ADR-034 trace_analytics), which added `cost` and `nonBilledCost` to NormalizedSpan as REQUIRED nullable fields. The batch resolver's makeSpan fixture predates them, so tsgo failed the file with TS2322. Blob resolution is cost-agnostic, so null is the honest fixture value. Caught by `pnpm typecheck:tests`, which is a SEPARATE CI step from `pnpm typecheck` — tsconfig.tsgo.json excludes **/*.test.ts, so the plain typecheck never looks at test files. Another semantic conflict the clean rebase hid. * fix(traces): address review — honour the resolver-order claim, close the third catch, unbreak the spans-less download Review of the previous commits surfaced four real defects, three of them in code those commits added. **The guard overclaimed.** Its message and comments promised "one resolution per input trace, IN INPUT ORDER" and said a resolver that "drops or reorders" would fail loudly — but it only compared lengths. A resolver returning the right COUNT in the wrong ORDER sailed through and scattered each trace's IO onto its neighbour: precisely the silent-corruption case the guard was added to catch. Advertising a check that doesn't exist is worse than no check, so the order is now actually verified (resolved spans carry the trace identity the resolution itself lacks). Renamed to TraceSpansBatchResolverContractError since it now polices two distinct violations. **A doc comment described a mechanism that does not exist.** It claimed this error escapes "like ClickHouseClientUnavailableError" — but that one escapes by being thrown OUTSIDE the try, not by an instanceof re-throw. Two opposite mechanisms; a reader would hunt for an allowlist that wasn't there. Now describes what the code actually does. **The unwrapped re-throw missed a third catch.** getTracesWithSpansByThreadIds sits ABOVE getTracesWithSpans and flattens what that method re-throws, so the descriptive error was destroyed on a live resolveBlobs path (thread router + evaluation-execution service). The class doc's "the read paths re-throw this unwrapped" was simply false there. Allowlisted. **getAllForDownload had the summary-export bug, untouched.** It still gated `resolveBlobs` on `includeSpans` — the exact pattern just fixed in ExportService. A download returns trace-level input/output whether or not spans are included, so a spans-less download silently truncated any offloaded trace. Same data loss, twin path, introduced by this PR. Fixed and covered. Also: the public-share branch is extracted into readPubliclySharedThread (the handler had grown to ~78 lines with the security-critical sequencing buried in prose), and it now re-projects onto authorizedTraceIds instead of re-deriving a chronological sort — those ids already carry the thread's order, so the thread read stays the single owner of what "thread order" means rather than the comparator living in a third place. The test fixture that returned an unsorted thread was contradicting the service's actual contract; corrected. The audit-log stub now mocks the auditLog function directly, matching translate/apiKey.myBindings/workflows.generateCommitMessage, instead of reaching through to the prisma singleton's internals. New guards verified falsifiable: removing the order check fails the misalignment test; restoring `resolveBlobs: input.includeSpans` fails the spans-less download test. * test(traces): make the public-share exclusion property actually load-bearing Test review found the security property under-guarded in the one scenario that matters. The multi-shared case (t1 and t3 shared, t2 an unauthorized sibling BETWEEN them) asserted only on the returned traces — and the mock's return value doesn't depend on its args, so a router that regressed to resolving ["t1","t2","t3"] would have returned the right traces and passed. Only an assertion on the CALL ARGS can catch the unauthorized sibling being de-offloaded, and there wasn't one. Added it, plus toHaveBeenCalledTimes(1) — toHaveBeenCalledWith alone proves a matching call exists, not that it is the only one, so a leaked second call resolving the siblings would have gone unseen. Verified: simulating a partial-authorization regression (resolve every thread trace id, ignoring the publicShare filter) now fails BOTH exclusion tests. Before this commit it failed only the single-shared one. Also: "returns the fully-resolved authorized traces" asserted only trace_id, so it passed identically pre- and post-fix — it claimed to check full resolution while being unable to see it. The resolved mock now carries a value distinguishable from the preview, and the test asserts on that. Nesting corrected to given -> when per CLAUDE.md (the three new blocks nested it() directly under "given"). * fix(traces): close the span-less blind spot in the resolver-order guard The identity check I added compares spans[0].traceId on each side — but a trace can legitimately have ZERO spans (fetchTracesWithSpansJoined builds its map from summary rows and defaults spans to []). A span-less trace has no traceId on EITHER side, so swapping it with a spans-ful trace was invisible to the identity check at both indices, and the real trace silently lost its spans: exactly the corruption the guard exists to stop, in the one case it couldn't see. Both resolvers derive resolvedSpans by mapping over the input spans, so span count is a genuine contract invariant. Checking it first catches the swap (the spans-ful index expects N, gets 0). Two span-less traces transposed remain invisible and are harmless — their resolutions are empty and interchangeable. Found by writing the falsifiability check properly. My first attempt at the test passed with the span-count check removed: it constructed a resolution whose resolvedSpans were empty but whose SIBLING still carried an identity, so the identity check caught it and the new check was never exercised. The real hole needs a trace with no span ROWS at all. Rebuilt the test that way; it now fails when the span-count check is removed. * fix(traces): actually resolve blobs on summary reads — the flag was inert My earlier "fix" for the summary-export data loss did not fix it. Setting `resolveBlobs: true` in ExportService and getAllForDownload changed nothing at runtime, because resolution lives inside enrichTracesWithSpans, which only ran: if (options.includeSpans && traces.length > 0) { ...resolveBlobs... } A summary-mode export sets includeSpans=false (`request.mode === "full"`), so `options.resolveBlobs` was never even READ. Zero event_log reads were issued and the truncated 64 KB preview shipped exactly as before — while a code comment asserted the opposite. Same for a spans-less download. Spans are fetched when the caller wants them OR wants full IO, because those are not the same thing: the full value is recoverable ONLY by de-offloading the spans' eventref pointers and recomputing trace IO from them (trace_summaries holds nothing but the preview). So a content-consuming summary read must fetch and resolve spans, then discard them — it keeps the recomputed trace-level IO and returns `spans: []`, so its payload shape is unchanged. AC5 is preserved by construction: resolveBlobs stays opt-in, so the list grid and the aggregations resolve nothing and issue zero event_log reads whether or not they ask for spans. The existing zero-read tests still pass. Why the old tests couldn't catch this: the router/export tests mock getAllTracesForProject and assert only that the FLAG IS FORWARDED. Such a test passes whether or not the flag has any runtime effect — it was green throughout. The new test drives the REAL service + REAL resolver with { includeSpans: false, resolveBlobs: true } and asserts the VALUE comes back full. Confirmed RED before this fix (getFromEventLog never called). Cost, stated: summary exports now do a span join + bounded event_log reads where they previously did neither. That is the tradeoff the reviewer explicitly accepted on the blocker thread, and there is no cheaper way to recover the value. * docs+test(traces): make getTracesByThreadId's chronological order a stated contract The public-share branch re-projects its authorized subset onto the order this method returns, rather than re-deriving a sort of its own. That is the right call — one owner for "thread order" — but it silently made an unstated implementation detail load-bearing on the anonymous, least-exercised path. The router's own tests mock this service, so deleting the sort here would have mis-ordered public-share threads with zero test failures anywhere. Stated in the JSDoc and pinned by a service-level test that feeds the bulk read's rows back out of order and asserts chronological output. Verified falsifiable: removing the sort fails it. * test(export): prove summary-mode blob resolution end-to-end against real ClickHouse The unit tests could not have caught the bug they were written for. They mocked getAllTracesForProject and asserted `resolveBlobs` was FORWARDED — an assertion that passes whether or not the flag does anything. It did nothing (resolution was gated behind includeSpans), and they stayed green while summary exports shipped the truncated preview. This test asserts the VALUE, through the real stack: real ClickHouse (trace_summaries + stored_spans + event_log) -> real ClickHouseTraceService (the includeSpans/resolveBlobs gate) -> real resolveOffloadedTracesBatch + real BlobStore (the event_log read) -> real ExportService in SUMMARY mode -> real CSV / JSONL serializers -> assert the exported bytes carry the de-offloaded value It seeds a genuinely offloaded trace the way an ingest leaves one: the full 200 KB value in event_log, the 64 KB preview + reserved eventref in stored_spans, the preview in trace_summaries. UNIQUE_TAIL sits past the preview boundary by construction, so it exists ONLY in the de-offloaded value — a preview-only export cannot contain it. AC5 is guarded in the same file: the list/search read over the SAME offloaded trace, without resolveBlobs, must still come back as the preview with the tail absent — so the fix demonstrably did not disarm the heavy-read protection. Falsifiable, and verified so: restoring the pre-fix `if (options.includeSpans)` gate fails all three export assertions, and the CSV assertion prints the truncated preview back at you. The AC5 assertion keeps passing, as it must. --------- Co-authored-by: MrKrustyKlaws[bot] <mrkrustyklaws[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
drewdrewthis
added a commit
that referenced
this pull request
Jul 16, 2026
…cks (#3082) * fix(ci): add NLP/langevals services and re-enable 7 e2e tests - Add langwatch_nlp and langevals service containers to e2e-ci.yml with Python urllib health checks (slim images lack curl) - Health check endpoints: /health for NLP, /healthcheck for langevals - Add ELASTICSEARCH_NODE_URL env var pointing to OpenSearch on :9201 - LANGWATCH_ENDPOINT uses host.docker.internal:5570 so NLP container can reach the test app running on the CI runner host - Replace test.fixme() with test.slow() in 7 previously-skipped tests: * evaluations-v3/http-agent-cell-rerun.spec.ts * evaluations-v3/http-agent-full-evaluation.spec.ts * members/admin-approves-invitation.spec.ts * members/admin-creates-immediate-invite.spec.ts * members/admin-rejects-invitation.spec.ts * scenarios/scenario-editor.spec.ts * scenarios/scenario-execution.spec.ts Closes #1802 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(e2e): fix test locators broken by app changes since original skip - scenarios/steps.ts: dismiss AI-assist modal ("I'll write it myself") before asserting the scenario form drawer; ScenarioCreateModal now intercepts the "New Scenario" click with an AI prompt first. - members/steps.ts: fix email-input placeholder from "Enter email address" to the actual placeholder "alice@example.com, bob@example.com". - evaluations-v3 http-agent tests: re-add test.fixme() with justification; these tests are blocked on missing testids (spreadsheet-cell, cell-play-button, save-agent-button) not present in current app source — unblocking them requires app-side changes beyond this CI-infra PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SufnT5rv6hGhd2jJ1Z5yeA * fix(ci): correct langwatch_nlp health check endpoint /health → /healthz The Go router registers /healthz (services/nlpgo/adapters/httpapi/router.go:61) but the health-cmd was probing /health, causing all 12 retries to fail. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SufnT5rv6hGhd2jJ1Z5yeA * fix(ci): poll langwatch_nlp health from runner instead of docker health-cmd The distroless base image (gcr.io/distroless/python3-debian12:nonroot) has no shell (/bin/sh), so --health-cmd always fails since Docker wraps the CMD-SHELL form with /bin/sh -c. The service starts fine on :5561 (confirmed by nlpgo_starting log) but every Docker health check exits immediately. Remove the Docker health check from the langwatch_nlp service options and replace it with an explicit poll step on the GitHub Actions runner, which has curl and can reach the container via the host-mapped port 5563. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SufnT5rv6hGhd2jJ1Z5yeA * fix(ci): remove langwatch_nlp service container — app auto-starts its own The langwatch app auto-starts a bundled nlpgo binary on :5563 at startup (logged as '✓ nlpgo: auto-start on :5563'). Adding a Docker service container on the same port caused a port conflict: the app's nlpgo would fail with 'listen tcp :5563: bind: address already in use' and enter a crash-loop, destabilising the Next.js server and causing all subsequent test navigation to fail. The LANGWATCH_NLP_SERVICE=http://localhost:5563 env var remains; the app's own nlpgo binary satisfies it without a container. Also removes the now-unnecessary 'Wait for NLP service' step that was polling the container that no longer exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SufnT5rv6hGhd2jJ1Z5yeA * fix(e2e): fix members URL, getOrgAndTeamIds relative-fetch, and scenario modal race Three root-cause fixes for the 8 remaining test failures: 1. givenIAmOnTheMembersPage navigated to /${projectSlug}/settings/members but the actual Next.js route is /settings/members (src/pages/settings/members.tsx has no [project] segment). Fixed by removing the slug prefix. 2. getOrgAndTeamIds used page.evaluate(() => fetch('/api/trpc/...')) which fails on about:blank (no base URL). Replaced with page.request.get using the same batch-tRPC pattern as getProjectSlug in helpers.ts. 3. whenIClickNewScenario only handled AICreateModal ("I'll write it myself") but in CI with no model providers configured, ScenarioCreateModal renders ModelProviderRequiredModal instead, with a "Proceed anyway" button. Fixed by racing both locators so whichever modal appears is dismissed. * fix(e2e): fix customRoleId null, ScenarioWelcomeModal, and toast timeout Three fixes for remaining CI failures: 1. seedWaitingApprovalInvite sent customRoleId: null which fails Zod validation (z.string().optional() accepts undefined, not null). Remove the field entirely so it defaults to undefined. 2. whenIClickNewScenario only handled the two post-create-button modals but not ScenarioWelcomeModal, which appears first in CI where the test user has zero scenarios. Add a sequential step to click "Create Your First Scenario" before the existing modal race. 3. thenISeeSuccessToast used a hardcoded 5 s timeout. The createInvites mutation round-trip can exceed 5 s under CI load, causing the toast to appear after the assertion gives up. Raise to 15 s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SufnT5rv6hGhd2jJ1Z5yeA * fix(e2e): purge accumulated invites before seed; fix Name field locator Members: CI DB persists invites across runs (SKIP_PRISMA_MIGRATE=true), so accumulated PENDING/WAITING_APPROVAL invites exhaust the free plan maxMembers=2 cap → 403 FORBIDDEN. purgeExistingInvites() now drains them via organization.getOrganizationPendingInvites + deleteInvite before seedWaitingApprovalInvite creates a fresh one. Scenarios: ScenarioForm's "Name" label is rendered by SectionHeader (<Text>/<p>), not a <label>, so the input has no accessible ARIA name. getByRole("textbox", { name: "Name" }) never matches. All four affected step functions now use getByPlaceholder(/angry refund request/i) which targets the actual placeholder attribute on the Input element. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SufnT5rv6hGhd2jJ1Z5yeA * fix(e2e): re-enable scenario tests; keep member-invitation tests skipped Scenario editor/execution: drive the CriteriaInput add-flow correctly — reveal the entry textarea via the "Add … criteria" button, commit with Enter, and match committed criteria as text within the criteria-list container. Assertions use .last() to select the visible dialog (Chakra renders duplicates). These 4 tests now pass in CI. Member-invitation tests stay skipped (test.fixme): free-tier self-hosted CI has no license, so the org resolves to FREE_PLAN (maxMembers=1) and the owner alone is already at the cap — createInviteRequest 403s "maximum number of team members" (verified via licenseEnforcement.checkLimit: current=1, max=1). Re-enabling needs a license/subscription provisioned for the CI org (tracked in #1811). Also: drop dead ELASTICSEARCH_NODE_URL (Elasticsearch removed in #4547; nothing reads it) and hoist the describe-level test.slow() to the top of the Scenario Execution suite so its whole-suite scope is explicit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(e2e): re-enable member-invitation tests via test enterprise license The 3 members-invitation e2e tests were skipped because a no-license self-hosted deployment resolves to FREE_PLAN (maxMembers=1): the org owner alone is already at the cap, so createInviteRequest 403s "maximum number of team members". Activate a pre-signed test ENTERPRISE license (maxMembers=100) for the org in auth.setup.ts, and trust the in-repo TEST public key via LANGWATCH_LICENSE_PUBLIC_KEY in e2e-ci. getActivePlan re-reads the license from Postgres on every call, so activating in setup takes effect with no app restart. No new secrets: only a public key plus an already-signed, test-only license (both from ee/licensing/__tests__/fixtures). Addresses #1811 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(e2e): scope member license to the members suite; fix latent invite-test bugs Running the re-enabled members tests surfaced issues the maxMembers=1 403 had masked (the license mechanism itself works — the 403 is gone): - Global license activation made the shared test org ENTERPRISE, regressing settings/plans-comparison (which asserts the Free plan is current). Scope it instead: withEnterpriseLicense() activates the test ENTERPRISE license before each members test and removes it after (license.remove), so every other suite still sees FREE_PLAN. Reverts the auth.setup.ts activation. - getOrgAndTeamIds used page.evaluate(fetch("/api/trpc/...")) — a relative URL that throws "Failed to parse URL" on an unnavigated page. Use page.request.get (batch shape), matching getProjectSlug. - givenIAmOnTheMembersPage navigated to /${slug}/settings/members; the members route is org-scoped at /settings/members (langwatch/src/routes.tsx). Fixed. Addresses #1802 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(e2e): correct member invite-flow locators (email placeholder, customRoleId) Second probe got past the earlier fixes and surfaced the last two invite-flow bugs (every locator now verified against the app source): - whenIFillEmailWith targeted placeholder "Enter email address" (that belongs to UserManagementDrawer). The Add-members dialog input placeholder is the example list "alice@example.com, bob@example.com" (AddMembersForm.tsx). Match a stable substring. - seedWaitingApprovalInvite sent teams[].customRoleId: null, but the createInviteRequest schema types it z.string().optional() — null fails with "Expected string, received null". Omit it. All other invite-flow locators confirmed against source: "Create invites" button, "Invite created successfully" toast, "Invite Link"/"Invites" headings, Invited/Pending Approval badges, Approve/Reject aria-labels, approved/rejected toasts. Addresses #1802 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(e2e): close invite-link dialog by role+name so the Invited row is visible The immediate-invite test created the invite correctly — the failure page snapshot shows the "Invited" row present — but the "Invite Link" modal stayed open, leaving the Invites table inert so thenISeeSentInviteFor saw no visible row. whenICloseInviteLinkDialog detected the dialog via getByRole("heading", "Invite Link"), which matches the two nested title headings (Dialog.Title > Heading) — a strict-mode multiple-match that throws and was caught, silently skipping the close. Match the dialog by role+name (a single element) and close it. With this, all three member-invitation tests pass (approve + reject already passed first-try in the prior run). Addresses #1802 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(e2e): fix stale review-flagged comments (activation site, feature file) Addresses code-review findings — no runtime change: - license.fixture.ts + e2e-ci.yml comments said auth.setup.ts activates the license, but activation moved to members/steps.ts withEnterpriseLicense(). Repointed both. - specs/members/update-pending-invitation.feature claimed the e2e specs "remain test.fixme() (#1811)"; the 3 admin @e2e scenarios now run and pass. Corrected the prose. @unimplemented stays because check-feature-parity.ts does not scan agentic-e2e-tests/ and these specs carry no @Scenario binding — parity-binding is a tracked follow-up. Addresses #1802 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(e2e): make license removal fail loud; document scoping preconditions Addresses test-reviewer findings on the license scoping: - removeEnterpriseLicense now checks response.ok()/tRPC error and throws, like its activateEnterpriseLicense sibling. Under single-worker execution a SILENT removal failure would strand the shared org on ENTERPRISE for the rest of the run and surface as a far-removed plans-comparison flake — exactly the class of failure this PR eliminates. Fail at the point of cause instead. - Docstring no longer overclaims full cleanup: license activation also create-if-absent provisions org retention-policy rows that removeLicense does not revert (harmless today; documented). - withEnterpriseLicense documents its hard dependency on fullyParallel:false / workers:1 so a future parallelism bump fails loud. Addresses #1802 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Elasticsearch was dead in the live read/write path — the
TraceService / EvaluationService / ExperimentRunService /
FilterService facades each had two backends but every call already
went through ClickHouse, with the ES backend instantiated and never
invoked. The BullMQ legacy stack under
src/server/background/hadbeen replaced piecewise by the event-sourcing collector (
recordSpan)but the old workers, queues, and the tasks that wrote to ES were
still wired at boot. Keeping both alive cost us the
@elastic/elasticsearchdependency, twobullmqqueues running onevery worker process, dead cron handlers triggering ES queries against
clusters most environments no longer provision, and a layer of facade
indirection in front of services that have one backend.
Deployment Impact
Env vars — none added or changed. Four variables stop being read anywhere in the codebase:
ELASTICSEARCH_NODE_URL,ELASTICSEARCH_API_KEY,IS_OPENSEARCH, andIS_QUICKWIT. Leaving them set is harmless (they are simply ignored); they can be deleted from secrets/config at leisure. No new secrets or config are required to upgrade.Helm values — no chart changes ship in this PR. Any elasticsearch/opensearch values in a deployment (subchart enablement, node URL, API key) become inert with this image and can be removed in a follow-up chart cleanup. The worker deployment is unchanged: same
start:workersentrypoint and the same Redis requirement — the process now runs the event-sourcing/GroupQueue stack plus the remaining Redis-backed queues (topic clustering, usage stats) instead of the legacy BullMQsrc/server/background/worker.Vanilla
helm installwith no overrides: app and workers boot and serve with no Elasticsearch/OpenSearch/Quickwit reachable — the ES client is gone from the build entirely (@elastic/elasticsearchremoved from dependencies, and pnpm overrides pin@elastic/elasticsearch/@opensearch-project/opensearchto-so they cannot resolve transitively). Required stores after this PR: Postgres, ClickHouse, Redis.BYOC dataplane: BYOC installs can decommission their ES/OpenSearch cluster once settled on this version — all reads and writes are ClickHouse. No migration or backfill runs at deploy: ES data is abandoned in place, so anything never migrated to ClickHouse stops being readable through the product. Keep ES up only as a rollback hedge; rolling back to a pre-PR image requires ES to be reachable again. One operational shift to know: REST
/api/collectordedup moves from ES content-hashes to RedisSET NX(1h TTL), so Redis absorbs that load (details in the verify notes below).What changed
Two larger refactors (below), plus a series of iteration fixes from review/CI feedback (TC restore, lockfile fix, crypto import, test fallout, build-chunk isolation, scenario tagging, TC circular-import fix, prompts polling fix, dead-import cleanup in
runEvaluation, ES-remnant purge) and three merges ofmainto keep the branch current.1. Remove Elasticsearch and the BullMQ legacy stack (196 files)
(
Elasticsearch{Trace,Evaluation,ExperimentRun,Filter,Analytics}Service).evaluationEsSync,experimentRunEsSync,annotationEsSync) — no more dual-write.builders, the ES experiments batch-evaluation repository, the ES
scenario-event service/repository.
input schema, the settings UI, and
organizations.update./cron/scenario_analyticsand/cron/traces_retention_period_cleanup;/cron/trace_analyticsiskept but stripped to only the SaaS usage-limit check (its ES
analytics half is gone), and
/cron/schedule_topic_clusteringstays (topic clustering was restored after review).
Also drop the ES task scripts the deleted handlers invoked (
elasticMigrate,cold/*,deleteTracesRetentionPolicy,syncAnnotations,syncTraceCosts,backfillTraceAnalytics,backfillScenarioAnalytics,fixCustomMetadataOutsideCustomField,clearTopics,rerunChecks).env.IS_QUICKWITbranches and replace ES query-buildertype imports with neutral structural types in the filter / analytics
/ common-router files.
Protectionsout ofsrc/server/elasticsearch/tosrc/server/traces/.src/server/background/entirely; move its pure helpers(
cost,common,evaluations,evaluationNameAutoslug,metrics,piiCheck,rag) intosrc/server/tracer/collector/for the app-wide consumers.
runEvaluation/runEvaluationForTrace/customEvaluationfrom the deleted evaluations worker into
src/server/evaluations/runEvaluation.ts, switching ESgetTraceById/getTracesGroupedByThreadIdforTraceService.getById/getTracesByThreadId.src/workers.ts: drop the 15-minute self-restart timer(helm/docker already restart-on-exit), drop the BullMQ Worker
startups, initialize the worker-role app, and start the EE
governance ingestion-puller worker via a new
ee/governance/services/pullers/pullerQueue.ts.httpProxyTracingfromscheduleTraceCollectionWithFallbacktogetApp().traces.recordSpanviaCollectorSpanUtils— the onlyremaining caller of the old collector queue.
fetchExistingMD5s+256-version cap) from
routes/collector.ts; the REST path now sharesthe
ingestNormalizedSpandedup gate with the OTLP path./api/start_workers,/api/rerun_checks,/api/cron/scenario_analytics,/api/cron/traces_retention_period_cleanup— the routes wereexclusively legacy-stack triggers.
review, and restored —
src/server/topicClustering/, the/settings/topic-clusteringpage + command-bar entry + route, andthe worker now running alongside the EE puller in
src/workers.ts.TraceUsageServiceCH-only (drop theEsClientFactoryconstructor parameter and the
splitProjectsByFlagfallback).src/server/elasticsearch.ts+src/server/elasticsearch/elastic/migrations entirely.@elastic/elasticsearchfrompackage.json+ lockfile.EvaluationService(deleteclickhouse-evaluation.service.ts;facade methods now throw on missing CH client instead of
null-falling-back; keep
getEvaluationInputsnullable for thelegitimate empty case).
ExperimentRunService(deleteclickhouse-experiment-run.service.ts; foldlistRunsForExperimentSlugPaginatedinto the renamed class).FilterService(renamed fromFilterServiceFacade, deleteclickhouse-filter.service.ts).SimulationFacade— deleted entirely. Every consumer (suiterouter, scenario events router, simulation-runs REST,
cancellation router, onboarding-checks service) now calls
getApp().simulations.runsdirectly.2. Isolate the BullMQ adapter to the EE governance puller (7 files)
After (1), the only BullMQ-importing file left in
src/server/wasthe context adapter at
adapters/bullmq.ts, and the only caller ofits
withJobContexthelper was the EE governance puller. The otherthree exports (
createContextFromJobData,getJobContextMetadata,JobDataWithContext) have no BullMQ dependency — they're pure ALSplumbing.
src/server/context/core.ts.withJobContext(the one piece that imports BullMQ'sJobtype) into
ee/governance/services/pullers/withJobContext.ts,co-located with its only consumer.
__contextshape + legacy__payloadmigration paths) into
ee/governance/services/pullers/__tests__/withJobContext.unit.test.ts.src/server/context/adapters/bullmq.ts; updateasyncContext.tsto re-export the pure helpers fromcoreonly.Result: BullMQ is confined to a small set of queue modules. (Note: the
later topic-clustering restore and the
makeQueueNamebuild-isolationcommit moved things around, so the BullMQ-importing files ultimately
settled under
src/server/queues/—queueWithFallback,withJobContext,makeQueueName— plussrc/server/topicClustering/(queue + worker) andee/governance/services/pullers/(the EE puller), rather than the singleee/.../pullers/folder this section originally described.) Droppingbullmq/bullmq-oteltherefore means migrating those three areas offit — more than a one-folder change, but still a contained set of callers.
How it works
(
EvaluationService,ExperimentRunService,FilterService)changed their not-found-CH-client semantics from "return
null,let the caller fall back to ES" to "throw — ClickHouse is the only
backend now." The exception is
EvaluationService.getEvaluationInputs,which keeps its
nullreturn because callers treat "no inputsrecorded" as a legitimate empty case, not an error.
TraceServicewas kept as a facade even though its ES backendis also gone, because the facade still owns prefix-resolution +
AmbiguousTraceIdPrefixError. Collapsing it would push that domainlogic into a router.
SimulationFacadewas deleted entirely because it had become apure pass-through after dropping ES routing. The only consumer-side
rename was
getScenarioSetsDataForProject→getScenarioSetsData(the underlying service method).makeQueueNamelives in its own module (src/server/queues/makeQueueName.ts). It's a Redis-Cluster hash-tag util that scenario.constants needs; keeping it standalone stops the SimulationsPage chunk from pullingioredisinto the client bundle via the~/server/redisbarrel.Test plan
pnpm typecheck) — clean on touched files (nonew TS errors vs main; remaining errors are pre-existing stale
Prisma client issues that also exist on
origin/main).pnpm lint) — touched files were auto-fixed; repolint count is now lower than
origin/main(-158 errors).make quickstart all-local:ingestion puller worker readyclean(
organizations.createAndAssign— path wherescheduleUsageStatsForOrganizationwas removed)sidebar has no Topic Clustering entry
POST /api/collectoraccepts a span (nofetchExistingMD5sdedup path), event-sourcing pipelinepersists to local CH (~13ms p50)
full data (exercises
TraceService.getById,getTracesByThreadId,EvaluationService.getEvaluationsMultiple)analytics.dataForFilterreturns dropdown options(exercises the renamed
FilterService)scenarios.getScenarioSetsDatareturns clean[](exercises the post-
SimulationFacadeconsumer path)annotation.createreturns the new annotation cleanly(no
esSynctry/catch left)/api/start_workers,/api/rerun_checks,/api/cron/scenario_analytics,/api/cron/traces_retention_period_cleanup) all return 404Anything surprising?
Organization.elasticsearchNodeUrl / .elasticsearchApiKey / .useCustomElasticsearchare NOT droppedin this PR — only the application layer that reads/writes them. The
columns still exist in
schema.prismaand come back asnullonevery
organization.getAll. A follow-up Prisma migration can dropthem once we're confident no environment depends on them.
bullmqandbullmq-otelstay inpackage.jsonbecause theEE governance ingestion puller still uses BullMQ's repeatable-jobs
(cron pattern) primitive — alongside the restored topic-clustering
queue/worker and the shared
src/server/queues/helpers. Those are theareas an eventual BullMQ removal has to migrate (see the corrected note
in the commit-2 section above).
@aws-sdk/s3-request-presignerwas already missing fromnode_moduleson this worktree — added back viapnpm installduring this work. The lockfile bumps that came along (
hono,qs,ws,mermaid,liquidjs,turbo,@opentelemetry/*minors)are unintentional but harmless.
null-→-soft-skip behavior.
runEvaluation.ts:293andevaluation-execution.service.ts:273calltraceService.getEvaluationsMultipleto enrichtrace.evaluationsbefore mapping; pre-PR a
nullreturn meant "no evals recorded" andthe worker proceeded with
[]. Post-PR the collapsedEvaluationService.getEvaluationsMultiplethrows on no-CH (matchingthe rest of the collapsed-facade rule), so a transient CH
unavailability now fails the evaluation job instead of producing
an empty-evals result. Arguably more correct (there's no data to
evaluate against) but it is a real behavior change in worker
semantics.
original timer was a memory-leak safety net specifically for the
BullMQ collector + evaluations workers, which held long-lived
references to enqueued job payloads and could accumulate RSS
across their processing loops. This PR removes both worker
classes, so the leak profile that motivated the timer is gone.
The two BullMQ queues that remain (topic-clustering, ingestion
puller) hold bounded state — topic clustering runs infrequently
and doesn't retain payloads across cycles; the ingestion puller
streams and doesn't accumulate. Supervised deployments
(helm/docker/systemd) get container-level OOM-restart as
first-class. For the
npx @langwatch/serverpath the originaltimer was scoped to, if a real leak surfaces the right fix is a
targeted RSS-check or supervised process wrapper — not a
periodic-restart hammer that also masks the underlying leak.
/api/collectordedup semantics changed materially, notjust relocated. The PR routes the REST path through
ingestNormalizedSpan(same as OTLP), which usesSpanDedupService: RedisSET NXkeyed on(tenantId, traceId, spanId), with a 60s processing TTL extendedto 3600s (1h) on success. The pre-PR REST path used
fetchExistingMD5s— content-hashes of full request params keptin ES with no time bound (only a 256-entry cap). Concrete
behavior changes: (a) retries/replays/backfills resending the
same span content more than 1h after original ingest are no
longer deduped; (b) two requests with the same span-id but
different content are now silently deduped as identical (old
MD5 check would have detected the mismatch and reprocessed). The
new gate is unit-tested at the service layer
(
trace-request-collection.service.unit.test.ts:95-119); aREST-through-route integration test for dedup is a follow-up.
make quickstart all-localagainst afresh local Postgres / ClickHouse / Redis. Worth knowing for
reviewers reproducing: needs
SKIP_HOST_REDIS_CHECK=1if you havea local Redis on
:6379(per CLAUDE.md, that's the documentedsingleton).