-
Notifications
You must be signed in to change notification settings - Fork 2
feat(terminal): add opt-in flow control #67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 4 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
424c2da
feat(terminal): add opt-in flow control
minpeter a4aeb38
Merge origin/main into feat/flow-control-631
minpeter 96f511e
fix(terminal): harden flow-control lifecycle
minpeter 73fb73f
fix(terminal): close flow-control review gaps
minpeter ae0e50d
fix(terminal): complete flow-control review
minpeter f586a57
fix(terminal): finish flow-control ownership review
minpeter b298f53
fix(terminal): close flow-control ownership gaps
minpeter a73be99
fix(terminal): drain retained flow-control state
minpeter 56f3364
fix(terminal): quiesce flow-control shutdown
minpeter 03338d2
Merge origin/main into feat/flow-control-631
minpeter 5d868de
test(terminal): flush flow-control PTY input
minpeter beda35e
test(terminal): await flow-control shell readiness
minpeter ec57a39
test(server): synchronize jumphost response teardown
minpeter 36e1161
fix(flow-control): close final shutdown gaps
minpeter c3b20da
fix(jumphost): drain buffered destination packets
minpeter f27049d
fix(jumphost): resume buffered output after backpressure
minpeter 8d1e1e4
Merge origin/main into feat/flow-control-631
minpeter db29c03
fix(flow-control): preserve final forwarding ownership
minpeter 57b7304
Merge origin/main into feat/flow-control-631
minpeter 7da329f
fix(recovery): protect returning sockets through terminal HUP
minpeter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| --- | ||
| packages: | ||
| et: | ||
| type: patch | ||
| --- | ||
|
|
||
| ## Add opt-in terminal flow control | ||
|
|
||
| Clients can now select lossless backpressure or oldest-output discard when | ||
| terminal output outruns the network, keeping Ctrl-C and prompt responses | ||
| bounded without changing the default session behavior. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| #![cfg(unix)] | ||
| #![forbid(unsafe_code)] | ||
|
|
||
| mod flow_control_tty_support; | ||
|
|
||
| use std::fs; | ||
| use std::io::{Read, Write}; | ||
| use std::sync::mpsc; | ||
| use std::thread; | ||
| use std::time::{Duration, Instant}; | ||
|
|
||
| use flow_control_tty_support::{ | ||
| receive_bytes, receive_until, Stack, ThrottleProxy, MAX_PROMPT_LATENCY, SATURATION_BYTES, | ||
| THROTTLE_BYTES_PER_SECOND, | ||
| }; | ||
| use portable_pty::{native_pty_system, CommandBuilder, PtySize}; | ||
|
|
||
| #[test] | ||
| fn flow_control_keeps_ctrl_c_and_prompt_responsive_on_a_slow_link() { | ||
| let evidence = std::env::var_os("ET_FLOW_QA_EVIDENCE_DIR").map(std::path::PathBuf::from); | ||
| if let Some(directory) = &evidence { | ||
| fs::create_dir_all(directory).unwrap(); | ||
| } | ||
|
|
||
| for mode in ["none", "backpressure", "discard"] { | ||
| let stack = Stack::start(); | ||
| let bytes_per_second = THROTTLE_BYTES_PER_SECOND; | ||
| let proxy = ThrottleProxy::start(stack.port, bytes_per_second); | ||
| let pair = native_pty_system() | ||
| .openpty(PtySize { | ||
| rows: 24, | ||
| cols: 80, | ||
| pixel_width: 800, | ||
| pixel_height: 480, | ||
| }) | ||
| .unwrap(); | ||
| let mut client = CommandBuilder::new(env!("CARGO_BIN_EXE_et")); | ||
| client.args([ | ||
| "--flow-control", | ||
| mode, | ||
| "--terminal-path", | ||
| stack.terminal.to_str().unwrap(), | ||
| "--serverfifo", | ||
| stack.router.to_str().unwrap(), | ||
| "-p", | ||
| &proxy.port.to_string(), | ||
| "127.0.0.1", | ||
| ]); | ||
| client.env( | ||
| "PATH", | ||
| format!( | ||
| "{}:{}", | ||
| stack.directory.display(), | ||
| std::env::var("PATH").unwrap() | ||
| ), | ||
| ); | ||
| client.env("TERM", "xterm-256color"); | ||
| let mut child = pair.slave.spawn_command(client).unwrap(); | ||
| drop(pair.slave); | ||
|
|
||
| let mut writer = pair.master.take_writer().unwrap(); | ||
| let mut reader = pair.master.try_clone_reader().unwrap(); | ||
| // Keep the test harness from adding its own half-megabyte output | ||
| // queue on top of the ET pipeline being measured. | ||
| let (sender, receiver) = mpsc::sync_channel(4); | ||
| let reader_thread = thread::spawn(move || { | ||
| let mut chunk = [0u8; 8192]; | ||
| loop { | ||
| match reader.read(&mut chunk) { | ||
| Ok(0) | Err(_) => return, | ||
| Ok(count) if sender.send(chunk[..count].to_vec()).is_err() => return, | ||
| Ok(_) => {} | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| writer | ||
| .write_all( | ||
| b"printf 'FLOW-%s\\n' START; while :; do printf \ | ||
| '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef'; done\n", | ||
| ) | ||
| .unwrap(); | ||
| let startup_timeout = Duration::from_secs(10); | ||
| let output = match receive_until(&receiver, Vec::new(), b"FLOW-START\r\n", startup_timeout) | ||
| { | ||
| Ok(output) => output, | ||
| Err(error) => { | ||
| child.kill().unwrap(); | ||
| drop(writer); | ||
| let _ = child.wait(); | ||
| reader_thread.join().unwrap(); | ||
| panic!("{mode}: waiting for FLOW-START: {error}"); | ||
| } | ||
| }; | ||
| let mut output = | ||
| match receive_bytes(&receiver, output, SATURATION_BYTES, Duration::from_secs(40)) { | ||
| Ok(output) => output, | ||
| Err(error) => { | ||
| child.kill().unwrap(); | ||
| drop(writer); | ||
| let _ = child.wait(); | ||
| reader_thread.join().unwrap(); | ||
| panic!("{mode}: saturating throttled link: {error}"); | ||
| } | ||
| }; | ||
| let interrupted = Instant::now(); | ||
| writer | ||
| .write_all(b"\x03printf 'FLOW-%s\\n' PROMPT\n") | ||
| .unwrap(); | ||
| let prompt_timeout = MAX_PROMPT_LATENCY; | ||
| let prompt = receive_until( | ||
| &receiver, | ||
| output.clone(), | ||
| b"FLOW-PROMPT\r\n", | ||
| prompt_timeout, | ||
| ); | ||
| let latency = interrupted.elapsed(); | ||
| if mode == "none" { | ||
| assert!( | ||
| prompt.is_err(), | ||
| "none baseline unexpectedly met the {MAX_PROMPT_LATENCY:?} latency criterion" | ||
| ); | ||
| } else { | ||
| output = prompt.unwrap_or_else(|error| { | ||
| panic!("{mode}: waiting for Ctrl-C prompt within {prompt_timeout:?}: {error}") | ||
| }); | ||
| assert!( | ||
| latency <= MAX_PROMPT_LATENCY, | ||
| "{mode} Ctrl-C-to-prompt latency {latency:?} exceeded {MAX_PROMPT_LATENCY:?}" | ||
| ); | ||
| } | ||
|
|
||
| child.kill().unwrap(); | ||
| drop(writer); | ||
| let _ = child.wait(); | ||
| while let Ok(chunk) = receiver.recv() { | ||
| output.extend(chunk); | ||
| } | ||
| reader_thread.join().unwrap(); | ||
| proxy.finish().unwrap(); | ||
|
|
||
| if let Some(directory) = &evidence { | ||
| fs::write(directory.join(format!("{mode}.ansi")), &output).unwrap(); | ||
| fs::write( | ||
| directory.join(format!("{mode}.json")), | ||
| format!( | ||
| "{{\"mode\":\"{mode}\",\"rate_bytes_per_second\":{bytes_per_second},\ | ||
| \"saturation_bytes\":{SATURATION_BYTES},\"ctrl_c_prompt_millis\":{},\ | ||
| \"expected_latency_failure\":{},\"scenario_pass\":true}}\n", | ||
| latency.as_millis(), | ||
| mode == "none" | ||
| ), | ||
| ) | ||
| .unwrap(); | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: When a client sends an unrecognized
flowcontrolvalue (any integer other than 0/1/2),FlowControlMode::try_from(value).ok()drops the error and.unwrap_or(FlowControlMode::None)silently downgrades the session toNone. BecauseNonekeeps the unbounded synchronous output path, a client that negotiates an unsupported or future mode fails open, defeating the output-bounding this feature exists to provide. Return an error for unrecognized modes instead of silently treating them as "no flow control" so the failure is explicit.Prompt for AI agents