Skip to content

Fix disk state manager debounced writes - #6807

Open
harsh21234i wants to merge 14 commits into
reflex-dev:mainfrom
harsh21234i:fix/disk-state-manager-debounced-writes
Open

Fix disk state manager debounced writes#6807
harsh21234i wants to merge 14 commits into
reflex-dev:mainfrom
harsh21234i:fix/disk-state-manager-debounced-writes

Conversation

@harsh21234i

@harsh21234i harsh21234i commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Fixes #6806

Summary

Fix debounced StateManagerDisk.set_state() writes so repeated updates for the same token persist the latest value instead of the first queued value.

Problem

When disk write debouncing was enabled, only the first write for a token was retained in _write_queue. A later write before the debounce flush was silently discarded, so background processing or close() could persist stale state.

Changes

  • Replace an existing queued payload with the latest state while preserving the original queue timestamp.
  • Keep the in-memory cache aligned when set_state() receives an arbitrary instance instead of the one returned by get_state().
  • Explicitly persist supplied BaseState instances, including untouched instances, so the cache never points to a state that was not written to disk.
  • Preserve the existing touched-state optimization for the normal same-instance path.

Testing

  • uv run pytest tests/units/test_state.py::test_state_manager_disk_debounced_set_state_flushes_latest_non_base_state -q
  • uv run pytest tests/units/test_state.py::test_state_manager_disk_set_state_updates_cache_for_arbitrary_instance -q
  • uv run pytest tests/units/test_state.py::test_state_manager_disk_set_state_persists_untouched_base_state -q
  • uv run ruff check reflex/istate/manager/disk.py tests/units/test_state.py
  • uv run ruff format --check reflex/istate/manager/disk.py tests/units/test_state.py
  • git diff --check

@harsh21234i
harsh21234i requested a review from a team as a code owner July 23, 2026 09:53
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@codspeed-hq

codspeed-hq Bot commented Jul 23, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 26 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing harsh21234i:fix/disk-state-manager-debounced-writes (8ba3b69) with main (f21eb16)

Open in CodSpeed

Footnotes

  1. 8 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@greptile-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a data-loss bug in StateManagerDisk where debounced writes for the same token would silently discard all but the first queued update, causing stale state to be flushed to disk. The fix makes QueueItem mutable so subsequent set_state() calls can update the queued state in-place while preserving the original debounce timestamp.

  • QueueItem is made mutable and gains a force_write flag; repeated calls to set_state() for the same debounced token now overwrite the queued state in-place rather than being dropped.
  • A force_write path is introduced so explicitly-supplied BaseState instances that differ from the cached object bypass the touched-state optimisation and are always persisted, preventing the in-memory cache from diverging from disk.
  • The non-debounce path now correctly updates self.states only after a successful disk write, while the debounce path updates the cache eagerly (intentional trade-off — reads within the same server session see the new state immediately).

Confidence Score: 5/5

  • The change is safe to merge; it fixes a silent data-loss bug without altering any public interfaces.
  • The debounce in-place update, force_write propagation, and cache-ordering fix are all correct under asyncio's single-threaded execution model. The timestamp-preservation design prevents debounce starvation under high write frequency. Three targeted regression tests directly exercise each new code path and confirm the fix end-to-end.
  • No files require special attention.

Important Files Changed

Filename Overview
reflex/istate/manager/disk.py Core bug fix: debounce path now updates queued item state in-place; adds force_write flag for explicitly-supplied BaseState instances; non-debounce path now updates cache after disk write. Logic is sound and asyncio-safe.
tests/units/test_state.py Three new focused tests cover: (1) debounced writes flush latest value, (2) cache alignment for arbitrary instances, and (3) force-persistence of untouched BaseState. Tests are well-structured and directly exercise the fixed code paths.
news/6807.bugfix.md Changelog entry correctly describes the fix.

Reviews (13): Last reviewed commit: "Correct state manager type cast" | Re-trigger Greptile

Comment thread reflex/istate/manager/disk.py Outdated
@harsh21234i

harsh21234i commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

hey @masenf @FarhanAliRaza can you go through this?

@masenf masenf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appreciate the contribution, but i'm inclined to close this one. if it's rescoped just to persisting the state in set_state, we could probably take that.

Comment thread reflex/istate/manager/disk.py Outdated
Comment thread reflex/istate/manager/disk.py Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread reflex/istate/manager/disk.py Outdated
@harsh21234i

Copy link
Copy Markdown
Contributor Author

Hi @masenf, I rescoped PR #6807 to persistence-only as requested.

I removed the in-memory cache changes and related test, kept the latest queued-value disk persistence fix, and updated
the PR description and news fragment. The focused regression test and Ruff checks pass.

@masenf

masenf commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

this leaves open the hole that i mentioned in my first comment

i'm okay with updating the state reference in the QueueItem (rather than recreating), but both of the fixes are necessary for handling the "calling set_state with some arbitrary instance instead of the instance returned by get_state" case.

i'm not convinced how important that case is to handle, but it's potentially a correctness issue, so I can see a case being made for supporting it -- but if we're going to support it, it needs to be consistent from both sides and not significantly impact performance

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread reflex/istate/manager/disk.py

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread tests/units/test_state.py Outdated
@harsh21234i

Copy link
Copy Markdown
Contributor Author

I’ve addressed both correctness cases: queued items are updated in place, and explicitly supplied BaseState instances are force-persisted when they differ from the cached instance. The immediate and debounced paths now keep persistence and cache behavior consistent. CI is passing; could you please take another look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

StateManagerDisk drops debounced set_state updates for non-BaseState tokens

2 participants