Hi Johannes, As for the first patch, a small fix is necessary as below. With the fix, the patch LGTM. Pushed to master.
Thanks! On Tue, 30 Jun 2026 17:36:59 +0900 Takashi Yano wrote: > Hi Johannes, > > The second and the third patches of your three additional patches LGTM. > Pushed to master. > Thanks! > > I'm reviewing the first one. Please wait. > > On Sat, 27 Jun 2026 09:18:41 +0200 (CEST) > Johannes Schindelin wrote: > > Hi Takashi, Mark, > > > > I had started working on those patches, been pulled away, and meant coming > > back to them but failed. The work was tracked in > > https://github.com/git-for-windows/msys2-runtime/pull/131, but I > > admittedly did not find the time to complete the work earlier. > > > > There are fixes in that PR (in addition to UI tests based on AutoHotKey > > that helped me catch a couple of bugs) for the following three issues: > > > > > > On Sat, 13 Jun 2026, Takashi Yano wrote: > > > > > diff --git a/winsup/cygwin/fhandler/pty.cc b/winsup/cygwin/fhandler/pty.cc > > > index b3a8d57cc..f4473bb69 100644 > > > --- a/winsup/cygwin/fhandler/pty.cc > > > +++ b/winsup/cygwin/fhandler/pty.cc > > > @@ -388,6 +388,52 @@ atexit_func (void) > > > } > > > } > > > > > > +void > > > +fhandler_pty_slave::req_fixup_pcon_state (void) > > > +{ > > > + while (true) > > > + { > > > + WaitForSingleObject (input_mutex, mutex_timeout); > > > + if (!get_ttyp ()->pcon_start_pid) > > > + break; > > > + /* Another request is on going. */ > > > + ReleaseMutex (input_mutex); > > > + yield (); > > > + } > > > + > > > + DWORD n; > > > + /* indicates that this "ESC[6n" is just for fixing-up corsor position > > > */ > > > + get_ttyp ()->req_fixup_pcon_cur_pos = true; > > > + get_ttyp ()->req_xfer_input = true; /* indicates that this "ESC[6n" > > > + is just for transfer input */ > > > + get_ttyp ()->pcon_start = true; > > > + get_ttyp ()->pcon_start_pid = myself->pid; > > > + WriteFile (get_output_handle (), "\033[6n", 4, &n, NULL); > > > + ReleaseMutex (input_mutex); > > > + while (get_ttyp ()->pcon_start_pid) > > > + /* wait for completion of fixing-up in master::write(). */ > > > + yield (); > > > > Both of these loops are unbounded, and both depend on somebody else > > clearing `pcon_start_pid`. If the master never replies (terminal closing, > > broken pipe, or the previous requester died mid-handshake), the exiting > > process spins forever in the second loop, and a stale slot wedges the next > > exiting process in the first one. This commit also drops the > > `pcon_start_pid = 0` reset that `close_pseudoconsole()` used to do, so the > > stale-slot case is no longer self-healing across pcon teardown either. > > > > Bounding both waits with a 3-second `GetTickCount64()` deadline, clearing > > our own `pcon_start_pid` on timeout only if it is still ours, and > > restoring the `close_pseudoconsole()` reset as a backstop makes the > > pathological case degrade to a slightly stale cursor rather than a hung > > exit. > > > > The fix I would propose is in > > https://github.com/git-for-windows/msys2-runtime/pull/131/changes/c366a1c02e66a242a3437f6b9335c2319c095c92: > > > > -- snip -- > > From c366a1c02e66a242a3437f6b9335c2319c095c92 Mon Sep 17 00:00:00 2001 > > From: Johannes Schindelin <[email protected]> > > Date: Thu, 25 Jun 2026 13:41:42 +0200 > > Subject: [PATCH] Cygwin: pty: bound the cursor-sync round-trip so an exiting > > process cannot hang > > > > The cursor-position fixup added in "Cygwin: pty: Fixup pty state after > > a cygwin app exits" runs from cleanup() on every foreground Cygwin-app > > exit while a pseudo console is active, and it waits on two unbounded > > loops for the master to answer the "ESC[6n" it just sent: one that > > spins until the pcon_start_pid slot is free, and one that spins until > > the master clears the slot again. pcon_start_pid is only ever cleared > > once master::write() parses the terminal's reply, so if that reply > > never comes, because the terminal is going away, the forwarding pipe > > is broken, or a previous requester died mid-handshake, the exiting > > process spins on yield() forever and never exits. > > > > Bound both waits with a three second deadline using GetTickCount64(), > > and on timeout clear our own pcon_start_pid slot, but only if it is > > still ours, so a give-up does not stomp a later requester. Also restore > > the pcon_start and pcon_start_pid reset that the same commit removed > > from close_pseudoconsole(); it is the backstop that keeps a requester > > which died without clearing its slot from wedging the next one. The > > worst case is now a slightly stale cursor after a timeout rather than a > > process that refuses to exit. > > > > Fixes: b34394d456b6 ("Cygwin: pty: Fixup pty state after a cygwin app > > exits") > > Assisted-by: Opus 4.8 > > Signed-off-by: Johannes Schindelin <[email protected]> > > --- > > winsup/cygwin/fhandler/pty.cc | 27 ++++++++++++++++++++++++++- > > 1 file changed, 26 insertions(+), 1 deletion(-) > > > > diff --git a/winsup/cygwin/fhandler/pty.cc b/winsup/cygwin/fhandler/pty.cc > > index c79fd1f975..669e18238b 100644 > > --- a/winsup/cygwin/fhandler/pty.cc > > +++ b/winsup/cygwin/fhandler/pty.cc > > @@ -226,6 +226,7 @@ atexit_func (void) > > void > > fhandler_pty_slave::req_fixup_pcon_state (void) > > { > > + ULONGLONG deadline = GetTickCount64 () + 3000; > > while (true) > > { > > WaitForSingleObject (input_mutex, mutex_timeout); > > @@ -233,6 +234,10 @@ fhandler_pty_slave::req_fixup_pcon_state (void) > > break; > > /* Another request is on going. */ > > ReleaseMutex (input_mutex); > > + if (GetTickCount64 () > deadline) > > + /* A previous requester is stuck; give up this sync rather than > > + spin forever. */ > > + return; > > yield (); > > } > > > > @@ -245,9 +250,25 @@ fhandler_pty_slave::req_fixup_pcon_state (void) > > get_ttyp ()->pcon_start_pid = myself->pid; > > WriteFile (get_output_handle (), "\033[6n", 4, &n, NULL); > > ReleaseMutex (input_mutex); > > - while (get_ttyp ()->pcon_start_pid) > > + deadline = GetTickCount64 () + 3000; > > + while (get_ttyp ()->pcon_start_pid && GetTickCount64 () <= deadline) > > /* wait for completion of fixing-up in master::write(). */ > > yield (); > > + /* If the master never answered (e.g. the terminal is going away), > > + clear our own request so a stale pcon_start_pid cannot wedge the > > + next requester. */ > > + if (get_ttyp ()->pcon_start_pid == (pid_t) myself->pid) > > + { > > + WaitForSingleObject (input_mutex, mutex_timeout); > > + if (get_ttyp ()->pcon_start_pid == (pid_t) myself->pid) > > + { > > + get_ttyp ()->req_fixup_pcon_cur_pos = false; > > + get_ttyp ()->req_xfer_input = false; > > + get_ttyp ()->pcon_start = false; > > + get_ttyp ()->pcon_start_pid = 0; > > + } > > + ReleaseMutex (input_mutex); > > + } > > } > > > > void > > @@ -4007,6 +4028,10 @@ fhandler_pty_slave::close_pseudoconsole (tty *ttyp, > > DWORD force_switch_to) > > ttyp->pcon_activated = false; > > ttyp->switch_to_nat_pipe = false; > > ttyp->nat_pipe_owner_pid = 0; > > + /* Safety net: if a req_fixup_pcon_state() requester died without > > + clearing its slot, do not leave pcon_start_pid set forever. */ > > + ttyp->pcon_start = false; > > + ttyp->pcon_start_pid = 0; > > } > > if (ttyp->pcon_handle_ready_event) > > { Clearing pcon_start and pcon_start_pid unconditionally here is not correct. The request from the other process may be active. Therefore, this should be: @@ -4228,6 +4249,13 @@ fhandler_pty_slave::close_pseudoconsole (tty *ttyp, DWORD force_switch_to) ttyp->pcon_activated = false; ttyp->switch_to_nat_pipe = false; ttyp->nat_pipe_owner_pid = 0; + /* Safety net: if a req_fixup_pcon_state() requester died without + clearing its slot, do not leave pcon_start_pid set forever. */ + if (ttyp->pcon_start_pid == myself->pid) + { + ttyp->pcon_start = false; + ttyp->pcon_start_pid = 0; + } } if (ttyp->pcon_handle_ready_event) { -- Takashi Yano <[email protected]>
