Skip to content

Commit 114e79f

Browse files
author
Ovtcharov
committed
fix: round-four findings — the deadlock behind the desync, clamped at the source
The round-three drain fix traded the pipe desync for a deadlock: once a mid-run terminal error went out, the drain suppressed every later event — including a permission prompt whose stdio wait is unbounded — so the worker parked forever and the process never served another turn. Terminal now cancels the run (the handler's cancelled event breaks both the confirm and input waits), and the invariant itself moved into CanonicalTranslator: nothing is emitted after the first terminal event, making every consumer loop — stdio, HTTP, email — correct by construction, with tests. The walk bound became a cap instead of a crash: a legitimate repo with a giant assets/ subtree now indexes what was found (loudly), instead of a RuntimeError advising the user to do what they had already done. The budget counts entries after pruning, lives in CodeIndexConfig with the other limits, and has tests. expanduser now applies to the STORED index root too, so a '~'-style allowed_paths config can't produce a root that rejects the very paths the refusal message recommends. Ctrl+V: a transiently busy Windows clipboard no longer fails a paste whose caption text was already in hand (a typed busy sentinel distinguishes it from a real decode failure), and a Linux box with neither wl-paste nor xclip finally says so instead of a silent no-op. Turns whose final carried an empty answer keep the user's question in history. SKILL.md now distinguishes the /query capacity 503 from the init 503 it previously taught was the only kind. The cancel path uses the shared dead-child reset helper instead of hand-rolling two-thirds of it.
1 parent d4043d3 commit 114e79f

11 files changed

Lines changed: 182 additions & 28 deletions

File tree

hub/agents/gaia/npm/SKILL.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -463,6 +463,11 @@ A healthy run streams `status` / `token` events and ends with one `final`. If
463463
`/v1/gaia/init` is 503, fix what its `hint` names and retry — the rest of your
464464
integration is fine.
465465

466+
**A 503 from `/query` itself is a different condition**: every retained
467+
session slot is busy and none is idle enough to evict (SPEC §5.2). Do NOT
468+
loop on `/v1/gaia/init` — it will report ready. Wait for a running turn to
469+
finish (or close an idle session) and retry the same `/query`.
470+
466471
For the full wire contract, lock schema, exit codes, and timeout table, see
467472
[`SPEC.md`](./SPEC.md). For the user-facing overview, see [`README.md`](./README.md)
468473
and <https://amd-gaia.ai/docs/guides/gaia>.

hub/agents/gaia/python/gaia_agent/stdio.py

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -883,6 +883,12 @@ def _run() -> None:
883883
streamed_answer = str(canonical.get("answer") or "")
884884
if canonical.get("type") in TERMINAL_TYPES:
885885
terminated = True
886+
# The reader is gone the moment a terminal event goes
887+
# out, so cancel the run: a worker that parks on a
888+
# confirmation nobody can ever see would otherwise wait
889+
# forever and this drain — which must outlive the worker
890+
# — would never return, wedging the whole process.
891+
handler.cancelled.set()
886892

887893
if not terminated:
888894
for canonical in translator.flush():
@@ -897,10 +903,11 @@ def _run() -> None:
897903
if terminated:
898904
# The normal exit. A turn that ended in an error event is not
899905
# recorded — replaying a failure as if it were an answer teaches
900-
# the model that the failure is what it said — and an empty
901-
# streamed answer is skipped for the same reason the fallback
902-
# path below skips one.
903-
if streamed_answer:
906+
# the model that the failure is what it said. An EMPTY final is
907+
# recorded (with its empty answer): dropping it would also drop
908+
# the user's question, and "try answering my last question
909+
# again" must not reach a model with no record it was asked.
910+
if streamed_answer is not None:
904911
_record_turn(agent, query, streamed_answer)
905912
return
906913
if "error" in result:
@@ -919,10 +926,9 @@ def _run() -> None:
919926
break
920927
elif isinstance(value, str):
921928
answer = value
922-
if answer:
923-
# An empty answer is not a turn worth replaying into every later
924-
# prompt — same rule as the streamed branch above.
925-
_record_turn(agent, query, answer)
929+
# Recorded even when empty — same reasoning as the streamed branch:
930+
# the question half of the pair must survive.
931+
_record_turn(agent, query, answer)
926932
_write({"type": "final", "answer": answer}, out)
927933
finally:
928934
# Every exit path, including the early returns above: leaving a dead

src/gaia/agents/tools/code_index_tools.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,10 @@ def _init_code_index_state(
6868
repo_path: Repository root (absolute or relative, resolved here).
6969
code_index_config: Optional pre-built ``CodeIndexConfig``.
7070
"""
71-
self._repo_path = os.path.abspath(repo_path)
71+
# expanduser everywhere a root is stored, or a '~'-style
72+
# allowed_paths config produces a "<cwd>/~" root that rejects the
73+
# very paths it was meant to allow.
74+
self._repo_path = os.path.abspath(os.path.expanduser(repo_path))
7275
self._code_index_config = code_index_config
7376
self._code_index_sdk: Optional[Any] = None
7477

src/gaia/code_index/sdk.py

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,10 @@ class CodeIndexConfig:
6464

6565
repo_path: str
6666
max_files: int = 5000
67+
#: Ceiling on directory entries the discovery walk may enumerate (counted
68+
#: after pruning). max_files caps what gets INDEXED; without this, walking
69+
#: a huge non-repo root could burn the whole tool timeout just listing.
70+
max_walk_entries: int = 200_000
6771
max_file_size_mb: float = 1
6872
chunk_overlap: int = 50
6973
embedding_model: str = DEFAULT_EMBEDDING_MODEL
@@ -604,19 +608,12 @@ def _discover_files(self) -> List[str]:
604608
# Bound the WALK, not just the result list: max_files caps how many
605609
# files are indexed, but enumerating a huge non-repo root (a home
606610
# directory, a whole drive) could burn the entire tool timeout just
607-
# listing entries before a single one was embedded.
608-
max_entries = max(self.config.max_files * 40, 200_000)
611+
# listing entries. A cap, not an error — a legitimate repo with a
612+
# giant assets/ subtree indexes what was found, exactly like the
613+
# max_files cap below, and the truncation is logged loudly.
609614
entries_seen = 0
610615

611616
for root, dirs, files in os.walk(str(self._repo_root)):
612-
entries_seen += len(dirs) + len(files)
613-
if entries_seen > max_entries:
614-
raise RuntimeError(
615-
f"stopped after scanning {entries_seen} directory entries "
616-
f"under {self._repo_root} without finishing — this does "
617-
"not look like a code repository. Point repo_path at the "
618-
"repository root itself."
619-
)
620617
rel_root = Path(root).relative_to(self._repo_root)
621618

622619
# Filter out skipped directories in-place
@@ -629,6 +626,13 @@ def _discover_files(self) -> List[str]:
629626
and not any(fnmatch.fnmatch(d, p) for p in ignore_patterns)
630627
]
631628

629+
# Counted AFTER pruning, so node_modules/.git and gitignored
630+
# trees cost nothing against the budget. Checked at the END of
631+
# the loop body (files from this directory are kept), so one
632+
# directory can overshoot by its own listing — which os.walk
633+
# already paid for — but partial results always survive.
634+
entries_seen += len(dirs) + len(files)
635+
632636
if len(result) >= self.config.max_files:
633637
break
634638

@@ -662,6 +666,17 @@ def _discover_files(self) -> List[str]:
662666

663667
result.append(abs_path)
664668

669+
if entries_seen > self.config.max_walk_entries:
670+
self.log.warning(
671+
f"discovery stopped after {entries_seen} directory "
672+
f"entries under {self._repo_root} (max_walk_entries="
673+
f"{self.config.max_walk_entries}) — indexing the "
674+
f"{len(result)} files found so far. If this is a code "
675+
"repository, raise max_walk_entries; if not, point "
676+
"repo_path at the repository root."
677+
)
678+
break
679+
665680
return result
666681

667682
def _read_gitignore_patterns(self) -> List[str]:

src/gaia/ui/sse_translation.py

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -156,12 +156,23 @@ def __init__(
156156
# Last user-facing status message, cleared as soon as any other event
157157
# is emitted — see ``_user_status``.
158158
self._last_status: Optional[str] = None
159+
# Latched once a terminal event goes out — see ``translate``.
160+
self._terminal_emitted = False
159161

160162
# -- public API --------------------------------------------------------
161163

162164
def translate(self, event: Dict[str, Any]) -> List[Dict[str, Any]]:
163-
"""Map one source event to zero or more canonical events."""
164-
return self._track_status(self._translate(event))
165+
"""Map one source event to zero or more canonical events.
166+
167+
Nothing is emitted after the first terminal event: the contract is
168+
"exactly one ``final`` or ``error`` ends every turn", and a reader
169+
stops at it — anything written afterwards sits unread in the pipe and
170+
gets consumed as the opening events of the NEXT turn. Clamping here
171+
makes every consumer loop (stdio, HTTP, email) trivially correct.
172+
"""
173+
if self._terminal_emitted:
174+
return []
175+
return self._clamp_terminal(self._track_status(self._translate(event)))
165176

166177
def _track_status(self, out: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
167178
"""Forget the last status once anything else reaches the wire.
@@ -208,7 +219,17 @@ def _translate(self, event: Dict[str, Any]) -> List[Dict[str, Any]]:
208219

209220
def flush(self) -> List[Dict[str, Any]]:
210221
"""Release any buffered ``tool_call`` at stream close."""
211-
return self._track_status(self._flush_pending())
222+
if self._terminal_emitted:
223+
return []
224+
return self._clamp_terminal(self._track_status(self._flush_pending()))
225+
226+
def _clamp_terminal(self, out: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
227+
"""Truncate a batch at its first terminal event and latch the clamp."""
228+
for i, e in enumerate(out):
229+
if e.get("type") in TERMINAL_TYPES:
230+
self._terminal_emitted = True
231+
return out[: i + 1]
232+
return out
212233

213234
# -- tool_call buffering (spec §6.3) -----------------------------------
214235

tests/unit/test_code_index_sdk.py

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -741,3 +741,45 @@ def fake_encode(texts, chunks):
741741
"hole.py" not in hashes
742742
), "a file that lost chunks must be re-tried on the next index"
743743
assert result.chunks_dropped >= 1
744+
745+
746+
class TestWalkBound:
747+
"""max_walk_entries caps ENUMERATION (max_files caps what gets indexed).
748+
749+
A cap, not an error: a legitimate repo with a giant subtree indexes the
750+
files found before the budget ran out, exactly like the max_files cap —
751+
the truncation is logged, never raised.
752+
"""
753+
754+
def test_walk_stops_at_the_entry_budget_and_keeps_partial_results(
755+
self, tmp_path, caplog
756+
):
757+
skip_if_unavailable()
758+
repo = tmp_path / "repo"
759+
repo.mkdir()
760+
# Nested, one file per subdir: the budget is checked per directory,
761+
# so truncation lands mid-walk and partial results must survive.
762+
for i in range(30):
763+
sub = repo / f"sub{i:02d}"
764+
sub.mkdir()
765+
(sub / "f.py").write_text(f"def f{i}(): pass\n", encoding="utf-8")
766+
767+
sdk = make_sdk(tmp_path)
768+
sdk.config.max_walk_entries = 35 # 30 subdir entries + a few files
769+
770+
import logging
771+
772+
with caplog.at_level(logging.WARNING):
773+
files = sdk._discover_files()
774+
775+
assert 0 < len(files) < 30
776+
assert any("max_walk_entries" in r.message for r in caplog.records)
777+
778+
def test_default_budget_does_not_touch_a_normal_repo(self, tmp_path):
779+
skip_if_unavailable()
780+
repo = tmp_path / "repo"
781+
repo.mkdir()
782+
for i in range(5):
783+
(repo / f"f{i}.py").write_text("x = 1\n", encoding="utf-8")
784+
sdk = make_sdk(tmp_path)
785+
assert len(sdk._discover_files()) == 5

tests/unit/test_final_usage_completeness.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,3 +83,32 @@ def test_a_turn_with_no_ttft_reports_none(self):
8383
def test_a_bare_answer_carries_no_usage_at_all(self):
8484
final = translate({"type": "answer", "content": "hi"})
8585
assert "usage" not in final
86+
87+
88+
class TestTerminalClamp:
89+
"""Nothing is emitted after the first terminal event.
90+
91+
The contract is one final|error per turn, and every reader stops at it —
92+
an event written afterwards sits unread in the pipe and opens the NEXT
93+
turn (the stdio pipe-poisoning bug). The clamp lives in the translator so
94+
every consumer loop is trivially correct.
95+
"""
96+
97+
def test_events_after_a_terminal_error_are_suppressed(self):
98+
t = CanonicalTranslator(run_id=None, agent_id="gaia", debug=False)
99+
first = t.translate({"type": "policy_alert", "reason": "blocked"})
100+
assert any(e.get("type") == "error" for e in first)
101+
102+
assert t.translate({"type": "status", "message": "still going"}) == []
103+
assert t.translate({"type": "answer", "answer": "late answer"}) == []
104+
assert t.flush() == []
105+
106+
def test_a_batch_is_truncated_at_its_terminal(self):
107+
t = CanonicalTranslator(run_id=None, agent_id="gaia", debug=False)
108+
out = t.translate({"type": "answer", "answer": "done"})
109+
# whatever preceded it, nothing FOLLOWS the terminal in the batch
110+
terminal_indices = [
111+
i for i, e in enumerate(out) if e.get("type") in ("final", "error")
112+
]
113+
assert terminal_indices, "an answer event must produce a terminal"
114+
assert terminal_indices[0] == len(out) - 1

tui/internal/client/subprocess.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,8 +237,11 @@ func (s *SubprocessClient) Send(ctx context.Context, query string) (<-chan inter
237237
// and reads nothing.
238238
defer func() {
239239
if ctx.Err() != nil {
240-
st.proc.reap()
241-
s.discard(st.proc)
240+
// resetDeadChild, not a hand-rolled subset: kill() is
241+
// idempotent on the already-killed child, and one shared
242+
// sequence means a future reset change cannot miss the
243+
// cancellation path.
244+
s.resetDeadChild(st.proc)
242245
}
243246
}()
244247

tui/internal/ui/chat/clipboard.go

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package chat
55

66
import (
7+
"errors"
78
"fmt"
89
"os"
910
"path/filepath"
@@ -171,10 +172,15 @@ func pasteFromClipboardOrImage() tea.Cmd {
171172
}
172173
png, ok, err := readClipboardImagePNG()
173174
if ok {
174-
// The image is the payload here. A decode failure surfaces —
175-
// silently pasting the caption URL instead would hide that the
176-
// image copy failed (the platform readers' documented contract).
177175
if err != nil {
176+
// A transient failure to OPEN the clipboard is not a broken
177+
// image — deliver the caption text already in hand rather
178+
// than failing a paste that has a usable payload.
179+
if errors.Is(err, errClipboardBusy) && hasText {
180+
return pasteClipboardMsg{text: text, err: nil}
181+
}
182+
// A real decode failure surfaces — silently pasting the
183+
// caption URL would hide that the image copy failed.
178184
return pasteImageMsg{err: err}
179185
}
180186
path, werr := writeClipboardImageToTemp(png)
@@ -184,6 +190,11 @@ func pasteFromClipboardOrImage() tea.Cmd {
184190
// No image at all — the caption-ish text is all there is.
185191
return pasteClipboardMsg{text: text, err: nil}
186192
}
193+
if err != nil {
194+
// Nothing to paste AND the platform reader knows why (e.g. no
195+
// wl-paste/xclip installed) — say so instead of a silent no-op.
196+
return pasteImageMsg{err: err}
197+
}
187198
return pasteClipboardMsg{text: text, err: terr}
188199
}
189200
}
@@ -221,6 +232,11 @@ func writeClipboardImageToTemp(png []byte) (string, error) {
221232
return f.Name(), nil
222233
}
223234

235+
// errClipboardBusy marks a clipboard that could not be OPENED (another app
236+
// holds it) — a transient condition distinct from an image that cannot be
237+
// decoded. Platform readers wrap their open failures with it.
238+
var errClipboardBusy = errors.New("clipboard is held by another application")
239+
224240
// looksLikeImageCaption reports whether clipboard text reads as the metadata
225241
// a "Copy image" action leaves beside the bitmap — a single URL (or data:
226242
// URI) — rather than content someone copied for its own sake.

tui/internal/ui/chat/clipboardimage_linux.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package chat
55

66
import (
77
"bytes"
8+
"fmt"
89
"os/exec"
910
)
1011

@@ -27,7 +28,12 @@ var linuxImageClipboardTools = []struct {
2728
// same reasoning atotto/clipboard's own Linux build already relies on for
2829
// text, via xclip/xsel).
2930
func readClipboardImagePNG() (data []byte, ok bool, err error) {
31+
anyTool := false
3032
for _, tool := range linuxImageClipboardTools {
33+
if _, lookErr := exec.LookPath(tool.name); lookErr != nil {
34+
continue
35+
}
36+
anyTool = true
3137
out, runErr := exec.Command(tool.name, tool.args...).Output()
3238
if runErr != nil {
3339
// Not installed, or nothing image/png-shaped on the clipboard
@@ -41,5 +47,13 @@ func readClipboardImagePNG() (data []byte, ok bool, err error) {
4147
}
4248
return out, true, nil
4349
}
50+
if !anyTool {
51+
// ok=false (no image was detected) but with the WHY: the caller
52+
// surfaces this only when there is nothing else to paste, so a
53+
// user without wl-paste/xclip learns what to install instead of
54+
// getting a silent no-op.
55+
return nil, false, fmt.Errorf(
56+
"no clipboard image tool found — install wl-paste (Wayland) or xclip (X11) to paste screenshots")
57+
}
4458
return nil, false, nil
4559
}

0 commit comments

Comments
 (0)