[docs] Document duplicate-event handling, and describe webhook token generation accurately - #3497
Conversation
#3381 changed replay to ignore an event whose class it has already consumed for an entity, instead of reporting a divergence that ends the run with CORRUPTED_EVENT_LOG. The v5 event sourcing guide still only described the write-path terminal-state guard, which is a different layer and does not cover the duplicates the write path permits. Adds a "Duplicate Events" section covering the class table, the types that belong to no class, the two invariants (consumers are offered the event first, skipped events do not advance the deterministic clock), and the info/error logging split. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: bdd3418 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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 |
Sim WorldSimulated world deterministic testing for races. Traces 🟠 Mint-ordered log — 3 fail of 41 total
Full trace: 🟢 Append-only log — 0 fail of 41 total
Full trace: |
Co-authored-by: Peter Wielander <mittgfu@gmail.com> Signed-off-by: Peter Wielander <mittgfu@gmail.com>
The error-message change in the previous commit left `hook.test.ts` asserting the old string, which fails the unit suite. Update the assertion, and apply the same correction to the places that still describe a generated token as random: the `createHook()` empty-token guard and its comment, the hooks and testing guides, the `resumeWebhook()` reference, and the vitest workbench comments. The v5 hooks guide additionally attributed webhook-endpoint security to that randomness. A generated token is drawn from the run's seeded sequence, so the guide now points at authenticating the request instead of relying on URL secrecy.
…cing-duplicate-events
#3467 greys out duplicates in the events views. What it can identify is narrower than what the runtime skips, because it reads the log without consumer state, so the page says where the two differ.
|
|
||
| ## Duplicate Events | ||
|
|
||
| Concurrent invocations replaying the same run share one event log and _may_ append to the log _before_ verifying that the transition is valid, in certain cases. We recommend validating transitions atomically with event inserts, otherwise duplicate events might be recorded in the log. |
There was a problem hiding this comment.
human review:
| Concurrent invocations replaying the same run share one event log and _may_ append to the log _before_ verifying that the transition is valid, in certain cases. We recommend validating transitions atomically with event inserts, otherwise duplicate events might be recorded in the log. | |
| Concurrent invocations replaying the same run share one event log and _may_ append to the log _without_ verifying that the transition is valid, in certain cases. This is meant as a performance optimization so worlds don't _need_ to verify every transition. We do still recommend that worlds validate transitions atomically with event inserts to prevent duplicate events being recorded in the log. |
There was a problem hiding this comment.
(AI) Applied verbatim.
| The remaining event types belong to no class and are never skipped: | ||
|
|
||
| - `hook_received`: a hook legitimately receives many payloads under one ID, so a second `hook_received` is not a repeat of anything. | ||
| - `hook_conflict`: hook creation must be idempotent, so getting a `hook_conflict` at any position indicates a failure to acquire the hook and can't come after `hook_created` for the same hook |
There was a problem hiding this comment.
@VaguelySerious but multiple hook_conflict written one after the other can still be inert? incase 2 writers both try to write a hook_conflict simultaneously, we also don't want to have a corrupted event log in that case no?
There was a problem hiding this comment.
although yes, it can't/shouldn't come after a hook_created for the same hook (correlation ID)
it CAN however come after a hook_created under a different correlation ID. think about this code:
createHook('a');
await sleep('1 minute');
createHook('a');
second one should conflict but will also have a different correlation ID
and it's valid
There was a problem hiding this comment.
(AI) Yes, inert. packages/core/src/workflow/hook.ts returns EventConsumerResult.Consumed for hook_conflict rather than Finished, so the consumer stays registered and claims every copy it is offered. Two writers racing the same conflict cannot corrupt the log, and the class check is never reached, which is why the type needs no class.
And you are right that a conflict can follow a creation under a different claim (createHook('a'), dispose, createHook('a') after another run took the token). The bullet has been rewritten to drop that reasoning.
pranaygp
left a comment
pranaygp
left a comment
There was a problem hiding this comment.
Reviewed the section against the runtime code. The verifiable claims check out: the class table matches ENTITY_EVENT_CLASS_BY_TYPE in packages/world/src/events.ts exactly, the no-class list covers all seven remaining types, the info/error logging split matches onDuplicateEvent in packages/core/src/workflow.ts:432, and the token claims match the seed at workflow.ts:353 and the ctx.generateNanoid() fallback at hook.ts:100. A sweep of v5 docs + packages/core finds no leftover "randomly generated" claims — the wording alignment is complete (v4 deliberately untouched, which matches #3381 being main-only).
Two things worth addressing, both detailed inline:
- The review-suggestion rewrite of the section's opening paragraph now contradicts the sentence that introduces the section (line 228 says the write path permits these duplicates; the new paragraph frames them as an atomicity gap a well-implemented backend would close), and it dropped the stale-prefix mechanism the next paragraph leans on.
- The linked
corrupted-event-logpage still lists "Duplicate completion events" as a common cause of the error — exactly the case #3381 now steps over. Following this section's own link lands the reader on a page saying the opposite. Worth fixing here or in a fast-follow.
The rest are wording/precision nits. CI is green apart from the workbench-python-workflow deployment failure, which is unrelated to this docs change.
|
|
||
| Those duplicates are committed but inert. The outcome was decided by the first event of its kind at a lower position in the log, and every replay reads that same event at that same position, so a later copy cannot change what the workflow observes. | ||
|
|
||
| To keep an inert copy from failing an otherwise healthy run, the runtime groups event types into **classes** and tracks, per entity, which classes the current replay has already consumed. When an event is offered to every registered consumer and none wants it, and its class is already recorded for that entity, the replay steps over it instead of reporting a [replay divergence](/docs/errors/replay-divergence) — which, once the recovery budget is exhausted, ends the run with [`CORRUPTED_EVENT_LOG`](/docs/errors/corrupted-event-log). |
There was a problem hiding this comment.
Two cross-page consistency points on this sentence:
- Terminology: this says "recovery budget"; the linked replay-divergence page calls it the "retry budget" (and the corrupted-event-log error message exposes
maxRecoveryReplays). Worth picking one term so a reader can match them up. - The linked page now contradicts this section.
docs/content/docs/v5/errors/corrupted-event-log.mdxstill lists as its first common scenario: "Duplicate completion events — Twowait_completedevents for a singlewait_created, or twostep_completedevents for the same step. The first is consumed normally, and the second resolves something already resolved, so no later consumer can claim it." That's the pre-[core] Ignore duplicate events per event class instead of failing the run #3381 behavior — post-[core] Ignore duplicate events per event class instead of failing the run #3381, a second event of an already-consumed class is exactly what gets stepped over (step_terminal,wait_completedare both in the class table above). Its frontmatter summary ("caused by duplicate or orphaned events") and the "such as duplicate or orphaned events" lead have the same problem. A reader following this link lands on a page saying duplicates fail runs. That page isn't in this PR's diff, but the contradiction is created by documenting the new behavior here — worth updating it in this PR or a fast-follow.
There was a problem hiding this comment.
(AI) Both fixed in this PR rather than a fast-follow.
Terminology: replay-divergence.mdx now says "recovery budget", matching this page and maxRecoveryReplays.
The contradiction: corrupted-event-log.mdx loses the "Duplicate completion events" scenario and the "duplicate or orphaned" framing in its summary and lead. Its first scenario is now the case that does still fail, an unclaimed event with no earlier event of its kind to defer to, with a link back to this section. The "Events after terminal state" bullet went too, since step_retrying after step_completed is either read past or an instance of that first scenario.
|
|
||
| - `hook_received`: a hook legitimately receives many payloads under one ID, so a second `hook_received` is not a repeat of anything. | ||
| - `hook_conflict`: hook creation must be idempotent, so getting a `hook_conflict` at any position indicates a failure to acquire the hook and can't come after `hook_created` for the same hook | ||
| - `attr_set` is written on every attribute write. |
There was a problem hiding this comment.
This is the only mention of attr_set on the entire page — the Event Types Reference above stops at Run/Step/Hook/Wait Events and never lists it, so a reader meeting the type here has nowhere on the page to learn what it is. Pre-existing gap, but this bullet is what exposes it. Cheapest fix is a link from the bullet (e.g. to Attributes or the setAttributes() reference); the fuller fix is a Run Attribute Events row in the reference tables above.
There was a problem hiding this comment.
(AI) Linked the bullet to the setAttributes() reference and said what the event records, so the type is no longer a dead end on this page. The Run Attribute Events row in the reference tables is the better fix and is out of scope here.
| --- | ||
| --- | ||
|
|
||
| Document how replay handles duplicate events in the event sourcing guide. |
There was a problem hiding this comment.
Non-blocking, and the PR body already flags this as a deliberate choice — but since @workflow/core's published error-message strings change (and hook.test.ts asserts them verbatim), a patch entry for @workflow/core would let the corrected wording ship in the next beta rather than riding along with the next unrelated core change. Author's call given the instruction to drop #3444's changeset.
There was a problem hiding this comment.
(AI) Added. @workflow/core behavior changes in this PR beyond the message strings (the duplicate-event logging drops to debug), so an empty changeset no longer covers it. .changeset/duplicate-event-log-level.md is a patch for workflow, @workflow/core, and @workflow/world.
| - `run_created` precedes every replay and is always consumed. | ||
| - `run_completed`, `run_failed`, and `run_cancelled` never reach the check. The runtime exits before replaying the workflow body once the log holds one of them, so no consumer ever takes one and no class is ever recorded for them. | ||
|
|
||
| Duplicates are visible in the run's logs. A repeat of the same event type is logged at `info`. A repeat that decides a class differently — a `step_failed` behind a `step_completed`, or the reverse — is logged at `error`. Ignoring it is still correct and still deterministic, but unlike a re-commit of the same outcome there is no reading in which both writers were right, so the discarded outcome is worth surfacing. |
There was a problem hiding this comment.
A repeat of the same event type is logged at
info.
this seems overly noisy if we expect this to happen often @VaguelySerious. if it's inert, we should only log it under debug
There was a problem hiding this comment.
... is logged at
error
let's not do that. there's nothing a user can actually do. at most it's an inert warning. we can either log it as such (but again - this is going to log on EVERY replay no?)
if so, it's maybe better to only log under debug
if it's only going to log once, then we should log as 'warning' and be explicit it's inert
There was a problem hiding this comment.
Yes, let's make it not log unless debug mode is enabled
There was a problem hiding this comment.
(AI) Done in 3a89807, and worth noting the starting point was already half-way there: info is gated behind DEBUG in packages/core/src/logger.ts, so only the error branch ever reached a console. Both branches now log at debug (DEBUG=workflow:runtime:*).
Your read on the frequency is right, and it is the reason the level matters: the duplicate is permanent in the log, so every later replay re-reads it and lands on the same check. Anything unconditional would print once per replay for the life of the run.
|
|
||
| Duplicates are visible in the run's logs. A repeat of the same event type is logged at `info`. A repeat that decides a class differently — a `step_failed` behind a `step_completed`, or the reverse — is logged at `error`. Ignoring it is still correct and still deterministic, but unlike a re-commit of the same outcome there is no reading in which both writers were right, so the discarded outcome is worth surfacing. | ||
|
|
||
| The observability UI greys out the events it can identify this way, which is a narrower set than the runtime's. It reads the log without consumer state, and a consumer for an entity that is still open legitimately claims a repeat — each retry of a step writes another `step_started`. So it marks a repeat only once a terminal event for the same entity sits earlier in the log, which is the point past which no consumer remains. |
There was a problem hiding this comment.
is it just greyed out or de have a tooltip explanation?
also dependiong on how sever this can be (if tehre are parallel fanout cases where the "normal" behaviour means we're going to have, say, 100s of inert duplicate events in the event log - then we shouldn't show them at all and/or group them in the UI and/or have a checkbox to toggle visibility of these inert events)
I'd be weary of it becoming noisy
There was a problem hiding this comment.
also had a tooltip
There was a problem hiding this comment.
(AI) Tooltip confirmed, and the docs paragraph now says so ("with the reason on hover") instead of just "greys out".
On volume: a fan-out that races enough replays can put many inert copies in one log, so the concern is fair. The current UI marks them in place rather than hiding them, which keeps the log a faithful record but does get noisy at that scale. Grouping or a visibility toggle is a UI change in the dashboard rather than anything this PR can carry, so it is better filed as a follow-up than bolted on here.
| * Generated webhook tokens are drawn from the run's deterministic sequence, | ||
| * based on the run ID, the workflow name, and the deployment ID, which are not | ||
| * trivial to guess but should not be considered secure. This is done so URLs stay | ||
| * stable across replays and across concurrent invocations of the same run. | ||
| * |
There was a problem hiding this comment.
Generated webhook tokens are drawn from the run's deterministic sequence, based on the run ID, the workflow name, and the deployment ID, which are not trivial to guess but should not be considered secure
too verbose/detailed for a tsdoc description? maybe just enoughhere to replace both these paragraphs with a permalink to the docs with the explanation of generated tokens?
Learn more about generated webhook tokens and its security contract: https://...
There was a problem hiding this comment.
(AI) Agreed. Both paragraphs are now one line plus a permalink to the hooks guide:
* A generated token is not trivial to guess but is not a security
* contract, so authenticate webhook requests themselves rather than
* relying on URL secrecy:
* https://workflow-sdk.dev/docs/foundations/hooks#token-design
Co-authored-by: Pranay Prakash <pranay.gp@gmail.com> Signed-off-by: Peter Wielander <mittgfu@gmail.com>
…ew feedback - `onDuplicateEvent` logs both branches at `debug`. A duplicate is permanent in the log, so every later replay re-reads it and would re-log it, and there is nothing to act on either way. - Shared fixtures and real-primitive tests for a second `hook_disposed` and a `hook_created` behind a disposal, so both hook classes are pinned on the runtime and observability sides. - Docs: correct the `hook_conflict` bullet (a conflict can follow a creation under a different token claim; its consumer absorbs repeats), state the hook write-path behavior, match the logging paragraph to the code, link `attr_set`, drop the now-wrong duplicate-completion scenario from `corrupted-event-log`, and unify "recovery budget". - Tighten the `HookOptions.token` tsdoc and the hooks Token Design note.
…cing-duplicate-events
|
No backport to The bulk of this commit documents and tunes the duplicate-event replay handling introduced by #3381, which does not exist on To override, re-run the Backport to stable workflow manually via |
2 participants
Follows #3381, which changed replay to step over an event whose class it has already consumed for an entity instead of reporting a divergence that, after the recovery budget, ends the run with
CORRUPTED_EVENT_LOG.The v5 event sourcing guide described only the write-path terminal-state guard. That is a different layer, and it says nothing about the duplicates the write path does permit: a second
step_created/step_started/wait_createdcommitted by an invocation replaying from a stale prefix.What this adds
A
## Duplicate Eventssection after## Terminal States, covering:step_terminalgroupsstep_completed/step_failed; every other class is a single type)hook_received/hook_conflictare deliveries,attr_setis written on every attribute write,run_createdprecedes every replay, and the terminal run types never reach a consumer at allinfovserrorlogging split, so a differently-decided outcome is findable in a run's logsTwo smaller edits: a sentence at the end of
## Terminal Statesseparating the write-path guard from replay-side handling, and a cross-link from the step-lifecycle callout about back-to-backstep_startedevents, which now has a second cause.Also folds in #3444
Closes #3444.
Carries @anir0y's
HookOptions.tokendocstring correction from that PR, without its changeset (this PR's changeset is empty). Verified against the code before copying:createWebhook()rejects an explicittokenoutright (packages/core/src/workflow/create-hook.ts:63), so "tokens are always randomly generated" was describing a path that does not exist.createCreateHookfalls back toctx.generateNanoid()(packages/core/src/workflow/hook.ts:99), whose PRNG is seeded on${runId}:${workflowName}:${deploymentId}(packages/core/src/workflow.ts:353), which is what keeps webhook URLs stable across replays and across concurrent invocations of one run.The old wording attributed webhook-endpoint security to that randomness, so it was wrong in the direction that matters.
The rest of the package described the same token the same wrong way, so the wording is aligned in one pass:
createWebhook()'s throw message no longer says the token is randomly generated, andhook.test.tsasserts the new string. (The message change alone would have failed the unit suite, which asserts the message verbatim.)createHook()empty-token guard and its comment say "a generated one" rather than "a randomly generated one" —createHook()mints its token from the same seeded sequence.resumeWebhook()reference, and the vitest workbench comments drop the same claim. The hooks guide's Token Design note went further and attributed webhook-endpoint security to that randomness, so it now points at authenticating the request (signature header, shared secret, or a check in the handler) instead of relying on URL secrecy.Two things left deliberately untouched:
docs/content/docs/v4/foundations/hooks.mdx:483andv4/testing/index.mdx). It is wrong there too, though for a different seed:stableseeds on${runId}:${workflowName}:${+startedAt}rather than the deployment ID. Fixing it here would drag a v4 change into a PR whose stated scope is v5, and the correction is backport-eligible on its own, so it wants a separate change.patchfor@workflow/coreis arguably warranted. Not adding one unasked, since dropping docs(core): describe webhook token generation accurately #3444's changeset was the instruction.Merged main in
The branch predated #3507, which replaced the fixed
MIN_DEFERRED_CHECK_DELAY_MS * 4sleeps inevents-consumer.test.tswithvi.waitForpolling. Without it,Unit Tests (windows-latest)failed twice here on two of those tests (leaves a duplicate run_cancelled to the parking path,does not track hook deliveries, whose consumers subscribe lazily) while main's Windows lane was green with the identical test file. Merging main in picks up the fix.The merge also brought #3467, which greys out duplicate events in the observability views, so the page now records where the UI's classification is narrower than the runtime's: the UI reads the log without consumer state, so it marks a repeat only once a terminal event for the same entity sits earlier in the log.
Version scope
v5 only. #3381 is on
mainand is not onstable, so the v4 page keeps describing v4 behavior.Docs Preview
🤖 Generated with Claude Code