Skip to content

fix: move chunk assembly off the event loop - #10427

Open
icyaaaww wants to merge 2 commits into
AstrBotDevs:masterfrom
icyaaaww:fix/nonblocking-chunk-assembly
Open

icyaaaww wants to merge 2 commits into
AstrBotDevs:masterfrom
icyaaaww:fix/nonblocking-chunk-assembly

Conversation

@icyaaaww

@icyaaaww icyaaaww commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

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 with uv sync --group dev.

uv run pytest tests/test_chunk_assembly_worker.py tests/unit/test_chat_chunked_upload.py tests/test_backup.py -k 'upload or chunk or cleanup or assembly' -q
25 passed, 62 deselected

uv run ruff check .
All checks passed!

uv run ruff format --check .
514 files already formatted

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 / 检查清单

  • New feature discussion: not applicable; this addresses blocking I/O in an existing upload path.
  • Verification steps and test results provided above.
  • No WebUI navigation, labels or page structure changed; no documentation/screenshot or entry-point mapping updates needed.
  • No new dependencies introduced.
  • No malicious code introduced.

@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 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


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

Comment thread astrbot/dashboard/services/chunked_upload_service.py Outdated

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