ci: e2e black-box job — boot web+worker, verify ingest→query over HTTP - #60
Merged
Conversation
…query over HTTP The existing `servertests` job only exercises the DB/tRPC layer in-process and never starts the servers, so the full public-API path (ingest -> queue -> worker -> GreptimeDB -> query) had no CI coverage after the ClickHouse->GreptimeDB cutover. Add `e2e-blackbox`, mirroring upstream's tests-web + tests-server-e2e recipe but adapted for the fork (no ClickHouse; GreptimeDB + MinIO started via docker run, reusing the servertests infra pattern). It builds web+worker, migrates Postgres and GreptimeDB, seeds example data, boots `pnpm run start`, waits on both health endpoints, then runs a curated green subset of HTTP `*-api.servertest.ts` (ingestion/otel/traces/observations/scores/sessions) plus the `__e2e__` suite (`test:e2e:server`). Ingestion queue/write intervals are pinned to 1ms so the async pipeline drains fast enough for the waitForExpect polls. The test list is intentionally a curated subset that grows as more paths turn green on GreptimeDB, matching this workflow's existing philosophy.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ntain permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR extends the existing CI workflow by adding a new black-box end-to-end job that boots the real web and worker HTTP servers and runs a curated subset of HTTP-based public API tests to validate the full ingest → queue → worker → GreptimeDB → query path.
Changes:
- Adds workflow-level
permissionsconfiguration (currently restricted tocontents: read). - Introduces a new
e2e-blackboxjob that provisions Postgres/Redis + GreptimeDB/MinIO, builds and starts the app, waits on health endpoints, and runs selected HTTP +__e2e__suites. - Captures app logs to aid debugging on failures.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback: the health-wait loops only `break` on success and otherwise fall through silently, so a service that never comes up surfaces as a misleading downstream error. Re-check once after the loop and exit with the container logs, making the root cause obvious.
GreptimeDB advertises text columns over the MySQL wire as VARCHAR with characterSet 33 (utf8mb3). mysql2's default reader mis-decodes that combination and turns every 4-byte UTF-8 code point (emoji, rare CJK) into U+FFFD — silent data corruption on read, even though the wire bytes are valid UTF-8. Add a typeCast on the SQL pool that decodes text columns straight from the raw buffer as UTF-8. Numeric / temporal / DECIMAL / BIGINT columns fall through to mysql2 so the decimalNumbers / bigNumberStrings handling is preserved. Verified against a live GreptimeDB v1.1.1: emoji round-trips intact and decimal/bigint still come back as precision-preserving strings. Also restore the full HTTP black-box test list in the e2e job and pin the trace delete delay/concurrency so the DELETE-then-poll tests don't time out.
The observations UI column definitions baked the table alias into
`clickhouseSelect` as a double-quoted identifier (`o."user_id"`) and omitted
`queryPrefix`, unlike the traces mapping. Over GreptimeDB's MySQL wire this
broke the advanced-filter path two ways:
1. The metadata EAV EXISTS fell back to correlating on the bare table name
(`observations.project_id`) instead of the outer alias `o`, because
`outerAlias = tablePrefix ?? table` and `tablePrefix` (queryPrefix) was
undefined. That both 500s ("No field named observations.project_id") and
is a tenant-isolation-relevant miscorrelation.
2. `o."col"` parses as alias `o` followed by a string literal — GreptimeDB
does not run in ANSI_QUOTES mode — yielding "No field named o".
Align the observations mapping with the traces mapping: bare `clickhouseSelect`
plus `queryPrefix`, leaving the computed obs-aggregate / tool / scores columns
(which carry ClickHouse expressions and route through their own remapping)
untouched. The translation now emits backtick-quoted, alias-qualified SQL.
Add a regression test asserting the metadata EAV correlates on the outer alias
and scalar predicates are backtick-quoted with no double-quoted identifiers.
…EXISTS
The public-API traces surface sends `score_categories` as a StringOptions filter
whose options are combined `name:string_value` strings (ClickHouse stored the
column as `concat(name, ':', string_value)` and matched with `hasAny`). The
GreptimeDB translation only special-cased the `categoryOptions` filter type, so a
StringOptions filter on `score_categories` fell through to a plain
`s.score_categories IN (...)` predicate — a dangling reference to a column/alias
that does not exist on the merged projection, which 500s ("No field named
s.score_categories").
Add ScoreCategoryMembershipFilter: parse the `name:string_value` options (split
on the first `:`) and emit a project-scoped, grain-correlated, soft-delete-aware
EXISTS over `scores` matching `(name, string_value)` pairs (NOT EXISTS for
`none of`). Route StringOptions on `score_categories` to it. Also make the
rollup-score field detection prefix-insensitive so the same routing works
whether the field arrives bare or as `s.<col>` (this also fixes the parallel
`scores_avg` NumberObject path).
Add a regression test asserting the categorical EXISTS shape and negation.
The public-API `environment` filter only constrained the trace row (`t.environment`); the `observations_stats` / `score_stats` rollup CTEs aggregated every observation/score for the trace regardless of environment. A trace whose observations or scores span environments therefore attached all of them, so `GET /traces?environment=X` returned observations/scores from other environments (asserted by "should fetch ... filtered by environment" and "should fetch traces with trace scores only" — both saw 2 where 1 was expected). Push the environment predicate into both rollup CTEs: compile the environment StringOptions once against each CTE's base table (`observations` / `scores`, no alias) and AND it into the CTE WHERE (via `extraFilterSql` for the obs CTE, and inline for the score CTE). Fresh uid() params avoid colliding with the trace-level predicate; the LEFT JOIN still keeps environment-matching traces that have zero in-environment observations/scores.
greptimeLatencyMs() derived latency via to_unixtime(), which GreptimeDB truncates to whole seconds. A sub-second observation, or one whose span straddles a second boundary, was therefore mis-measured by up to ~1s — e.g. a 1.5s observation could read as 2.0s and be dropped by a `latency <= 1.9` filter. Because the error depends on where the wall-clock timestamps fall within their seconds, the public API latency-filter test was flaky (passing or failing by alignment), and latency display/ordering carried the same imprecision. start_time/end_time are stored at millisecond precision, and CAST(<timestamp> AS BIGINT) yields epoch milliseconds directly, so subtract the cast greatest/least bounds instead of round-tripping through to_unixtime. Verified against a live GreptimeDB: a 0.5s span now measures 500ms (was 0), a 2.5s span 2500ms (was 2000), and the latency filter test passes deterministically.
… coalescing The ingestion rebuild-coalescing guard compared the Redis watermark against `job.data.timestamp.getTime()`. `job.data.timestamp` is typed as a Date, but BullMQ round-trips job data through JSON in Redis, so a dequeued job carries it back as an ISO string — and `String.prototype.getTime` does not exist, so the guard threw `TypeError: getTime is not a function`. The guard is only reached once a coalesce watermark exists, i.e. from the second update onward for a given entity (the first update short-circuits on the null watermark). So with LANGFUSE_INGESTION_COALESCE_REBUILDS enabled (the default), every follow-up rebuild of an already-seen trace/observation/score threw and retried forever: entity updates silently never merged, and reads kept returning the first snapshot. This is what the public-API "merge metadata across multiple trace updates" black-box test caught. Coerce through `new Date(job.data.timestamp)` so the comparison works whether the value arrives as a Date or an ISO string. Verified end-to-end against a live web+worker+GreptimeDB stack: the second update now merges and the test passes deterministically.
The e2e job disabled LANGFUSE_CACHE_PROMPT_ENABLED (copied from upstream's tests-web tuning), but this job also runs the __e2e__ suite, whose "Prompts endpoint > creates and returns a prompt" test asserts that fetching a prompt populates its Redis cache key (`expected 0 to be 1` when caching is off). Upstream only disables prompt caching for the plain server-test project, not for test:e2e:server. Drop the override so prompt caching runs at its default (enabled) and the __e2e__ prompt-cache assertion holds.
…r a deleted trace deleteTracesFromGreptime only tombstones child observations/scores that are already visible in the projection tables (it resolves child ids via readProjectionIdsByTraceIds). A child that was still in-flight in raw_events (written but not yet projected) when its parent trace was deleted — or one that is ingested late, after the delete — therefore never receives its own tombstone. A later rebuild or reconciliation replays its (tombstone-free) raw_events history and resurrects it as an orphan projection under a trace that no longer exists. raw_events has no trace_id column (the trace id lives inside the event body), so children cannot be enumerated by trace id at delete time. Instead each observation/score rebuild now checks whether its parent trace is deleted (isParentTraceDeleted -> parseRawEventHistory(trace history).deleted) and rebuilds soft-deleted when it is. Wired into both the live ingestion path (ingestionQueue) and the reconciliation path (which is the healer for pre-existing orphans), alongside the existing project-tombstone guard. Cost: one extra tag-indexed raw_events read per observation/score rebuild. Correctness-first; gate behind a flag or short-TTL positive cache if it shows up on the ingestion hot path at scale. Adds a unit test for the new firstTraceId helper.
The local media signed upload/download routes rejected any non-PUT/GET method with 405 before any CORS handling. Browser SDK uploads (and <audio>/<video> range reads) are cross-origin from the app to the Langfuse host and send an OPTIONS preflight for PUT + custom headers; the preflight 405'd, so the browser never issued the real request and the upload/download silently failed. Every other public API route runs the shared cors middleware. Run it here too and answer OPTIONS before the method guard.
processClickhouseTraceDelete deletes from GreptimeDB but logged "from Clickhouse" on both the info and error paths, which misleads operators reading worker logs into looking at the wrong backend. The function name is kept for upstream parity; only the runtime log strings are corrected.
…tTraceDeleted - Add pull-requests: read to the workflow permissions block. dorny/paths-filter reads the PR's changed-file list on pull_request events, and the least-privilege contents:read-only block can fail the `changes` gate on shallow/fork checkouts, blocking all downstream jobs. Everything else stays read-only. - Cover isParentTraceDeleted in deletion.test.ts: empty traceId -> false (no query), latest tombstone -> true, live-only -> false, live-after-tombstone (re-create) -> false. Both address Copilot review comments on #60.
… NaN Same BullMQ JSON round-trip issue as the ingestion coalescing guard: job.data.timestamp is typed Date but arrives as an ISO string, so `Date.now() - job.data.timestamp` evaluated to NaN and the langfuse.dlq_retry_delay histogram silently recorded NaN. Coerce through Date(). The other job.data.timestamp consumers were already safe (retry-handler and evalService both wrap it in new Date(...)); only this one did raw arithmetic.
…tchTimestamp helper The queue payload contract declares `timestamp: z.date()`, but BullMQ round-trips job data through JSON in Redis, so a dequeued job carries `timestamp` back as an ISO string. That mismatch has already bitten two consumers (ingestion coalescing threw "getTime is not a function"; the DLQ delay metric computed NaN), and each was patched with its own `new Date(...)` coercion. Add a single `jobDispatchTimestamp(job): Date` helper next to the queue contracts and route every consumer through it (ingestionQueue, dlqRetryService, retry-handler, evalQueue), so the coercion lives in one documented place and a future consumer can't reintroduce the same bug by trusting the declared type. No contract change — `new Date()` accepts both a Date and an ISO string.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
web/src/pages/api/public/media/[mediaId]/upload.ts:44
- This route now explicitly supports CORS preflight via
OPTIONS, but the 405 response advertises onlyPUTas allowed. For correctness/clarity (and consistency with the explicitOPTIONSsupport), includeOPTIONSin theAllowheader.
if (req.method !== "PUT") {
res.setHeader("Allow", "PUT");
res.status(405).end();
return;
web/src/pages/api/public/media/[mediaId]/download.ts:29
- This route now explicitly supports CORS preflight via
OPTIONS, but the 405 response advertises onlyGETas allowed. For correctness/clarity (and consistency with the explicitOPTIONSsupport), includeOPTIONSin theAllowheader.
if (req.method !== "GET") {
res.setHeader("Allow", "GET");
res.status(405).end();
return;
The helper exists precisely because a Redis-replayed job carries `timestamp` as an ISO string despite the schema declaring `z.date()`, but its parameter type still said `Date` — undermining the helper's intent and misleading call sites. Widen the accepted type to `Date | string` (the Date constructor accepts both); callers passing a `Job` whose `data.timestamp` is typed `Date` still satisfy it.
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.
What
Adds a new CI job
e2e black-box (HTTP ingest -> query)to.github/workflows/ci.yml.It is the only job that boots the real web (:3000) and worker (:3030) HTTP servers and drives the full public-API path end to end:
Why
After the ClickHouse -> GreptimeDB cutover, the existing
servertestsjob only exercises the DB/tRPC layer in-process and never starts the servers. The 31 HTTP*-api.servertest.ts(whichfetchlocalhost:3000) and the__e2e__suite therefore had no CI coverage — the whole ingest→query black-box path was running only locally against a livepnpm run devstack.How
Mirrors upstream's
tests-web+tests-server-e2erecipe, adapted for the fork:services:; GreptimeDB + MinIO viadocker run(custom start commands GitHubservices:can't express), reusing theservertestsjob pattern.pnpm run build→db:deploy(Postgres) +greptime:migrate→db:seed:examples→pnpm run start &(turbo starts web+worker) → wait on both health endpoints.*-api.servertest.tssubset:ingestion / otel / traces / observations / scores-v2 / sessions(self-provision their project).test:e2e:server—__e2e__/api.servertest.ts(ingest → pollGET /traces/{id}→ assert worker evaljobExecution) +otel-tenant-isolation.LANGFUSE_INGESTION_QUEUE_DELAY_MS=1+LANGFUSE_INGESTION_WRITE_INTERVAL_MS=1so the async pipeline drains fast enough for thewaitForExpectpolls.Notes
Verification
*-api.servertest.tsfiles + the two__e2e__files + thetest:e2e:serverscript exist.