-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix: bind-mount local --target by default to avoid slow file-by-file … #840
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c7c7096
7c1e37c
6e62ea7
b04bfa9
e190683
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| """Thread-safe SDK-event pump for the TUI. | ||
|
|
||
| The scan runs on a separate thread and produces SDK events at a high rate during | ||
| LLM streaming. Delivering each event to the Textual UI with a blocking | ||
| ``App.call_from_thread`` per event blocks the scan thread and floods the UI | ||
| message pump, starving input handling. Instead the scan thread enqueues events | ||
| (non-blocking) and the UI loop drains them in bounded batches via | ||
| :func:`drain_queue`, keeping the UI responsive. | ||
|
|
||
| ``drain_queue`` is a pure function (no Textual/UI dependency) so it can be unit | ||
| tested without a live terminal app. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| import queue | ||
| from typing import TYPE_CHECKING | ||
|
|
||
|
|
||
| if TYPE_CHECKING: | ||
| from collections.abc import Callable | ||
|
|
||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Default number of events ingested per UI tick. Bounds UI work per frame so a | ||
| # burst of events cannot monopolize the event loop. | ||
| DEFAULT_MAX_DRAIN = 100 | ||
|
|
||
|
|
||
| def drain_queue[T]( | ||
| q: queue.Queue[T], | ||
| sink: Callable[[T], None], | ||
| *, | ||
| max_items: int = DEFAULT_MAX_DRAIN, | ||
| ) -> int: | ||
| """Drain up to ``max_items`` items from ``q`` (FIFO) into ``sink``. | ||
|
|
||
| Returns the number of items processed. Remaining items are left on the queue | ||
| for the next call (never dropped). A ``sink`` error on one item is logged and | ||
| does not stop the drain. | ||
| """ | ||
| processed = 0 | ||
| for _ in range(max_items): | ||
| try: | ||
| item = q.get_nowait() | ||
| except queue.Empty: | ||
| break | ||
| try: | ||
| sink(item) | ||
| except Exception: | ||
| logger.exception("SDK event ingest failed for one item; continuing") | ||
| processed += 1 | ||
| return processed |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1225,11 +1225,16 @@ def collect_local_sources(targets_info: list[dict[str, Any]]) -> list[dict[str, | |
| workspace_subdir = details.get("workspace_subdir") | ||
|
|
||
| if target_info["type"] == "local_code" and "target_path" in details: | ||
| # Local directory targets bind-mount read-only by default (issue | ||
| # #725): the SDK LocalDir entry copies a tree into the sandbox | ||
| # file-by-file, which stalls for hours on large repos. A bind mount | ||
| # is applied at container-create time and is effectively instant. | ||
| # An explicit ``mount: False`` still forces the file-by-file copy. | ||
| local_sources.append( | ||
| { | ||
| "source_path": details["target_path"], | ||
| "workspace_subdir": workspace_subdir, | ||
| "mount": bool(details.get("mount", False)), | ||
| "mount": bool(details.get("mount", True)), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Docker uses a remote daemon, this default passes the CLI machine's local path directly as a host bind source. The daemon resolves that path on its own host, so an ordinary Prompt To Fix With AIThis is a comment left during a code review.
Path: strix/interface/utils.py
Line: 1237
Comment:
**Remote Daemon Cannot See Target**
When Docker uses a remote daemon, this default passes the CLI machine's local path directly as a host bind source. The daemon resolves that path on its own host, so an ordinary `--target` can fail during container creation or expose the wrong directory; the previous `LocalDir` path uploaded the client-side files.
How can I resolve this? If you propose a fix, please make it concise. |
||
| } | ||
| ) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An oversized
local_codetarget withmount: Falsestill reaches this warning, butcollect_local_sourcespreserves that flag andbuild_session_entriescreates a file-by-fileLocalDircopy. The command now proceeds into the same long import that the size guard previously blocked while incorrectly reporting that the target will be bind-mounted.Prompt To Fix With AI