fix(memory): preserve DaprSession created_at across writes

[adityasingh2400]

DaprSession.add_items rewrites the whole metadata document on every call and sets created_at to the current clock, so a session's persisted created_at always equals updated_at and its age is unrecoverable after the second turn.

Every other backend sets it once: RedisSession uses hsetnx, MongoDBSession uses setOnInsert, and the SQL backends insert the row once. This reads the stored created_at first and keeps it when present.

The regression test is a direct port of test_add_items_preserves_created_at_metadata from the Redis suite, and it fails on main with created_at == "2000" instead of "1000". The full Dapr suite is green at 46 passed, along with ruff and mypy.

This is a fresh take on #3236, which was closed with "If the approach is still useful, happy to revisit with a new PR".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e016c25169

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2c7eb25343

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[adityasingh2400]

Two Codex findings here. I took the first and I am pushing back on the second.

Not failing after the batch is committed. This one is right and it is my bug, fixed in d4f4d3b. The messages key is saved first, so once the metadata loop runs the caller's items are already in the session. Exhausting the metadata retry budget raised out of add_items(), and the natural response to that error is to retry the call, which appends the same batch a second time. The metadata retry loop is new in this PR, so the PR introduced the window.

The post-commit metadata refresh is now best effort and logs a warning instead of raising. I also moved _read_created_at inside the try, since a failure there is equally post-commit and should not surface either.

There is a regression test. On the previous commit it fails with RuntimeError: etag mismatch escaping add_items, which is exactly the false failure described. With the fix the append is visible, get_items returns the item, and the warning is emitted. The file is at 48 passed, with ruff, ruff format and mypy clean.

Tying created_at to the message-write winner. I do not think this one is worth doing, and I want to be explicit about why rather than quietly skip it.

The concern is real but bounded. Both writers are appending to the same brand new session, so the window between one committing messages and the other creating metadata is the concurrent request window, and created_at has one second resolution. The wrong value is the other racing writer's timestamp from the same moment, not an arbitrary one.

Fixing it properly means making the creation stamp part of the same commit that wins the messages key, and those are two separate keys. The Dapr state API used here writes them independently, so getting that guarantee needs a transaction across both keys, which is a much larger change to this backend than the bug justifies. Anything short of that just moves which of the two racers wins.

The existing test_concurrent_first_add_items_does_not_regress_created_at already pins the property that actually matters, which is that a late writer cannot overwrite an established created_at with a later timestamp. Happy to revisit if you would rather have the stronger guarantee and are open to the transactional write it needs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d4f4d3ba55

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[adityasingh2400]

The P1 is right, and it was mine from the previous commit. Fixed in d829b2f.

My warning interpolated the caller-supplied session_id and the provider exception straight onto the record, so a sidecar error carrying tenant or backend detail was logged regardless of the data flags. The old record read literally:

WARNING openai.agents:dapr_session.py:422 DaprSession stored the new items for session
metadata_gives_up but could not update its metadata: etag mismatch

It now goes through log_model_and_tool_action_warning with a fixed message, and the session id is passed as diagnostic context rather than interpolated:

log_model_and_tool_action_warning(
    logger,
    "DaprSession stored the new items but could not update the session metadata",
    error,
    diagnostic_extra=lambda: {"session_id": self.session_id},
)

DONT_LOG_MODEL_DATA and DONT_LOG_TOOL_DATA both default to True, so the default path emits only the fixed message with no identifier and no exception. This also matches the existing log_model_and_tool_action_error call already in this file.

The regression test now asserts the absence as well as the presence, that the record contains "could not update" but contains neither the session id nor the exception text. It fails on d4f4d3b against the captured log above and passes here. The file is at 48 passed with ruff, ruff format and mypy clean.

[seratch]

Thanks for the contribution. The sequential created_at bug is valid, and preserving an existing value with ETag-guarded updates is the right direction.

Before merging, please narrow the concurrency guarantee. Dapr specifies that writes without an ETag use last-write-wins, even when first-write is requested. The new test instead makes FakeDaprClient reject a missing-ETag write after another writer creates the key, so it passes under behavior the real Dapr API does not provide.

Please request first_write only when a real metadata ETag was read, replace the missing-metadata race test with a stale non-null ETag retry test, and update the comments accordingly. The sequential preservation and post-commit best-effort handling can remain unchanged.

[adityasingh2400]

You are right, and this one is worth stating plainly: my test was passing because of the fake, not because of anything Dapr guarantees. Fixed in 7c30e9a.

FakeDaprClient.save_state rejects a write whose etag is None once the key exists. Real Dapr does not, it treats a write with no etag as last-write-wins even when first-write concurrency is requested. So the missing-metadata race test asserted a property the store does not provide, and asking for first_write on the create implied a guarantee that was never there.

Three changes:

first_write is now requested only when a real metadata etag was read:

options=self._get_state_options(
    concurrency=(Concurrency.first_write if metadata_etag is not None else None)
),

The comment no longer claims the create is protected. It says the guard applies once metadata exists, that two concurrent creates can both succeed with the loser's created_at winning by a fraction of a second, and that every write after that is etag guarded, which is the case that stops a later append from resetting an established created_at.

test_concurrent_first_add_items_does_not_regress_created_at is replaced by test_stale_metadata_etag_retries_and_keeps_created_at. It establishes metadata, lets a second append supersede that etag, then has a writer save against the now stale but non-null etag and asserts it is rejected, re-reads, and keeps created_at at 1000 while advancing updated_at.

On verification, two things worth being precise about. The new test passes on both the old and new commit, because the stale-etag path is unchanged, so there is no fail-before to show for it. The change that does have one is the removal: I re-ran the old race test against the corrected source and it now fails with created_at at 2000 rather than 1000, since the etag-less create is last-write-wins. That is the fake-only behavior you identified, and it is why the test had to go rather than be adjusted.

The sequential preservation and the post-commit best-effort handling are unchanged. 48 passed, with ruff, ruff format and mypy clean over both the source and the test.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7c30e9af4c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Read creation metadata consistently before rewriting it

When the session uses the default DAPR_CONSISTENCY_EVENTUAL, an append immediately following the first metadata save can read a stale absence here. _read_created_at() then returns no timestamp or etag, so add_items() generates a new created_at and performs an unguarded last-write-wins save, recreating the timestamp-reset bug even for sequential writes. Ensure this preservation read has read-after-write consistency or use conditional creation semantics; the in-memory test client currently cannot reproduce an eventually consistent missing read.

AGENTS.md reference: AGENTS.md:L134-L134

Useful? React with 👍 / 👎.

[adityasingh2400]

This is a real window, but I do not think a code branch is the right answer, so I have documented it instead. Reasoning, in case you disagree.

The read at this line already carries the session's configured level through _get_read_metadata(). Under DAPR_CONSISTENCY_EVENTUAL a stale read is the behavior the caller asked for, not a gap in the guard. Neither remedy in the comment is available to me here. Forcing this one read to strong would silently override an explicit constructor argument, and strong reads are a per-component capability that not every state store offers. Dapr has no create-if-absent primitive, and a write without an etag is documented as last-write-wins even when first_write is requested, which is exactly what the comment above the save already says.

I did consider one narrowing fix: add_items already holds existing_messages from the messages read, so a missing created_at combined with a non-empty history could be treated as a stale read and the metadata write skipped. I rejected it. The messages read one step earlier runs at the same consistency level, so if the metadata read is stale that read is likely stale too, and a stale one there has already caused something worse than a reset timestamp through the last-write-wins save on the messages key. Guarding only the metadata would give a false assurance about a store the caller configured to be eventually consistent, and by your own note the in-memory client cannot exercise the path, so it would be untestable machinery on top of that.

The remedy that actually works is DAPR_CONSISTENCY_STRONG, and nothing was pointing users at it. The consistency docstring now says the eventual level can read back a stale conversation or a stale created_at, and to use strong when a session is appended to from more than one place, or in quick succession, and the history and timestamps have to be exact.

make lint, make typecheck clean, 56 passed across the two Dapr test files.

[seratch]

Thanks for the update. The previous missing-ETag race-test issue is addressed correctly, but one representation mismatch remains.

The Dapr Python SDK returns an empty string for a missing state ETag, not None. As a result, metadata_etag is not None still treats the create path as having a real ETag and requests first_write, contrary to the new contract and comments. The stale-ETag test does not cover this branch and, as noted, also passes on the previous commit.

Please normalize response.etag or None in _read_created_at, make the fake return "" for missing ETags, and add an assertion that metadata creation does not request first_write. Please also describe concurrent ETag-less creation simply as last-write-wins, without the timing bound or claiming every later write is guarded.

[adityasingh2400]

Good catch, and it is worse than a documentation mismatch: the guard was not actually doing anything on the create. Fixed in b62d510.

All four points:

_read_created_at now normalizes with response.etag or None, and its docstring says why, so callers test for a real etag rather than for one particular empty representation.

FakeDaprClient.get_state now returns "" for a missing etag the way the SDK does, instead of None.

test_metadata_creation_does_not_request_first_write asserts the create sends no etag and leaves concurrency unspecified, then that the following update does send an etag with first_write. This is the fail-before you were pointing at: with the fake corrected and without the normalization, it fails with assert '' is None, so the create really was requesting first_write on an empty string.

One thing I got wrong while writing that test and want to flag rather than bury: I first asserted the create concurrency was None, and it is Concurrency.unspecified, because StateOptions defaults the field when we do not pass one. The assertion now pins unspecified, which is the accurate statement of what is sent.

The comment no longer carries the timing bound or the every-later-write claim. It now says concurrent etag-less creation is last-write-wins and stops there.

Separately, Codex left a P2 on this same read that I do not think I should act on unilaterally. Under the default DAPR_CONSISTENCY_EVENTUAL, an append shortly after the first metadata save can read a stale absence, generate a fresh created_at, and save it unguarded, which recreates the bug for sequential writes. Fixing that properly means reading metadata with strong consistency regardless of the session setting, which changes the store interaction for every append, so it seems like your call rather than mine. Codex also notes the in-memory client cannot reproduce it, so I would not be able to cover it with a test in this suite. Happy to do it here or leave it for a follow-up.

Verification: 49 passed, with ruff, ruff format and mypy clean over both the source and the test.