Skip to content

[runtime] Cover the cross-loader action key failure and document action renaming - #1094

Merged
wenjin272 merged 1 commit into
apache:mainfrom
purushah:fix-action-state-key-stability
Sep 15, 2026
Merged

wenjin272 merged 1 commit into
apache:mainfrom
purushah:fix-action-state-key-stability

Conversation

@purushah

@purushah purushah commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Linked issue: N/A (follow-up to the action-key fix that landed through #1100)

Purpose of change

Durable action-state keys used to derive the action segment from Action.hashCode(). For Java actions that hash folds in JavaFunction's Class<?>[] parameterTypes, and Class.hashCode() is a per-JVM identity hash, so every key changed on restart and recovery never replayed completed actions. The fix, deriving the segment from the plan-unique action.getName(), is already on main through #1100, which was stacked on this branch.

This PR carries what remains: a regression test that reproduces the restart failure, removal of a test that could not detect it, and two clarifications to the upgrade guidance #1100 added.

Behavioral Semantics

Behavioral contracts

  1. Two Actions with the same name whose JavaFunction parameter types are distinct Class objects, loaded by different class loaders, produce the same state key.
  2. Recovery reuses completed results for an action only while its name is unchanged; renaming an action makes its recorded results unreachable and it executes again on the next recovery.

Failure behavior

Nothing in this change raises or changes a runtime path. The test skips, rather than fails, in the rare case that the two Class objects happen to receive equal identity hashes, because there is then nothing to discriminate.

Tests

Contract Test
1, key stable across class loaders ActionStateUtilCrossClassLoaderTest, new
2, renaming semantics documented; pinned indirectly by ActionStateUtilTest.testActionUUIDSegmentDerivesFromActionName, already on main

ActionStateUtilCrossClassLoaderTest loads an Event subclass through two isolating class loaders, which yields two distinct Class objects with different identity hashes, the same effect a JVM restart has on JavaFunction.hashCode(). The loader redefines only the subclass and delegates everything else to the parent, so Action's signature check against Event still passes. Verified against main with the action segment temporarily switched back to Action.hashCode(): the test fails on the action-UUID segment; with main as is, it passes.

ActionStateUtilTest.testSameActionNameYieldsSameKeyAcrossInstances is removed. Both instances share the same Class objects inside one JVM, so it passed before the fix too.

The earlier cross-JVM test that launched two java processes is not carried over. As noted in review, separate JVMs do not guarantee different identity hashes and the test passes on JDK 17 with the old derivation restored, so it added CI time without discriminating the bug.

JAVA_HOME=<jdk17> mvn test -pl runtime -am -Dtest='ActionStateUtil*': 29 tests, 0 failures. mvn spotless:check under JDK 17 passes.

API

No public API or runtime change.

Documentation

  • doc-needed
  • doc-not-needed
  • doc-included

Two edits to the upgrade hint under Exactly-Once Action Consistency in deployment.md: the incompatible key-format change is dated to Flink Agents 0.4, and a sentence states that an action's recorded results survive a code change only while its name is unchanged.

Was this patch authored or co-authored using generative AI tooling?

  • Yes
  • No

Generated-by: Claude Code (Claude Fable 5.1)

🤖 Generated with Claude Code

@github-actions github-actions Bot added doc-not-needed Your PR changes do not impact docs fixVersion/0.4.0 priority/major Default priority of the PR or issue. labels Sep 3, 2026
@purushah
purushah force-pushed the fix-action-state-key-stability branch from ba6d9e9 to 42ef26c Compare September 3, 2026 23:22
@github-actions github-actions Bot added doc-not-needed Your PR changes do not impact docs and removed doc-not-needed Your PR changes do not impact docs labels Sep 3, 2026
@github-actions github-actions Bot added doc-included Your PR already contains the necessary documentation updates. and removed doc-not-needed Your PR changes do not impact docs labels Sep 9, 2026
@purushah
purushah force-pushed the fix-action-state-key-stability branch 2 times, most recently from 730720a to c34d2c0 Compare September 9, 2026 14:29
@github-actions github-actions Bot added doc-included Your PR already contains the necessary documentation updates. and removed doc-included Your PR already contains the necessary documentation updates. labels Sep 9, 2026

@rob-9 rob-9 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! these address my earlier comments.

everything looks good. I've added two non-blocking suggestions below about documenting future action changes and qualifying the separate-JVM test claim.

Comment thread docs/content/docs/operations/deployment.md Outdated
@wenjin272

Copy link
Copy Markdown
Contributor

Thanks for working on this, and apologies—I mistakenly merged #1100 before reviewing and merging this PR, even though it was based on your changes. I preserved your Co-authored-by attribution in the squash commit.

The core action-name-based identifier change is already on main through #1100. Could you rebase this PR onto the latest main and keep the remaining test updates, adapting them to the new key encoder API? We can retain the upgrade guidance already added by #1100 and clarify that the incompatible state-format change starts with Flink Agents 0.4, rather than adding a separate upgrade note.

…on renaming

The name-based action segment landed on main through apache#1100. Carry over
the remaining pieces:

Add ActionStateUtilCrossClassLoaderTest, which loads an Event subclass
through two isolating class loaders so the parameter Class objects get
different identity hashes, the same effect a JVM restart has on
JavaFunction.hashCode(). It fails with the hash-based derivation
restored and passes on main.

Remove testSameActionNameYieldsSameKeyAcrossInstances: both instances
share the same Class objects inside one JVM, so it passed before the
fix too.

Date the incompatible key-format change to Flink Agents 0.4 in the
upgrade hint and state that an action's recorded results survive a code
change only while its name is unchanged.

Generated-by: Claude Code (Claude Fable 5.1)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@purushah purushah changed the title [bug][runtime] Derive durable-state action UUID from the plan-unique action name [runtime] Cover the cross-loader action key failure and document action renaming Sep 14, 2026
@purushah
purushah force-pushed the fix-action-state-key-stability branch from c34d2c0 to 9875cc2 Compare September 14, 2026 20:54
@github-actions github-actions Bot added doc-included Your PR already contains the necessary documentation updates. and removed doc-included Your PR already contains the necessary documentation updates. labels Sep 14, 2026
@purushah

Copy link
Copy Markdown
Contributor Author

Rebased onto main. The action-name derivation is dropped from this PR since #1100 carries it, thanks for preserving the attribution. What remains: the class-loader regression test adapted to ActionStateKeyEncoder via ActionStateTestUtils, removal of the same-JVM test that could not detect the bug, and two edits to the upgrade hint from #1100: the format change is dated to 0.4, and a sentence covers action renaming. The cross-JVM test is not carried over, see the thread on it.

@wenjin272 wenjin272 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the rebasing. LGTM

@wenjin272
wenjin272 merged commit b2aa864 into apache:main Sep 15, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-included Your PR already contains the necessary documentation updates. fixVersion/0.4.0 priority/major Default priority of the PR or issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants