fix(agentspan): snapshot assembled messages so a retry keeps conversation history - #1480
fix(agentspan): snapshot assembled messages so a retry keeps conversation history#1480ling-senpeng13 wants to merge 1 commit into
Conversation
manan164
left a comment
There was a problem hiding this comment.
Does this happen with all worker task that we lose input parameters on retry?
7805f7e to
c621c44
Compare
Yes — re-resolving taskToBeRetried.getInputData().putAll(taskInput); // taskToBeRescheduled |
| protected void detachAssembledInputFromDefinition(TaskModel task, String... assembledKeys) { | ||
| try { | ||
| WorkflowTask sharedDefinition = task.getWorkflowTask(); | ||
| if (sharedDefinition == null || sharedDefinition.getInputParameters() == null) { | ||
| return; |
There was a problem hiding this comment.
Can we rename method and define the arguments statically?
private static final Set<String> ASSEMBLED_CHAT_INPUT_KEYS =
Set.of("messages", "tools");
protected final void preserveAssembledChatInputOnRetry(TaskModel task) {
// Existing deep-copy and removal logic using ASSEMBLED_CHAT_INPUT_KEYS.
}
Something like this I feel is more clear
|
|
||
| /** | ||
| * Detach mapper-assembled input keys from the task's carried {@link WorkflowTask} definition so | ||
| * a RETRY or RERUN of the task keeps the assembled values. | ||
| * | ||
| * <p>Mappers like the chat-complete ones build parts of the task input imperatively in Java — | ||
| * conversation history from prior tasks, f-string substituted message text, tool lists — that | ||
| * are not expressible as {@code ${...}} references. Retry/rerun do not re-run the mapper; they | ||
| * copy the task and re-resolve the carried definition's {@code inputParameters} over it, which | ||
| * would overwrite the assembled value with the definition's static template. Removing the | ||
| * assembled keys from a task-owned copy of the definition means re-resolution simply never | ||
| * produces those keys, so the copied attempt's own {@code inputData} — the fully assembled | ||
| * conversation the first attempt actually used — survives untouched. This also avoids ever | ||
| * round-tripping conversation content through the template engine (message text containing | ||
| * {@code $}{...} patterns must never be re-interpreted) and does not grow the task payload. | ||
| * | ||
| * <p>The deep copy is load-bearing: {@code task.getWorkflowTask()} is the shared instance from | ||
| * the cached {@link com.netflix.conductor.common.metadata.workflow.WorkflowDef}; mutating it | ||
| * would leak into every other execution using that definition. New schedules (including | ||
| * DO_WHILE iterations) always map from the definition's own instance, so they are unaffected. | ||
| * | ||
| * <p>Best-effort: a failure must not fail scheduling — the fallback is the pre-existing | ||
| * behaviour (a later retry loses the assembled input). | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * Detach mapper-assembled input keys from the task's carried {@link WorkflowTask} definition so | |
| * a RETRY or RERUN of the task keeps the assembled values. | |
| * | |
| * <p>Mappers like the chat-complete ones build parts of the task input imperatively in Java — | |
| * conversation history from prior tasks, f-string substituted message text, tool lists — that | |
| * are not expressible as {@code ${...}} references. Retry/rerun do not re-run the mapper; they | |
| * copy the task and re-resolve the carried definition's {@code inputParameters} over it, which | |
| * would overwrite the assembled value with the definition's static template. Removing the | |
| * assembled keys from a task-owned copy of the definition means re-resolution simply never | |
| * produces those keys, so the copied attempt's own {@code inputData} — the fully assembled | |
| * conversation the first attempt actually used — survives untouched. This also avoids ever | |
| * round-tripping conversation content through the template engine (message text containing | |
| * {@code $}{...} patterns must never be re-interpreted) and does not grow the task payload. | |
| * | |
| * <p>The deep copy is load-bearing: {@code task.getWorkflowTask()} is the shared instance from | |
| * the cached {@link com.netflix.conductor.common.metadata.workflow.WorkflowDef}; mutating it | |
| * would leak into every other execution using that definition. New schedules (including | |
| * DO_WHILE iterations) always map from the definition's own instance, so they are unaffected. | |
| * | |
| * <p>Best-effort: a failure must not fail scheduling — the fallback is the pre-existing | |
| * behaviour (a later retry loses the assembled input). | |
| */ | |
| /** | |
| * Preserves the chat input assembled during initial task mapping when the task is retried. | |
| * | |
| * A retry copies the previous task input and then resolves the workflow task's input parameters | |
| * again. That would replace the assembled conversation and tools with their original definitions. | |
| * Removing those parameters from the task's private copy of the definition prevents that overwrite. | |
| * | |
| * The workflow task is copied first because its original definition may be shared by other | |
| * workflow executions. | |
| */ | |
There was a problem hiding this comment.
Suggesting this to be more readable and simple explanation of above
| */ | ||
| protected void detachAssembledInputFromDefinition(TaskModel task, String... assembledKeys) { | ||
| try { | ||
| WorkflowTask sharedDefinition = task.getWorkflowTask(); |
There was a problem hiding this comment.
Code is correct here. I might suggest little simpler version:
WorkflowTask definition = task.getWorkflowTask();
if (definition == null || definition.getInputParameters() == null) {
return;
}
Map<String, Object> definitionInput = definition.getInputParameters();
if (ASSEMBLED_CHAT_INPUT_KEYS.stream().noneMatch(definitionInput::containsKey)) {
return;
}
WorkflowTask taskDefinition =
objectMapper.convertValue(definition, WorkflowTask.class);
Map<String, Object> retryableInput =
new HashMap<>(taskDefinition.getInputParameters());
ASSEMBLED_CHAT_INPUT_KEYS.forEach(retryableInput::remove);
taskDefinition.setInputParameters(retryableInput);
task.setWorkflowTask(taskDefinition);
We don't need to catch exception here as well. That's already covered in the calling locations -- can rely on their error-handling I feel
NicholasDCole
left a comment
There was a problem hiding this comment.
Couple suggestions. Please check
c621c44 to
3cbc629
Compare
…on — both mappers, shared mechanism
Supersedes the snapshot approach with its inverse, and extends the fix to
the plain LLM_CHAT_COMPLETE mapper, which has the identical bug (its
getHistory() walks prior turns and f-string substitutes message text —
all imperative, all clobbered by retry's template re-resolution).
Retry and rerun do not re-run task mappers: they copy the task and
re-resolve the carried WorkflowTask definition's inputParameters over it,
overwriting mapper-assembled input with the static template. Instead of
snapshotting the assembled messages INTO a task-owned definition copy —
which round-trips conversation content through the template engine on
retry (text containing ${...} would be re-interpreted) and doubles the
task payload — DETACH the assembled keys from the task-owned copy:
re-resolution then never produces those keys, and the copied attempt's
own inputData, the exact conversation the first attempt used, survives
retry and rerun untouched.
The shared helper lives in AIModelTaskMapper and is called by both chat
mappers; the deep copy protects the cached WorkflowDef's shared instance,
and new schedules (including DO_WHILE iterations) map from the
definition's own instance, so they are unaffected.
Tests: detach semantics, retry simulation (replicating
taskToBeRescheduled's copy+putAll) preserving the conversation, a
negative control documenting the pre-fix clobber, template-pattern
content surviving verbatim, shared-definition immutability, no-op and
null paths. ai+agentspan suites: 1301 tests green.
5a7a242 to
e6eaf3c
Compare
The bug
Retrying an
LLM_CHAT_COMPLETEtask in an agent workflow loses the entire conversation. The retried attempt sees only the static[system, user]template from the workflow definition, not the history the first attempt had.Why it happens
Conversation history is never expressed as a
${...}reference in the workflow definition.AgentChatCompleteTaskMapperassembles it imperatively in Java at scheduling time — walking the workflow's completed tasks, extracting tool results, condensing if the context window demands it — and writes the result into the task'sinputData.Retry doesn't re-run the mapper. It copies the task and re-resolves the definition:
Since the definition's
inputParameterscontain only the static template,putAlloverwrites the assembled conversation with it. The history existed solely in the first attempt'sinputData, and that's what gets replaced.Fix
The mapper strips
messagesandtoolsfrom the task's own copy of the definition before it's persisted, so at retry timegetWorkflowTask().getInputParameters()doesn't have them, the re-resolvedtaskInputintaskToBeRescheduled()doesn't have them, andputAllcan't overwrite the assembled values which is the root cause of this bug.AIModelTaskMapper#detachAssembledInputFromDefinition(TaskModel, String... keys), shared by both chat-complete mappers.Testing