fix: resolve node ID collisions made by a memoized resubmit

[Joibel]

• Ran make pre-commit -B
• Signed-off commits with Conventional Commit messages
• PR title is a conventional commit message (it becomes the release notes entry)
• Unit or e2e tests cover the change
• For features: an associated issue and a feature description file (make feature-new)
• Opened as draft; will mark "Ready for review" once builds are green

Follow-up to #16883, which fixed the controller side of the same problem.

Stacked on #17027, which unbreaks main; the first commit here is that PR. Rebasing once it merges leaves one commit.

Motivation

#16883 made the controller tolerate node ID hash collisions: a name that loses the 32-bit slot to another name is stored under the widened NodeID64 instead, and ResolveNode looks in both places. Memoized resubmit was left alone, and it is the one place that rehashes an entire existing graph.

Resubmit gives the workflow a new name. A node ID is a hash of the node name, which starts with the workflow name, so every ID changes: a collision the original workflow had dissolves, and one it never had can appear. convertNodeID returned newWf.NodeID(name) unconditionally, so under a new collision:

• the node whose name lost the race is silently dropped from the new status, the same graph corruption fix(controller): resolve node ID hash collisions instead of corrupting the graph #16883 fixed in the controller; and
• a reference to a node the reset plan deleted can be mapped onto the ID of a node that is kept, which then survives the pruning applyResetPlan does and leaves a false edge to an unrelated node.

Rare, but a resubmit picks a fresh random name each time, so it is reachable by chance rather than by anything the user did.

Modifications

convertNodeID is replaced by newNodeIDs, which works the whole mapping out before the plan is applied:

• nodes are assigned in node name order, so the result does not depend on map iteration order, which applyResetPlan has;
• a name whose 32-bit slot has already gone to another name is widened with NodeID64, matching what the controller does when it creates a node, so ResolveNode finds it afterwards;
• nodes the plan deletes are mapped too, so a reference to one cannot alias a node that is kept;
• a name that collides at both widths is refused with an error rather than corrupting the graph, as initializeNode refuses it.

Verification

Two unit tests. TestNewNodeIDsWidensACollisionMadeByTheRename uses a pair of withItems sibling names that do not collide under the old workflow name but do under the new one (found by search, since the hash is fixed), and checks that nothing is lost and both nodes are resolvable by name afterwards. TestNewNodeIDsKeepsDeletedNodesOutOfTheWay checks every node is mapped to a distinct ID. An end-to-end test through FormulateResubmitWorkflow is not possible: it picks the new name at random, so a collision cannot be forced.

go test ./workflow/util/... and the resubmit/retry tests in workflow/controller and server/workflow pass; golangci-lint run ./workflow/util/... is clean. Full make pre-commit -B not run — codegen needs the checkout at its $GOPATH path and this was built in a worktree; nothing here is generated.

Documentation

Not needed, no user-visible behaviour or configuration changes.

AI

Claude Code (Claude Opus 5) found the gap while analysing the CI failure fixed in #17027, and wrote the change, the tests and the commit message. The colliding name pair was found by brute-force search over FNV-1a 32.

Summary by CodeRabbit

• Bug Fixes
  • Improved workflow resubmission and retry handling when renamed nodes produce identifier collisions.
  • Ensured node identifiers remain unique, including when reset plans remove nodes.
  • Added fallback identifier widening for collisions and clearer handling when both identifier sizes conflict.

[codecov]

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.77%. Comparing base (5642521) to head (bc08dfa).

Files with missing lines	Patch %	Lines
workflow/util/util.go	91.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17028      +/-   ##
==========================================
+ Coverage   44.75%   44.77%   +0.02%     
==========================================
  Files         575      575              
  Lines       39591    39610      +19     
==========================================
+ Hits        17717    17734      +17     
- Misses      21872    21874       +2     
  Partials        2        2              

Flag	Coverage Δ
unit-tests	44.70% <91.66%> (+0.02%)	⬆️
unit-tests-windows	31.71% <91.66%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: argoproj/argo-workflows/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: cd403793-83f9-42d4-b8d0-3b3f30da769b

📥 Commits

Reviewing files that changed from the base of the PR and between 5642521 and bc08dfa.

📒 Files selected for processing (2)

• workflow/util/node_id_collision_test.go
• workflow/util/util.go

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

Memoized resubmission now precomputes unique node IDs after renaming. It widens colliding IDs to 64-bit values, rejects collisions at both widths, and drops references to nodes absent from status. New tests cover rename collisions and reset-plan deletions.

Changes

Memoized resubmit node ID mapping

Layer / File(s)	Summary
Precomputed collision-safe node IDs workflow/util/util.go	FormulateResubmitWorkflow computes renamed node IDs before memoization. mapID uses the computed values, and missing status nodes produce empty IDs for reset-plan removal.
Collision and reset-plan coverage workflow/util/node_id_collision_test.go	Tests verify 32-bit collision widening, unique mappings when nodes are deleted, and successful node name resolution.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Suggested reviewers: isubasinghe

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary change: fixing node ID collisions during memoized resubmission. It also follows the Conventional Commit format.
Description check	✅ Passed	The description covers the motivation, modifications, verification, documentation impact, AI use, related issues, tests, and lint results. It notes that the full pre-commit check was not run and inclu…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}