Reuse one event-log deduplication helper - #3110
Conversation
🦋 Changeset detectedLatest commit: 8002bc6 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 Benchmarks⏳ Benchmarks are running for
commit Backend:
📜 Previous results (2)4135003Mon, 27 Jul 2026 17:33:16 GMT · run logs
d5547b6Sat, 25 Jul 2026 01:02:27 GMT · run logs
ℹ️ Metric definitions & methodologyBest/P75/P90/P99 deltas compare against the most recent benchmark run on Metrics — TTFS: time to first step body (in-deployment start() → first step body, deployment clocks) · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (whole-run time outside step bodies, in-deployment anchored) · SL: stream latency (in-deployment write → read propagation, readAt - writtenAt) · SO: stream overhead (end-to-end write+consume time beyond the modelled generation window) Scenarios — step: one trivial no-op step, no stream; no hooks, so the run stays in turbo mode (in-process fast path) · stream: one streaming step; no hooks, so the run stays in turbo mode (in-process fast path) · hook + stream: registers a hook before one step, which exits turbo mode (dispatch path) · 1020 steps: 1020 trivial sequential steps; STSO is measured between consecutive steps in the given step ranges, and WO is the whole-run overhead outside step bodies · stream latency: parallel reader/writer steps on a dedicated stream; SL is the in-deployment write->read propagation (readAt - writtenAt) · stream overhead (text): writer streams 300 variable-length text token deltas paced at 100/s for 3s (a haiku-size LLM's token throughput) while a parallel reader drains the whole stream; SO is the end-to-end write+consume time beyond the 3s generation window (overhead/backpressure) · stream overhead (structured): same workload as stream overhead (text), but each delta is an AI-SDK-style structured object ({ type: 'text-delta', id, text }) instead of a raw string, so the SO gap vs the text scenario is the added serialization cost 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 · SL 50/60/125 · SO 250/500/1000 · STSO (1-20) 20/30/60 · STSO (101-120) 30/45/90 · STSO (1001-1020) 40/60/120 All metrics are measured from deployment-side timestamps only. Runs are triggered by an in-deployment route that stamps the anchor ( Cold starts are kept in the numbers on purpose — they are part of real bursty-workload latency. The workbench deployment cold-starts the |
🧪 E2E Test Results
⏳ Tests are running... _Started at: _ ✅ All tests passed E2E Test SummarySummary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
✅ vercel-multi-region
|
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Reviewed the consolidation. The refactor is behavior-preserving — I diffed all three replaced loops against the helper and they're semantically identical (build ID set from target, append first-seen IDs in source order, keep existing entries in place). Two observations worth addressing before merge, both small.
1. The default-argument form mildly pessimizes two of the three call sites. Both replaced loops in runtime.ts were guarded by if (delta.events.length > 0) / if (loaded.events.length > 0), so an empty merge cost nothing. Default parameters are evaluated at call time, before the body runs, so targetIds = new Set(target.map(...)) now builds a full ID set over cachedEvents even when there is nothing to merge — and an early if (events.length === 0) return; inside the body wouldn't prevent it.
The empty case isn't hypothetical: the second site is the incremental-fetch path, which returns nothing new whenever the cursor is already at the tip. Measured cost of the wasted Set construction per replay iteration:
| event log | per empty merge | × 200 replay iterations |
|---|---|---|
| 500 | 0.02 ms | ~5 ms |
| 5,000 | 0.26 ms | ~51 ms |
| 20,000 | 1.11 ms | ~223 ms |
Modest, but it's per-iteration work proportional to log size in the replay loop — the same O(N)-per-replay shape the recent step-result memoization work removed — and the fix keeps the consolidation intact:
export function appendUniqueEvents(
target: Event[],
events: readonly Event[],
targetIds?: Set<string>
): void {
if (events.length === 0) return;
const ids = targetIds ?? new Set(target.map((event) => event.eventId));
// …
}Worth noting the PR's "Paginated merging remains linear" claim is true, but the regression is on the non-paginated sites, which the summary doesn't cover.
2. ./runtime/helpers is a published subpath, so exporting appendUniqueEvents widens @workflow/core's public API — which by this repo's convention (and the same note I left on #2903) makes the changeset minor rather than patch. Alternatives, both fine by me: keep it unexported and pass the helper via the existing import if the sharing is intra-file, or export it and bump the changeset. Just flagging so the bump type stays meaningful for the v5 line.
Everything else checks out: parameter reorder applied consistently at all call sites; the paginated loop still threads its retained loadedEventIds (so pagination keeps its single-set behavior); and the withPreconditionRetry concurrency comment I asked for on #2946 is still accurate — the default argument is evaluated synchronously at call time, so "builds its dedup set synchronously right before appending" still describes the code. Verified locally: helpers 38/38 and full core 1504 (+3 pre-existing expected-fails).
CI's two failures are both the nextjs-webpack dev.test.ts HMR rebuild-count test (canary + stable) — the long-running flake family that's now hit at least five unrelated PRs; a refactor confined to event-array merging can't reach dev-server rebuild accounting.
Happy to approve as soon as the changeset bump is settled — item 1 I'd take but won't insist on.
2 participants
What
appendUniqueEventshelper inruntime/helpers.tsWhy
The same event-ID merge behavior was implemented in multiple runtime branches. Keeping the small helper beside the other runtime helpers makes ordering and deduplication semantics explicit. Empty merges return without scanning the existing event log, while the pagination loop retains its ID set across pages so it does not repeatedly rescan previously loaded events.
Impact
Behavior-preserving refactor. Existing events stay in place; first-seen new IDs append in source order. Empty merges are constant-time and paginated merging remains linear in the number of loaded events.
Verification
pnpm --filter @workflow/core... buildpnpm --filter @workflow/core exec vitest run src/runtime/helpers.test.ts(38 passed)pnpm --filter @workflow/core typecheckgit diff --check