Conversation
Reported in #56 from a real session on 2.7.2 with 2.7.3 out. Three defects. 1. `BANNER=1 hellish` did nothing. There was no such knob. The two that existed, HELLISH_NO_BANNER and HELLISH_ALWAYS_BANNER, are two names for the two ends of one tri-state and neither is the one anybody guesses. There is now a single HELLISH_BANNER=0|1. Both old names still work -- they are documented and they are in people's rc files, so dropping them would be a worse bug than the one being fixed. Precedence is "off wins": NO_BANNER, then HELLISH_BANNER, then the legacy force, because a stray force in a login file must never override a deliberate silence on the command line. show_welcome() also read HELLISH_NO_BANNER itself, so the "should it show" rule lived in two places and only one of them would have learned about the new name. It now asks banner_should_show() and nothing else. 2. The banner could never announce a pending release. banner_should_show() re-showed the panel for "an update the user has not been told about" by testing `s.notified` -- a flag owned by a DIFFERENT channel, the prompt's one-shot between-commands notice. Whichever fired first silenced the other, and the prompt always won: the session that discovers a release draws its banner before the background check has written anything, and by the next session `notified` was set and the banner was gated off for good. So status_line()'s "X available - run update --now" row, which renders perfectly well, was unreachable in the normal flow. Users only ever learned about a release by typing `update`, which is exactly what the report shows. The banner now records what IT announced, in its own `announced` field, and re-shows whenever a pending version is not the one it last named. Two channels, two records, neither able to mute the other. It announces each release once and then leaves the standing reminder to the prompt badge -- which already persisted correctly across sessions and is now pinned so it stays that way. 3. The check was started after the banner that reports its result. main() called show_welcome() and only then maybe_spawn_update_check(), guaranteeing the discovering session read a cold cache. Swapped. Unchanged, and now guarded: the check is a detached double-fork and the prompt never waits on the network (#20). The new test points the shell at a black-holed endpoint with a cold cache and asserts first output still arrives in well under a second. tests/banner_update_test.py drives real ptys against a seeded state file -- no network, so it is deterministic on any runner -- and covers the knob's four precedence cases, the banner announcing a pending release unprompted, announcing it once per version rather than nagging, announcing again when a newer one appears, the badge persisting across sessions and clearing once the shell is current, and the two startup-timing cases. 5 checks fail before this change. Verified: pty suite 26 ok / 0 failed, make update-test green, update_badge_test and update_ui_test green, norm clean. Closes #56
…minal Reported in #58: `top &` then Ctrl-D, and the terminal you came back to had no echo and spliced your next command into garbage. Intermittently -- the same keystrokes sometimes warned and stayed, sometimes walked out. Four defects, stacked, each hiding the next. 1. Ctrl-D never asked the guard. exit_stopped_guard() existed and worked, but only the `exit` BUILTIN called it. handle_eof_or_error() and handle_eof() set should_exit themselves. Every report came in through Ctrl-D. Both now go through rl_eof_exit_ok(), one place that answers "may this EOF end the shell". 2. The guard read a stale job table. A job stopped by SIGTTIN/SIGTTOU is only recorded when the shell next reaps, at the top of a later REPL turn. `cat &` followed immediately by exit saw JOB_RUNNING and let the user out; the same pair with any command in between saw JOB_STOPPED and warned. That is the whole of the "sometimes it works" -- a race, not a flake. It reaps before deciding. 3. The warning never re-armed. exit_warned was cleared only when a BUILTIN ran, so an external command left it set: warn once, run `ps`, press Ctrl-D, and the shell walked out over a job it had already been told about. It is now aged one turn at a time in open_cycle, which is bash's actual rule -- the warning stands only while the previous turn was itself an attempt to leave. A single Ctrl-D also reaches BOTH EOF paths in one turn, so the first warned and the second read exit_warned as "already told them" and left. rl.eof_refused makes one keypress get one answer. 4. And the damage itself, which survived all three. On the deliberate exit the shell restored the terminal and left while the hung-up jobs were still dying -- and a full-screen program does not die quietly: it repaints, restores its own terminal idea and prints a farewell. With 25 stopped `top`s that is 25 processes writing to the tty AFTER the restore, on a terminal that now belongs to the parent. Hence the screenful of blank lines and the mangled command. jobs_hangup_on_exit() now sends SIGCONT+SIGHUP (continue first -- a stopped process never runs its handler) and DRAINS them, bounded to two seconds, before off() hands the terminal back. tty_snapshot_save/restore keeps the settings the shell started with, because a background job stopped mid-raw-mode can never restore them itself. The choice, since shells differ. bash refuses first and then leaves the jobs behind; it can afford that because it is normally the session leader and the kernel hangs them up when the terminal goes. A nested hellish is not, so a job left stopped there holds a raw terminal forever. hellish keeps bash's warning semantics -- refuse the first attempt, obey the second -- and then hangs up cleanly on the way out. It is also deliberately stricter than bash in one case: bash's `jobs` marks what it lists as notified and then leaves without a word, silently abandoning your stopped jobs; hellish warns anyway. tests/exit_stopped_jobs_test.py drives real ptys, uses bash as the oracle for the damaging cases, and covers: the race, both exit paths, the warning re-arming after an external command AND a builtin, many stopped jobs at once, raw-mode jobs, that the terminal comes back usable, and that nothing of ours survives. 8 checks fail before this change. Verified: pty suite 27 ok / 0 failed, golden suite 3790/3790 vs bash, norm clean. Closes #58
fix(jobs): leaving a shell with stopped jobs no longer wrecks the terminal
Two reports from real sessions. One of them could leave a terminal unusable, which is what makes this worth shipping on its own. #58 Leaving a shell that still held a stopped job wrecked the terminal. Four defects stacked: Ctrl-D never asked the stopped-jobs guard; the guard read a job table it had not reaped, so the answer depended on typing speed rather than on the jobs; the warning was forgotten only by a builtin, so any external command in between let the next Ctrl-D walk out; and on the exit the user did mean, the shell handed the terminal back while the jobs it had just hung up were still dying and still writing to it. #56 A new release stayed invisible until you typed `update`. The banner line that announces one was unreachable -- it asked a flag owned by the prompt's one-shot notice, and the prompt always spoke first. Also adds HELLISH_BANNER=0|1, since there was no knob anyone guessed. Patch, not minor: no new builtin, no new syntax. HELLISH_BANNER is a new name for a control that already existed at both ends, and both old names still work. version.h, npm/package.json, the release notes and wiki/context.md agree; `make update-config-test` green; the binary reports 2.7.4.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Ships v2.7.4. Two reports from real sessions; one of them could leave your
terminal unusable, so this is worth taking on its own if you are on 2.7.x.
#58 — leaving a shell with a stopped job wrecked the terminal
top &then Ctrl-D, and the terminal you came back to had no echo and spliced yournext command into garbage — sometimes. Four defects, stacked, each hiding the next:
exitbuiltin called it. Both end-of-input paths set "time to leave" themselves.the terminal is only recorded when the shell next reaps — on a later prompt. So
top &then an immediate exit saw a running job and let you out, while the samepair with any command in between saw a stopped one and warned. That is the entire
"sometimes it works": a race, not a flake.
command in between left it standing: warn once, run
ps, press Ctrl-D, and theshell walked out over a job it had already been told about.
terminal back and left while the jobs it had just hung up were still dying —
and a full-screen program does not die quietly. Dozens of
tops writing to thetty after the shell let go of it. It now hangs them up, waits for them, and
only then restores the terminal — including the settings it started with, because
a job stopped halfway through raw mode never can.
#56 — a new release stayed invisible until you typed
updateThe banner's "X available — run
update --now" line renders fine and was simplyunreachable: it asked a flag owned by the prompt's one-shot notice, and the prompt
always spoke first. The banner now keeps its own record and announces each release
once. Plus
HELLISH_BANNER=0|1, since there was no knob anybody guessed.Verification
developis 33/33 green on848c305, including the full platform matrix, thehard corpus, allocator parity and the pty gates. Locally: pty suite 27 ok / 0
failed, golden suite 3790/3790 vs bash, norm clean.
Closes #58