Closes#231. Stacked on #229 — based on t3code/resolve-issue-199, so review that one first and this diff will shrink to its own commit once #229 merges.
What
SSHSession.#requestLoop and serveSession each counted every channel request a session had ever received and aborted the channel past 4096. Both counters are monotonic: they bound a session's whole lifetime, not the work outstanding at any moment.
That was the right shape when they were written, because the queue behind them was unbounded and a lifetime ceiling was the only thing between an undrained channel and unbounded growth. #229 bounds the queue at its source, which leaves these guarding nothing — and costing something. The loop drains and discards each request, so memory does not grow with the count; the counter only limits total CPU over the session's life. window-change is among the requests it counts, and a client sends one per terminal resize, so a long-lived interactive shell can reach 4096 and be killed for behaving normally.
Why the handlers changed too
Removing the counters only leaves the peer bounded if the backlog cap is the thing bounding it, which needs the loop to be uniformly serial. windowChange and signal were typed => void and invoked bare, and TypeScript's void-return assignability accepts an async handler, so one could be passed and left running unawaited — with the lifetime counter as the only cap on how many a peer could start. They are now void | Promise<void> and awaited. Existing synchronous handlers stay assignable; an async one that previously ran detached now blocks the next request on that session, which is the safer semantic and the one every other request type already had.
Breaking change
SessionLimits.maxRequests is removed. maxEnv and maxEnvBytes are unaffected.
Testing
test, typecheck, typetest, and lint for @webnet/ssh and @webnet/sftp, plus repository-wide lint and format:check.
The test covering the removed counter is replaced by two: one driving a session past the old 4096 ceiling and asserting all 5000 requests are serviced, using a want_reply round trip as an in-order barrier rather than a timing guess; and one asserting an async signal handler never overlaps itself. Restoring the bare call makes the second fail.
Closes #231. Stacked on #229 — based on `t3code/resolve-issue-199`, so review that one first and this diff will shrink to its own commit once #229 merges.
## What
`SSHSession.#requestLoop` and `serveSession` each counted every channel request a session had ever received and aborted the channel past 4096. Both counters are monotonic: they bound a session's whole lifetime, not the work outstanding at any moment.
That was the right shape when they were written, because the queue behind them was unbounded and a lifetime ceiling was the only thing between an undrained channel and unbounded growth. #229 bounds the queue at its source, which leaves these guarding nothing — and costing something. The loop drains and discards each request, so memory does not grow with the count; the counter only limits total CPU over the session's life. `window-change` is among the requests it counts, and a client sends one per terminal resize, so a long-lived interactive shell can reach 4096 and be killed for behaving normally.
## Why the handlers changed too
Removing the counters only leaves the peer bounded if the backlog cap is the thing bounding it, which needs the loop to be uniformly serial. `windowChange` and `signal` were typed `=> void` and invoked bare, and TypeScript's void-return assignability accepts an `async` handler, so one could be passed and left running unawaited — with the lifetime counter as the only cap on how many a peer could start. They are now `void | Promise<void>` and awaited. Existing synchronous handlers stay assignable; an async one that previously ran detached now blocks the next request on that session, which is the safer semantic and the one every other request type already had.
## Breaking change
`SessionLimits.maxRequests` is removed. `maxEnv` and `maxEnvBytes` are unaffected.
## Testing
`test`, `typecheck`, `typetest`, and `lint` for `@webnet/ssh` and `@webnet/sftp`, plus repository-wide `lint` and `format:check`.
The test covering the removed counter is replaced by two: one driving a session past the old 4096 ceiling and asserting all 5000 requests are serviced, using a `want_reply` round trip as an in-order barrier rather than a timing guess; and one asserting an async `signal` handler never overlaps itself. Restoring the bare call makes the second fail.
Both loops counted every request a session had ever received and aborted
the channel past 4096. That was the right shape while the queue behind
them was unbounded; now that Channel bounds the backlog at its source, a
lifetime ceiling guards nothing and costs something. window-change is
counted, and a client sends one per terminal resize, so a long-lived
interactive shell could be killed for behaving normally.
Removing them leaves the peer bounded by the backlog cap only if the loop
is uniformly serial, so windowChange and signal are now awaited. They were
typed `=> void` and called bare, and void-return assignability let an
async handler through to run unawaited.
Removes SessionLimits.maxRequests, which is a breaking change.
Closes#231.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reviewed only this PR's own diff (t3code/resolve-issue-199...t3code/resolve-issue-231), not the base branch's #229.
The safety argument holds. With the lifetime counters gone, Channel._deliverRequest (packages/ssh/src/channel.ts:172) is the only remaining backstop. It aborts a channel once #requestQueue exceeds maxQueuedChannelRequests (default 4096). Both request loops (SSHSession#requestLoop and serveSession's loop) now process requests strictly serially, including the newly-awaited windowChange/signal, so the queue can never grow past that cap no matter how many requests a peer has sent over the session's life. Every request kind (env, pty-req, window-change, signal, exec/shell/subsystem) is drained one at a time and discarded, so memory is bounded by queue depth, not by history. I don't see a path a peer gains that it didn't have before.
Awaiting windowChange/signal doesn't introduce a deadlock or block the shared read loop. ConnectionMux's read loop only pushes onto each channel's own request queue via _deliverRequest and never waits on serveSession's consumer, so a slow or hanging handler stalls only its own channel. It fills that channel's queue until _deliverRequest aborts it, the same isolation the removed counter used to provide. Errors thrown by an awaited handler are still caught by the loop's existing per-iteration try/catch at the end of the switch block in session.ts, so a throwing async handler fails only that one request, same as exec/shell/subsystem already did.
Both replacement tests in session.test.ts pass for the right reasons, not vacuously. "a session serviced past the old lifetime ceiling keeps working" sends 5000 signal requests past the old 4096 ceiling and asserts the session is still alive and exits cleanly; reverting the counter removal fails this at request 4097. "an async signal handler is awaited rather than left running" fires 20 non-reply signal requests at a handler that tracks concurrent invocations and asserts peak overlap is 1; reverting the await on handlers.signal?.(...) lets all 20 fire concurrently and fails the assertion.
Nothing left over: no stale maxRequests/MAX_PEER_REQUESTS references remain in session.ts or session.test.ts, no other lifetime-style counter exists elsewhere in the package, and SessionLimits.maxRequests was never mentioned in README.md before this change, so there's no doc note to update despite it being a breaking API removal.
No defects found. This is a clean, well-reasoned removal.
Reviewed only this PR's own diff (t3code/resolve-issue-199...t3code/resolve-issue-231), not the base branch's #229.
The safety argument holds. With the lifetime counters gone, Channel._deliverRequest (packages/ssh/src/channel.ts:172) is the only remaining backstop. It aborts a channel once #requestQueue exceeds maxQueuedChannelRequests (default 4096). Both request loops (SSHSession#requestLoop and serveSession's loop) now process requests strictly serially, including the newly-awaited windowChange/signal, so the queue can never grow past that cap no matter how many requests a peer has sent over the session's life. Every request kind (env, pty-req, window-change, signal, exec/shell/subsystem) is drained one at a time and discarded, so memory is bounded by queue depth, not by history. I don't see a path a peer gains that it didn't have before.
Awaiting windowChange/signal doesn't introduce a deadlock or block the shared read loop. ConnectionMux's read loop only pushes onto each channel's own request queue via _deliverRequest and never waits on serveSession's consumer, so a slow or hanging handler stalls only its own channel. It fills that channel's queue until _deliverRequest aborts it, the same isolation the removed counter used to provide. Errors thrown by an awaited handler are still caught by the loop's existing per-iteration try/catch at the end of the switch block in session.ts, so a throwing async handler fails only that one request, same as exec/shell/subsystem already did.
Both replacement tests in session.test.ts pass for the right reasons, not vacuously. "a session serviced past the old lifetime ceiling keeps working" sends 5000 signal requests past the old 4096 ceiling and asserts the session is still alive and exits cleanly; reverting the counter removal fails this at request 4097. "an async signal handler is awaited rather than left running" fires 20 non-reply signal requests at a handler that tracks concurrent invocations and asserts peak overlap is 1; reverting the await on handlers.signal?.(...) lets all 20 fire concurrently and fails the assertion.
Nothing left over: no stale maxRequests/MAX_PEER_REQUESTS references remain in session.ts or session.test.ts, no other lifetime-style counter exists elsewhere in the package, and SessionLimits.maxRequests was never mentioned in README.md before this change, so there's no doc note to update despite it being a breaking API removal.
No defects found. This is a clean, well-reasoned removal.
codinget
marked the pull request as ready for review 2026-08-24 20:27:54 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
codinget
Closes #231. Stacked on #229 — based on
t3code/resolve-issue-199, so review that one first and this diff will shrink to its own commit once #229 merges.What
SSHSession.#requestLoopandserveSessioneach counted every channel request a session had ever received and aborted the channel past 4096. Both counters are monotonic: they bound a session's whole lifetime, not the work outstanding at any moment.That was the right shape when they were written, because the queue behind them was unbounded and a lifetime ceiling was the only thing between an undrained channel and unbounded growth. #229 bounds the queue at its source, which leaves these guarding nothing — and costing something. The loop drains and discards each request, so memory does not grow with the count; the counter only limits total CPU over the session's life.
window-changeis among the requests it counts, and a client sends one per terminal resize, so a long-lived interactive shell can reach 4096 and be killed for behaving normally.Why the handlers changed too
Removing the counters only leaves the peer bounded if the backlog cap is the thing bounding it, which needs the loop to be uniformly serial.
windowChangeandsignalwere typed=> voidand invoked bare, and TypeScript's void-return assignability accepts anasynchandler, so one could be passed and left running unawaited — with the lifetime counter as the only cap on how many a peer could start. They are nowvoid | Promise<void>and awaited. Existing synchronous handlers stay assignable; an async one that previously ran detached now blocks the next request on that session, which is the safer semantic and the one every other request type already had.Breaking change
SessionLimits.maxRequestsis removed.maxEnvandmaxEnvBytesare unaffected.Testing
test,typecheck,typetest, andlintfor@webnet/sshand@webnet/sftp, plus repository-widelintandformat:check.The test covering the removed counter is replaced by two: one driving a session past the old 4096 ceiling and asserting all 5000 requests are serviced, using a
want_replyround trip as an in-order barrier rather than a timing guess; and one asserting an asyncsignalhandler never overlaps itself. Restoring the bare call makes the second fail.Reviewed only this PR's own diff (t3code/resolve-issue-199...t3code/resolve-issue-231), not the base branch's #229.
The safety argument holds. With the lifetime counters gone, Channel._deliverRequest (packages/ssh/src/channel.ts:172) is the only remaining backstop. It aborts a channel once #requestQueue exceeds maxQueuedChannelRequests (default 4096). Both request loops (SSHSession#requestLoop and serveSession's loop) now process requests strictly serially, including the newly-awaited windowChange/signal, so the queue can never grow past that cap no matter how many requests a peer has sent over the session's life. Every request kind (env, pty-req, window-change, signal, exec/shell/subsystem) is drained one at a time and discarded, so memory is bounded by queue depth, not by history. I don't see a path a peer gains that it didn't have before.
Awaiting windowChange/signal doesn't introduce a deadlock or block the shared read loop. ConnectionMux's read loop only pushes onto each channel's own request queue via _deliverRequest and never waits on serveSession's consumer, so a slow or hanging handler stalls only its own channel. It fills that channel's queue until _deliverRequest aborts it, the same isolation the removed counter used to provide. Errors thrown by an awaited handler are still caught by the loop's existing per-iteration try/catch at the end of the switch block in session.ts, so a throwing async handler fails only that one request, same as exec/shell/subsystem already did.
Both replacement tests in session.test.ts pass for the right reasons, not vacuously. "a session serviced past the old lifetime ceiling keeps working" sends 5000 signal requests past the old 4096 ceiling and asserts the session is still alive and exits cleanly; reverting the counter removal fails this at request 4097. "an async signal handler is awaited rather than left running" fires 20 non-reply signal requests at a handler that tracks concurrent invocations and asserts peak overlap is 1; reverting the await on handlers.signal?.(...) lets all 20 fire concurrently and fails the assertion.
Nothing left over: no stale maxRequests/MAX_PEER_REQUESTS references remain in session.ts or session.test.ts, no other lifetime-style counter exists elsewhere in the package, and SessionLimits.maxRequests was never mentioned in README.md before this change, so there's no doc note to update despite it being a breaking API removal.
No defects found. This is a clean, well-reasoned removal.