Conversation
With reentrancy enabled, each dispatched method call gets its own state change tracker, but activation, reminders and timers run on the default tracker because no reentrancy id reaches them. A key read during activation stays cached there with change kind none forever, while method calls write the same key through their own trackers. A reminder callback that later reads that key is served the stale activation value. An app that skips its write because the value looks unchanged loses that write silently: nothing is logged anywhere, because no write is ever issued. Drop the default tracker's clean copies of keys written through a reentrancy-scoped tracker, so the next read reloads them from the runtime. Reported in dapr/dapr#10532, where a reminder callback's read-modify-write of an actor state key never persisted while the identical write from an ordinary method call did, and only with reentrancy enabled. Should be backported. Signed-off-by: joshvanl <me@joshvanl.dev>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1227 +/- ##
==========================================
+ Coverage 83.89% 83.90% +0.01%
==========================================
Files 123 123
Lines 10265 10272 +7
==========================================
+ Hits 8612 8619 +7
Misses 1653 1653 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A concurrent default-tracker read can reinsert stale state after invalidation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Fixes stale actor-state caching after reentrant saves by invalidating affected default-tracker entries.
Changes:
- Invalidates clean default-tracker entries after scoped saves.
- Adds regression coverage for reentrant cache invalidation.
| File | Description |
|---|---|
tests/actor/test_state_manager.py |
Tests reentrant save cache invalidation. |
dapr/actor/runtime/state_manager.py |
Invalidates default-tracker entries; a critical concurrent-read race remains. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
CasperGN
left a comment
There was a problem hiding this comment.
LGTM. The fix is in the right place: save_state is the only point where a reentrancy-scoped write lands, and dropping only the default tracker's clean copies is the right call, since a dirty default entry is a pending write that shouldn't be discarded. tests/actor passes (190), and the new test fails if the invalidation is removed.
None of these block:
- Test gaps. Two cases worth pinning: a reentrant remove (the next default read should report the key missing), and a default entry that is dirty (not
none), which should survive the invalidation. - Context cleanup in the test.
reentrancy_ctx.set(...)andset_state_context(...)are undone only if the middle of the test succeeds. If an assertion fails there, the context var stays set for later tests in the same thread. Resetting with the token in atry/finallywould keep a failure local. - Scope note, for a follow-up if at all. The same staleness exists one level up. In a reentrant chain A → B → A, if the outer A call reads a key before calling out, and the nested A call writes and saves it, the outer call's own tracker still serves the old value when it reads again. This PR doesn't cover that, and it isn't reachable from
save_statethe same way, so it's fine to leave out here.
|
@JoshVanL @CasperGN hey just a heads up that this fix, although correct, also introduces a performance regression (an extra data refresh from the state store). There is an alternative solution, which doesn't incur the performance regression, which has been adopted by the dotnet SDK here dapr/dotnet-sdk#1912 During this work, we also found several other Actor optimisation opportunities in the dotnet SDK, so it might be work assessing the python Actor implementation for these opportunities too! dapr/dotnet-sdk#1913 |
@olitomlinson - thank you for this detail! I've run through the dotnet PRs and with the help of my friend ported through #1237, #1238 and #1239 - they're on top of this so should be good to run all 4 through and then cut a release. |
With reentrancy enabled, each dispatched method call gets its own state change tracker, but activation, reminders and timers run on the default tracker because no reentrancy id reaches them. A key read during activation stays cached there with change kind none forever, while method calls write the same key through their own trackers.
A reminder callback that later reads that key is served the stale activation value. An app that skips its write because the value looks unchanged loses that write silently: nothing is logged anywhere, because no write is ever issued.
Drop the default tracker's clean copies of keys written through a reentrancy-scoped tracker, so the next read reloads them from the runtime.
Reported in dapr/dapr#10532, where a reminder callback's read-modify-write of an actor state key never persisted while the identical write from an ordinary method call did, and only with reentrancy enabled.
Should be backported.
cc @olitomlinson