Skip to content

Resubmit copies node ID hashes and suffixes instead of re-allocating them #16891

Description

@Joibel

Summary

#16883 makes node ID collisions safe during workflow execution: initializeNode and every lookup go through Workflow.ResolveNode, so a name that loses its hash slot gets a recorded hashSuffix and a distinct ID. FormulateResubmitWorkflow (memoized resubmit) predates that and still computes destination IDs by bare hashing while renaming every node (workflow/util/util.go, newNode.ID = newWF.NodeID(newNode.HashName()) and convertNodeID). Two consequences, both found by @thc1006 reviewing #16883:

  1. Copied suffixes become unreachable. The rename changes the hash input, so the collisions that justified the old suffixes dissolve, but the suffixes are copied verbatim. A ~1 node is still found (the resolver tolerates one empty slot ahead of it), but a ~2 node ends up behind two empty slots and is unreachable by name: the controller re-creates it fresh at slot 0 and the copied node is orphaned along with its children. Needs a three-name family in the source workflow.

  2. The rename can mint fresh collisions. Destination IDs are not checked against the map being built, so a new-prefix collision (same birthday odds over the node count as the table on Workflow controller occurs fatal error: stack overflow when specific named workflow is reconciled #16376) silently merges two nodes: Nodes.Set keeps whichever the random map iteration wrote last, and convertNodeID points both nodes' parent references at the survivor. A node is lost outright; depending on where the two sat, the references can close a loop. This is pre-existing on main.

Proposed fix

Resubmit should re-allocate instead of copying:

  • Two passes over the old nodes. Pass 1 walks them in a deterministic order (sorted, so repeated resubmits of one workflow agree) and allocates each new ID through the resolver against the map being built — stale suffixes drop away, fresh collisions gain them — recording old ID → new ID. Pass 2 emits the nodes and rewrites Children, BoundaryID and OutboundNodes through that map.
  • This is safe at resubmit time and only there: the new workflow has no pods, task results or derived pod names yet, so IDs can still be chosen freely. It cannot be retrofitted to live workflows.

One decision to make in the implementation: resubmit drops some nodes (the onExit subtree, unexecuted skips) but currently rewrites other nodes' references to them by bare hash, leaving dangling IDs — which under re-allocation could coincide with a real node's ID. The clean option is to prune references to nodes that do not survive the copy (every walker already tolerates a missing child); the conservative option is to reserve slots for them. The pruning question should be settled in review there, not smuggled into #16883.

FormulateRetryWorkflow is unaffected: it keeps the workflow name, so IDs never change.

Version

main (pre-existing; unchanged by #16883, which scopes itself to execution-time collisions)

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions