Skip to content

Stop reading SSH output the webview has not caught up with - #159

Open
Sadykhzadeh wants to merge 4 commits into
fix/session-lock-scopefrom
fix/terminal-output-flow-control
Open

Sadykhzadeh wants to merge 4 commits into
fix/session-lock-scopefrom
fix/terminal-output-flow-control

Conversation

@Sadykhzadeh

@Sadykhzadeh Sadykhzadeh commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Closes #158. Stacked on #140 (session.rs). Merge #140 first; this PR's base then becomes main.

What this changes

Core: a credit window per SSH session

OutputCredit (core/src/output.rs) counts bytes handed to the UI layer and not yet acknowledged, against OUTPUT_WINDOW = 2 MiB.

  • The window is several full 64 KiB batches deep, so throughput isn't bound by the IPC round trip, and memory stays bounded.
  • An over-acknowledgement saturates at zero, so it can't bank credit for later output.
  • Waiting for room uses Notify::notify_one, which keeps a permit if the refund lands before the reader starts waiting.

In the reader:

  • Each flush spends the batch length.
  • channel.wait() is polled only while there is room. Another branch waits for room otherwise.
  • Resizes and an already-due flush keep working while it is stalled.
  • Because russh delivers channel data through a bounded queue, a stalled reader backs up into russh, then the SSH window, then TCP, and finally the server.

SshSession::retire, used by close, close_all and the reader's own cleanup, lifts the limit. A reader stalled on credit therefore still reads to EOF, flushes the tail and announces the close, instead of waiting on acknowledgements that will never come.

Backend command

session_output_ack(session_id, bytes) refunds credit. An id without a live SSH session is accepted and ignored: acknowledgements trail their output and can arrive after a close, and local terminals are not metered.

Frontend

api.sessionOutput(sessionId, onData, onEnd) acknowledges batches as the webview receives them:

  • Coalescing. Acks are sent once 256 KiB is pending, or 30 ms after the first unacknowledged batch, so a trickle of output never leaves credit stranded.
  • End of stream. The zero-length end-of-stream frame is never acknowledged.
  • Failures. A failed ack is swallowed, since the pane can't act on it.
  • Call sites. TerminalPane.vue only gains the sessionId argument. The e2e fixture accepts the new command.

Acks happen on receipt, not after xterm has rendered. What this bounds is the IPC queue, which is what the issue is about. xterm's own write buffer has its own cap.

Tests

  • OutputCredit units. 7 unit tests in output.rs.
  • core/tests/output_flow_control.rs. A loopback russh server floods 64 KiB chunks and the client never acks.
    • Delivered output stops between the window and the window + 128 KiB, and the server's own send counter stops too.
    • An ack lets about one more window through.
    • close() on a stalled session still announces the close, and late acks are ignored.
    • With the throttle disabled, the test fails with "output kept growing".
  • desktop/src/api.sessionOutput.test.ts. 5 Vitest cases: coalescing, threshold, per-session ids, end of stream, and a failed ack being swallowed.

Interaction with #123

#123 puts app commands behind the capability ACL. Whichever of the two merges second needs an allow-session-output-ack entry in permissions/app-commands.toml and in the capability.

Test plan

  • cargo test -p clavyn-core: 125 unit + 5 + 32 + 1 + 4 + 9 integration, all pass (after rebasing on the current Keep the session maps unlocked across network I/O #140)
  • cargo test -p clavyn-desktop (Linux, local): 107 + 2
  • cd desktop && npx vue-tsc --noEmit: clean
  • cd desktop && npm test: 46 files / 388 tests
  • cd desktop/e2e && UI_RECORD=0 npm test: 73 passed
  • node scripts/verify/check-comments.mjs: ok
  • Real tauri dev with a slow webview: not exercised; throttling is tested at the core level and with mocked IPC

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@greptile-apps

greptile-apps Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

TREX TREX could not finish testing

  • Plan what to test
  • Set up the environment and test the changed flows
  • Post test results

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Bound SSH output queued ahead of the webview

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Limit each SSH session’s unacknowledged output so a slow webview cannot grow the IPC queue
 indefinitely.
• Acknowledge received batches in the webview and lift the limit during session closure.
• Test backpressure through a loopback SSH server, plus acknowledgement and credit behavior.
Diagram

sequenceDiagram
    participant P as SSH peer
    participant R as SSH reader
    participant C as OutputCredit
    participant I as IPC channel
    participant W as Webview
    participant A as Ack command
    P->>R: Channel output
    R->>C: Spend batch bytes
    R->>I: Send batch
    I->>W: Deliver batch
    W->>A: Ack coalesced bytes
    A->>C: Refund bytes
    C-->>R: Room to read
    Note over P,R: Reader stops polling when credit is spent
    Note over R,C: Closing lifts the limit
Loading
High-Level Assessment

Keep the per-session credit window and acknowledge on webview receipt: this applies backpressure at the IPC queue the fix targets. Acknowledging only after rendering would meter a different buffer, while acknowledging every batch would add avoidable IPC calls. Review against the stacked session.rs change from #140 before merging.

Files changed (9) +593 / -8

Bug fix (6) +296 / -8
output.rsAdd per-session output credit accounting +189/-0

Add per-session output credit accounting

• Introduces a 2 MiB unacknowledged-output window with saturating refunds, a wakeable room wait, and a permanent lift for closing sessions. Seven unit tests cover thresholds, refunds, wakeups, and lifting.

core/src/output.rs

session.rsPause SSH reads when output credit is spent +36/-4

Pause SSH reads when output credit is spent

• Shares credit with each SSH reader, charges delivered batches, and stops polling channel output until room returns while preserving resizes and due flushes. Retirement lifts the limit, and the session manager accepts acknowledgements for live sessions while ignoring missing ones.

core/src/session.rs

commands.rsExpose the session output acknowledgement command +17/-0

Expose the session output acknowledgement command

• Adds a Tauri command that refunds SSH session credit. Acknowledgements for local terminals or sessions that have already closed succeed without effect.

desktop/src-tauri/src/commands.rs

main.rsRegister the output acknowledgement command +1/-0

Register the output acknowledgement command

• Makes the new Tauri command available to the webview.

desktop/src-tauri/src/main.rs

api.tsCoalesce acknowledgements when output reaches the webview +52/-3

Coalesce acknowledgements when output reaches the webview

• Adds the acknowledgement API and tracks received bytes per output channel, sending them at 256 KiB or after 30 ms. It excludes end-of-stream frames, clears pending credit at stream end, and swallows failed acknowledgement calls.

desktop/src/api.ts

TerminalPane.vueAssociate the terminal output sink with its session +1/-1

Associate the terminal output sink with its session

• Passes the session ID to the output sink so received batches can be acknowledged against the correct session.

desktop/src/components/TerminalPane.vue

Tests (3) +297 / -0
output_flow_control.rsVerify backpressure against a flooding SSH server +200/-0

Verify backpressure against a flooding SSH server

• Adds a loopback russh integration test that checks delivered output and server sends stall without acknowledgements, resume after a refund, and still announce closure when stalled. It also checks that late acknowledgements are harmless.

core/tests/output_flow_control.rs

tauri-fixture.jsAccept output acknowledgements in the e2e fixture +1/-0

Accept output acknowledgements in the e2e fixture

• Treats the new acknowledgement command as a successful no-op so terminal e2e flows can use the updated frontend API.

desktop/e2e/tauri-fixture.js

api.sessionOutput.test.tsTest webview acknowledgement timing and boundaries +96/-0

Test webview acknowledgement timing and boundaries

• Tests delayed and threshold-based coalescing, session-specific IDs, end-of-stream behavior, and swallowed command failures.

desktop/src/api.sessionOutput.test.ts

@qodo-code-review

qodo-code-review Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Code Review by Qodo


View medium (1)
Remediation recommended
1. Retries can uncap terminal output 🐞
Description
sessionOutput resends the same byte count when an acknowledgement promise rejects, but
SessionManager::ack treats every call as a fresh refund rather than identifying the output already
acknowledged. If the backend applied the first acknowledgement but its response failed, a retry
arriving after further output is spent subtracts those bytes again and lets the reader advance
without the corresponding output being received.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A retry after an acknowledgement with an unknown outcome can refund the same output twice and defeat the credit window.
## Fix Focus Areas
- desktop/src/api.ts[117-138]
- core/src/session.rs[406-415]
- core/src/output.rs[166-176]
## Recommended Fix
Give acknowledgements a per-stream cumulative received-byte position or another deduplicated identity, and have the backend refund only newly acknowledged bytes. Cover a retry whose first attempt was applied but whose response was lost.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Dismiss ↗ | View ↗


Comment thread core/src/session.rs
Comment thread core/src/session.rs
The reader handed every batch to the IPC channel without waiting, and
large payloads sit in Tauri's in-memory queue until the webview fetches
them, so a fast or hostile server could grow that queue without bound.

Each SSH session now meters delivered output against a 2 MiB window of
unacknowledged bytes. The webview acknowledges what it receives through
session_output_ack, coalesced to every 256 KiB or 30 ms, and the reader
stops polling the channel while the window is spent. russh hands channel
data over through a bounded queue, so the stall reaches the server via
the SSH window and TCP. Closing a session lifts the limit so the reader
still reaches EOF, flushes the tail and announces the close.
The reader handed a batch to the UI layer and only then counted it
against the window. An acknowledgement that overtook the spend would be
clamped against nothing outstanding and lost, and enough of those would
leave the window spent for good.
@Sadykhzadeh
Sadykhzadeh force-pushed the fix/terminal-output-flow-control branch from 99900b3 to b91ca46 Compare October 17, 2026 06:57
Comment thread desktop/src/api.ts Outdated
The acknowledgement was dropped on failure. A reader that had used up its
window then waited for credit that no further output would ever trigger,
so one lost IPC call could freeze a busy session for good.
Comment thread desktop/src/api.ts Outdated
Comment thread core/src/sftp.rs Outdated
Comment thread desktop/src/api.ts Outdated
…wledgements for as long as the stream is open

The previous commit picked up stale copies of sftp.rs and session.rs, which
undid the close signal on single SFTP requests and moved the credit spend
back behind the batch hand-off. A capped retry could also give up during a
long outage and leave the session paused for good.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant