Repository navigation
Gate the Ctrl-C handler's output on input typed after the echo - #1352
Conversation
it_ctrl_c_interrupts compared "READY\r\n^CCAUGHT\r\n" byte for byte, but n_tty sends SIGINT (isig) before it queues the "^C" echo and writes the echo only at the end of the receive call (flush_echoes). A handler that prints at once can write in between. On main, Host-Root-x64-SnapshotEnabled failed TRY 1 with fcvm returning "READY\r\nCAUGHT\r\n^C": same bytes, the guest kernel's order. - The handler now runs `read line` before `echo CAUGHT`, and the case types "\n" only after it has seen "^C". A newline's echo is marked before the line wakes the reader and n_tty_write sends marked echoes first, so both sides produce "READY\r\n^C\r\nCAUGHT\r\n". - a_typed_signal_key_is_compared_only_after_its_echo checks every case that compares stdout: a Type step holding VINTR, VQUIT or VSUSP must be followed by WaitFor of its echo and then a Type. It fails on the old table and asserts that it fires on the old step list. Tested: the new test failed on the unchanged table and passes with the fix; make test-root FILTER="-E 'binary(test_exec_podman_parity)'": 2 passed, 66 cases, 0 differences, it_ctrl_c_interrupts exit 130 for all three targets.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 56 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
it_ctrl_c_interruptsintest_exec_matches_podman_execcompared an output order that the kernel does not fix. This changes the case so the order is fixed, and adds a check over the case table that rejects the old shape.The failure
Host-Root-x64-SnapshotEnabled on main failed TRY 1 and passed on retry:
The same bytes in a different order. The order comes from the guest kernel's line discipline, not from fcvm: the client's terminal is raw before the session starts (
RawModeGuard::enterinsrc/commands/tty.rs), so the only^Cecho is the guest PTY's, and fc-agent forwards the master's bytes in the order it reads them.Mechanism
For a signal key,
n_tty_receive_signal_char(drivers/tty/n_tty.c, 6.18.50) callsisig()first, which sends SIGINT, and only thenecho_char()+commit_echoes(). Below the commit watermarkcommit_echoeswrites nothing; the echo goes out inflush_echoes()at the end of__receive_buf. The shell's handler runs on another CPU, and itsecho CAUGHT(n_tty_write->process_echoes) sends only echoes already marked. If it writes between__isigandecho_char,CAUGHT\r\nreaches the master before^C. On bare metal the window is a few instructions; in a VM the vCPU running the flush work can be descheduled by the host inside it. 600 runs of the same script on a host PTY (300 unpinned, 300 pinned to one CPU) all came out^Cfirst.Fix
The handler now reads a line before it prints, and the case types that line only after it has seen
^C:A newline's echo is marked (
commit_echoes) before the line wakes the reader (n_tty_receive_handle_newline), andn_tty_writesends marked echoes before the handler's bytes, so the expected output isREADY\r\n^C\r\nCAUGHT\r\nfor podman and fcvm alike. The case still checks that Ctrl-C reaches the command, that the PTY echoes it, and exit 130.a_typed_signal_key_is_compared_only_after_its_echochecks every case that compares stdout: aTypestep holding VINTR, VQUIT or VSUSP must be followed byWaitForof its echo and then aType. It also asserts that the check fires on the old step list.Contract: test harness only, no product code. Downstream impact: removes one source of retries in the Host-Root lanes.
Evidence
Before the case change, the new test failed:
With it:
The gated script on a host PTY gave
READY\r\n^C\r\nCAUGHT\r\nin 100 of 100 runs.make lintpasses.