Repository navigation
fix(nrepl): stop the harness freezing when an eval errors - #881
Merged
Merged
Conversation
A failed eval was treated as a dead socket. `nrepl-eval` wrapped the whole
eval in a retry that reconnected and re-ran it on ANY error:
(try (nrepl-eval-inner repaired)
([err] (nrepl-reconnect) (nrepl-eval-inner repaired)))
Two bad outcomes followed.
For a TIMEOUT the slow work was simply run again, doubling the wait (a 3 s
timeout took 6 s). Worse, the retry reconnects first: `connect-nrepl-inner`
did an unbounded blocking read for the clone reply, and when the server had
gone away that read never returned. A blocking C socket read is not reachable
by the Janet interrupt, so dirge's eval/interrupt machinery could not stop
it — the worker sat there until the host's own deadline and the session froze
completely.
Fix, in three parts:
- `nrepl-connection-error?` retries only on genuine transport failures
(broken pipe / closed socket / connection reset / EOF / not connected). An
ordinary eval error — a compile error, a thrown exception — now propagates
with the connection intact.
- A timeout is not retried. The eval keeps running server-side after the
timeout, so its late reply would desync the next eval; the connection is
dropped instead (`nrepl-disconnect`) and the next `nrepl_eval` call
reconnects via its existing `.nrepl-port` path. `nrepl-timeout-error?`
gates this.
- The clone handshake is bounded by a new `nrepl-connect-timeout` (10 s,
deliberately independent of `nrepl-eval-timeout`, which may be minutes for
a long computation). An endpoint that accepts the TCP connection but never
answers now fails fast with "nREPL clone handshake failed: timeout" instead
of wedging. `nrepl-read-msg` already took an optional timeout; the
handshake simply passes it now.
Regression tests (`cargo test --features plugin nrepl`):
- `nrepl_timeout_is_not_retried_as_a_connection_error` — timeout text must not
be classified as a retryable connection error; transport failures must.
- `nrepl_timeout_detection_is_precise` — only timeout wording may drop the
connection; a compile error or thrown exception must leave it intact.
Verified live against a real REPL at 127.0.0.1:8230: a 3 s timeout now
returns in 3 s (previously 6 s), after which the connection reports "not
connected"; a silent-accept server fails the connect in ~3 s instead of
hanging past the 30 s worker deadline (the freeze being fixed).
The three modal free-text fields — the `question` tool's custom answer, the plan-approval feedback box, and the permission deny-note — each hand-rolled a `match key.code` in `dispatch_modal!` that knew only `KeyCode::Backspace`. Two keys the compose box has always handled were therefore dead in every one of them: - Backspace. A terminal whose `kbs` is `^H` (MobaXterm, screen, some serial consoles) sends the raw BS byte `0x08`, which crossterm reports as Ctrl+H rather than `KeyCode::Backspace`. The compose editor binds `delete_char_back` to Ctrl+H, so it was fine; the modal matchers dropped the key entirely, and a typo could not be corrected. - Ctrl+G. The compose box opens `$EDITOR` via `external_editor`; the modals had no arm for it, so the chord did nothing. Route all three fields through one pure decider, `entry_edit(key, mods) -> EntryEdit` (Insert / Backspace / ExternalEditor / Ignore). It accepts BOTH decodings of Backspace, maps Ctrl+G to `ExternalEditor`, keeps Ctrl+C/D as `Ignore` so each caller still reads them as the panic gesture, and folds AltGr (Ctrl+Alt+printable on Windows) back to text. Ctrl+G suspends the TUI around `$EDITOR` via the new `edit_entry_in_editor()` helper, mirroring the compose path (`suspend_tui_for_subprocess` → `external_editor::edit_text` → `resume_tui_after_subprocess`); this is why `dispatch_modal!` now takes the `user_tx` channel as a second argument. One decider instead of three near-copies is also why the question field had once pushed the literal `c` of a Ctrl+C into the user's answer (dirge-x2zf): the copies drifted. A guard test scans every `.push(c)` text site in mod.rs and fails if one bypasses `EntryEdit::Insert`. Regression tests (`cargo test --bin dirge entry_`): Ctrl+H deletes like Backspace, Ctrl+G maps to the external editor, plain `g` inserts, AltGr inserts, Ctrl+C/D are inert, Enter/Esc stay with the caller.
yogthos
added a commit
that referenced
this pull request
Oct 29, 2026
New advisory (Oct 28) flags 0.13.4 for exponential event growth with ENABLE_WIKILINKS on crafted input. Semver-compatible bump; audit gate was red on main and PR #881.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A failed eval was treated as a dead socket.
nrepl-evalwrapped the whole eval in a retry that reconnected and re-ran it on ANY error:Two bad outcomes followed.
For a TIMEOUT the slow work was simply run again, doubling the wait (a 3 s timeout took 6 s). Worse, the retry reconnects first:
connect-nrepl-innerdid an unbounded blocking read for the clone reply, and when the server had gone away that read never returned. A blocking C socket read is not reachable by the Janet interrupt, so dirge's eval/interrupt machinery could not stop it — the worker sat there until the host's own deadline and the session froze completely.Fix, in three parts:
nrepl-connection-error?retries only on genuine transport failures (broken pipe / closed socket / connection reset / EOF / not connected). An ordinary eval error — a compile error, a thrown exception — now propagates with the connection intact.nrepl-disconnect) and the nextnrepl_evalcall reconnects via its existing.nrepl-portpath.nrepl-timeout-error?gates this.nrepl-connect-timeout(10 s, deliberately independent ofnrepl-eval-timeout, which may be minutes for a long computation). An endpoint that accepts the TCP connection but never answers now fails fast with "nREPL clone handshake failed: timeout" instead of wedging.nrepl-read-msgalready took an optional timeout; the handshake simply passes it now.Regression tests (
cargo test --features plugin nrepl):nrepl_timeout_is_not_retried_as_a_connection_error— timeout text must not be classified as a retryable connection error; transport failures must.nrepl_timeout_detection_is_precise— only timeout wording may drop the connection; a compile error or thrown exception must leave it intact.Verified live against a real REPL at 127.0.0.1:8230: a 3 s timeout now returns in 3 s (previously 6 s), after which the connection reports "not connected"; a silent-accept server fails the connect in ~3 s instead of hanging past the 30 s worker deadline (the freeze being fixed).