fix(diff): prevent event loop blocking and watcher crash during file write (#1607)#1659
fix(diff): prevent event loop blocking and watcher crash during file write (#1607)#1659
Conversation
Signed-off-by: Kai <me@kaiyi.cool>
There was a problem hiding this comment.
Pull request overview
Fixes the write/replace tool freezing issue (#1607) by moving CPU-heavy diff computation off the event loop and hardening the root hub watcher against crashes and shutdown signals.
Changes:
- Refactors diff generation to run in a background thread, adding large/huge file fast paths and thresholds.
- Updates file write/replace tools to await the new async diff builder.
- Makes the shell’s root hub watcher resilient to handler exceptions and queue shutdown, with added test coverage and release note updates.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/kimi_cli/utils/diff.py |
Splits diff building into sync + async wrapper using asyncio.to_thread; adds thresholds and early-exit logic. |
src/kimi_cli/tools/file/write.py |
Awaits async build_diff_blocks to avoid blocking the event loop. |
src/kimi_cli/tools/file/replace.py |
Awaits async build_diff_blocks to avoid blocking the event loop. |
src/kimi_cli/ui/shell/__init__.py |
Prevents watcher death by handling QueueShutDown and logging handler exceptions. |
tests/utils/test_diff_utils.py |
Adjusts tests to call the new sync helper for deterministic diff assertions. |
tests/utils/test_diff_render.py |
Updates diff-block offset assertions and uses sync diff helper. |
tests/utils/test_diff_nonblocking.py |
Adds tests ensuring thread delegation and large/huge file behavior. |
tests/ui_and_conv/test_root_hub_watcher_resilience.py |
Adds tests verifying watcher survives handler exceptions and exits on shutdown. |
docs/zh/release-notes/changelog.md |
Adds unreleased notes for diff non-blocking + watcher resilience + huge-file summary behavior. |
docs/en/release-notes/changelog.md |
Same as above (English). |
CHANGELOG.md |
Same as above (top-level changelog). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except QueueShutDown: | ||
| return | ||
| try: | ||
| await self._handle_root_hub_message(msg) |
There was a problem hiding this comment.
except Exception: here will also catch asyncio.CancelledError (Python >=3.12), which can prevent the watcher task from being cancelled cleanly (e.g., on shutdown) if cancellation happens while awaiting _handle_root_hub_message. Please special-case asyncio.CancelledError (re-raise it) before logging other exceptions.
| await self._handle_root_hub_message(msg) | |
| await self._handle_root_hub_message(msg) | |
| except asyncio.CancelledError: | |
| # Allow task cancellation to propagate cleanly. | |
| raise |
| ) | ||
| from kimi_cli.utils.diff import ( |
There was a problem hiding this comment.
These imports from kimi_cli.utils.diff are split across two separate from ... import (...) blocks (and one uses parentheses for a single name). This is likely to be reformatted by ruff/isort and may fail import-sorting checks; consider combining them into a single import statement.
| ) | |
| from kimi_cli.utils.diff import ( |
|
|
||
| Runs the CPU-bound diff computation in a thread to avoid blocking | ||
| the event loop. | ||
| """ |
There was a problem hiding this comment.
The old_text == new_text short-circuit currently lives inside _build_diff_blocks_sync, so build_diff_blocks() still schedules a to_thread() call even when there is no diff. If this fast-path is expected to be common, consider checking old_text == new_text in the async wrapper before calling asyncio.to_thread to avoid unnecessary threadpool work.
| """ | |
| """ | |
| if old_text == new_text: | |
| return [] |
Signed-off-by: Kai <me@kaiyi.cool>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 837ba355ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { old_text: oldText, new_text: newText, path: filePath } = data; | ||
|
|
||
| // Summary mode: file too large for inline diff, show a compact summary instead | ||
| if (data.is_summary) { |
There was a problem hiding this comment.
Keep hook order stable in DiffContent
DiffContent now returns early for is_summary before the useEffect/useMemo hooks later in the component, which makes hook execution conditional. If a live update reuses this component instance and toggles is_summary (e.g., when display data is replaced during streaming), React will hit a hook-order mismatch (Rendered fewer hooks than expected) and break rendering for that tool output. Please keep hooks unconditional by branching in JSX after hooks, or split summary/non-summary into separate components.
Useful? React with 👍 / 👎.
Related Issue
Resolve #1607
Description
Fix the Write/Replace tool freezing issue reported in #1607. Root cause analysis identified three problems:
A. Event loop blocking —
build_diff_blocks()runs Python'sdifflib.SequenceMatcher(O(n²)) synchronously inside an async function, blocking the event loop for large files. Fix: offload toasyncio.to_thread().B. Silent watcher death —
_watch_root_wire_hub()inShellhas zero exception handling, unlike its counterpart inwire/server.py. Any exception in_handle_root_hub_messagekills the watcher silently, permanently breaking the approval flow. Fix: addtry/exceptmatching the wire server pattern, plusQueueShutDownhandling for graceful shutdown.C. Large file performance — For files >2000 lines, enable
autojunk=Truefor faster (slightly less precise) diffs. For files >10000 lines, skip diff entirely and show a summary block. Also short-circuit immediately whenold_text == new_text.Changes
src/kimi_cli/utils/diff.py— Refactorbuild_diff_blocksinto sync_build_diff_blocks_sync+ async wrapper withto_thread; add large/huge file thresholds and early-exit for unchanged contentsrc/kimi_cli/tools/file/write.py/replace.py—awaitthe now-asyncbuild_diff_blockssrc/kimi_cli/ui/shell/__init__.py— Add exception resilience andQueueShutDownhandling to_watch_root_wire_hub