Fix repeated Deep Agents tool calls with identical arguments - #1806
Open
1fanwang wants to merge 15 commits into
Open
Fix repeated Deep Agents tool calls with identical arguments#18061fanwang wants to merge 15 commits into
1fanwang wants to merge 15 commits into
Conversation
Signed-off-by: 1fanwang <1fannnw@gmail.com>
…ct-tool-call-cache
There was a problem hiding this comment.
🟡 Changes recommended
The cache-key change needs workflow patching to preserve deterministic replay of existing histories.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes duplicate Deep Agents tool calls being incorrectly served from cache.
Changes:
- Includes tool call IDs in cache keys.
- Adds regression coverage.
- Documents corrected behavior.
File summaries
| File | Description |
|---|---|
temporalio/contrib/deepagents/workflow.py |
Updates tool-result caching. |
tests/contrib/deepagents/test_tools.py |
Tests repeated identical calls. |
CHANGELOG.md |
Records the fix. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ct-tool-call-cache
Replaying a history recorded under the old (name, args) key with the new key raises TMPRL1100: the previously-deduped second call now emits an Activity command the history does not contain (reproduced with a recorded history and Replayer). patched() keeps old histories on the old key while new executions run repeated calls independently.
tool_call_id in the activity input is a per-invocation workflow.uuid4() nonce (nothing supplies __tool_call_id__), so keying the cache by it made every entry permanently unhittable: repeats ran, but cross-continue-as-new reuse died and the carried snapshot grew without bound. Key by (tool identity, per-run occurrence index) instead: replay and post-CAN runs regenerate the carried conversation's calls in order, so the same keys reproduce — repeats each get their own Activity AND completed calls are still reused after continue-as-new. Occurrence counters reset with the cache in set_result_cache. Patch id renamed accordingly (the prior id was never released). Also: the test fixture now honors run_deep_agent's (input, state_snapshot=None) signature contract, and a new test pins key regeneration across a simulated snapshot carry.
Contributor
|
Thanks for the find - this is a real bug and a good catch. It's true that under However, it looks like there were two latent problems in the original approach, both verified empirically - and now fixed on this branch:
Also merged latest |
The same served-stale-result bug existed at both sibling dispatch seams: a repeated identical backend op returned the first op's cached result (a read after an intervening write saw stale contents; a repeated execute never ran), and a repeated identical live model call was served the first response, defeating deliberate resampling. All three dispatchers now share one occurrence-keyed helper behind a single patch id (deepagents.cache-key-per-occurrence; the earlier tool-only id was never released). Regression e2e per seam: read-write-read sees fresh state with three backend_op activities, and two identical prompts produce two invoke_model activities with distinct responses. Record-and-replay verified: a main-recorded old-key history replays cleanly, and a new-code history round-trips.
Under max_cached_workflows=0 the workflow re-registers a fresh MutatingBackend on every replayed activation, so an instance-level write counter could be reset between the write activity and the second read (observed on the faster CI cells: read-at-write-count:0 twice). Keep the fake's state on the class, as real backends keep theirs externally.
The previous commit's class-level counter did not fix the replay race: under max_cached_workflows=0 each replayed activation constructs the fake in a FRESH sandbox, whose re-imported module carries its own copy of the class, so a replay landing between the write and the second read re-registers an instance with reset in-memory state (reproduced locally: read-at-write-count:0 twice). Disk state survives sandbox re-imports and re-registrations, exactly as a real backend's external state does.
There was a problem hiding this comment.
🟡 Changes recommended
Resetting occurrence counters can still return stale cached results after continue-as-new and during migration from old cache keys.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
Review follow-up: resetting occurrence counters at each continue-as-new reintroduced cross-boundary staleness — the continued run resumes from the carried transcript and never re-executes prior dispatches, so a genuinely new identical call computed occurrence 0 and hit the previous run's entry, and the carried payload-only entries from pre-upgrade runs were orphaned either way. The coherent end state: new executions do not cache at all (repeated identical calls are legitimate work; replay needs no cache because history supplies recorded results), and the snapshot stops carrying the cache. The patch gate now selects between legacy-cache and no-cache so pre-change histories, whose recorded dedup decisions depend on the cache, replay unchanged (re-verified by record-and-replay in both directions). Adds the cross-boundary e2e: an identical backend op on both sides of a real continue-as-new executes on both sides, with the first run's close event pinned to CONTINUED_AS_NEW.
…cy coverage - Patch id renamed to deepagents.retire-result-cache: the old name described superseded semantics, and intermediate-build histories carrying the same marker with per-occurrence meaning would replay nondeterministically under the reused id. The changelog now states that deferring the patch retains the full legacy dedup behavior. - Inbound snapshot cache entries are seeded only on legacy executions; on new executions they were dead weight, and the changelog documents the upgrade-hop behavior (a repeated identical call re-executes rather than reusing a possibly-stale carried result). - The unpatched branch gets executable coverage: a checked-in history recorded on pre-change main (two identical tool calls deduped to one activity) replays through the Replayer with the plugin installed — provenance and deprecate_patch-bounded lifetime documented in the test. The three dispatchers' copy-pasted legacy dance is factored into one _legacy_lookup helper. - Cross-boundary e2e hardened: integer completion gate (lexicographic string compare broke at count 10) and maximum_attempts=1 so the per-attempt disk counter cannot be skewed by activity retries.
…-cache branch - The module docstring, run_deep_agent docstring, and README still promised the retired dedup/cache-carry (a duplicate-side-effect trap for users relying on the documented guarantee); all three now state that repeated identical calls run their own Activities and nothing is deduplicated, with the legacy behavior scoped to pre-change replays. - Second checked-in fixture: a continued-run history recorded pre-change whose post-boundary identical call was served entirely from the CARRIED cache (zero invoke_tool activities) — replaying it exercises the gated rehydrate-and-hit branch end to end, which the first fixture (single-run dedup) never reached. - test_state_snapshot_roundtrip and the CAN test module docstring are annotated as legacy-branch plumbing tied to the patch's deprecate_patch cleanup; the backend-freshness e2e gets the same maximum_attempts=1 hardening round 1 gave the cross-boundary test; the replay-fixture module uses distinct Python symbols with defn-name overrides matching the recorded histories.
Under cache eviction (e.g. max_cached_workflows=0) a backend_op activity scheduled just before the eviction can start after the evicted TemporalBackend wrapper is garbage-collected, and the replay that would re-register the ref only happens once that activity completes. The GC finalizer now retires the entry into a bounded store that the activity's lookup falls back to, instead of dropping it outright.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR: Under
run_deep_agent, the continue-as-new result cache deduped repeated identical calls — a second tool call with the same name+args (likewise a repeated backend op or identical live model call) was served the first call's cached result instead of running its own Activity. This PR retires that cache for new executions: repeats are legitimate work, single-run replay needs no cache (history supplies recorded results), and under resume-from-transcript continue-as-new semantics (#1804) a carried cache entry could only ever serve a stale result. Patch-gated so in-flight histories replay unchanged.What was changed
call_tool,call_model, andcall_backend_opno longer consult or populate the result cache on new executions — every dispatch runs its own Activity. The continue-as-new snapshot stops carrying__temporal_cache__.workflow.patched("deepagents.cache-key-per-occurrence")selects between legacy-cache and no-cache: histories recorded under the old behavior contain dedup decisions (a repeated call answered with no Activity scheduled), so their replay keeps the cache. Verified by recording a dedup history onmainand replaying it through this branch (the ungated version fails with TMPRL1102/1100-class nondeterminism), plus a new-code round-trip.Why?
Dedup was wrong at every seam: a side-effecting tool requested twice ran once (with the second call's ToolMessage carrying the first call's
tool_call_id); a repeatedreadafter an interveningwritereturned stale contents and a repeatedexecutenever ran; identical live model calls defeat deliberate resampling; and post-continue-as-new, a genuinely new identical call could be served the previous run's result.How was this tested
Per-seam regression e2e (two identical tool calls → two
invoke_toolactivities; read→write→read → threebackend_opactivities seeing fresh disk state; two identical prompts → twoinvoke_modelactivities with distinct responses), the cross-boundary e2e (an identical backend op on both sides of a real continue-as-new executes on both sides, first run pinned toCONTINUED_AS_NEW), and record-and-replay verification in both directions. Full deepagents suite: 36 passed.