Fix REPL evaluations in headless mode #50

2026-06-03T23:30:45-07:00

brendanjohnharris commented

2026-06-03 23:30:45 -07:00

(Migrated from github.com)

Two bugs stopped ex from returning a result in headless mode (--headless): every call hung for 30s and turned into a stuck background job.

In headless mode, nothing reads eval results back from the gate.
When you run code, the gate sends the result back over a socket, and drain_stream_messages! is what reads that socket and hands the result to the waiting ex call. But drain_stream_messages! is only ever called by the TUI's screen-redraw loop (tui/view.jl). Headless mode has no TUI, so the socket is never read.
Fix: in ConnectionManager.start!, start a background task that calls drain_stream_messages! on a loop. Only run it in headless mode (task_queue === nothing) so it doesn't fight the TUI loop for the same socket. Tracked as mgr.drain_task and shut down cleanly in stop!.
TCP gates ended up with a connection that had no auth token.
TCP gates are supposed to be connected only by _poll_tcp_gates!, which uses the auth token from tcp_gates.json. But when it connects one, the server also drops a mode=:tcp marker file in its sock_dir (so it can reconnect after a restart). The file-watcher then notices that marker and connects the same gate a second time. That tokenless connection wins, so every request gets rejected with "Authentication required." (The old guard only skipped the marker once the gate was already tracked, missing the gap before that.)
Fix: have the file-watcher ignore mode=:tcp markers entirely, _poll_tcp_gates! is the only thing that should connect TCP gates.

Two bugs stopped ex from returning a result in headless mode (--headless): every call hung for 30s and turned into a stuck background job. 1. In headless mode, nothing reads eval results back from the gate. When you run code, the gate sends the result back over a socket, and drain_stream_messages! is what reads that socket and hands the result to the waiting ex call. But drain_stream_messages! is only ever called by the TUI's screen-redraw loop (tui/view.jl). Headless mode has no TUI, so the socket is never read. Fix: in ConnectionManager.start!, start a background task that calls drain_stream_messages! on a loop. Only run it in headless mode (task_queue === nothing) so it doesn't fight the TUI loop for the same socket. Tracked as mgr.drain_task and shut down cleanly in stop!. 2. TCP gates ended up with a connection that had no auth token. TCP gates are supposed to be connected only by _poll_tcp_gates!, which uses the auth token from tcp_gates.json. But when it connects one, the server also drops a mode=:tcp marker file in its sock_dir (so it can reconnect after a restart). The file-watcher then notices that marker and connects the same gate a second time. That tokenless connection wins, so every request gets rejected with "Authentication required." (The old guard only skipped the marker once the gate was already tracked, missing the gap before that.) Fix: have the file-watcher ignore mode=:tcp markers entirely, _poll_tcp_gates! is the only thing that should connect TCP gates.

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2026-06-03 23:31:28 -07:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a headless-only background “drain pump” task to process stream messages, and adjusts session discovery behavior for TCP gates.

Changes:

Add drain_task to ConnectionManager and start it in start! for headless mode.
Wait for drain_task to finish during stop!.
Change discover_sessions behavior to always skip mode=:tcp session adverts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull request overview > [!NOTE] > Copilot was unable to run its full agentic suite in this review. Adds a headless-only background “drain pump” task to process stream messages, and adjusts session discovery behavior for TCP gates. **Changes:** - Add `drain_task` to `ConnectionManager` and start it in `start!` for headless mode. - Wait for `drain_task` to finish during `stop!`. - Change `discover_sessions` behavior to always skip `mode=:tcp` session adverts. --- 💡 <a href="/kahliburke/Kaimon.jl/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

src/gate_client.jl

					
				@ -0,0 +631,4 @@

				        # TCP sessions are exempt — a process can legitimately host both IPC and TCP gates.

				        if session_mode != :tcp && (name, pid) in known_name_pids

				            # Clean up the stale duplicate

				            _remove_session_files(mgr.sock_dir, session_id)

This change makes discover_sessions skip all mode=:tcp adverts unconditionally. That appears to contradict the comment about using the server-written advert for reconnect-after-restart; with an unconditional continue, these adverts can never be used for discovery/reconnect. If the intent is only to avoid replacing an already-connected TCP gate, consider restoring a conditional skip (e.g., skip only when already tracked), or otherwise implement the reconnect path elsewhere and update the comment to match the actual behavior.

This change makes `discover_sessions` skip *all* `mode=:tcp` adverts unconditionally. That appears to contradict the comment about using the server-written advert for reconnect-after-restart; with an unconditional `continue`, these adverts can never be used for discovery/reconnect. If the intent is only to avoid replacing an already-connected TCP gate, consider restoring a conditional skip (e.g., skip only when already tracked), or otherwise implement the reconnect path elsewhere and update the comment to match the actual behavior.

src/gate_client.jl

					
				@ -0,0 +1701,4 @@

				            disconnect!(conn)

				            push!(to_remove, conn)

				        elseif conn.status != :evaluating

				            conn.status = :evaluating

A fixed sleep(0.1) polling loop can cause unnecessary wakeups/latency tradeoffs (and the interval is a magic number). Consider using a blocking wait/poll mechanism (e.g., ZMQ poll with timeout, a Channel/Condition, or an event-driven trigger) and/or extract the interval into a named constant/config so it’s easier to tune and reason about.

A fixed `sleep(0.1)` polling loop can cause unnecessary wakeups/latency tradeoffs (and the interval is a magic number). Consider using a blocking wait/poll mechanism (e.g., ZMQ poll with timeout, a `Channel`/`Condition`, or an event-driven trigger) and/or extract the interval into a named constant/config so it’s easier to tune and reason about.

src/gate_client.jl

					
				@ -0,0 +1704,4 @@

				            conn.status = :evaluating

				            _fire_sessions_changed(mgr)

				        end

				    elseif result !== nothing

Using Threads.@spawn can move execution to a different OS thread. If drain_stream_messages! touches ZMQ sockets (common for “drain”/SUB reads), this is risky because ZMQ sockets are thread-affine and generally must not be accessed across threads. A safer approach is to use cooperative concurrency (@async) so the task stays on the same thread as socket ownership, or ensure sockets used by the drain pump are created and exclusively used inside that spawned task.

Using `Threads.@spawn` can move execution to a different OS thread. If `drain_stream_messages!` touches ZMQ sockets (common for “drain”/SUB reads), this is risky because ZMQ sockets are thread-affine and generally must not be accessed across threads. A safer approach is to use cooperative concurrency (`@async`) so the task stays on the same thread as socket ownership, or ensure sockets used by the drain pump are created and exclusively used inside that spawned task.

kahliburke commented

2026-06-19 15:54:09 -07:00

(Migrated from github.com)

Thanks @brendanjohnharris — both fixes are now on 2.0-integration (308b69b). This PR predated a large refactor (the gate was extracted into a standalone KaimonGate package, gate_client.jl was split, and the request path moved to a protocol-v2 DEALER/ROUTER design), so it couldn't merge as-is — I re-applied your two fixes to the current structure after confirming both bugs are still present:

1. Headless eval hang. Your root cause still holds under protocol-v2: eval results arrive on the SUB stream and are routed to the waiting caller's per-request inbox by drain_stream_messages!, which only the TUI render loop drives. Headless has no render loop, so the inbox is never filled and ex hangs to timeout. Re-implemented as a headless-only (task_queue === nothing) drain pump in start!, tracked as mgr.drain_task and joined in stop!. TUI mode is unaffected (the pump doesn't start when a task_queue exists).

2. TCP tokenless double-connect. Confirmed discover_sessions still only skipped a mode=:tcp marker once already tracked, leaving the gap you described. Since TCP gates are owned exclusively by _poll_tcp_gates! (which connects with the token from tcp_gates.json), the file-watcher now ignores mode=:tcp markers entirely.

Verified: full Kaimon suite (914) green, and a headless smoke test (isolated cache, real gate subprocess, no TUI) now returns eval "1+1" => "2" in ~0.8s instead of hanging. Closing as incorporated — credit is yours. Thanks for the clear writeup of both root causes!

Thanks @brendanjohnharris — both fixes are now on `2.0-integration` (`308b69b`). This PR predated a large refactor (the gate was extracted into a standalone `KaimonGate` package, `gate_client.jl` was split, and the request path moved to a protocol-v2 DEALER/ROUTER design), so it couldn't merge as-is — I re-applied your two fixes to the current structure after confirming both bugs are still present: **1. Headless eval hang.** Your root cause still holds under protocol-v2: eval results arrive on the SUB stream and are routed to the waiting caller's per-request inbox by `drain_stream_messages!`, which only the TUI render loop drives. Headless has no render loop, so the inbox is never filled and `ex` hangs to timeout. Re-implemented as a headless-only (`task_queue === nothing`) drain pump in `start!`, tracked as `mgr.drain_task` and joined in `stop!`. TUI mode is unaffected (the pump doesn't start when a `task_queue` exists). **2. TCP tokenless double-connect.** Confirmed `discover_sessions` still only skipped a `mode=:tcp` marker once already tracked, leaving the gap you described. Since TCP gates are owned exclusively by `_poll_tcp_gates!` (which connects with the token from `tcp_gates.json`), the file-watcher now ignores `mode=:tcp` markers entirely. Verified: full Kaimon suite (914) green, and a headless smoke test (isolated cache, real gate subprocess, no TUI) now returns `eval "1+1" => "2"` in ~0.8s instead of hanging. Closing as incorporated — credit is yours. Thanks for the clear writeup of both root causes!