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".

[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.

[adityasingh2400]

Ready for another look when you have a moment. All four points from your last review are in at 16d0e24.

One thing you may not see from the PR page: no checks have run on this head. The Tests workflow is sitting at action_required, so it needs approving before there is anything green to merge against.

[seratch]

Thanks for the update. The ETag changes now match Dapr's contract: missing ETags remain last-write-wins, while stale real ETags are retried against the latest metadata.

One merge blocker remains. Please remove or narrow the new claim that DAPR_CONSISTENCY_STRONG makes rapid or multi-writer history and timestamps exact. The Dapr Python async client currently sends state_metadata only as request metadata and does not set GetStateRequest.consistency, so this setting does not provide the stated read guarantee. Keep this PR scoped to preserving created_at when stored metadata is observed; the read-consistency plumbing should be handled separately.

Please also expand the metadata-failure warning test to inspect the complete LogRecord under model-redacted, tool-redacted, and fully diagnostic modes, while continuing to assert that the committed items remain visible. After those focused changes and green CI, this should be ready for another review.

[adityasingh2400]

Both done in 8d0e136.

You were right about the consistency claim, and I checked the SDK rather than take it on trust. dapr/aio/clients/grpc/client.py builds api_v1.GetStateRequest(store_name=..., key=..., metadata=state_metadata). The proto does have a consistency field, it is just never set on that path, so the level only ever travels as request metadata. Docstring is back to naming the two constants and the default, nothing about read guarantees.

The warning test is now parametrized over model-redacted, tool-redacted and fully diagnostic. It asserts the committed item is still readable in all three, and inspects the record rather than the rendered string, since the session id rides in extra and the exception in exc_info and neither shows up in getMessage(). Redacted asserts msg == "%s", no exc_info, no diagnostic context attribute, and that neither the session id nor the error text appears anywhere in record.__dict__. Diagnostic asserts msg == "%s: %s", a RuntimeError in exc_info, and the context equal to {"session_id": ...}.