Add grain-side status compare-and-set helper#70
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds atomic compare-and-set functionality for command status transitions in the Orleans-based command idempotency system. The changes implement a grain-side helper method to ensure status transitions remain serialized within the grain boundary.
- Added
TrySetStatusAsyncmethod to the command idempotency grain interface and implementation for atomic status transitions - Updated Orleans store to delegate status updates to the new grain API instead of using separate get/set operations
- Fixed a race condition in the memory cache store's lock cleanup logic
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
ICommandIdempotencyGrain.cs |
Added TrySetStatusAsync method signature for atomic status transitions |
CommandIdempotencyGrain.cs |
Implemented compare-and-set logic and refactored TryStartProcessingAsync to use switch expression |
OrleansCommandIdempotencyStore.cs |
Simplified TrySetCommandStatusAsync to use new grain method instead of separate get/set calls |
MemoryCacheCommandIdempotencyStore.cs |
Fixed race condition in lock cleanup by using atomic key-value pair removal |
ManagedCode.Communication.Orleans/Grains/CommandIdempotencyGrain.cs
Outdated
Show resolved
Hide resolved
| return true; | ||
|
|
||
| case CommandExecutionStatus.Failed: | ||
| await MarkFailedAsync(state.State.ErrorMessage ?? "Status set to failed"); |
There was a problem hiding this comment.
[nitpick] Using the existing error message or a generic fallback may not accurately represent the reason for the status change. Consider accepting an error message parameter in TrySetStatusAsync to provide more specific failure context.
…in.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
KSemenenko
merged commit c63f7c4
into
codex/analyze-project-for-idempotent-command-support-zgadv1
![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.
ℹ️ 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
| public async Task<bool> TrySetStatusAsync(CommandExecutionStatus expectedStatus, CommandExecutionStatus newStatus) | ||
| { | ||
| if (state.State.Status != expectedStatus) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
The new TrySetStatusAsync compares expectedStatus directly against state.State.Status without applying the expiration logic used by GetStatusAsync. After a command’s ExpiresAt passes, GetStatusAsync reports NotFound even though the persisted status is still, for example, Completed. Clients that observe NotFound and call TrySetCommandStatusAsync(..., expectedStatus: NotFound, …) will now receive false because the compare uses the stale persisted value, whereas the previous implementation compared against GetStatusAsync and allowed the transition attempt. This makes compare‑and‑set unreliable for expired entries and will break callers that rely on GetStatusAsync’s view of the world. Consider normalizing expired states before the equality check (or clearing them) so the CAS path sees the same status that GetStatusAsync exposes.
Useful? React with 👍 / 👎.
* Guard completed Orleans commands from retries * Handle disposed service provider when resolving CommunicationLogger (#68) * Add grain-side status compare-and-set helper (#70) * Add grain-side status compare-and-set helper * Update ManagedCode.Communication.Orleans/Grains/CommandIdempotencyGrain.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
Testing
~/.dotnet/dotnet test ManagedCode.Communication.Tests/ManagedCode.Communication.Tests.csprojhttps://chatgpt.com/codex/tasks/task_e_68ea8121e2b48326a95e4cdac666eaa6