Repository: microsoft/terminal (language: cpp)

Issue:
Exiting terminal while running `npm` in a pane does not terminate processes from terminal
### Windows Terminal version

1.16.10261.0

### Windows build number

10.0.19045.0

### Other Software

Node.js v16.14.2

### Steps to reproduce

1. Open Windows Terminal
2. Split panes
3. Start a process in one pane (I was using `npm run start` to run a local website with React)
4. Observe website running in web browser
5. Use the `x` button on the windows terminal window to exit windows terminal
6. Background process is still running and website is still working in web browser

### Expected Behavior

When quitting windows terminal, background processes should be stopped. The local server running the React website should also be terminated when windows terminal exits.

### Actual Behavior

Background process continues running indefinitely. Task manager does not indicate any windows terminal tasks running in the background. The background task appears to be buried and is difficult to discover so the easiest way to terminate the process is to restart the machine. Restarting the machine terminates the background process and website is no longer hosted.


Discussion / hints:
This issue seems very similar to #541 
Pretty confident that this isn't #541 - that was a specific bug from like, almost 4 years ago now. 

If anything, I'd think that #14544 would have helped here. Can you try this with Terminal Preview (v1.17), and see if that fixes this/?
This issue is still reproducible for me on preview version 1.17.1023
Repro steps from #15708

> 1. Create a folder and clone my sample repro: `npx degit https://github.com/jsoldi/windows-terminal-bug`. It's just a plain server that does nothing.
> 2. Do `npm install`
> 3. Open the Windows Terminal on the project folder and run `npm start 3000` and then visit `localhost:3000` on a browser.
> 4. Close the terminal.
> 5. Reload `localhost:3000` and the server should still be running
> 
> If the process actually stops, try repeating steps 3-5 one more time.
I think it's important to highlight that this only seems to occur with npm projects. The process will actually stop either if I run the script directly with `node index.js`, or if I do `npm start` from CMD or PS, so somehow the combination of terminal and npm seems to cause the issue.
Another repro:

> 1.17.11461.0, 10.0.19044.3086, node v18.15.0
> 
> ### Steps to reproduce
> 
> 1. open terminal
> 2. split pane (shift+alt+-)
> 3. run node (npm run dev)
> 4. close terminal (alt+f4 or `X` button)
If I run npm in a PS command, I observe this same behavior:
`wt new-tab pwsh -c "& {npm run start}"` 

If I run npm in a PS job, the background job is terminated correctly when I close the pane/tab/window
`wt new-tab pwsh -c "& {Start-Job {npm run start}}"`
Same here with the PS command. I was debugging a ps1 script and noticed that each time I run it the port number increments. Then I had to go to the task manager and manually kill 20+ node processes... .__. This is weird. 
Why is this bug not getting more attention? It makes the terminal completely useless for anything other than very specific toy projects and people unaware of the bug will get their system resources slowly eaten up until things start failing. 
Tried to recreate the bug with different methods. As both #15708 and #15801 mentioned, running the provided repo through `node` directly doesn't trigger the behavior. Using `yarn`, instead of `npm`, also doesn't trigger this behavior. 
This bug is kind of scary, to be honest. I started a project and then when I thought the server was closed it was still running (after 7h that I first closed the terminal). I checked everything and found nothing. But then the last shot was looking into the Process Explorer and bang: node.js still running initialized by command line npm start. 
Hey can anyone check if this still repros on 1.22 Preview/? We did some improvements to the way we generate CTRL_CLOSE events over the last couple of releases, and we're pretty confident we're doing it right now. Thanks!
I am still experiencing this issue on the 1.22 preview (specifically v1.22.2362.0). In my case I am running a local server for a Angular website. The development server will persist after the terminal is exited.
This is still happening on preview version 1.22.2702.0. To replicate just run `npm run start` from the terminal, where `package.json` contains `"scripts": { "start": "node ./index.js" }` and `index.js` is:

```typescript
// index.js
import express from 'express';

const app = express();
const port = 3000;
app.get('/', (req, res) => { res.send('Hello World'); });
app.listen(port, () => { console.log(`Server is running at http://localhost:${port}`); });
```

Then close the terminal and the server will still be running.

Temporary solution for Windows:
```shell
taskkill /IM "node.exe" /F
```
Mac and Linux:
```bash
#!/bin/bash
pkill -f "node"
```

Fix patch:
diff --git a/src/host/input.cpp b/src/host/input.cpp
index fd92154020c..952d3a02915 100644
--- a/src/host/input.cpp
+++ b/src/host/input.cpp
@@ -321,28 +321,35 @@ void ProcessCtrlEvents()
         return;
     }
 
-    auto Status = STATUS_SUCCESS;
+    const auto ctrl = ServiceLocator::LocateConsoleControl();
+
     for (const auto& r : termRecords)
     {
-        /*
-         * Status will be non-successful if a process attached to this console
-         * vetoes shutdown. In that case, we don't want to try to kill any more
-         * processes, but we do need to make sure we continue looping so we
-         * can close any remaining process handles. The exception is if the
-         * process is inaccessible, such that we can't even open a handle for
-         * query. In this case, use best effort to send the close event but
-         * ignore any errors.
-         */
-        if (SUCCEEDED_NTSTATUS(Status))
-        {
-            Status = ServiceLocator::LocateConsoleControl()
-                         ->EndTask(r.dwProcessID,
-                                   EventType,
-                                   CtrlFlags);
-            if (!r.hProcess)
-            {
-                Status = STATUS_SUCCESS;
-            }
-        }
+        // Older versions of Windows would do various things if the EndTask() call failed:
+        // * XP: Pops up a "Windows can't end this program" dialog for every already-dead process.
+        // * Vista - Win 7: Simply skips over already-dead processes.
+        // * Win 8 - Win 11 26100: Aborts on an already-dead process (you have to WM_CLOSE conhost multiple times).
+        //
+        // That last period had the following comment:
+        //   Status will be non-successful if a process attached to this console
+        //   vetoes shutdown. In that case, we don't want to try to kill any more
+        //   processes, but we do need to make sure we continue looping so we
+        //   can close any remaining process handles. The exception is if the
+        //   process is inaccessible, such that we can't even open a handle for
+        //   query. In this case, use best effort to send the close event but
+        //   ignore any errors.
+        //
+        // The corresponding logic worked like this:
+        //   if (FAILED(EndTask(...)) && r.hProcess) {
+        //       break;
+        //   }
+        //
+        // That logic was removed around the Windows 11 26100 time frame, because CSRSS
+        // (which handles EndTask) now waits 5s and then force-kills the process for us.
+        // Going back to the Win 7 behavior then should make shutdown a lot more robust.
+        // The bad news is that EndTask() returns STATUS_UNSUCCESSFUL no matter whether
+        // the process was already dead, or if the request actually failed for some reason.
+        // Hopefully there aren't any regressions, but we can't know without trying.
+        LOG_IF_NTSTATUS_FAILED(ctrl->EndTask(r.dwProcessID, EventType, CtrlFlags));
     }
 }
diff --git a/src/server/ProcessList.cpp b/src/server/ProcessList.cpp
index e304556692a..e01e20bb8de 100644
--- a/src/server/ProcessList.cpp
+++ b/src/server/ProcessList.cpp
@@ -211,9 +211,15 @@ ConsoleProcessHandle* ConsoleProcessList::GetOldestProcess() const
     {
         termRecords.clear();
 
+        // The caller (ProcessCtrlEvents) expects them in newest-to-oldest order,
+        // because that's how Windows has historically always dispatched these events.
+        auto it = _processes.crbegin();
+        const auto end = _processes.crend();
+
         // Dig through known processes looking for a match
-        for (const auto& p : _processes)
+        for (; it != end; ++it)
         {
+            const auto p = *it;
             // If no limit was specified OR if we have a match, generate a new termination record.
             if (!dwLimitingProcessId ||
                 p->_ulProcessGroupId == dwLimitingProcessId)


Test patch: