Skip to content

fix(core): prevent stale reads during queued preference deletions - #10780

Open
fading723 wants to merge 2 commits into
AstrBotDevs:masterfrom
fading723:fix/preference-deletion-visibility
Open

fading723 wants to merge 2 commits into
AstrBotDevs:masterfrom
fading723:fix/preference-deletion-visibility

Conversation

@fading723

@fading723 fading723 commented Oct 26, 2026 •

Copy link
Copy Markdown

SharedPreferences removes entries from its in-memory overlay before queued deletion operations commit. Reads during this interval can fall back to SQLite and return values already marked for deletion.

This race also affects conversation selection. While ConversationManager.delete_conversation() awaits removal of sel_conv_id, a concurrent lookup can restore the deleted conversation ID to the selection cache. That invalid selection remains cached after the database operation completes.

This change preserves pending deletion state across reads until the corresponding database operation commits, while allowing subsequent writes to remain visible.

Modifications / 改动点

  • Track pending key removals and scope clears by operation identity. Completion of an earlier operation cannot clear the state of a later deletion.
  • Apply deletion filtering and subsequent writes consistently to point and range reads, preserving persisted record IDs and timestamps.
  • Release deletion markers after successful persistence. Retry synchronous removals and scope clears in place up to three times, with asynchronous backoff, before processing later writes. If retries are exhausted, flush() reports the first synchronous deletion failure once; asynchronous failures continue to propagate directly to callers. Failed deletions remain masked locally until a subsequent successful mutation.
  • Add SQLite regression coverage for deletion ordering, scope isolation, initialization replay, automatic retry, terminal failure reporting, scheduler cleanup, record metadata, and concurrent conversation selection. Event barriers control concurrency; the two new barrier waits allow up to 30 seconds for slow CI environments.

The implementation retains on-demand database reads and does not preload the preferences table. Public method signatures, database schemas, and dependencies are unchanged.

  • This is NOT a breaking change.

Screenshots or Test Results / 运行截图或测试结果

Validation environment: Windows, Python 3.12.14.

python -m pytest \
  tests/unit/test_shared_preferences.py \
  tests/unit/test_session_management_service.py \
  tests/unit/test_conversation_restart.py \
  tests/unit/test_bot_cleanup.py \
  tests/test_conversation_commands.py \
  tests/unit/test_star_context.py \
  tests/unit/test_astr_main_agent.py \
  -q --disable-warnings
364 passed, 81 warnings in 21.70s

The conversation-selection regression reproduces the defect in the original implementation. Concurrency tests use explicit event barriers with SQLite to control the persistence interval. Additional coverage verifies that synchronous deletions recover automatically after transient failures without overtaking later writes, exhausted retries surface through flush(), asynchronous failure propagation remains unchanged, and successful retries release deletion markers. The four automatic-retry cases fail against the implementation before this review update.

Formatting and static checks passed:

ruff format .
ruff check .
ruff format --check tests/unit/test_shared_preferences.py
ruff check --no-force-exclude tests/unit/test_shared_preferences.py
git diff --check

Checklist / 检查清单

  • This PR contains a bug fix and does not introduce a new feature requiring prior discussion.
  • Automated regression tests cover the changes, and verification commands and results are provided above.
  • Documentation impact has been reviewed. No WebUI navigation, page structure, labels, or API schemas change; documentation screenshots and generated clients require no updates.
  • No additional dependencies are introduced.
  • The changes do not introduce malicious code.

Summary by Sourcery

Prevent stale preference and conversation-selection reads until queued deletions are successfully persisted.

Bug Fixes:

  • Prevent reads from returning values marked for deletion while queued preference removals or scope clears are still pending.
  • Prevent concurrent conversation lookups from restoring deleted conversation selections.
  • Preserve failed deletion state for retry while propagating persistence failures appropriately.

Enhancements:

  • Keep later writes visible across pending deletions and preserve persisted preference metadata during range reads.
  • Add ordered retries for synchronous deletions and ensure cleanup continues when flushing fails.

Tests:

  • Add regression coverage for deletion ordering, scope isolation, initialization replay, failure recovery, metadata preservation, and concurrent conversation selection.

@sourcery-ai sourcery-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.

Hey - I've found 2 issues

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="astrbot/core/utils/shared_preferences.py" line_range="90" />
<code_context>
+                self._pending_removals.pop((scope, scope_id, key), None)
             elif action == "remove" and key is not None:
                 self._cache.pop((scope, scope_id, key), None)
+                self._pending_removals[(scope, scope_id, key)] = operation
             elif action == "clear":
                 keys = [
</code_context>
<issue_to_address>
**Failed deletions are never retried**

When the database deletion fails for a synchronous remove or clear, `_drain_write_queue` logs a failed deletion and completes the queue item without retrying it; synchronous callers receive no failure signal. The marker hides the old row only in this process, so the deletion appears successful until restart, when the still-persisted value becomes visible again.

Retry failed deletions or surface the failure to the caller and provide a retry path that does not depend on another caller issuing the same deletion.
</issue_to_address>

### Comment 2
<location path="tests/unit/test_shared_preferences.py" line_range="389" />
<code_context>
+        store.clear("plugin", "example")
+
+    try:
+        await asyncio.wait_for(entered.wait(), timeout=3)
+        # The first deletion and intervening put are committed; the second is pending.
+        persisted = await database.get_preference("plugin", "example", "token")
</code_context>
<issue_to_address>
**Correct deletion tests time out**

When the event loop or database operation takes longer than three seconds to reach the event signal, in `test_earlier_deletion_does_not_unmask_a_later_pending_deletion` and `test_concurrent_conversation_lookup_cannot_restore_a_deleted_selection`, the `asyncio.wait_for` calls raise `TimeoutError` if a loaded or slow test environment takes over three seconds to reach the mocked database operation, so the tests fail even when the implementation is correct.

Use a more generous timeout or another synchronization strategy that does not make the test depend on a three-second wall-clock deadline.

Also at `tests/unit/test_shared_preferences.py:525`.
</issue_to_address>

Sourcery assessment

Approval pending. 2 findings to address first.

Blocking findings: astrbot/core/utils/shared_preferences.py:90, tests/unit/test_shared_preferences.py:389


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

Comment thread astrbot/core/utils/shared_preferences.py
Comment thread tests/unit/test_shared_preferences.py Outdated

@sourcery-ai sourcery-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.

Sourcery assessment

Approved.

This branch has not been deployed

No deployments
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.

1 participant