Skip to content

Allow session signals after EOF - #1510

Open
paulcakeface wants to merge 1 commit into
mscdex:masterfrom
paulcakeface:fix-signal-after-eof
Open

Allow session signals after EOF#1510
paulcakeface wants to merge 1 commit into
mscdex:masterfrom
paulcakeface:fix-signal-after-eof

Conversation

@paulcakeface

@paulcakeface paulcakeface commented Aug 20, 2026

Copy link
Copy Markdown

Summary

Allow ClientChannel.signal() to send a session signal after the client has sent SSH EOF but before the channel is closed.

Problem

Channel.signal() currently requires both this.writable and outgoing.state === 'open'.

After stdin is finished, the channel can still be alive at the SSH protocol level while its outgoing state is eof. RFC 4254 states that sending SSH_MSG_CHANNEL_EOF does not close the channel, and the channel may remain open for other channel messages.

This matters for cancellation/timeout handling. A downstream user, tufantunc/ssh-mcp 2.3.1, closes remote stdin and later tries to send INT/TERM on timeout. With ssh2 1.17.0 those signals are silently skipped once the channel is in EOF state, so the API reports a timeout while the remote process can continue running.

I reproduced this against OpenSSH: the timeout returned after about 1.26s, while the remote process remained alive at +3s and +8s. Sending the same signals before EOF works.

Fix

Treat signal() as a session channel request rather than writable stdin data:

  • allow it while outgoing.state is open or eof;
  • do not gate it on Node's writable flag.

The regression test models the post-EOF channel state directly because the in-process ssh2 test server closes its side on received EOF, unlike the OpenSSH target used to reproduce the real issue.

Validation

  • node test/test-exec.js
  • npm run lint -- --no-cache
  • npm test
  • git diff --check

All pass on current master.

I also tested the downstream paired behaviour against a real OpenSSH server: using SSH EOF without closing the channel, plus this change, the timed-out remote process was gone at +3s and +8s.

@paulcakeface paulcakeface mentioned this pull request Aug 20, 2026
Closed
@tufantunc tufantunc mentioned this pull request Aug 20, 2026
Merged
tufantunc added a commit to tufantunc/ssh-mcp that referenced this pull request Aug 20, 2026
@tufantunc @claude
fix: stop a timed-out or cancelled command on the host, not just in t…
889a860 
…he reply

exec closes stdin as soon as the command is dispatched, because a command that
reads stdin would otherwise wait for input nobody will send. ssh2's
Channel.signal() writes the request only while the channel is writable and its
outgoing state is 'open', and end() clears both — silently. So every signal sent
to stop a command was discarded inside ssh2 and the command ran to completion on
the host, while the caller was told it had timed out (#146).

Measured against OpenSSH 10.3p1, a 30s sleep as the victim:

  end() then INT/TERM/close()      alive at +4s, alive at +9s
  INT/TERM without closing stdin   gone by +4s
  end() then close(), no signal    alive at +9s
  end() then the signal below      gone by +4s

A delivered signal is the only thing that stops a non-tty command; closing the
channel does not, for the same reason killing a local `ssh host 'sleep 30'`
leaves the sleep running. That also means the reporter's proposed end() -> eof()
change does nothing by itself: 'eof' fails ssh2's check exactly as writable
false does. It works only together with mscdex/ssh2#1510.

Rather than wait for that — ssh2 releases roughly annually — the request now
goes through the protocol object ssh2's own method would have used. A channel
request after EOF is legal SSH; only ssh2's bookkeeping objected.

Cancellation was affected too, which the report did not mention: the same closed
stdin sits in front of the abort handler, so a command an operator explicitly
cancelled kept running. For a server whose job is to gate what an agent may run,
"stopped" is a claim it makes on every timeout and cancellation, and it was true
for neither.

The ladder gained a rung: INT, TERM, KILL, then drop the channel. The old last
rung was close(), measured to stop nothing, so a command that ignored INT and
TERM ran forever. And if no signal reaches the wire at all, the error says so
rather than assuming — that path should be unreachable today, and exists so a
future ssh2 that moves what this depends on brings back a visible failure
instead of a silent one.

Interactive sessions were never affected: they write ^C into a live pty and do
not close stdin while a command runs.

Verified: the two integration cases fail on the parent commit and pass here, and
six mutations die — removing the fallback (7 tests), dropping the KILL rung,
dropping the unref, dropping the close-cancellation, never appending the
warning, and restoring the old timeout path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tufantunc added a commit to tufantunc/ssh-mcp that referenced this pull request Aug 20, 2026
@tufantunc @claude
fix: stop a timed-out or cancelled command on the host, not just in t…
17ab27d 
…he reply (#147)

exec closes stdin as soon as the command is dispatched, because a command that
reads stdin would otherwise wait for input nobody will send. ssh2's
Channel.signal() writes the request only while the channel is writable and its
outgoing state is 'open', and end() clears both — silently. So every signal sent
to stop a command was discarded inside ssh2 and the command ran to completion on
the host, while the caller was told it had timed out (#146).

Measured against OpenSSH 10.3p1, a 30s sleep as the victim:

  end() then INT/TERM/close()      alive at +4s, alive at +9s
  INT/TERM without closing stdin   gone by +4s
  end() then close(), no signal    alive at +9s
  end() then the signal below      gone by +4s

A delivered signal is the only thing that stops a non-tty command; closing the
channel does not, for the same reason killing a local `ssh host 'sleep 30'`
leaves the sleep running. That also means the reporter's proposed end() -> eof()
change does nothing by itself: 'eof' fails ssh2's check exactly as writable
false does. It works only together with mscdex/ssh2#1510.

Rather than wait for that — ssh2 releases roughly annually — the request now
goes through the protocol object ssh2's own method would have used. A channel
request after EOF is legal SSH; only ssh2's bookkeeping objected.

Cancellation was affected too, which the report did not mention: the same closed
stdin sits in front of the abort handler, so a command an operator explicitly
cancelled kept running. For a server whose job is to gate what an agent may run,
"stopped" is a claim it makes on every timeout and cancellation, and it was true
for neither.

The ladder gained a rung: INT, TERM, KILL, then drop the channel. The old last
rung was close(), measured to stop nothing, so a command that ignored INT and
TERM ran forever. And if no signal reaches the wire at all, the error says so
rather than assuming — that path should be unreachable today, and exists so a
future ssh2 that moves what this depends on brings back a visible failure
instead of a silent one.

Interactive sessions were never affected: they write ^C into a live pty and do
not close stdin while a command runs.

Verified: the two integration cases fail on the parent commit and pass here, and
six mutations die — removing the fallback (7 tests), dropping the KILL rung,
dropping the unref, dropping the close-cancellation, never appending the
warning, and restoring the old timeout path.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@github-actions github-actions Bot mentioned this pull request Aug 20, 2026
Merged
@tufantunc tufantunc mentioned this pull request Aug 20, 2026
Merged
tufantunc added a commit to tufantunc/ssh-mcp that referenced this pull request Aug 20, 2026
@tufantunc @claude
fix: verify the stop instead of assuming it, and say so when it canno…
76eaf66 
…t be verified (#149)

* fix: verify the stop instead of assuming it, and say so when it cannot be verified

Follow-up to #146 from a review round on it. Three ways the new kill ladder could
still fail silently — each the same shape as the bug it follows, a stop that
reports success without having happened.

A signal could be "delivered" through a dead socket. ssh2 hands every packet to
onWrite, which is `if (isWritable(sock)) sock.write(data)` (lib/client.js:303):
an unwritable socket drops it with no error and no return value. Measured against
1.17.0 — immediately after client.end(), sock.writable is false while the
channel's outgoing state is still 'eof', so every guard passed and the call
returned without throwing. Reachable whenever a connection is closed under a
running command, including by the idle reaper, which does not consult
activeChannels.

A timeout that fired before the exec channel existed left the command running.
ssh2 invokes the exec callback on CHANNEL_SUCCESS, which OpenSSH sends *after*
forking the command, and openWithRetry can spend three attempts before that. The
caller was told it timed out; the command then started, ran to completion, held a
channel and had its output discarded. The comment I added in the first fix
asserted this could not happen. A late channel is now stopped on arrival, and it
does not warn — the command is stopped, just later, and a warning that fires when
nothing is wrong stops being read.

close-session on a background session dropped the channel without signalling —
the rung this project measured as stopping nothing. It reported status 'closed'
while the command kept running. It now goes through the same ladder as exec.

Two claims corrected rather than the code:

  - The return value means the request was *dispatched*, not that the process
    stopped. Nothing acknowledges a signal request, and it reaches the session
    leader rather than the process group: measured, `sh -c 'trap "" INT TERM;
    sleep N'` loses the shell to KILL and leaves the sleep orphaned. A test pins
    that, SECURITY.md documents the whole ladder including that cancellation
    reaches SIGKILL without a separate policy check, and the message no longer
    implies termination.
  - The comment claiming the ssh2 workaround would retire itself was wrong: our
    own copy of ssh2's condition keeps refusing the public path even after
    mscdex/ssh2#1510 lands, and the tripwire grepped for substrings that diff
    preserves. There is now one named predicate to widen, and a tripwire that
    calls ssh2's real method instead of reading its source.

Also: ssh.unstopped is set on all three settle paths through one helper (both
cancellation paths computed it and dropped it before the span, so it could never
be true for a cancelled command); the signal name is a union type; and ssh2 is
pinned exactly for as long as the internals path exists, since the tripwires run
here and not at a consumer's install.

The unit test for the fallback's error path spread the factory instead of calling
it, so it exercised the public path and left the only ssh2-internals-dependent
branch uncovered — measured at 96.66% lines, 100% with the missing parentheses.
The same trap the factory's own comment was written about, one call later.

Verified: 642 tests against the live containers, and four mutations die —
removing the transport check, removing the late-channel guard (2 tests), reverting
the background-session close, and restoring the missing parentheses.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix: audit the stop that signals a host, and correct the claims about what it stops

Second review round on the #146 fix. Two Highs, several Mediums, and one claim of
mine that was simply false.

close-session signalled a remote process with no policy check and no audit
record. My previous commit made BackgroundSession.close() stop its command
(INT/TERM/KILL) instead of only dropping the channel — which was measured to stop
nothing. That turned a harmless unaudited call into a real one: signal-process
puts the same three signals through policy, approval and the audit log, while
close-session reached the connection directly. Traced on a readOnly profile: a
read-only `tail -f` is allowed, then close-session delivers SIGKILL, because the
readOnly gate lives inside the engine that call never enters. It now goes through
runAudited like open-session, its tool description says what it does, and
shutdown closes connections before the audit log — with the old order the records
this adds were written to a closed stream.

The transport guard I added last commit copied one of ssh2's three conjuncts.
`isWritable` is `writable && _readableState && ended === false`, and ssh2 added
the last two for nodejs/node#36029. Measured: on a peer FIN with allowHalfOpen a
socket sits at writable=true while isWritable() is false — indefinitely — and a
ProxyJump connection's transport is exactly that, since ssh2's channels default
to allowHalfOpen. So the guard was blind on the path that has its own integration
test, and green only in the self-inflicted `client.end()` case I happened to
measure. It now mirrors the whole predicate and fails closed on a shape it cannot
read.

The escalation was abandoned mid-ladder. Its rungs are timers, and
SSHConnection.close() ends the client on the line after closing its sessions, so
a background command that ignored INT got nothing further — the case KILL exists
for. Closing a background session now waits for the ladder, bounded by its own
length, and reports whether the stop was even dispatched instead of always
saying "closed".

The process-group claim was wrong, and wrong in the direction that understates a
SIGKILL. I wrote that a signal reaches the session leader and orphans children.
OpenSSH uses killpg() on the process group (session.c, session_signal_req):
measured on 10.3p1, a shell and its child share one pgid and one KILL request
removes both. The "orphan" I cited was debris leaked by my own earlier probe —
which is also why the test built on it was red on a clean container and green on
its second run, reading its own garbage. That test is replaced by one that pins
the measured behaviour and reaps with kill -9, since anything inheriting SIG_IGN
from a trap cannot be reaped by pkill's default signal.

SECURITY.md also claimed signal-process classifies its signals as destructive.
Measured: `kill -KILL <pid>` classifies as safe, so ask-destructive never
prompted for it. The document now says that and names the settings that do gate
it. The classifier is unchanged — making kill destructive is a behaviour change
that needs a deliberate call, and it is queued.

Also: ssh.unstopped on every settle path with the deferred case marked
ssh.stopDeferred rather than asserting a clean stop it cannot know; the late
channel's stop recorded on its own span, since the exec span is already ended;
stopAndDescribe lifted to module scope so it is testable without a module mock;
the exact ssh2 pin reverted — the lockfile already froze CI, the behavioural
tripwire is the better guard, and an exact pin freezes a published consumer's SSH
client until we cut a release.

The late-channel test's fake was in the post-end() state, which that path never
reaches, so it exercised the fallback instead of the public branch production
uses; it now transitions on end() like the real thing. The background-session
test's 20s poll budget measured nothing (the command dies in ~5ms) and could
overrun its own timeout; it asserts elapsed time instead.

Verified: 647 tests against the live containers, 29 e2e, and four mutations die —
one-conjunct transport check, failing open on an unreadable transport, not
awaiting the ladder, and close-session going around the pipeline.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix: audit the stop instead of gating it, and stop claiming a close that did not happen

Third review round. Round 2's fixes produced round 3's defects, at the same seam
every time — the gap between stopping a command and the claims made about having
stopped it. The pattern only broke by narrowing the change rather than adding
another layer.

close-session could refuse to close. Routing it through runAudited let policy
veto a *release*: `session:close <name>` classifies as safe, and a readOnly
profile can open a background session — a `tail -f` classifies read-only — but was
then denied permission to close it, with no other way to stop the command until
the 1h cap expired. Measured through the real engine: `deny, role-binding`. Same
for the viewer role, for ask-all (a prompt on every close), and for an exhausted
quota (which wedged the profile entirely). A control whose refusal mode is "the
thing you asked me to stop keeps running" is worse than the unaudited stop it
replaced. The record was the part worth having, so the close now happens
unconditionally and the record is written after it, with ruleId session-release so
the log distinguishes it from an engine decision, and the session's kind in the
command so a remote SIGKILL is not indistinguishable from ending a local shell.

The awaited ladder's answer was discarded. `waitForChannelClose` returns false
when the whole escalation elapsed with the channel still open — the strongest
evidence available that the command survived INT, TERM and KILL — and close()
threw it away and returned 'closed'. That is the false claim this branch exists to
remove, in the function added to remove it. CloseOutcome gained the third state it
was always computing.

Sequential teardown was measured at 10.0s for five commands that ignore INT and
TERM, against Docker's 10s default grace: the container was SIGKILLed mid-teardown
and the later sessions got no escalation at all, which is worse than before the
wait existed. Sessions and connections now close concurrently, shutdown is bounded
at 5s, and compose sets an explicit stop_grace_period. The wait is also skipped
when nothing was dispatched — no rung can reach a transport that refused the
first, so that was 3.5s of measured dead time.

The reaper fired close() without awaiting it, so `reapIdleConnections` — which
gates on sessionCount, zeroed in the same tick — ended the client microseconds
after the first signal and discarded TERM and KILL. That was the one hole left in
the guarantee waitForChannelClose exists for.

Two comments of mine were false. session-tools said signal-process classifies its
signals as destructive; the same diff had already corrected that in SECURITY.md,
and `kill -KILL` measures as safe. And the shutdown reorder was justified by audit
records that path never writes — nothing under src/ssh touches the audit store —
so it rescued nothing while putting the final flush behind a teardown that now
waits. Audit flushes first again.

SECURITY.md's stop section is now a table of all six triggers with what each does
and does not check or record, because the previous version named three, claimed
close-session was policy-gated, and claimed ssh.unstopped was set on every stop
when two of three paths do not set it.

Verified: 659 tests against the live containers, 29 e2e. Six mutations die,
including two that survived the first attempt and drove new tests — hardcoding
CloseOutcome to 'closed' (which 49 tool-layer tests had missed, because they stub
the connection rather than the session) and reverting the reaper to
fire-and-forget.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@paulcakeface