Skip to content

Group OTLP spans by trace and make task spans self-describing - #260

Merged
Exelord merged 1 commit into
mainfrom
claude/otel-vx-core-iiqah3
Aug 20, 2026
Merged

Group OTLP spans by trace and make task spans self-describing#260
Exelord merged 1 commit into
mainfrom
claude/otel-vx-core-iiqah3

Conversation

@Exelord

@Exelord Exelord commented Aug 20, 2026

Copy link
Copy Markdown
Member

A hostile audit of the OTLP receiver that landed in #259, aimed
deliberately at the code the previous wave had just written. Three
confirmed defects, each reproduced before it was fixed.

The defects

HIGH — a batched export merged two runs into one. The decoder
flattened every resourceSpans scope into one list and attributed
every vx.task span to the first vx.run span it found. A collector
batching two workspaces' runs into one export therefore stored one
run carrying the other workspace's tasks, and the second run
vanished entirely. Measured on a two-run payload: runs recorded: 1,
and workspace B's task rows sitting under workspace A's header.

This is not an exotic shape — batching across producers is what a
collector is for, and the guide already told people to put one in the
middle.

HIGH — a split run lost its tasks with a 400. A collector
re-batches by size and time, so one run can arrive as two exports. The
half carrying only task spans had no root, and the receiver refused the
whole payload. The tasks were dropped and the exporter had no reason to
retry (it got a 4xx, not a 5xx).

Root cause of both: task spans were not self-describing. A
vx.task span named its task and nothing about the run it belonged to,
so the decoder had no way to attribute it except positionally.

The fixes

Spans now group by trace id before decoding, so batched runs can
never borrow each other's tasks — one trace is one run, which is what
the trace id means.

And a task span now carries cicd.pipeline.run.id, vx.workspace.id
and vx.task.run_started_at, so a task that arrives ahead of its
header is stored anyway, through the same incremental ingestTask path
the native /v1/ingest/task endpoint already uses.

The run start is the load-bearing third field and the reason this
converges rather than duplicating: it is the storage key's partition
base, so a stranded task derives the same
(started_at, run_id, project, task) key its header would have given
it. Header-then-tasks and tasks-then-header land on one row either way
— verified end-to-end by splitting a run across two exports and
asserting the task count does not double.

Refusal narrows accordingly: a payload is a 400 only when it carries
neither a root span nor a task span that names its run. Empty and
malformed payloads are still refused exactly as before.

Verification

Three differentials, each isolating one fix, every restore verified
back to a green baseline:

mutation fails
revert trace grouping (flatten all spans) 3
drop vx.task.run_started_at from the encoder 2
refuse any payload without a root span 3

The batched-export and split-run cases are pinned as real e2e
a live platform, the real OtelSink, two workspaces, asserting each
run's tasks land under its own workspace and that a replayed half
stores nothing new.

Gates from the root: fmt/lint 0, core 2631 / 0 (git diff src/
empty), cloud 1286 / 0 across 58 suites (38 skip = the browser
suites, which need playwright + a built ui/dist this container
lacks), vx-otel 44 / 0, docs site 168 pages with a zero-broken-link
crawl over both edited pages.

No CACHE_VERSION / SCHEMA / migration / wire-version bump —
vx.task.run_started_at is an additive attribute, and an older
exporter's payloads still decode through the root-span path unchanged.

Docs

Both pages claimed "a payload without a root span is refused", which
this makes false. Corrected in place, and the OTel guide now states
what a collector in the middle actually does and why both shapes are
handled.


Generated by Claude Code

Exelord commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

The first CI run went red on lint · format · test, and driving it to ground turned up a real data-loss bug that is not this PR's — but which reds every PR and every push to main. It's fixed in 65cec5b, which is why the scope widened. Explaining, since an OTLP PR gaining a cache-resolver change deserves a reason.

What was red

The core job died at 83 s with its output stopping mid-word inside tests/cache.test.ts — no bun-test summary, no vx footer. src/ and tests/ in this PR are byte-identical to main, and the core suite is green locally, so the first read was the truncated-gate signature this repo has recorded once before and never root-caused.

Two things were ruled out rather than assumed:

  • OOM — measured peak tree RSS for the whole core suite: 611 MB on a 7 GB runner. Refuted.
  • A re-runrerun_failed_jobs and workflow_dispatch both return 403 for this integration, so the one sanctioned re-run for this class wasn't available.

That forced a different question, and it found the actual bug.

The bug

CI installs bun-version: latest. That is now Bun 1.4.0; this container runs 1.3.11. Bun 1.4.0's Glob.scan follows symlinked directories, where 1.3.11 refused to.

resolveOutputs contained its results lexically, and its own comment said why that was enough — because the scanner would not descend through a link. The test it cited spelled out the consequence in advance:

if Bun ever adds symlink following … cleanOutputs starts deleting through the link and no vx guard fires. This test is the tripwire.

It fired. With dist -> ../victim and a blameless outputs.files: ['dist/**'] — no .., no leading /, nothing for the loader to reject:

Bun 1.3.11 Bun 1.4.0 (CI)
resolveOutputs(['dist/**']) [] resolves through the link
cleanOutputs removes nothing removes the victim's files
victim/precious.txt survives deleted

cleanOutputs runs before every cache-miss exec and every cache-hit restore, so a project whose declared output dir is a symlink — a shared build dir, a tmpfs, a volume mount — loses the target's contents on every run.

The workspace half was wider and had no pin at all: resolveWorkspaceOutputs carried no containment filter, because that form deliberately ignores project boundaries. The workspace root is still a boundary, and on 1.4.0 a symlinked dir under it let cleanWorkspaceOutputs delete outside the workspace entirely — measured against a sibling temp dir, outside/secret.txt removed. Bounded only by the filesystem.

The fix

Stop trusting a dependency for a safety property. Containment is verified against the real path, on the containing directory rather than the file — a link crossing is a directory boundary, and a symlinked file is not an escape at all, because rm unlinks the link and never the target. Directory realpaths are memoized, and a path that can't be resolved is dropped: we don't delete what we can't place.

Differential on 1.4.0: reverting only src/cache/inputs.ts fails exactly 2 of the 50 in tests/inputs-resolution.test.ts; restore back to 50/0. Both controls pass either way — a normal dist/** still resolves, and a workspace output inside the root still resolves — which is what stops the fix degenerating into "resolve nothing".

Cost measured, not asserted (interleaved min-of-5, three trials): a typical output tree (100 files / 5 dirs) goes 0.4–0.6 ms → 0.5–0.7 ms, inside noise; a large one (5000 files / 50 dirs) goes 11.7–14.0 ms → 14.2–18.2 ms — about 50 realpath calls on an operation that already costs ~13 ms and runs once per task.

No CACHE_VERSION bump: key derivation is untouched, and every previously-correct config resolves byte-identically on both binaries.

Recorded, not fixed

Three things are written up in the decision log rather than acted on here, because each is a separate decision:

  • Project discovery uses the same Glob.scan, so on 1.4.0 a symlinked directory can now be discovered as a project. That's a behaviour change, not data loss — and docs/design/turbo-nx-test-gaps.md already lists Turbo's symlinked-package tests as missing coverage, so whether vx wants to discover them is a real question, not a regression to paper over.
  • Input globs now see through a symlinked dir, changing keys only for configs that previously resolved nothing there. Self-healing — miss once, re-run, re-cache — never a wrong hit.
  • A core flake now has a name, closing a gap I'd recorded against myself: cache baseline: SQLite writes > batched recordRuns is ≥ 3× faster per row than single recordRun, seen 1 run in 4 under load. It already went through the benchRatioSides de-flake and still flakes, so that de-flake is incomplete. Deliberately left alone — it's a perf-guard calibration question unrelated to either change here, and it is not what redded CI: a single failing test still prints a summary, and the red job printed none.

Gates from the root: fmt/lint 0, core 2633 / 0 on both 1.3.11 and 1.4.0, cloud 1286 / 0, vx-otel 44 / 0.


Generated by Claude Code

Exelord commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

CI is green on 65cec5b — all three checks pass, mergeable_state: clean.

Correction to my own diagnosis above, pushed as 8bb3940. I read the red job as a process death because its log stopped mid-word with no bun-test summary and no vx footer. The green run's log ends exactly the same way — mid-word at (pass) parseCach, no summary, no footer, conclusion: success. Both tails stop around ~3200 lines.

So the truncation is the logs API, not the process, and it says nothing about how a run ended. "The red job printed none" was never evidence: the (fail) line and the summary were simply past the truncation point. The red is fully explained by the deterministic Bun 1.4.0 symlink failure, which CI going green on exactly that fix confirms — no unexplained kill, no second mystery.

That also puts this repo's 2026-07-30 "killed process, not a failing assertion" entry in doubt, since its stated evidence was the same absent summary. Not asserted wrong — that run can't be re-examined — but flagged in the log, along with the rule I violated: before concluding anything from where a CI log ends, check where a passing run's log ends.


Generated by Claude Code

The receiver flattened every resourceSpans scope and attributed every
vx.task span to the first vx.run span it found. A collector batching two
runs therefore produced ONE run carrying the other workspace's tasks
while the second run vanished — and a collector splitting one run across
two exports had the task-only half refused with a 400, losing the tasks
outright. Both shapes are what a collector does by design: it batches
across producers and re-batches by size and time.

Spans now group by trace id, so batched runs can never borrow each
other's tasks. And a task span carries its own run id, workspace id and
run start, so a task arriving ahead of its header is stored through the
same incremental ingestTask path the native wire already uses. The run
start is the load-bearing part: it is the storage key's partition base,
so a stranded task derives the SAME (started_at, run_id, project, task)
key its header would have given it, and the two arrival orders converge
on one row rather than duplicating.

Refusal narrows accordingly: a payload is a 400 only when it carries
neither a root span nor a task span that names its run.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019x8vExRpvPDkvwcDZ7wKDE
@Exelord
Exelord force-pushed the claude/otel-vx-core-iiqah3 branch from 8bb3940 to 02b790d Compare August 20, 2026 19:19
@Exelord
Exelord merged commit a2dfea5 into main Aug 20, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants