fix(sessions): preserve Dapr session created_at across writes#3236
fix(sessions): preserve Dapr session created_at across writes#3236adityasingh2400 wants to merge 3 commits into
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cf43e322e
ℹ️ 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".
| await self._dapr_client.save_state( | ||
| store_name=self._state_store_name, |
There was a problem hiding this comment.
This fix reads existing metadata and then overwrites the metadata key without any ETag/concurrency option. With the default eventual reads or two DaprSession instances writing the same session, a stale/missing metadata read leaves created_at = now, and this unconditional save can clobber an older created_at, so the field can still advance across writes despite the commit intent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 47c39c0c: Guard the metadata write with an ETag and a first-write-only path. Concurrent writers no longer clobber created_at; tests cover the lost-update and ETag-mismatch cases.
3ce6b86 to
14b11ab
Compare
DaprSession.add_items() rewrote the session metadata on every call, clobbering created_at with the current timestamp. The field is now read back from the metadata key and only initialized on first write, so created_at reflects the true session start while updated_at advances. Also removes a duplicated etag assignment in pop_item().
The metadata read-modify-write was unconditional, so two writers (or one writer with a stale eventual read) could clobber an existing created_at. Apply the same first-write concurrency + retry-on-conflict pattern used for the messages key so the field is preserved across concurrent writes.
14b11ab to
47c39c0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 046ab23325
ℹ️ 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".
| metadata = { | ||
| "session_id": self.session_id, | ||
| "created_at": created_at, | ||
| "updated_at": now, |
There was a problem hiding this comment.
When concurrent DaprSession writers hit the metadata ETag conflict path, the retry loop reuses the now value captured before the first metadata read. If another writer has already saved newer metadata, this retry can successfully overwrite updated_at with an older timestamp (even earlier than the preserved created_at), so the field no longer reflects the last write. Recompute the timestamp inside each retry or preserve the maximum existing updated_at.
Useful? React with 👍 / 👎.
2 participants
Summary
DaprSession.add_items()rewrote the session metadata on every call, clobberingcreated_atwith the current timestamp on each write. The field now reflects the true session start time, whileupdated_atadvances as expected.created_atif it parses as a dict with that field.etag = getattr(response, "etag", None) or Noneline inpop_item()(dead code).Test plan
test_metadata_preserves_created_atassertscreated_atis stable across twoadd_itemscalls one second apart whileupdated_atadvances.tests/extensions/memory/suite passes (180 passed, 5 skipped).ruff checkandruff format --checkclean on touched files.