Repository navigation
Conversation
Author
|
Pre-existing failure evidence — referenced in the PR description as "the first comment". Evidence: the 41 full-suite failures are pre-existing on unmodified masterTwo full-suite runs were executed with the exact same command, on the same machine and environment:
The 41 failed test IDs are identical in both runs (diff of the two FAILED lists is empty); the only difference in passed count is +19, exactly the 19 tests added by this PR. All 26 collection errors are
Per-file failure counts (identical in both runs)
The 41 failing test IDs (same list in both runs)Collection errors (26, identical in both runs)All are
Full raw logs are retained locally ( Benchmark script — referenced in the Notes section of the PR description: """Benchmark: guarded cleanup DELETE with NOT EXISTS, conversations without a user_id index.
Reproduces the number quoted in the PR Notes:
- 100,000 conversation rows, no user_id index
- per-cleanup median / p95 over 200 runs (SQLite, WAL)
Run: python 基准_索引notexists_benchmark.py
"""
import os
import sqlite3
import statistics
import tempfile
import time
def main() -> None:
n_rows = 100_000
n_umos = 1000
runs = 200
db = os.path.join(tempfile.gettempdir(), "bench_cleanup.db")
for suffix in ("", "-wal", "-shm"):
try:
os.remove(db + suffix)
except FileNotFoundError:
pass
con = sqlite3.connect(db)
cur = con.cursor()
cur.execute("PRAGMA journal_mode=WAL")
cur.execute(
"CREATE TABLE conversations ("
"inner_conversation_id INTEGER PRIMARY KEY, user_id TEXT, "
"created_at DATETIME, updated_at DATETIME)"
)
cur.execute("CREATE TABLE umo_aliases (id INTEGER PRIMARY KEY, umo TEXT UNIQUE)")
cur.execute("CREATE TABLE umo_senders (id INTEGER PRIMARY KEY, umo TEXT)")
cur.executemany(
"INSERT INTO conversations (inner_conversation_id,user_id,created_at,updated_at) "
"VALUES (?,?,?,?)",
(
(i, f"qq:GroupMessage:{i % n_umos}", "2026-10-26", "2026-10-26")
for i in range(n_rows)
),
)
cur.executemany(
"INSERT INTO umo_aliases (umo) VALUES (?)",
((f"orphan:{i}",) for i in range(runs)),
)
con.commit()
plan = cur.execute(
"EXPLAIN QUERY PLAN DELETE FROM umo_aliases WHERE umo=? "
"AND NOT EXISTS (SELECT 1 FROM conversations WHERE user_id=?)",
("x", "x"),
).fetchall()
print("plan:", plan)
times = []
for i in range(runs):
umo = f"orphan:{i}"
t0 = time.perf_counter()
cur.execute(
"DELETE FROM umo_aliases WHERE umo=? "
"AND NOT EXISTS (SELECT 1 FROM conversations WHERE user_id=?)",
(umo, umo),
)
times.append((time.perf_counter() - t0) * 1000)
con.commit()
print(
f"rows={n_rows} runs={runs} "
f"median_ms={statistics.median(times):.3f} "
f"p95_ms={sorted(times)[int(runs * 0.95)]:.3f} "
f"min_ms={min(times):.3f} max_ms={max(times):.3f}"
)
con.close()
if __name__ == "__main__":
main() |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/dashboard/services/conversation_service.py" line_range="353-356" />
<code_context>
+ Conversations are already deleted when this runs, so a cleanup error
+ is reported as a structured field instead of a failed request.
+ """
+ try:
+ return await self._cleanup_orphaned_metadata(umos)
+ except Exception as exc:
+ logger.warning(f"清理会话元数据失败: {exc!s}")
+ return {
+ "deleted_aliases": 0,
</code_context>
<issue_to_address>
**Cleanup error handling is extracted**
When metadata cleanup raises an exception, `_safe_cleanup_metadata` handles the cleanup exception and logs it outside the single- and batch-delete main function blocks, contrary to the required placement of error handling and logging.
Handle the cleanup exception and logging directly in the single- and batch-delete main function blocks.
</issue_to_address>Sourcery assessment
Needs a human reviewer. With cleanup enabled, alias and sender metadata rows for sessions without remaining conversations are permanently deleted; reverting the code will not restore customized values. The deletion is opt-in and limited to those sessions, so the unrecoverable loss is bounded.
Comment on lines
+353
to
+356
| try: | ||
| return await self._cleanup_orphaned_metadata(umos) | ||
| except Exception as exc: | ||
| logger.warning(f"清理会话元数据失败: {exc!s}") |
Contributor
There was a problem hiding this comment.
⚪ Low · Cleanup error handling is extracted
When metadata cleanup raises an exception, _safe_cleanup_metadata handles the cleanup exception and logs it outside the single- and batch-delete main function blocks, contrary to the required placement of error handling and logging.
Handle the cleanup exception and logging directly in the single- and batch-delete main function blocks.
Prompt for AI agents
In `astrbot/dashboard/services/conversation_service.py` at lines 353-356:
**Cleanup error handling is extracted**
When metadata cleanup raises an exception, `_safe_cleanup_metadata` handles the cleanup exception and logs it outside the single- and batch-delete main function blocks, contrary to the required placement of error handling and logging.
Handle the cleanup exception and logging directly in the single- and batch-delete main function blocks.From AGENTS.md L73:
Handle edge cases, error catching, and logging directly inside the main function block instead of offloading them.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.
Motivation
Closes #10704.
Deleting a conversation (single or batch) from Data & Logs → Conversations only removes the row from
conversations, leaving orphanedumo_aliases/umo_sendersrows that the UI offers no way to clean up. #10761 only covers deleting a whole bot, not an individual conversation.Following the two-layer plan in #10704 (comment), this PR keeps the precise primitive and adds an explicit, default-off cleanup option at the service layer. Backward compatible:
cleanup_metadatadefaults tofalseon both single and batch delete paths.Modifications
astrbot/core/db/__init__.py,sqlite.py)delete_umo_alias(umo) -> boolprimitive.delete_umo_metadata_if_unreferenced(umo) -> dict: deletes the UMO's alias and sender metadata only when no conversation references it. Both DELETEs carry theNOT EXISTSguard in the statement itself, inside one transaction, so metadata is not deleted while a committed conversation still references the UMO. Returns{alias_deleted, senders_deleted, retained}.conversation_service.py)delete_conversationacceptscleanup_metadata(defaultFalse) for both single and batch paths.metadata_cleanup: {deleted_aliases, deleted_senders, retained_umos}; UMOs kept because other conversations still reference them are listed instead of reporting a plain success.errorfield.dashboard/api/conversations.py,schemas.py)cleanup_metadataquery parameter onDELETE /conversations/{id}and body field on batch delete; both default tofalse.openspec/openapi-v1.yaml,docs/public/openapi.jsonand the generated dashboard SDK are updated.changelogs/unreleased.md).Screenshots or Test Results
Verification steps (local AstrBot 4.29.3):
umo_aliasesandumo_sendersare gone (row counts before/after).retained_umos.All four steps passed on local AstrBot 4.29.3 with sanitized test data. Screenshots are attached via GitHub's uploader (sanitized): the cleanup checkbox in the delete dialog, the list after cleanup, and same-UMO retention after deleting one of two conversations.
Tests:
tests/unit/test_conversation_metadata_cleanup.py: 19 tests (db / service / API)dad255cwith the same command: 4469 passed / 41 failed / 79 skipped / 26 collection errors — the 41 failures and 26 collection errors are pre-existing environment issues (per-file counts and commands in the first comment)ruff check .,ruff format --check,vue-tsc,npm run build, i18n key check all passChecklist
Notes
conversationshas no index onuser_id; theNOT EXISTSsubquery therefore scans the table. On a local SQLite database with 100,000 conversation rows, one guarded cleanup took ~4.8 ms median (p95 ~5.4 ms, 200 runs; measured with a local benchmark script included in the first comment). Conversation deletion is low-frequency; this PR does not change the schema.docs/zh/use/webui.mdanddocs/en/use/webui.md; neither mentions alias cleanup on conversation deletion. Changelog entry included inchangelogs/unreleased.md.delete_umo_aliasprimitive. If feat(db): add UMO alias deletion method #10746 lands first, this PR will be rebased and the duplicate primitive dropped; only the service/API/UI delta remains.Summary by Sourcery
Enable users to optionally remove orphaned conversation metadata when deleting individual or multiple conversations while preserving the existing default behavior.
New Features:
Enhancements:
Documentation:
Tests:
Chores: