Repository navigation
Conversation
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/chunked_upload_service.py" line_range="374" />
<code_context>
+ async with session.lock:
+ await self._run_file_operation(self._cleanup_session_files, upload_id)
+
+ def _cleanup_session_files(self, upload_id: str) -> None:
+ """Remove session files under the session lock in a worker thread.
+
</code_context>
<issue_to_address>
**Janitor skips cleanup passes**
When a cleanup worker removes a session while `_cleanup_expired_sessions` iterates `self.sessions`, `_cleanup_session_files` mutates `self.sessions` in a worker thread, so the event-loop iteration can raise `RuntimeError` and abort the scan, delaying cleanup of expired sessions until a later interval.
Keep `self.sessions` mutations on the event-loop thread.
Also at `astrbot/dashboard/services/chunked_upload_service.py:276`, `astrbot/dashboard/services/chunked_upload_service.py:372`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if the worker-thread or cancellation handling is wrong, assembly could write an incomplete destination or remove the session's chunks while an upload is still being used, leaving a bounded file-state error that requires re-uploading or rerunning the operation. Reverting restores the previous execution model, but any already-written or deleted files would need cleanup or recovery.
Blocking findings: astrbot/dashboard/services/chunked_upload_service.py:374
This was referenced Oct 12, 2026
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.
Modifications / 改动点
Chunked attachment and backup completion both await
ChunkedUploadService.assemble(). Previously that async method performed all file reads, copying, size validation and chunk-directory deletion synchronously, preventing the event loop from serving other work while large files were merged.Move merge/validation, chunk publication and directory cleanup to worker threads using the existing AnyIO dependency.
Serialize disk mutations per upload session. Revalidate sessions after acquiring the lock so queued writes cannot publish to a removed session.
Keep the session registry on the event-loop thread. Cleanup workers receive only a directory path and return success/failure; failed or cancelled cleanup retains bookkeeping for a retry.
Wait for workers to finish before propagating cancellation, including repeated cancellation, to prevent cleanup racing open files. Remove the merged destination on cancellation.
Add controlled slow-worker tests verifying event-loop progress, merge contents, and abort/cancellation coordination.
This is NOT a breaking change. Public method signatures and upload formats are unchanged.
Screenshots or Test Results / 运行截图或测试结果
Windows, Python 3.12.8, based on master
8831d9502, dependencies installed withuv sync --group dev.A preliminary controlled copy probe on the original implementation showed that a scheduled loop callback could run only after assembly returned. This is not a disk-throughput benchmark. Regression tests hold a worker until the loop releases it and verify another operation on that session waits. Existing upload tests cover owner checks, chunk retries, size mismatches and cleanup failures.
The selected tests emit warnings about audioop deprecation and cleanup-coroutine creation in synchronous test contexts. Full-suite testing was not performed. No screenshot applies to this backend change.
Tradeoffs: operations on one upload session are serialized; different sessions remain independent. Cancellation waits for in-flight filesystem operations rather than interrupting a thread. No disk timeout or hard termination is introduced.
Checklist / 检查清单