Skip to content

[Python] Durable thread compaction and history providers (ADR 0032) - #59

Draft
Ahmed Muhsin (ahmedmuhsin) wants to merge 24 commits into
mainfrom
python/durable-thread-compaction
Draft

[Python] Durable thread compaction and history providers (ADR 0032)#59
Ahmed Muhsin (ahmedmuhsin) wants to merge 24 commits into
mainfrom
python/durable-thread-compaction

Conversation

@ahmedmuhsin

Copy link
Copy Markdown
Contributor

Summary

Makes core's compaction and history providers work on the durable runtime with no changes to a user's agent. Implements ADR 0032 for Python. .NET is not implemented yet.

An agent that already works in core can be registered with the worker or AgentFunctionApp and gets durable conversation history automatically. An attached CompactionProvider keeps working, and a provider the user chose deliberately (Cosmos, Redis, file) is left alone.

What changed

DurableHistoryProvider backs conversation history with the agent's durable entity state. Because it is an ordinary core HistoryProvider, compaction configured the normal way runs against durable history unchanged.

Zero-configuration registration. The entity substitutes the history provider at construction, which covers both hosts. Nothing configured gets a durable provider, an InMemoryHistoryProvider is replaced while preserving source_id and skip_excluded, and external or service-managed history is left alone. The caller's agent is never mutated.

Workflow context parity (L3). The orchestrator projects full_conversation according to the executor's context_mode and context_filter, then delivers it to the agent entity. Previously a downstream agent received only the last message's text.

The session is persisted. Anything context providers keep in session state was discarded at the end of every turn, including tool approval rules and queued approval requests. The entity now persists the serialized session, which also replaces a hand-rolled serviceSessionId field.

prune_history is an opt-in registration flag that deletes compacted-out messages from durable state. It is lossy, so it is off by default.

Bugs found while building this

  • DurableAgentStateMessage.to_dict() dropped extension_data while from_dict() read it, so compaction annotations were destroyed on every save.
  • History ownership came from the chat client's STORES_BY_DEFAULT alone. Core's rule is that an explicit store wins, so an agent using the Responses API with store=False silently lost its conversation.
  • The per-operation session carried no id, so external providers keyed their storage on a different id every turn.
  • Session state came back as plain dicts after a cold start, because core's type registry is process-local and seeds only Message.

Each failed silently rather than raising, which is why they are called out here.

Testing

Unit tests pass for both packages. Durable Task integration is 40/40 and Azure Functions integration is 42/42, both against real infrastructure.

Three new samples carry integration coverage: compaction on the standalone worker, compaction on Azure Functions, and an external Redis history store.

Two pre-existing integration tests were passing regardless of behavior and are fixed here. test_06 never asserted which branch ran, and test_07 had an unreachable assert behind a bare except: pass.

Follow-ups

Core gaps are recorded in the ADR rather than fixed here. The main one is that the store-rewrite hook is bound to session state rather than to the provider, which ADR-0019 raised as an open question and left unanswered.

.NET parity needs the same message schema fixes. Its existing [JsonExtensionData] property looks like it covers this but does not, since annotations are lost when converting to and from ChatMessage rather than at the JSON boundary.

Adds DurableHistoryProvider, a core HistoryProvider whose store is the agent's durable entity state. Because it is an ordinary provider, a CompactionProvider configured the normal way runs against durable history unchanged.

Compaction is reconciled by message id rather than by position, since strategies may insert messages (summaries) as well as annotate them. That required persisting message ids and making DurableAgentStateMessage serialization symmetric: extension_data was read on load but silently dropped on save, so compaction annotations were destroyed on every turn.

The ADR records the core interface gaps found while doing this.
The '!python/packages/**' negation earlier in the file un-ignored everything beneath it, so the integration test .env files holding endpoints and credentials were staged by a plain 'git add'. A trailing '**/.env' rule wins over that negation; .env.example templates stay tracked.
…session id

Two ways an agent that works in core could silently lose its conversation under the durable runtime, both failing without an error:

- Ownership of history was decided from the chat client's STORES_BY_DEFAULT alone. Core's rule is that an explicit 'store' in the agent's options wins, so an agent using the Responses API with store=False kept a plain in-memory provider that the durable runtime never persists.

- The entity built its per-operation session without an id, so core generated a fresh one each turn. External history providers (Cosmos, Redis, file) key their storage on session.session_id and were therefore reading and writing a different key on every turn.
…ADR 0032

Documents the two rules the fixes above depend on (store precedence over STORES_BY_DEFAULT, and stable session ids for external providers), and restores the entity lifetime/TTL section. TTL is the natural sibling of the retention setting this ADR introduces - the retention rationale already refers to it - and the .NET/Python parity gap it describes belongs in this repository.
…mpaction

Three samples, each showing an agent configured the ordinary core way running durably with no changes: compaction on the standalone worker (13) and on Azure Functions (14), and a user-owned external history store (14, Redis).

The Redis sample defines its own small provider rather than depending on agent-framework-redis, whose only release is a beta that no longer imports against current core.

Integration coverage asserts against real storage: compaction annotations and message ids survive entity serialization, an external provider keeps the whole conversation under one key, and a downstream workflow agent can reference the upstream conversation. Existing continuity tests were strengthened to assert recall rather than a bare 200.
Core documents the per-provider 'state' dict handed to before_run/after_run as durable for the life of the session and persists it through AgentSession.to_dict(). The entity built a fresh session per operation, so everything providers kept there was discarded at the end of every turn: tool approval rules and queued approval requests, todo lists, background-task state, memory extraction state. Nothing failed - agents just silently started over.

That is a poor fit for a runtime whose headline scenario is long-running human-in-the-loop, where an approval flow that spans turns cannot work if the pending requests are dropped between them.

The entity now persists the whole serialized session instead of individual fields, which also removes the hand-rolled serviceSessionId state field and its capture/restore helpers - that id is already part of AgentSession.to_dict(). The durable history provider's own slice is excluded, since it is derived from conversationHistory and would otherwise duplicate the transcript.

Restore applies the stored state onto a session built by the agent's own create_session(), preserving its session type.

Known limitation, recorded in the ADR: core's state type registry is process-local and only pre-registers Message, so to_dict-based values come back as plain data rather than their original class. Core's own state is mostly plain data and its tool-approval accessor takes either form, so this is latent; the fix belongs in core.
Core deserializes session state through a type registry it seeds with exactly one entry (Message); anything else must be registered explicitly, and the registry is process-local. to_dict-based types are never auto-registered - only Pydantic models are, and only as a side effect of serializing. A durable entity routinely restores in a process that never serialized the value, so provider state came back as plain dicts instead of its own classes.

Before restoring, the entity now registers the serializable types already loaded in the process. Nothing is imported from persisted data, so this cannot load code the application has not already loaded itself, and that is sufficient in practice: whoever put a value in the state bag had to import its class to construct it. The walk covers SerializationMixin subclasses and costs tens of microseconds.

Pydantic values in state remain uncovered (they are keyed by class name and walking every BaseModel subclass would be broad and collision-prone). Core seeding the registry with the types it ships would make this unnecessary - register_state_type() is already public and documents cold-start restore as its motivating case.
The gaps section claimed compaction 'bypasses the provider', which overstates it and would not survive review. Only one of CompactionProvider's two hooks is coupled to session state: before_strategy acts on the loaded invocation context and already works for every provider, so external stores do get in-run context bounding. What they do not get is the framework rewriting their store.

Whether that is a defect depends on who owns the store - not rewriting a user's Cosmos container is defensible, but durable entity state is framework-owned, which is what makes it a real problem here rather than a reasonable omission.

It is also unresolved rather than decided: ADR-0019 names three compaction points, scopes in Redis and Cosmos, and leaves the mechanism as an explicit open question that shipped unanswered. The languages then diverged - .NET put store reduction on the provider (IChatReducer, InMemory only; Cosmos has none), Python put it in CompactionProvider reaching into session state - and neither offers it to external providers.

Also corrects the knock-on claims elsewhere in the ADR that both core hooks apply 'unchanged', since L2 in fact carries workaround code, and cross-references the two gaps recorded in other sections.
These gaps are being followed up rather than fixed, so the ADR has to be the durable record. Three were under-captured:

- The per-service-call cadence split was only ever discussed, never written down. Added as gap 4: history providers move to per-model-call while CompactionProvider stays per-run, so compaction annotates after the last flush. Latent (HarnessAgent only), but the symptom would be missing annotations rather than an error.

- The .NET parity note said 'add extension data', which is misleading. .NET already has an ExtensionData property, but it is [JsonExtensionData] - the JSON overflow bucket, not a mapping of ChatMessage.AdditionalProperties. Annotations are lost at the conversion boundary, and MessageId does not exist at all. Anyone auditing for 'is extension data persisted?' would see the property and wrongly close the item.

- Recorded that the store-precedence rule is re-derived here because core does not expose it, that drift would present as silent conversation loss, and that the only real net is the compaction sample rather than the unit tests.
The ADR had grown into two documents in one hat: a forward-looking design decision in the present tense, followed by a retrospective implementation log, with no signal where one ended and the other began. Adds a short orientation note, marks the status accepted (Python implemented, .NET pending), and fixes the Context section's claim that the durable layer benefits from neither hook 'today' - no longer true.

Also notes once that .NET's ChatHistoryProvider and Python's HistoryProvider are the same concept, since the decision sections use one name and the implementation sections the other.

Deduplication: service-managed scope was stated four times and storage-capacity-is-separate five; each now has one home plus pointers. The per-option pros/cons lists restated Decision Outcome almost verbatim and are now one entry per option. Three Cross-Cutting bullets that repeated the drivers and the L1/L2 table are gone, as is the 'Why Option 6 over Option 2' paragraph now covered by the options summary.

Validation was written as intent; it now separates what is actually covered in Python from what is still outstanding, so the .NET gap is visible rather than implied.

549 -> 504 lines, 5267 -> 4877 words, with no information removed.
Punctuation pass over the material added on this branch: the ADR, the three new sample READMEs, and the docstrings, comments and log messages in the new provider, entity and sample code. Em dashes become commas, parentheses or sentence breaks, and semicolons joining independent clauses become separate sentences. Colons are kept only where they label something (Args, Returns, 'Chosen option', 'Workaround') rather than standing in for a conjunction.

Pre-existing text is left alone, so the em dashes still in _models.py, _workflows/context.py, _workflows/orchestrator.py, tests/test_app.py and samples/README.md are untouched - none of those lines are from this branch, and rewriting them would add unrelated churn.

Also fixes an indentation slip introduced while editing a comment in _history_provider.py.
The decision has not been accepted yet, so status goes back to proposed. Deciders and consulted are left blank rather than naming people who have not signed off.

Also reverts one word in the orientation note: it said the design was 'realized' in Python, which implied a settled decision. It is a prototype, which is how the rest of the ADR already describes it.
…hat could not fail

The session state bag was the one change on this branch verified only in-process. Its unit tests keep the session dict in memory, so they cannot show that the blob survives the entity's JSON encoding, that it carries the entity's own session id rather than a per-operation one, or that the durable history slice is really left out. A new test in test_13 reads the entity back from the scheduler and asserts all three against the real payload.

Two pre-existing tests were passing regardless of behavior:

- test_06 test_conditional_branching scheduled one spam email and asserted only that the orchestration COMPLETED, never which branch ran, so it would pass if the condition sent every email down the same path. It now asserts the branch-specific output and covers the legitimate branch too, which a stale comment implied was once intended.

- test_07 test_hitl_orchestration_timeout wrapped the wait in 'except (RuntimeError, TimeoutError): pass'. Since the shared helper raises on FAILED, its assert was unreachable and the test passed on every outcome including a hung orchestration. It now waits on the client directly and asserts the run failed with an approval timeout rather than for some other reason.
Copilot AI review requested due to automatic review settings July 31, 2026 12:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements ADR 0032 for Python by making durable agents/workflows reuse core conversation-history + compaction plumbing (via a durable-backed HistoryProvider), persisting per-session provider state across turns, and forwarding upstream workflow context to downstream agent nodes for parity with in-process execution.

Changes:

  • Add DurableHistoryProvider + automatic history-provider substitution so core compaction runs unchanged against durable entity-backed history (with opt-in prune_history retention).
  • Persist serialized AgentSession (provider state + service conversation id) in durable entity state across turns, excluding the durable history slice to avoid transcript duplication.
  • Add workflow context projection (context_mode / context_filter) into RunRequest.context_messages, plus new/updated unit + integration tests and samples.

Reviewed changes

Copilot reviewed 50 out of 51 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
schemas/durable-agent-entity-state.json Extends durable agent state schema to include persisted serialized session payload.
python/samples/README.md Documents new conversation-history/compaction samples.
python/samples/azure_functions/14_conversation_compaction/requirements.txt Dependencies for the new Azure Functions compaction sample.
python/samples/azure_functions/14_conversation_compaction/README.md Explains durable-backed history + compaction behavior for the Functions sample.
python/samples/azure_functions/14_conversation_compaction/local.settings.json.template Local settings template for the Functions compaction sample.
python/samples/azure_functions/14_conversation_compaction/host.json Durable Functions host configuration for the sample.
python/samples/azure_functions/14_conversation_compaction/function_app.py Functions sample wiring demonstrating durable-backed history + compaction.
python/samples/azure_functions/14_conversation_compaction/demo.http Ready-made HTTP sequence for exercising compaction behavior.
python/samples/14_external_history_redis/worker.py New sample worker hosting an agent whose history is stored in Redis.
python/samples/14_external_history_redis/sample.py Combined worker+client runner for the external Redis history sample.
python/samples/14_external_history_redis/requirements.txt Dependencies for the external Redis history sample.
python/samples/14_external_history_redis/redis_history_provider.py Minimal sample HistoryProvider implementation backed by Redis.
python/samples/14_external_history_redis/README.md Explains external-store history behavior under the durable runtime.
python/samples/14_external_history_redis/client.py Sample client that demonstrates recall via Redis-backed history.
python/samples/14_external_history_redis/.env.example Environment template for the Redis history sample.
python/samples/13_conversation_compaction/worker.py New durabletask sample worker for durable-backed history + compaction.
python/samples/13_conversation_compaction/sample.py Combined worker+client runner for the compaction sample.
python/samples/13_conversation_compaction/requirements.txt Dependencies for the durabletask compaction sample.
python/samples/13_conversation_compaction/README.md Explains durable-backed history + compaction semantics for durabletask.
python/samples/13_conversation_compaction/client.py Sample client validating bounded context + recent recall.
python/samples/13_conversation_compaction/.env.example Environment template for the durabletask compaction sample.
python/packages/durabletask/tests/test_workflow_context_parity.py Unit tests for projecting upstream workflow conversation into downstream runs.
python/packages/durabletask/tests/test_durable_history_provider.py Unit tests for durable-backed history provider + compaction persistence/pruning.
python/packages/durabletask/tests/test_durable_history_autoswap.py Unit tests for auto-swapping history providers without mutating the user agent.
python/packages/durabletask/tests/integration_tests/test_14_dt_external_history_redis.py Integration tests for external-store history continuity + stable session id.
python/packages/durabletask/tests/integration_tests/test_13_dt_conversation_compaction.py Integration tests for durable compaction behavior + session persistence shape.
python/packages/durabletask/tests/integration_tests/test_08_dt_workflow.py Adds assertion that downstream workflow agents receive upstream conversation.
python/packages/durabletask/tests/integration_tests/test_07_dt_single_agent_orchestration_hitl.py Fixes HITL timeout test to assert failure reason instead of swallowing errors.
python/packages/durabletask/tests/integration_tests/test_06_dt_multi_agent_orchestration_conditionals.py Strengthens conditional-branch assertions to validate correct branch output.
python/packages/durabletask/tests/integration_tests/test_01_dt_single_agent.py Makes conversation-continuity test actually depend on persisted history recall.
python/packages/durabletask/agent_framework_durabletask/_workflows/orchestrator.py Adds L3 context projection into agent tasks via context_messages.
python/packages/durabletask/agent_framework_durabletask/_workflows/dt_context.py Routes agent-task creation through shared build_agent_task helper.
python/packages/durabletask/agent_framework_durabletask/_workflows/context.py Extends orchestration-context protocol to accept optional context_messages.
python/packages/durabletask/agent_framework_durabletask/_worker.py Adds prune_history defaults/overrides when registering agents as entities.
python/packages/durabletask/agent_framework_durabletask/_shim.py Introduces build_agent_task + forwards optional context_messages to executors.
python/packages/durabletask/agent_framework_durabletask/_models.py Adds RunRequest.context_messages wire field with (de)serialization.
python/packages/durabletask/agent_framework_durabletask/_history_provider.py New durable-backed HistoryProvider + auto-swap logic and compaction reconciliation.
python/packages/durabletask/agent_framework_durabletask/_executors.py Extends run-request construction to carry orchestration id + optional context messages.
python/packages/durabletask/agent_framework_durabletask/_entities.py Switches entity execution to use core context pipeline + persists session + dedupes upstream context.
python/packages/durabletask/agent_framework_durabletask/_durable_agent_state.py Adds persisted session + message id + extension metadata to durable state entries/messages.
python/packages/durabletask/agent_framework_durabletask/_constants.py Adds new durable state field constants for message id + session.
python/packages/durabletask/agent_framework_durabletask/init.py Exports newly added durable history + task-building utilities.
python/packages/azurefunctions/tests/test_app.py Updates tests for entity creation signature to include prune_history.
python/packages/azurefunctions/tests/integration_tests/test_14_conversation_compaction.py New integration coverage for Functions-hosted durable compaction sample.
python/packages/azurefunctions/tests/integration_tests/test_01_single_agent.py Strengthens Functions continuity test to require history-based recall.
python/packages/azurefunctions/agent_framework_azurefunctions/_workflow_af_context.py Uses shared build_agent_task and updates protocol signature for context messages.
python/packages/azurefunctions/agent_framework_azurefunctions/_orchestration.py Aligns orchestration id propagation via executor hook instead of overriding get_run_request.
python/packages/azurefunctions/agent_framework_azurefunctions/_entities.py Adds prune_history option when creating agent entities.
python/packages/azurefunctions/agent_framework_azurefunctions/_app.py Adds app-level + per-agent prune_history plumbing through entity setup.
docs/decisions/0032-durable-thread-compaction.md Adds ADR 0032 describing the approach and Python prototype notes.
.gitignore Ignores .env files repo-wide while keeping .env.example tracked.

Comment on lines +366 to +369
deduped = [m for m in messages if not m.message_id or m.message_id not in known_ids]
if not deduped and messages:
return [messages[-1]]
return deduped

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 002b9ef.

The kept copy now drops its message id, so it no longer collides with the copy already in history. Two stored messages under one id meant only the later one was ever reachable in the position map, so the earlier copy could never be annotated or excluded. It gets a fresh id assigned on the next load.

Added test_repeated_context_does_not_duplicate_message_ids covering a cycle that re-delivers the same upstream conversation three times.

CI type-checks tests with mypy in addition to ruff and pyright. I ran the other two locally but not mypy, so all four Python jobs failed on the first push.

Most errors were stub clients and stub agents passed where the full client or agent protocol is expected. Rather than scattering per-call-site ignores, each affected test file now builds its agent through a small helper that relaxes the type once. That also removed some duplicated construction.

Two were real rather than cosmetic. test_durable_history_provider instantiated the abstract HistoryProvider directly, which now uses a concrete stub, and test_durabletask_workflow_initial_input had a context stub whose prepare_agent_task predated the context_messages parameter this branch adds to the protocol.

The remaining local mypy error is in integration_tests/conftest.py and comes from redis typing in my environment. CI does not report it, and the file is untouched here.
Copilot AI review requested due to automatic review settings July 31, 2026 12:50
redis-py types lrange differently depending on version, so annotating the result as list[str] passed locally and failed on CI with list[bytes | str]. The helper now takes the result loosely and coerces each entry, which holds either way.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 51 out of 52 changed files in this pull request and generated 1 comment.

Comment on lines +260 to +264
if position is None:
inserted = self._insert_new_message(binding, message, after=last_known)
if inserted is not None:
last_known = inserted
continue

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 002b9ef. This was a real corruption path, not a theoretical one.

_shift_positions now moves recorded positions along whenever an insertion pushes an entry's messages, and _prune removes by identity rather than by index so it cannot be thrown off by an earlier insertion in the same flush.

Worth noting why it was not caught: entries normally hold a single message, so an insertion never shifted a tracked neighbour. A workflow node receives the upstream conversation as several messages in one request entry, which is where it bites. test_insertion_keeps_later_positions_valid builds that shape and fails with annotation did not reach up-3 without the fix.

Copilot AI review requested due to automatic review settings July 31, 2026 12:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 51 out of 52 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

python/packages/durabletask/agent_framework_durabletask/_history_provider.py:270

  • The stored_by_id map stores (entry, index) positions, but _insert_new_message() mutates entry.messages during the same flush. After an insertion, indices for later messages in that entry become stale, so entry.messages[index] can point at the wrong message (potentially overwriting annotations or pruning the wrong item). Storing direct message references (or re-resolving by message_id after insertions) would avoid index drift.
            entry, index = position
            stored = entry.messages[index]
            stored.extension_data = annotations
            last_known = position
            if self.prune_excluded and annotations and annotations.get(EXCLUDED_KEY):

python/samples/azure_functions/14_conversation_compaction/function_app.py:81

  • This trailing triple-quoted string is an unused string literal (not a docstring), so it has no effect and adds dead code. Convert it to comments or fold it into the module docstring at the top of the file.

Comment on lines +169 to +173
if not message.message_id:
# Give every loaded message a stable identity so compaction results can be
# reconciled back onto durable state on flush.
message.message_id = f"durable_{id(entry):x}_{index}"
stored.message_id = message.message_id

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1f38a6b, though the reasoning needed a correction.

Within a single load and flush cycle the id is consistent, and get_messages writes it back onto the stored message, so it is persisted from then on. A cold start before the first flush therefore regenerates a fresh consistent set rather than breaking reconciliation, since nothing from the abandoned cycle survived to disagree with.

The real hazard is address reuse. A later run can allocate an entry at an address an earlier run already used, producing an id that run persisted. Two stored messages then share a key in the position map, which is the same corruption as the duplicate id issue on this PR.

Took the suggested scheme:

scope = entry.correlation_id or entry.created_at.isoformat()
return f"durable_{entry.json_type.value}_{scope}_{index}"

The entry type is required because a request and its response share a correlation id.

test_generated_ids_survive_a_cold_start strips the ids from a persisted blob, loads it twice into independent entities, and compares. With id(entry) restored it fails with synthesized ids changed across a cold start.

…peat

Both issues come from review on PR 59 and both were real. Each corrupts durable state quietly rather than raising.

flush() looked messages up by an index recorded before the run, then inserted compaction-generated summaries into the same entry. The insertion pushed every later message along by one, so the recorded index then pointed at the wrong message and its annotations were written there. Pruning had the same flaw and could delete the wrong message. Positions are now shifted alongside the insertion, and pruning removes by identity rather than index.

This stayed hidden because entries normally hold a single message. A workflow node receives the upstream conversation as several messages in one request entry, which is where it bites. The new regression test builds that shape and fails without the fix.

_drop_already_stored() kept the newest message when the whole upstream context was already recorded, so the agent still had an input, but it kept the id too. Two stored messages under one id collide in the position map, so only the later one was ever annotated and the earlier copy could never be excluded. The kept copy now drops its id and is assigned a fresh one on load.
Copilot AI review requested due to automatic review settings July 31, 2026 13:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 51 out of 52 changed files in this pull request and generated no new comments.

Suppressed comments (1)

python/packages/durabletask/agent_framework_durabletask/_durable_agent_state.py:882

  • to_chat_message() passes self.extension_data directly as additional_properties. Because this is a mutable dict that is also retained on the durable state message, downstream code (e.g., compaction annotating Message.additional_properties) can accidentally mutate durable state in-place before the provider’s explicit flush/persist step runs, leading to surprising side effects (especially on error paths). Make a defensive copy when constructing the Message.
        if self.extension_data is not None:
            kwargs["additional_properties"] = self.extension_data

…identity

Third issue from review on PR 59, and also real, though not for the reason given.

Messages stored without an id were given one built from id(entry). Within a single load and flush cycle that is consistent, and the id is written back into durable state, so a cold start before the first flush just regenerates a fresh consistent set rather than corrupting anything.

The actual hazard is address reuse. A later run can allocate an entry at an address a previous run already used, producing an id that run persisted. Two stored messages then share a key in the position map, which is the same corruption the duplicate id fix addressed.

The id now comes from the entry type, its correlation id or created_at, and the message index, all of which are persisted. The entry type is needed because a request and its response share a correlation id. The new test reloads the same state twice and fails when the id is taken from object identity.
Copilot AI review requested due to automatic review settings July 31, 2026 13:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 51 out of 52 changed files in this pull request and generated 1 comment.

Comment on lines +204 to +208
"session": {
"type": "object",
"description": "Serialized agent session carried between turns: the per-provider state bag and any service-issued conversation id. The agent's own history provider slice is excluded, since conversationHistory is the record of truth.",
"properties": {
"session_id": { "type": "string" },

@cgillum Chris Gillum (cgillum) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some initial comments on just the parts of the ADR that I've read so far. I haven't been able to get through the full PR yet.


The first two are per-operation and identical in both runtimes. The third is cumulative.
`ConversationHistory` is one blob appended to every turn and re-persisted whole, so it is bounded by
the backend's state-size limit (e.g. classic Azure Storage ~1 MB/entity), whereas a core process is

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Azure Storage has no limit. It's DTS that has the 1MB limit.

That said, we'd still want this behavior for Azure Storage because of the performance degradation that happens when payloads get large like this.

(especially LLM summarization) must not corrupt or diverge persisted state across retries.
- **Message-list correctness.** Preserve atomic groups (assistant tool-call plus tool-result, and
reasoning pairings) so the model input stays valid.
- **Cover both surfaces.** Durable agents **and** durable workflows, in **both** languages.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the MAF compaction features work for workflows? Durable workflows are architected quite differently from agent entities, so the same constraints don't necessarily apply.

- **Message-list correctness.** Preserve atomic groups (assistant tool-call plus tool-result, and
reasoning pairings) so the model input stays valid.
- **Cover both surfaces.** Durable agents **and** durable workflows, in **both** languages.
- **No-op for service-managed storage.** When the service owns the conversation (a

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "no-op" bullet is confusing because Durable Agents are effectively "service-managed storage" where the entity is the "service". That said, conversation compaction still applies because we're allowing users (per this ADR) to configure service-side compaction. It might be best to just remove this bullet rather than try to explain the nuance.

compacts `ConversationHistory` inside the entity operation before checkpoint.
- **Option 3, on-storage maintenance compaction.** Compact persisted history from a separate
entity signal or operation, decoupled from the request path.
- **Option 4, workflow-level compaction hook.** Apply a strategy at the `AgentExecutor`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workflows were not mentioned in the context / problem statement section so it's unclear why we're discussing them here.

configured. If only an in-run filter is configured, durable trims the model input just like core and
the store still grows - the context window is identical in both runtimes, and storage capacity is a
separate concern. Auto-deriving a lossy reducer would use a context-window tool to solve a storage
problem and **silently destroy the durable record**. Capacity is addressed by the backend instead:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "silently destroy the durable record" mean? This feels like a gross overstatement. Entity storage is not some system of record that needs to be protected. It's a simple state bag just like any storage provider. The "durability" is about not losing state when there is a failure - it doesn't imply that state is immutable or anything like that.

- **Option 4, workflow-level compaction hook.** Apply a strategy at the `AgentExecutor`
`context_mode` / `context_filter` boundary that governs the `full_conversation` chained between
agent executors.
- **Option 5, auto-derive a durable store reducer.** When only an in-run filter is configured,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually thinking it might make sense to combine both option 5 and 6. I'm not confident that we can provide a good experience for users if they hit the 1MB limit in production. It therefore might be better to have a built-in reducer that's on by default and let customers opt-out of it if they can't tolerate lossiness and are willing to deal with broken entities that hit the 1MB limit.

reducer. Capacity limits surface explicitly rather than truncating.
- Neutral: a larger entity change than a bespoke compaction pass, and it must preserve the existing
`ConversationHistory` consumer contract (`AgentRunHandle` response polling, audit/replay, TTL).
- Bad: L2 carries workaround code because upstream binds the store-rewrite hook to session state.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not understanding what this point is saying. What is the "upstream" and what "session state" is being referred to? Also, what is the "gap" that could be closed?

- **No-op for service-managed storage.** When the service owns the conversation (a
`ConversationId` or `service_session_id` is set), the client has no history to compact.

## Considered Options

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's another option which is to use the Durable Task SDK's large payload storage extension, which is also designed to help customers work around the 1MB limit. One advantage it has over option 6 is that it's compatible with DTS purge operations and the DTS dashboard (or at least it will be - Tomer's team is working on this). I'm not sure if we'd necessarily choose this over option 6 but it's worth listing and evaluating.

The three samples added on this branch read AZURE_OPENAI_ENDPOINT and AZURE_OPENAI_MODEL. CI only sets FOUNDRY_PROJECT_ENDPOINT and FOUNDRY_MODEL, so every worker subprocess died with KeyError: 'AZURE_OPENAI_MODEL' and took both new integration test classes down with it.

Local runs passed because my integration .env happens to carry both sets of variables. Every other sample in the repo uses the Foundry pair, so this was a convention break my environment hid.

The samples now use FoundryChatClient, with the env templates, requirements, Functions settings template, and READMEs updated to match. default_options store=False still carries the point of the compaction samples, since Foundry stores conversations on the service by default too. Verified against the real service: test_13 is 5 for 5 and test_14 is 4 for 4, so compaction is genuinely operating on client-side history.
Copilot AI review requested due to automatic review settings July 31, 2026 17:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 51 out of 52 changed files in this pull request and generated no new comments.

if not known_ids:
return messages

deduped = [m for m in messages if not m.message_id or m.message_id not in known_ids]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:copilot: This de-duplication is ineffective for messages produced by the durable workflow itself. build_agent_executor_response() constructs both user and assistant Message objects without message_id, so every forwarded message satisfies not m.message_id and is retained. On a cycle such as A -> B -> A, A re-appends the prior conversation and sends duplicate context to the model on every iteration, with compounding state growth. The current tests hide this by assigning IDs manually. Please stamp deterministic, replay-safe IDs when constructing the workflow conversation and add a cyclic test that uses the production response builder.

raise TypeError(
f"Agent {type(self.agent).__name__} exposes context providers but does not support create_session()."
)
session: Any = create_session(session_id=self._state_provider.session_id)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:copilot: _state_provider.session_id contains only the entity key. Workflow entities use executor_id as the entity name and the orchestration instance ID as the key, so every agent node in one workflow receives the same core session.session_id. Any external HistoryProvider keyed by that value will mix the histories of different nodes. Please derive the core session ID from the full entity identity (name plus key), and cover two workflow agents using session-keyed external providers.

if not callable(to_dict):
return

payload = cast("dict[str, Any]", to_dict())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:copilot: The durable provider's transient state is removed only after session.to_dict() has already deeply serialized it. That slice contains the full working message buffer and _positions, whose values reference DurableAgentStateEntry objects. With newer supported Agent Framework versions, this can repeatedly serialize the transcript and emit unsupported-type warnings even though the result is immediately discarded. Please temporarily remove the provider slice before calling to_dict(), or keep the transient buffer/index outside session.state.

"type": "object",
"description": "Serialized agent session carried between turns: the per-provider state bag and any service-issued conversation id. The agent's own history provider slice is excluded, since conversationHistory is the record of truth.",
"properties": {
"session_id": { "type": "string" },

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:copilot: This makes Python's AgentSession.to_dict() shape part of the shared state schema, but .NET sessions naturally serialize as conversationId plus stateBag, and Python also emits a currently undeclared type field. The same PR persists compaction-critical messageId and extensionData fields that are absent from chatMessage, while the state version remains 1.1.0. Please model session as an opaque, versioned, runtime-discriminated payload, declare the new message fields as round-trip-required, and bump the minor schema version.

durable_history = self._find_durable_history_provider()
if isinstance(state, dict) and durable_history is not None:
cast("dict[str, Any]", state).pop(durable_history.source_id, None)
self.state.data.session = payload

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:copilot: Arbitrary provider state is assigned to durable entity state without first proving that it is JSON-compatible. Core may warn and leave unsupported values unchanged; persist_state() then fails, and the exception handler calls persist_state() again with the same poisoned session payload, masking the original agent result/error and failing the entity operation. Please validate the candidate payload before replacing the last valid session state, and add coverage for a provider that stores an unsupported value.

already has an `ExtensionData` property, but it is `[JsonExtensionData]`, System.Text.Json's
overflow bucket for *unmapped JSON properties*, not a mapping of `ChatMessage.AdditionalProperties`
where compaction annotations live. `FromChatMessage`/`ToChatMessage` copy neither
`AdditionalProperties` nor `MessageId` (which .NET does not have at all), so annotations are lost

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:copilot: ChatMessage.MessageId does exist in the pinned Microsoft.Extensions.AI.Abstractions version. More importantly, .NET compaction does not store exclusion state in AdditionalProperties: exclusions live on CompactionMessageGroup, and the incremental group index (including full ChatMessage copies) lives in the CompactionProvider session-state entry. Mapping AdditionalProperties and MessageId is still good round-trip hygiene, but it is not sufficient to persist .NET compaction. Please correct this statement and record the unresolved choice between persisting a duplicate transcript in session state or losing incremental compaction state.

the opt-in reducer and pluggable storage. Costs a larger entity change that must preserve the
`ConversationHistory` consumer contract (response polling, audit, TTL). L2 also does not come free,
because upstream binds the store-rewrite hook to session state, so the provider publishes a working
buffer and reconciles it itself (see "Core Interface Gaps").

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:copilot: Option 6 is sound as an abstraction, but this cost analysis misses a .NET blocker. In current .NET core, CompactionProvider stores its incremental CompactionMessageIndex in AgentSession.StateBag, and each group retains full ChatMessage objects plus exclusion state. A durable provider that also persists the session must therefore store the conversation once in ConversationHistory and again inside session state. Omitting that provider state discards exclusions and summaries and can rerun summarization, while returning only included messages causes CompactionMessageIndex.Update() to rebuild the index. This is not inherent to the history-provider approach, but Option 6 cannot provide bounded .NET entity state until core can persist lightweight compaction metadata keyed by MessageId, or the durable implementation uses a different store-rewrite compaction path. Please record this as a .NET prerequisite/blocker rather than only the Python reconciliation cost.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants