Uh oh!
There was an error while loading. Please reload this page.
[core] Fix time parsing for region-tagged run IDs - #2943
Conversation
…version Two fixes to the step latency telemetry, both surfaced by production data: 1. TTFS reported exactly 0 for every run with a region-tagged run ID. Tagging schemes set the MSB of the ULID's 48-bit timestamp as a tag bit, which runIdCreatedAt() decoded as part of the creation time — placing the TTFS anchor millennia in the future so every sample clamped to 0. The tag bit is now cleared before use, and durations more negative than a plausible clock skew drop the sample entirely instead of clamping, so a future anchor corruption surfaces as missing data rather than an all-zero distribution. 2. Latency samples now carry the @workflow/core version that computed them (eventData.sdkVersion, lifted into the v4 frame meta). The transport user-agent identifies the HTTP client package, which resolves independently of core in consumer lockfiles, so it cannot attribute measurements to the runtime code that produced them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 8a7f1cf The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Workflow Benchmarkscommit Backend:
📜 Previous results (2)0d300e8Wed, 15 Jul 2026 19:33:17 GMT · run logs
461aae4Wed, 15 Jul 2026 18:24:40 GMT · run logs
Avg deltas compare against the most recent benchmark run on Metrics — TTFS: time to first step body execution · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (time outside step bodies, client start → last step body exit) · SL: stream latency (first chunk write → visible to the reader) Scenarios — stream: one step that streams chunks back to the client; no hooks, so the run stays in turbo mode · hook + stream: registers a hook before the same streaming step, which exits turbo mode · 1020 steps: 1020 trivial sequential steps; STSO is measured between consecutive steps in the given step ranges 🟢/🔴 mark percentiles within/above target. Targets (p75/p90/p99, ms) — TTFS 200/300/600 · SL 50/60/125 · STSO (1-20) 20/30/60 · STSO (101-120) 30/45/90 · STSO (1001-1020) 40/60/120 TTFS/WO compare client vs deployment clocks and SL compares the step runner’s clock vs the client’s (NTP-synced in CI). WO ends at the last step body exit, the closest observable proxy for the final step-completion request. |
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests▲ Vercel Production (1 failed)fastify (1 failed):
Details by Category❌ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
✅ vercel-multi-region
❌ Some E2E test jobs failed:
Check the workflow run for details. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TooTallNate
left a comment
There was a problem hiding this comment.
Both fixes are well-motivated and the diagnosis writeup is excellent. I verified the mechanics independently:
ulid@3.0.1'sdecodeTimebound isTIME_MAX = 2^48 - 1, so a tagged timestamp (real time + 2^47) decodes without throwing and the subtract-after-guard inrunIdCreatedAtis correct. Using subtraction rather than a bitwise mask also matters here since JS bitwise ops truncate at 32 bits. ✅- Drop-instead-of-clamp is the right failure mode: a future multi-bit tagging scheme that the MSB-clear doesn't fully handle would surface as missing samples via the
MAX_CLOCK_SKEW_MSguard rather than an all-zero distribution. ✅ - v4 wire compat:
parseV4EventMetain workflow-server picks known fields and ignores unknown meta keys, so older servers tolerate the newsdkVersionframe meta. ✅ version.tsis genversion-generated inbuild/typecheck/devscripts, and release builds run after the changeset version bump, so the reported version is correct on published artifacts. ✅
One blocking issue on release ordering for the sdkVersion field (inline).
| ? { eventCount: tracking.eventCount } | ||
| : {}), | ||
| optimizations, | ||
| sdkVersion: coreVersion, |
There was a problem hiding this comment.
Blocking (release ordering, not this code): core attaches sdkVersion to step-terminal eventData unconditionally, but workflow-server's persistence strip list (CLIENT_LATENCY_TELEMETRY_FIELDS in lib/data/events.ts) is ['ttfs','stso','stepCount','eventCount','optimizations'] and does not include sdkVersion, and snapshotClientLatencyTelemetry doesn't consume it either.
On the v4 wire this is safe (world-vercel moves it to frame meta; the server ignores unknown meta fields and never copies meta into eventData). But on the v2/v3 wire, eventData is sent raw, so sdkVersion would be persisted into the stored, user-visible event's eventData, violating the server's documented invariant that latency telemetry is "aggregate telemetry, not run state" and stays out of event reads and DynamoDB. The affected cohort is old @workflow/world-vercel + new @workflow/core resolved independently in a consumer lockfile, which is exactly the mixed-version scenario this PR's motivation describes (unless the old v3 client path zod-strips unknown eventData keys before POSTing; I couldn't rule that in or out from the current repo since the v3 implementation was replaced).
Since merging here auto-releases via changesets, can we land and deploy the server-side counterpart first (add sdkVersion to the strip list + pick it up in snapshotClientLatencyTelemetry/metric tags)? That both closes the leak and makes the field actually consumable, so the release order becomes server-then-client and this PR merges as-is.
There was a problem hiding this comment.
sorry this wasn't ready for review yet, should have made it a draft PR. The core version stuff is something the AI add unilaterally, removed now
TooTallNate
left a comment
There was a problem hiding this comment.
Re-reviewed after the sdkVersion removal — my previous blocking item is resolved, and the remaining diff is the TTFS fix I already verified mechanically (ulid decodeTime accepts tagged timestamps since TIME_MAX = 2^48 - 1; subtraction rather than a 32-bit-truncating bitwise mask; drop-instead-of-clamp surfaces future anchor corruption as missing data instead of a zeroed distribution). Test coverage is thorough at all three layers (codec, compute, runtime e2e).
The EXECUTION_ADJACENCY addition (icn1 tolerating hnd1) is a reasonable geo-elastic tolerance — Seoul→Tokyo is nearer than the already-listed syd1.
One recommendation inline about delegating the tag-bit knowledge to @workflow/world-vercel/run-id's decode() — non-blocking since the current code is correct, but worth doing before merge while the file is hot.
| try { | ||
| return decodeTime(ulidPart); | ||
| const time = decodeTime(ulidPart); | ||
| return time >= TIMESTAMP_TAG_BIT ? time - TIMESTAMP_TAG_BIT : time; |
There was a problem hiding this comment.
Recommendation (non-blocking): consider delegating the tag-bit handling to the scheme's own codec instead of hand-rolling the clear here:
import{decodeTime}from'ulid';import{decode}from'@workflow/world-vercel/run-id';// in the try block:returndecodeTime(decode(ulidPart).ulid);decode() already returns the ULID with the tag bit cleared, throws on malformed input (your existing catch covers that), and @workflow/core already depends on @workflow/world-vercel, so this adds no new edge (run-id is a leaf module, no cycle).
The reason it matters: the tagging scheme carries a 5-bit version field, i.e. it explicitly anticipates layout evolution. If a future version moves bits around, decode() in world-vercel gets updated as the single source of truth and this anchor stays correct automatically, whereas a local 2^47 subtraction can silently diverge. The MAX_CLOCK_SKEW_MS drop guard you added would at least turn that divergence into missing data rather than bad data, which is why I'm not calling this blocking — but single-sourcing the codec removes the failure mode entirely.
If you'd rather keep core free of scheme knowledge as a matter of principle, the current MSB-clear is defensible too (a set MSB can never be a real timestamp under any scheme) — in that case maybe just soften the doc comment's implication that MSB-tagging is a general World.createRunId() convention, since today it's specifically world-vercel's.
There was a problem hiding this comment.
Applied in 8a7f1cf — runIdCreatedAt now delegates to decode() from @workflow/world-vercel/run-id (verified: the dependency already exists in core's dependencies, ./run-id is an exported subpath, and world-vercel has no dependency back on core, so no cycle). The hand-rolled TIMESTAMP_TAG_BIT constant is gone; the doc comment now points at the codec as the single source of truth. Tests (34) and typecheck pass.
Review feedback: instead of hand-rolling the timestamp tag-bit clear (2^47 subtraction), use decode() from @workflow/world-vercel/run-id, which returns the ULID with the tag bit cleared and throws on malformed input. The tagging scheme carries a 5-bit version field, so if the layout ever evolves the codec stays the single source of truth and this anchor tracks it automatically. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
No backport to This fixes a bug in the TTFS/STSO step-latency telemetry feature, which is main-only: the touched source files ( To override, re-run the Backport to stable workflow manually via |
Summary
TTFS reported exactly 0 for every run with a region-tagged run ID (#1981 rollout). Run-ID tagging schemes (
World.createRunId(), e.g. world-vercel's region-tagged IDs) set the MSB of the ULID's 48-bit timestamp as a tag bit.runIdCreatedAt()— the TTFS anchor — decoded it as part of the creation time, placing the anchor ~4,460 years in the future, soMath.max(0, end − anchor)clamped every sample to 0. In production, the entire TTFS distribution (p50 through max) for the affected cohort has been exactly 0 since Jul 14 ~08:30 UTC; untagged cohorts report real values throughout.runIdCreatedAt()now clears the tag bit before use (a set MSB can never be a real creation time).computeStepLatencyEventData()now drops samples whose raw duration is more negative than a plausible clock skew (60 s) instead of clamping — a future anchor corruption will surface as missing data, not an all-zero distribution silently dragging every percentile down.Testing
run-id-time.test.ts: tagged-ULID decode (tag bit cleared).step-latency.test.ts: corrupt-anchor samples dropped (TTFS-only and mixed TTFS/STSO cases); small-skew clamping unchanged.runtime.test.ts: end-to-end latency drive with a region-tagged run ID asserts TTFS ≈ run age (the exact production regression shape).🤖 Generated with Claude Code
Also included
The multi-region e2e lane failed deterministically on this PR with
workflow executed in hnd1, tagged icn1. This is the geo-elastic execution locality the test'sEXECUTION_ADJACENCYmap already documents (kix1-tagged runs observed executing in hnd1/Tokyo) — production lifecycle metrics show icn1-tagged runs executing in hnd1 about half the time over the last 6h, with tagging/data placement remaining strictly icn1. Addedhnd1toicn1's tolerated execution set.