Skip to content

Commit 6030d04

Browse files
committed
Contain agent-pane wta in a Job Object + watch its exit
When the agent pane's wta.exe dies (crash / external taskkill / clean exit), the pane was left bound to a defunct child. Subsequent input or protocol writes from the UI thread would then block on the dead pipe, producing the MoAppHang observed in WER. And the wta children (npx → npm → node → ACP adapter), which inherit the conpty client handles, were left running indefinitely. ConptyConnection state can't detect wta exit on its own: those inherited handles keep the pipe open until the LAST descendant exits, not when the root wta dies. PowerShell etc. don't show this because shells don't normally fork long-lived children that inherit stdio. Fix: - Wrap the agent pane's wta in a Job Object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE. Closing our handle to the job terminates every member — wta and all its descendants. This handles the orphan-process leak directly and (because WT crashing also drops the handle) guarantees cleanup even on abnormal WT exit. - Watch wta's process handle with RegisterWaitForSingleObject. When wta exits for any reason, the threadpool callback marshals teardown to the UI thread, which closes the pane and the job (reaping any surviving children in the process). - Heap-allocated AgentPaneWtaWaitContext (weak_ref + cancellation flag), plus an UnregisterWaitEx(INVALID_HANDLE_VALUE) at teardown, make the wait safe to race with deliberate teardown (settings rebuild, user pane close, page destruction). Scope is intentionally limited to the agent pane: - ConptyConnection is not modified — ordinary shell panes keep their existing free-running lifetime (Start-Process notepad etc. still survive pane close as users expect). - Delegation wta (the ?<prompt> path through WtaProcessLauncher) is left as a follow-up. Its children leak in the same way and should get the same Job-Object treatment, but the launcher is a separate code path. There is a small race window between ConptyConnection's CreateProcessW and our AssignProcessToJobObject (we hook in via TermControl.Initialized after the spawn). In practice wta does ACP / npx setup before it spawns any child, so the window is empty. A future tightening could move the job assignment into ConptyConnection with CREATE_SUSPENDED + ResumeThread for strict zero-race semantics.
1 parent 0a56b23 commit 6030d04

4 files changed

Lines changed: 289 additions & 0 deletions

File tree

.github/actions/spelling/allow/apis.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ ENDSESSION
3737
enumset
3838
environstrings
3939
EXACTSIZEONLY
40+
EXECUTEDEFAULT
41+
EXECUTEONLYONCE
4042
EXPCMDFLAGS
4143
EXPCMDSTATE
4244
filetime

.github/actions/spelling/expect/expect.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,7 @@ ETW
528528
EUDC
529529
eventing
530530
evflags
531+
EOFs
531532
execd
532533
executionengine
533534
exemain
@@ -1641,6 +1642,7 @@ tdbuild
16411642
TDP
16421643
Teb
16431644
Techo
1645+
taskkill
16441646
tellp
16451647
teraflop
16461648
terminalcore
@@ -2016,6 +2018,7 @@ testname
20162018
transitioning
20172019
unk
20182020
unparseable
2021+
unregisters
20192022
Virt
20202023
VMs
20212024
webpage

src/cascadia/TerminalApp/TerminalPage.cpp

Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,14 @@ namespace winrt::TerminalApp::implementation
236236

237237
TerminalPage::~TerminalPage()
238238
{
239+
// Critical ordering: disarm the wta process watch BEFORE other
240+
// members destruct. The threadpool wait holds a raw pointer into
241+
// _agentPaneWtaWaitContext (a unique_ptr member). If we let that
242+
// member destruct while the wait is still registered, a wta exit
243+
// after this point would fire the callback against freed memory.
244+
// _TearDownAgentPaneWtaWatch calls UnregisterWaitEx blocking, so
245+
// by the time it returns no callback can fire.
246+
_TearDownAgentPaneWtaWatch();
239247
}
240248

241249
// Method Description:
@@ -1332,9 +1340,191 @@ namespace winrt::TerminalApp::implementation
13321340
a.delegateCustomCommand != b.delegateCustomCommand;
13331341
}
13341342

1343+
// Heap-allocated context for the threadpool wait callback. Keeps the
1344+
// weak_ref + cancellation flag alive independently of TerminalPage so the
1345+
// callback never dereferences freed memory. Owned by
1346+
// _agentPaneWtaWaitContext on the page; freed only after the wait has
1347+
// been unregistered (callback can no longer fire) or after the callback
1348+
// has run and we marshal back to the UI thread.
1349+
struct TerminalPage::AgentPaneWtaWaitContext
1350+
{
1351+
winrt::weak_ref<TerminalPage> page;
1352+
std::atomic<bool> cancelled{ false };
1353+
};
1354+
1355+
// Arm process-exit detection + Job-Object containment for the agent
1356+
// pane's wta. Called from the agent pane's Initialized callback once
1357+
// ConptyConnection has spawned wta and RootProcessHandle is valid.
1358+
//
1359+
// The Job carries JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE: closing our handle
1360+
// (or losing it via WT crash) terminates wta + every descendant. wta's
1361+
// future children inherit job membership automatically.
1362+
//
1363+
// Race window: wta could (in principle) spawn a child between
1364+
// CreateProcessW and AssignProcessToJobObject. wta does ACP/npx setup
1365+
// before spawning anything, so in practice the window is empty. A
1366+
// future tightening could move this into ConptyConnection with
1367+
// CREATE_SUSPENDED + AssignProcessToJobObject + ResumeThread.
1368+
void TerminalPage::_SetupAgentPaneWtaWatch(HANDLE wtaProcessHandle) noexcept
1369+
try
1370+
{
1371+
if (!wtaProcessHandle || wtaProcessHandle == INVALID_HANDLE_VALUE)
1372+
{
1373+
return;
1374+
}
1375+
if (_agentPaneWtaWait)
1376+
{
1377+
// Already armed for some prior wta — disarm first.
1378+
_TearDownAgentPaneWtaWatch();
1379+
}
1380+
1381+
// ORDER MATTERS. The watch (death detection) must be set up BEFORE
1382+
// the Job Object (orphan containment). If we created the job first
1383+
// and a later step failed, the local `unique_handle job` would
1384+
// destruct, fire KILL_ON_JOB_CLOSE, and turn a "watch arm failed"
1385+
// soft error into a hard kill of wta + its children. Setting up
1386+
// the watch first also means death detection still works even if
1387+
// job containment is unavailable (e.g. nested-job restrictions).
1388+
1389+
// 1) Independent SYNCHRONIZE handle so the wait survives across
1390+
// the ordering of ConptyConnection cleanup.
1391+
HANDLE dup{ nullptr };
1392+
if (!DuplicateHandle(GetCurrentProcess(), wtaProcessHandle,
1393+
GetCurrentProcess(), &dup,
1394+
SYNCHRONIZE, FALSE, 0))
1395+
{
1396+
_agentPaneLog("DuplicateHandle(wta) failed");
1397+
return;
1398+
}
1399+
wil::unique_handle wtaDup{ dup };
1400+
1401+
// 2) Heap-allocate the callback context.
1402+
auto ctx = std::make_unique<AgentPaneWtaWaitContext>();
1403+
ctx->page = get_weak();
1404+
1405+
// 3) Register the wait. From here on, if we early-return without
1406+
// committing to members, UnregisterWaitEx is the cleanup; ctx
1407+
// auto-frees via unique_ptr.
1408+
HANDLE waitHandle{ nullptr };
1409+
if (!RegisterWaitForSingleObject(
1410+
&waitHandle,
1411+
wtaDup.get(),
1412+
&TerminalPage::_OnAgentPaneWtaExit,
1413+
ctx.get(),
1414+
INFINITE,
1415+
WT_EXECUTEONLYONCE | WT_EXECUTEDEFAULT))
1416+
{
1417+
_agentPaneLog("RegisterWaitForSingleObject(wta) failed");
1418+
return;
1419+
}
1420+
1421+
// 4) Commit the watch immediately. Death detection is the core
1422+
// fix — we keep it even if job setup below fails (we just lose
1423+
// the orphan-cleanup nice-to-have, not the hang fix).
1424+
_agentPaneWtaHandle = std::move(wtaDup);
1425+
_agentPaneWtaWait = waitHandle;
1426+
_agentPaneWtaWaitContext = std::move(ctx);
1427+
1428+
// 5) Now attempt Job Object containment. This is best-effort;
1429+
// failure just means orphans won't be reaped automatically.
1430+
wil::unique_handle job{ CreateJobObjectW(nullptr, nullptr) };
1431+
if (!job)
1432+
{
1433+
_agentPaneLog("CreateJobObject failed; orphan cleanup disabled (death-watch still active)");
1434+
return;
1435+
}
1436+
JOBOBJECT_EXTENDED_LIMIT_INFORMATION limits{};
1437+
limits.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
1438+
if (!SetInformationJobObject(job.get(), JobObjectExtendedLimitInformation, &limits, sizeof(limits)))
1439+
{
1440+
_agentPaneLog("SetInformationJobObject(KILL_ON_JOB_CLOSE) failed; orphan cleanup disabled (death-watch still active)");
1441+
return;
1442+
}
1443+
if (!AssignProcessToJobObject(job.get(), wtaProcessHandle))
1444+
{
1445+
_agentPaneLog("AssignProcessToJobObject failed; orphan cleanup disabled (death-watch still active)");
1446+
return;
1447+
}
1448+
// Commit the job immediately after Assign — no fallible call may
1449+
// appear between Assign and this assignment, or the local `job`
1450+
// destructor would fire KILL_ON_JOB_CLOSE on the live wta.
1451+
_agentPaneJob = std::move(job);
1452+
_agentPaneLog("agent pane wta watch armed (job + process wait)");
1453+
}
1454+
CATCH_LOG()
1455+
1456+
// Disarm wait + drop job. Closing the job handle terminates wta and
1457+
// every surviving descendant (KILL_ON_JOB_CLOSE) — single source of
1458+
// truth for "stop the agent process group".
1459+
void TerminalPage::_TearDownAgentPaneWtaWatch() noexcept
1460+
{
1461+
if (_agentPaneWtaWaitContext)
1462+
{
1463+
_agentPaneWtaWaitContext->cancelled.store(true, std::memory_order_release);
1464+
}
1465+
if (_agentPaneWtaWait)
1466+
{
1467+
// INVALID_HANDLE_VALUE blocks until any in-flight callback
1468+
// completes — safe because we never call this from inside the
1469+
// callback (the callback dispatches to the UI thread and the
1470+
// UI-thread continuation handles teardown there).
1471+
UnregisterWaitEx(_agentPaneWtaWait, INVALID_HANDLE_VALUE);
1472+
_agentPaneWtaWait = nullptr;
1473+
}
1474+
_agentPaneWtaWaitContext.reset();
1475+
_agentPaneWtaHandle.reset();
1476+
_agentPaneJob.reset(); // KILL_ON_JOB_CLOSE — descendants die here.
1477+
}
1478+
1479+
// Threadpool callback: wta exited. We can't touch most TerminalPage
1480+
// state from here (wrong thread + no UI marshalling), so just marshal
1481+
// a teardown request to the UI thread.
1482+
void NTAPI TerminalPage::_OnAgentPaneWtaExit(PVOID context, BOOLEAN /*timedOut*/) noexcept
1483+
try
1484+
{
1485+
auto* ctx = static_cast<AgentPaneWtaWaitContext*>(context);
1486+
if (!ctx || ctx->cancelled.load(std::memory_order_acquire))
1487+
{
1488+
return;
1489+
}
1490+
const auto strong = ctx->page.get();
1491+
if (!strong)
1492+
{
1493+
return;
1494+
}
1495+
const auto weak = ctx->page;
1496+
strong->Dispatcher().RunAsync(
1497+
winrt::Windows::UI::Core::CoreDispatcherPriority::Normal,
1498+
[weak]() {
1499+
const auto p = weak.get();
1500+
if (!p)
1501+
{
1502+
return;
1503+
}
1504+
// If we were disarmed between callback fire and dispatch
1505+
// resolving, bail — a deliberate teardown is already in
1506+
// flight (or finished) and we'd otherwise tear down a
1507+
// freshly-built replacement pane.
1508+
if (!p->_agentPaneWtaWait)
1509+
{
1510+
return;
1511+
}
1512+
_agentPaneLog("agent pane wta process exited — tearing down pane");
1513+
p->_agentPanePreWarming = false;
1514+
p->_TeardownAgentPane(); // closes job → reaps surviving children
1515+
p->_UpdateBottomBarState();
1516+
});
1517+
}
1518+
CATCH_LOG()
1519+
13351520
// Close the shared agent pane if it is still alive.
13361521
void TerminalPage::_TeardownAgentPane()
13371522
{
1523+
// Disarm + close job FIRST. Closing the job kills wta and every
1524+
// descendant (KILL_ON_JOB_CLOSE) so conpty actually sees its
1525+
// pipe go EOF, ControlCore transitions through Closed, and the
1526+
// existing teardown path runs cleanly.
1527+
_TearDownAgentPaneWtaWatch();
13381528
if (auto p = _agentPane.lock())
13391529
{
13401530
_agentPaneLog("_TeardownAgentPane: closing agent pane");
@@ -1849,6 +2039,12 @@ namespace winrt::TerminalApp::implementation
18492039
_agentPaneLog("agent pane closed — _agentPane cleared");
18502040
if (auto self = weakSelf.get())
18512041
{
2042+
// Backstop disarm — the pane can be closed through
2043+
// generic pane-close paths (Ctrl+W, tab close, etc.)
2044+
// that don't go through _TeardownAgentPane. Without
2045+
// this, the wta job + wait would stay armed and the
2046+
// process tree would leak.
2047+
self->_TearDownAgentPaneWtaWatch();
18522048
self->_agentPane.reset();
18532049
self->_agentPanePreWarming = false;
18542050
self->_lastNotifiedAgentTabId.reset();
@@ -1913,6 +2109,21 @@ namespace winrt::TerminalApp::implementation
19132109
{
19142110
return;
19152111
}
2112+
// wta is now running (connection.Start() ran inside
2113+
// _InitializeTerminal just before Initialized fired). Arm
2114+
// the job + process-exit watch before doing anything else
2115+
// so we can't miss an early-crash scenario.
2116+
if (const auto tc = termControlWeak.get())
2117+
{
2118+
if (const auto conn = tc.Connection())
2119+
{
2120+
if (const auto conpty = conn.try_as<winrt::Microsoft::Terminal::TerminalConnection::ConptyConnection>())
2121+
{
2122+
const auto raw = reinterpret_cast<HANDLE>(conpty.RootProcessHandle());
2123+
self->_SetupAgentPaneWtaWatch(raw);
2124+
}
2125+
}
2126+
}
19162127
// Pre-warm has done its job: connection.Start() ran inside
19172128
// _InitializeTerminal just before this event, so wta.exe is
19182129
// launching. Drop the reconcile guard before doing anything
@@ -2384,6 +2595,10 @@ namespace winrt::TerminalApp::implementation
23842595
_agentPaneLog("agent pane closed — _agentPane cleared");
23852596
if (auto self = weakSelf.get())
23862597
{
2598+
// Backstop disarm — see _AutoCreateHiddenAgentPane's
2599+
// Closed handler for rationale. Generic pane-close
2600+
// paths don't route through _TeardownAgentPane.
2601+
self->_TearDownAgentPaneWtaWatch();
23872602
self->_agentPane.reset();
23882603
self->_lastNotifiedAgentTabId.reset();
23892604
self->_agentSessionsViewActive = false;
@@ -2393,6 +2608,41 @@ namespace winrt::TerminalApp::implementation
23932608
});
23942609
}
23952610

2611+
// Arm the wta process watch + Job Object for this newly-created
2612+
// agent pane too (the auto-create path hooks Initialized to do the
2613+
// same thing — see _AutoCreateHiddenAgentPane).
2614+
if (const auto termControl = newPane->GetTerminalControl())
2615+
{
2616+
auto weakSelfForWatch = get_weak();
2617+
auto tokenHolder = std::make_shared<winrt::event_token>();
2618+
*tokenHolder = termControl.Initialized([
2619+
weakSelfForWatch,
2620+
termControlWeak = winrt::make_weak(termControl),
2621+
tokenHolder
2622+
](auto&&, auto&&) {
2623+
if (const auto tc = termControlWeak.get())
2624+
{
2625+
tc.Initialized(*tokenHolder);
2626+
}
2627+
const auto self = weakSelfForWatch.get();
2628+
if (!self)
2629+
{
2630+
return;
2631+
}
2632+
if (const auto tc = termControlWeak.get())
2633+
{
2634+
if (const auto conn = tc.Connection())
2635+
{
2636+
if (const auto conpty = conn.try_as<winrt::Microsoft::Terminal::TerminalConnection::ConptyConnection>())
2637+
{
2638+
const auto raw = reinterpret_cast<HANDLE>(conpty.RootProcessHandle());
2639+
self->_SetupAgentPaneWtaWatch(raw);
2640+
}
2641+
}
2642+
}
2643+
});
2644+
}
2645+
23962646
const auto& activeTab = _GetFocusedTabImpl();
23972647
if (!activeTab)
23982648
{

src/cascadia/TerminalApp/TerminalPage.h

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -634,6 +634,40 @@ namespace winrt::TerminalApp::implementation
634634
std::vector<std::shared_ptr<AgentDelegationEntry>> _agentPipeServers;
635635
void _RemoveAgentPipeServer(AgentDelegationEntry* entry);
636636

637+
// Agent-pane wta lifetime management. Solves two problems at once:
638+
//
639+
// (1) Detect wta death (crash / external taskkill / clean exit) so we
640+
// tear the pane down. Conpty StateChanged can't be used because
641+
// wta spawns long-lived children (npx → npm → node → ACP adapter)
642+
// that inherit the conpty client handles, so the pipe never EOFs
643+
// even when wta itself is gone.
644+
// (2) Reclaim those orphaned children. Without explicit job
645+
// containment, killing wta leaves node.exe / npx running.
646+
//
647+
// _agentPaneJob: KILL_ON_JOB_CLOSE Job Object containing wta and (by
648+
// inheritance) all its descendants. Resetting the handle terminates
649+
// every member — used during teardown to sweep orphans, and (via OS)
650+
// when WT itself exits abnormally. Only the agent pane uses a job;
651+
// ordinary shell panes keep their existing free-running lifetime.
652+
//
653+
// _agentPaneWtaHandle: duplicated SYNCHRONIZE handle to wta. Owned
654+
// independently of ConptyConnection's _piClient so the wait stays
655+
// valid across teardown ordering.
656+
//
657+
// _agentPaneWtaWait + _agentPaneWtaWaitContext: RegisterWaitForSingleObject
658+
// registration. Context is heap-allocated (so the threadpool callback
659+
// has stable storage even if TerminalPage tears down before the wait
660+
// unregisters); it carries a weak_ref + cancelled flag so we can race
661+
// safely with deliberate teardown.
662+
struct AgentPaneWtaWaitContext;
663+
wil::unique_handle _agentPaneJob;
664+
wil::unique_handle _agentPaneWtaHandle;
665+
HANDLE _agentPaneWtaWait{ nullptr };
666+
std::unique_ptr<AgentPaneWtaWaitContext> _agentPaneWtaWaitContext;
667+
void _SetupAgentPaneWtaWatch(HANDLE wtaProcessHandle) noexcept;
668+
void _TearDownAgentPaneWtaWatch() noexcept;
669+
static void NTAPI _OnAgentPaneWtaExit(PVOID context, BOOLEAN timedOut) noexcept;
670+
637671
// Pending pipe handles for the next agent-pane wta launch. Set by
638672
// _OpenOrReuseAgentPane just before _MakeTerminalPane runs, consumed
639673
// by _CreateConnectionFromSettings when it builds the ConptyConnection

0 commit comments

Comments
 (0)