Preserve sub-orchestration parent link across ContinueAsNew - #31
Conversation
| None, | ||
| history_mgr.parent_instance.clone(), | ||
| history_mgr.parent_id, | ||
| Some(carry_forward_events.clone()), |
There was a problem hiding this comment.
Can we wire this into WorkItem::ContinueAsNew event directly instead of pulling from the history?
Needs to be backcompat though.
There was a problem hiding this comment.
Done in 4f9d29c. The parent link (parent_instance/parent_id) is now stamped directly onto WorkItem::ContinueAsNew by the CAN producer in execution.rs, and the consumers read it from the work item.
For backcompat, both new fields are #[serde(default)] + skip_serializing_if, so old enqueued messages deserialize cleanly and the wire format is unchanged. To stay correct during rolling upgrades (a CAN enqueued by an older node that didn't stamp the fields), the consumers fall back to deriving the link from the previous execution's history when the fields are absent. Added test_workitem_reader_can_preserves_parent_link_from_work_item for the direct path and kept ..._from_history for the fallback path.
A sub-orchestration that called continue_as_new() lost its parent link: the next execution started with parent_instance/parent_id/parent_execution_id set to None, so when it eventually completed or failed the parent was never notified and hung forever waiting on the child. Stamp the parent link onto WorkItem::ContinueAsNew when the runtime enqueues it, and read it back when the next execution's OrchestrationStarted event is created. Consumers fall back to the previous execution's history when the work item carries no parent fields, so ContinueAsNew messages enqueued by older runtimes keep working during a rolling upgrade. The new work item fields are Option and marked #[serde(default)] with skip_serializing_if, keeping the wire format backward and forward compatible. Adds unit coverage for both the work-item and history-fallback paths, plus an end-to-end test asserting a parent observes completion of a child that continued as new.
78d3716 to
8d38ba9
Compare
affandar
left a comment
There was a problem hiding this comment.
Thanks — wiring the parent link onto WorkItem::ContinueAsNew is exactly what I was after, and the write path in execution.rs looks right. Two things on the read path, plus a test ask.
| None, | ||
| None, | ||
| None, | ||
| parent_instance.clone().or_else(|| history_mgr.parent_instance.clone()), |
There was a problem hiding this comment.
Let's not keep the history fallback here.
As written there are two sources of truth for the parent link, and the or_else hides which one actually supplied it. If we ever stop stamping the field correctly, this quietly papers over it and we don't find out until something subtler breaks downstream.
I'd prefer just:
parent_instance.clone(),
parent_id,If an older node enqueued the ContinueAsNew without these, None is the right answer. That's exactly today's behaviour, so it isn't a regression — just a case that stays unfixed during the rolling-upgrade window. I'd rather have a narrow, honest gap than a second code path that's hard to test and easy to get subtly wrong.
Same change needed in the poison path in orchestration.rs.
There was a problem hiding this comment.
Done in 3b7fdb5 — the or_else is gone, this reads the work item directly and None when absent.
You were right to push on this, and for a sharper reason than the review assumed: the fallback wasn't just a redundant second source of truth, it was masking a bug in the write path. Removing it made sample_sub_orchestration_continue_as_new_fs fail deterministically.
HistoryManager::append() pushes to the delta but doesn't refresh the cached parent_instance/parent_id/parent_execution_id fields — those are only populated by from_history() from persisted history. When a child starts and continues-as-new within the same turn (which is exactly what that test does — LoopChild CANs immediately on input 0), the dispatcher appends OrchestrationStarted to the delta, so at CAN time the cached fields are still None and the work item shipped an empty parent link.
So the field was never actually being stamped for a first-turn CAN, and or_else(history_mgr.parent_instance) was quietly re-deriving it on the read side. Two code paths, and the one I claimed was authoritative was dead.
Fixed in execution.rs by sourcing the link from parent_link (from extract_context(), which does scan the delta) rather than the cached metadata. There's now exactly one source of truth and it's the one that works.
| None, | ||
| // Prefer the parent link carried on the work item; fall back to the | ||
| // previous execution's history for messages enqueued by older runtimes. | ||
| parent_instance.clone().or_else(|| history_mgr.parent_instance.clone()), |
There was a problem hiding this comment.
Same as state_helpers.rs — drop the or_else history fallback and read the work-item fields directly, None when absent.
There was a problem hiding this comment.
Done — same change here, work-item fields read directly, no history fallback.
| /// runtimes won't carry this, in which case the runtime falls back to | ||
| /// deriving the parent link from the previous execution's history. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| #[serde(default)] |
There was a problem hiding this comment.
Doc needs to move with the code change above. "in which case the runtime falls back to deriving the parent link from the previous execution's history" should become something closer to:
Absent on messages enqueued by older runtimes, in which case the parent link is not preserved for that execution.
Better to state the consequence plainly than imply it's handled.
There was a problem hiding this comment.
Done — reworded to your suggestion:
Absent on messages enqueued by older runtimes, in which case the parent link is not preserved for that execution.
Applied to all three fields.
| /// continue-as-new alongside `parent_instance`. Optional for the same | ||
| /// backward-compatibility reason described above. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| #[serde(default)] |
There was a problem hiding this comment.
Can we add a serde test pinning the wire contract these Option fields rely on? Same shape as tests/parent_execution_id_serde_tests.rs, which just landed on main via #28:
Noneomits the key entirely — so aContinueAsNewfrom a root orchestration serializes byte-identically to the previous release- JSON without the keys decodes to
None - A pre-upgrade decoder tolerates the added keys
Right now the backcompat claim rests entirely on skip_serializing_if + default, with nothing pinning it. Drop those attributes and every existing test still passes while old nodes start seeing "parent_instance":null on the wire.
There was a problem hiding this comment.
Added tests/continue_as_new_parent_link_serde_tests.rs, same shape as parent_execution_id_serde_tests.rs:
Noneomits all three keys entirely- JSON without the keys decodes to
None - A
PreUpgradeWorkItemmirror (the variant minus the three fields) decodes a new payload — pins that an old node tolerates the added keys - Plus a byte-identity assertion: a root-orchestration
ContinueAsNewfrom a new node serializes to exactly what the pre-upgrade shape emits
Your point about dropping the attributes and everything still passing was the right test of the test — that's what (1) and (4) are guarding.
I also flipped the now-obsolete ..._from_history unit test to test_workitem_reader_can_without_parent_link_does_not_infer_from_history, which asserts None even when history does carry a parent link. That pins the removed fallback so it can't creep back in.
The read paths now treat WorkItem::ContinueAsNew as the single source of truth for the parent link, with no or_else fallback to the previous execution's history. Absent fields mean None, matching pre-existing behaviour during the rolling-upgrade window. Removing the fallback exposed a real bug in the write path. HistoryManager::append() pushes to the delta but does not refresh the cached parent_instance/parent_id/parent_execution_id fields, which are only populated by from_history() from persisted history. When a child starts and continues-as-new within the same turn, the dispatcher appends OrchestrationStarted to the delta, so the cached fields are still empty and the CAN work item shipped an empty parent link. The or_else was silently covering for it. The CAN producer now reads parent_link, which comes from extract_context() and accounts for the delta. Also adds a serde test pinning the wire contract the new Option fields rely on, and flips the obsolete unit test to assert the link is not inferred from history.
|
All four points addressed in 3b7fdb5. Heads-up on one of them: dropping the The CAN producer now reads Validation: |
Preserves the parent link when a sub-orchestration rolls over with ContinueAsNew, so the logical child can still notify its parent when a later execution completes or fails.
Summary:
Validation: