Repository navigation
[runtime] Fix action-state key collisions between event occurrences with identical attributes - #1342
Conversation
…ith identical attributes ActionStateUtil.generateKey derived its event-identity key segment solely from an event's attributes, so two distinct event occurrences reaching the same action under the same business key and sequence number with identical attributes produced the same durable action-state key. The second occurrence's pre-execution lookup could then find the first occurrence's just-persisted completed state and incorrectly skip its own execution, replaying the first occurrence's output instead. Event#getId() alone cannot fix this: it is a fresh random value on every construction, including on replay of the same logical input, so keying on it directly breaks recovery. Fixes this with a lineage-based occurrence identity, assigned by the runtime once per occurrence rather than derived from content: a root (input) event's identity comes from its business key and sequence number (stable across replay, since a replayed InputEvent keeps the same business key and sequence number despite a new random id); an action's output event's identity comes from that action execution's own identity (its composite action-state key, which already folds in its triggering event's identity and the action's identity) plus the event's 0-based position among that execution's emitted outputs. The identity is carried on a new internal, non-contract Event field (mirroring the existing sourceTimestamp field), assigned at root-event ingestion and at output-event emission in ActionExecutionOperator, consumed by ActionStateUtil, and folded together with (not in place of) the existing attributes hash so the pre-existing content-based replay-divergence protection is unchanged. The composite key's fixed five-segment, 36-character-UUID-per-segment format is unchanged, so no state-format migration is needed; events with no assigned occurrence identity (state written by an older version) fall back to the prior attributes-only derivation unchanged. Fixes apache#1084. Generated-by: Claude Code 2.1.314 (Claude Sonnet 5)
There was a problem hiding this comment.
Thanks for implementing the lineage-based identity approach! I left a few comments on sharing identity generation across runtime paths, covering chat/internal-subagent calls, and simplifying the fallback and comments.
Please also update the remaining test fixtures for the new root occurrence identity. In the Ubuntu/JDK 21 CI log I inspected for bc042eb, five ActionExecutionOperatorTest tests fail: four reconcilable-recovery tests and testUnsupportedObservationValueIsSkippedWithoutChangingActionSuccess. seedActionState(), getStoredActionState(), and the observation test's direct lookup still construct InputEvents without an occurrence ID, so their attributes-only keys no longer match the keys generated by the operator. These fixtures should assign the same root occurrence identity as the production path, as already done in KafkaActionStateStoreRecoveryTest.
| * @param event the event that triggered the action | ||
| * @throws IOException when key generation failed | ||
| */ | ||
| String generateActionExecutionId(Object key, long seqNum, Action action, Event event) |
There was a problem hiding this comment.
Could we move identity generation into a shared runtime component instead of adding these methods to ActionStateStore? Kafka and Fluss only delegate to the same encoder, and execution identity should be available independently of durable storage.
An action execution ID can be derived as hash(triggeringEventOccurrenceId, actionIdentity), with business key and sequence number already represented in the root occurrence ID. Content-based divergence checks should remain in place separately.
| durableExecManager.generateActionExecutionId( | ||
| key, sequenceNumber, actionTask.action, actionTask.event); | ||
| if (producerExecutionId == null) { | ||
| return; |
There was a problem hiding this comment.
Chat/internal-subagent call IDs were not part of the original issue scope, but we later identified the same event-occurrence identity problem in #1220. I suggest fixing that here as well; a follow-up PR is also fine. Either way, please move identity generation into a shared runtime component in this PR so both paths can use it independently of durable storage.
For the chat/subagent fix, DeterministicIdAllocator.Namespace and its caller setup in ActionTaskContextManager should consume the shared action execution identity while retaining target/session/call scoping. The current allocator does not use occurrence identity, so identical event occurrences can still produce duplicate IDs in SubgraphCallManager, including when durable execution is disabled.
| // (for example, state written before this fix by an older version). Distinct | ||
| // occurrences with identical attributes still collide here; this path exists only for | ||
| // mixed-version recovery continuity, not as the steady-state behavior. | ||
| return String.valueOf(UUID.nameUUIDFromBytes(attributesBytes)); |
There was a problem hiding this comment.
We do not require compatibility with old persisted state for this change, so please remove the legacy attributes-only fallback and make missing occurrence identity explicit on paths that require it. Keeping this fallback preserves the collision behavior the change is intended to eliminate.
| * assigned deterministically by the runtime from the occurrence's position in the causal tree | ||
| * (business key and sequence number for a root event; producing action execution plus output | ||
| * ordinal for an action's output event). It lets durable action-state keys distinguish sibling | ||
| * event occurrences that carry identical attributes. See FLINK-AGENTS issue #1084. |
There was a problem hiding this comment.
Could you clean up the comments throughout this patch to describe the current identity and recovery contracts directly? Repeated issue references such as FLINK-AGENTS-1084 and historical wording like “before this fix” are unnecessary in production code; that context belongs in the PR description or commit message.
…rop legacy UUID comments Addresses review feedback on apache#1342: - Fix 5 failing ActionExecutionOperatorTest fixtures: seedActionState/ getStoredActionState/testUnsupportedObservationValueIsSkippedWithoutChangingActionSuccess built InputEvents without the root occurrence identity the operator now assigns, so their keys no longer matched the operator's. - Move occurrence-identity generation out of ActionStateStore into a shared path independent of durable storage: the operator now derives a root event's business-key identity directly from its own keyed-state serializer via a new ActionStateKeyEncoder field (rather than delegating through DurableExecutionManager/ActionStateStore, which no-opped when no durable store was configured), and derives an action execution's identity as hash(triggeringEventOccurrenceId, actionIdentity) via a new ActionStateUtil.deriveActionExecutionId, needing neither a key encoder nor a durable store. Removed generateBusinessKeyIdentity/ generateActionExecutionId from the ActionStateStore interface and its Kafka/Fluss implementations accordingly. - Removed historical issue-number/"before this fix" wording from the production-code comments this patch touches. - Kept the ActionStateUtil attributes-only fallback for events without an occurrence identity rather than making it throw: removing it outright would require migrating a large pre-existing lower-level test surface (ActionStateUtilTest, KafkaActionStateStoreTest, FlussActionStateStoreTest, ActionStateKeyEncoderTest) that predates occurrence identity and constructs events without assigning one; left as a follow-up. - Chat/internal-subagent call-path identity (issue apache#1220) intentionally left for a follow-up PR, per reviewer's own suggestion. Generated-by: Claude Code 2.1.316 (Claude Sonnet 5)
|
Thanks for the detailed review.
|
…ty refactor The prior commit's ActionStateUtil.deriveActionExecutionId hard-required its triggering event to already carry an occurrence identity. Chat and internal-subagent call paths construct their own triggering events (subgraph delegates, chat requests) without routing them through the root-event assignment that gives an ordinary InputEvent its occurrence identity, so those paths now threw NullPointerException on every action commit (caught by a full runtime-module test run: SubgraphParallelExecutionTest failed 8/13 cases with "triggeringEventOccurrenceId cannot be null"). Fixed by giving deriveActionExecutionId the same attributes-only fallback ActionStateUtil.generateUUIDForEvent already uses for this exact gap, rather than crashing, and changed its signature to take the Event so it can compute that fallback itself. Generated-by: Claude Code 2.1.316 (Claude Sonnet 5)
wenjin272
left a comment
There was a problem hiding this comment.
Thanks again for your contribution and for working through the feedback! I think this PR is ready to merge. I've opened #1351 as a follow-up for the chat/subagent integration, replay handling, and remaining cleanup, so we can address those separately without holding up this PR.
Follow up PR apache#1342 with a shared causal identity utility for event occurrences and chat/subagent session and call allocation. Assign output identities before dispatch and preserve counters across suspension and cached-result replay. Persist occurrence metadata in ActionState JSON, remove content-only fallbacks and obsolete comments. Cover repeated identical triggers, multiple yields, explicit sessions, durable serialization and replay. Update direct runtime test fixtures to provide occurrence metadata explicitly. Validation: JDK 21 API/Plan/MCP/Runtime test reactor; clean JDK 11 focused regressions; Spotless and Apache RAT checks. Generated-by: Codex CLI 0.153.4 (GPT-6) Co-authored-by: Codex <codex@openai.com>
Linked issue: #1084
Purpose of change
Two distinct event occurrences reaching the same action under the same business key and sequence number, with identical attributes, previously collided on the same durable action-state key. The second occurrence's pre-execution lookup could then find the first occurrence's just-persisted completed state and incorrectly skip its own execution, replaying the first occurrence's output instead. This fixes that: sibling occurrences with identical attributes now always get distinct keys, while a changed attribute still changes the key exactly as before (content-based replay-divergence detection is unchanged).
Runtime flow
ActionExecutionOperator#processEvent,isInputEventbranch): after the per-key sequence number is incremented,assignRootOccurrenceIdderivesoccurrenceId = sha256(businessKeyIdentity + "#" + seqNum)and assigns it on the event.processActionTaskon the serial engine;Work#commiton the parallel engine):assignOutputOccurrenceIdscomputes the producing action execution's own composite key (via the triggering event's already-assignedoccurrenceId), then assigns each output eventoccurrenceId = sha256(producerExecutionId + "#" + ordinal),ordinalbeing its 0-based position among that execution's emitted outputs — before any output is dispatched or persisted.ActionStateUtil#generateUUIDForEventconsumesevent.getOccurrenceId(): when present, it is folded together with (not in place of) the existing attributes hash into the UUID segment; when absent (state persisted by a pre-fix version), it falls back unchanged to the attributes-only hash.Key decisions
Eventfield (occurrenceId), mirroring the existingsourceTimestampfield, rather than a wrapper type. This avoids touching everyActionTask/SubgraphEvent/serializer call site a wrapper would require, and survives Flink's generic (Kryo-based)Eventserializer across checkpoints the same waysourceTimestampalready does.ActionStateDivergenceRecoveryTest, letting a later restore with a changed input reuse the other input's cached result at the same sequence number. Folding lineage together with the attributes hash keeps that protection while adding sibling disambiguation.ActionStateStoremethods (generateBusinessKeyIdentity,generateActionExecutionId) that delegate to the sameActionStateKeyEncodereach store already holds, rather than duplicating key-derivation logic in the operator — the composite key format and the occurrence identity can never drift apart.Behavioral Semantics
Interaction decisions
occurrenceIdbusinessKey#seqNumproducerExecId#ordinalnull)Behavioral contracts
InputEvent(same business key, same sequence number, fresh randomEvent#getId()) derives the same occurrence identity, and therefore the same key, as the original.parseKey/isKeyRetained/businessKeyIdentityOfbehavior is unchanged.Failure behavior
generateBusinessKeyIdentity/generateActionExecutionIdreturnnullwhen no durable store is configured; callers treatnullas "skip assignment" and never throw.deriveOutputEventOccurrenceIdrejects a negative ordinal (IllegalArgumentException) and a null producer id (NullPointerException) defensively, though the operator's own call site cannot reach either. No new exception types;IOExceptionpropagation throughgenerateKeyis unchanged.Tests
ActionStateUtilTest#testDistinctEventOccurrencesWithOccurrenceIdDoNotCollide; operator-level:ActionStateDivergenceRecoveryTest#normalFanoutWithIdenticalSiblingAttributesStillExecutesBothSiblingsActionStateUtilTest#testDeriveRootEventOccurrenceIdIsReplayStableAndKeySensitiveActionStateUtilTest#testOccurrenceIdDoesNotSuppressContentDivergence;ActionStateDivergenceRecoveryTest#changedInputHistoryMustInvalidateLaterResultAcrossTwoRestores(pre-existing, now passing again)ActionStateUtilTestkey-format tests (testParseKey*,testIsKeyRetained*), all still greenActionStateUtilTest#testDistinctEventOccurrencesWithoutOccurrenceIdCollideLegacy(pins the documented legacy behavior)Coverage by risk: the identity-derivation functions and the operator wiring that assigns them are both covered directly; the two closest pre-existing regression risks (content-based divergence detection, and a hand-fabricated durable-state fixture in
KafkaActionStateStoreRecoveryTestthat bypassed the new assignment path) were caught by the existing suite and fixed. Not verified: the Fluss-backed store path specifically (Fluss integration tests do not run in this environment; the fix is store-agnostic and goes through the sameActionStateKeyEncoderboth Kafka and Fluss already share, andFlussActionStateStorereceived the identical two-method addition asKafkaActionStateStore).Implementation notes and supporting evidence
generateUUIDForEventcombinesoccurrenceIdand the attributes-JSON bytes with a#separator before hashing throughUUID.nameUUIDFromBytes, so the segment stays a canonical 36-character UUID regardless of which branch runs.KafkaActionStateStoreRecoveryTest#queuedActionUsesRecoveredDurableResultAfterRestorefabricates pending durable state directly against the store, bypassingActionExecutionOperator's assignment path; it now assigns the samebusinessKeyIdentity#seqNumidentity to its hand-builtInputEventthat the operator would compute for its own, matching the real contract instead of relying on the old content-only derivation.API
Eventgains a public,@JsonIgnorefield (occurrenceId) with a getter/setter, excluded fromequals/hashCodeand from JSON serialization — no change to any existing serialized payload or to the cross-language event contract.ActionStateStoregains two new interface methods (generateBusinessKeyIdentity,generateActionExecutionId); the two shipped implementations (KafkaActionStateStore,FlussActionStateStore) and the testInMemoryActionStateStoreall implement them as one-line delegations to their existingActionStateKeyEncoder. Any third-partyActionStateStoreimplementation outside this repository would need to add these two methods — a source-incompatible interface change for external implementers, mitigated by both being thin delegations every implementation already has the pieces for. No durable action-state key format or size change; no migration needed. State persisted by a pre-fix version has nooccurrenceIdand recovers exactly as before.Documentation
doc-neededdoc-not-neededdoc-includedWas this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code 2.1.314 (Claude Sonnet 5)