Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
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
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SharedPreferencesremoves 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 ofsel_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 / 改动点
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.The implementation retains on-demand database reads and does not preload the preferences table. Public method signatures, database schemas, and dependencies are unchanged.
Screenshots or Test Results / 运行截图或测试结果
Validation environment: Windows, Python 3.12.14.
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:
Checklist / 检查清单
Summary by Sourcery
Prevent stale preference and conversation-selection reads until queued deletions are successfully persisted.
Bug Fixes:
Enhancements:
Tests: