jpmschweitzer/clide
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

cache the kernel ref so dispose() can actually clean up (T-366)

Browse Source 
ClideKernel.of(context) is an illegal ancestor lookup inside
dispose(); both panes wrapped it in catch (_) and silently did
nothing. The terminal pane therefore never sent pane.close (backend
PTY + daemon pane leaked per closed pane) and the Claude pane never
removed its settings listener. Both now cache KernelServices in
didChangeDependencies and the swallow-everything helpers are gone.
New terminal_pane_test covers the close-on-dispose path; note in it
why the whole tree must unmount (harness Overlay keeps
initialEntries across rebuilds).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
jpmschweitzer
2026-06-12 00:24:27 +02:00
co-authored by Claude Fable 5
parent 466383671d
commit 75fc2719a0
6 changed files with 107 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.pql/changelog/ticket_history/2026-06.sql
+2
View File
@@ -3939,3 +3939,5 @@ Related bug with its own ticket (T-369): unguarded params[i+1] lookahead in SGR
Tests: test/terminal/escape/parser_test.dart (786 LOC) splits along the same seams — keep parser_test.dart for top-level dispatch/SBC/rollback, add csi_parser_test.dart (intermediates capture, DECSCUSR), sgr_handler_test.dart (bounds + colon form + 256/RGB), mode_handler_test.dart, osc_parser_test.dart, window/DA splits as convenient. The _RecordingHandler fixture is reusable across all of them.', NULL, '2026-06-11 22:10:19', '2026-06-11 22:10:19', '2026-06-11 22:10:19', NULL, '2942deccaff5b534ac3e33ac23b4d9fa', 2) ON CONFLICT(hash) DO NOTHING;
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBJ5T7HAQ9CA8XQMX43A2C', 'status', 'backlog', 'in_progress', NULL, '2026-06-11 22:14:48', '2026-06-11 22:14:48', '2026-06-11 22:14:48', NULL, '6f607da089f1326c07f4c17a6153c014', 2) ON CONFLICT(hash) DO NOTHING;
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBJ5T7HAQ9CA8XQMX43A2C', 'status', 'in_progress', 'done', NULL, '2026-06-11 22:18:25', '2026-06-11 22:18:25', '2026-06-11 22:18:25', NULL, '307a819cf82cf2ed49b3fd9e192992b6', 2) ON CONFLICT(hash) DO NOTHING;
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBV0465906BY3QFAY9F1YM', 'status', 'backlog', 'in_progress', NULL, '2026-06-11 22:18:56', '2026-06-11 22:18:56', '2026-06-11 22:18:56', NULL, '6b229d2b93c00f6197b026610c921273', 2) ON CONFLICT(hash) DO NOTHING;
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBV0465906BY3QFAY9F1YM', 'status', 'in_progress', 'done', NULL, '2026-06-11 22:24:07', '2026-06-11 22:24:07', '2026-06-11 22:24:07', NULL, '6e0986eb4b0c15223d7b373bbbc421cb', 2) ON CONFLICT(hash) DO NOTHING;
.pql/changelog/tickets/2026-06.sql
+6
View File
@@ -3979,3 +3979,9 @@ Fix: close the master fd in the natural-exit path (or let close() proceed to fd
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBJ5T7HAQ9CA8XQMX43A2C', 'bug', '06FBHBGHNEQTAEPGNJKN42C1E8', 'PTY master fd leaks on every natural child exit', 'lib/src/pty/native_pty.dart:444-450 — on child EOF, _reap() sets _dead = true but never closes _fd; a later close() short-circuits at `if (_dead) return;` (line ~460) so _nativeClose(_fd) (line ~477) never runs. Every terminal/Claude pane whose child exits on its own leaks an fd and a pty device for the life of the app. Two independent reviewers confirmed.
Fix: close the master fd in the natural-exit path (or let close() proceed to fd teardown when dead). Add a test asserting the fd is released after child EOF.', 'done', 'critical', NULL, NULL, NULL, '2026-06-11 21:54:49', '2026-06-11 22:18:25', NULL, '62168ec7a6bcdd154adc173c9cf9275f', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash);
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBV0465906BY3QFAY9F1YM', 'bug', '06FBHBGHNEQTAEPGNJKN42C1E8', 'Kernel lookup in dispose() leaks shell PTYs and settings listeners', 'lib/builtin/terminal/src/terminal_pane.dart:131-137 calls ClideKernel.of(context) from dispose() — illegal ancestor lookup, swallowed by catch (_) — so pane.close is never sent and the backend PTY + daemon pane leak on every closed terminal pane. The same idiom leaks the settings listener in every disposed ClaudePane (lib/builtin/claude/src/claude_pane.dart:460-466).
Fix: cache the kernel ref in didChangeDependencies, delete the catch-alls. Combined with the PTY natural-exit fd leak this is a two-stage leak pipeline. Tests: closing a terminal pane sends pane.close; disposing a ClaudePane removes its settings listener.', 'in_progress', 'high', NULL, NULL, NULL, '2026-06-11 21:56:01', '2026-06-11 22:18:56', NULL, 'a6c200ce295ef6e192688761f664ac9d', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash);
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHBV0465906BY3QFAY9F1YM', 'bug', '06FBHBGHNEQTAEPGNJKN42C1E8', 'Kernel lookup in dispose() leaks shell PTYs and settings listeners', 'lib/builtin/terminal/src/terminal_pane.dart:131-137 calls ClideKernel.of(context) from dispose() — illegal ancestor lookup, swallowed by catch (_) — so pane.close is never sent and the backend PTY + daemon pane leak on every closed terminal pane. The same idiom leaks the settings listener in every disposed ClaudePane (lib/builtin/claude/src/claude_pane.dart:460-466).
Fix: cache the kernel ref in didChangeDependencies, delete the catch-alls. Combined with the PTY natural-exit fd leak this is a two-stage leak pipeline. Tests: closing a terminal pane sends pane.close; disposing a ClaudePane removes its settings listener.', 'done', 'high', NULL, NULL, NULL, '2026-06-11 21:56:01', '2026-06-11 22:24:07', NULL, '167f8b01f1fcf2ef0bcc845585ca17aa', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash);
CHANGELOG.md
+5
View File
@@ -36,6 +36,11 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit.
terminal or Claude pane whose process ended naturally left its pty device
open for the life of the app; natural exit now releases the fd. (T-360)
- **Closing a terminal pane now closes its shell.** Pane disposal looked up
the kernel illegally and swallowed the failure, so `pane.close` was never
sent — the backend PTY and daemon pane leaked on every closed terminal
pane, and Claude panes leaked a settings listener the same way. (T-366)
- **Accepting ExitPlanMode now leaves plan mode in the conversation panel.**
Approving Claude's plan (the ExitPlanMode tool) transitioned the underlying
session out of plan mode, but clide's tracked permission mode didn't follow,
lib/builtin/claude/src/claude_pane.dart
+11 -12
View File
@@ -141,6 +141,10 @@ class _ClaudePaneState extends State<ClaudePane> {
@override
void didChangeDependencies() {
super.didChangeDependencies();
// Cache the kernel for dispose() — ancestor lookups there are illegal,
// and the old lookup-and-swallow leaked the settings listener on every
// disposed pane (T-366).
_kernel = ClideKernel.of(context);
// Spawn once, after the kernel is available.
if (!_spawned) {
_spawned = true;
@@ -168,7 +172,7 @@ class _ClaudePaneState extends State<ClaudePane> {
@override
void dispose() {
activeClaudeConfig?.removeListener(_onConfigChanged);
_kernel()?.settings.removeListener(_onSettingsChanged);
_kernel?.settings.removeListener(_onSettingsChanged);
_projectSub?.cancel();
_projectSub = null;
_statusSub?.cancel();
@@ -314,7 +318,7 @@ class _ClaudePaneState extends State<ClaudePane> {
// a fresh spawn vs connecting to existing on-disk history (the seed read
// from the transcript/sidecar). Surfaces the resume path in `make run`.
final seeded = _conversation?.items.length ?? 0;
_kernel()?.log.info(
_kernel?.log.info(
'claude',
'pane $_orchId bound session ${_sessionId ?? '?'} in $repoRoot — '
'${seeded > 0 ? 'connected to history ($seeded seeded item(s))' : 'fresh session (no history)'}',
@@ -422,7 +426,7 @@ class _ClaudePaneState extends State<ClaudePane> {
/// re-bind the pane to it.
Future<void> _resumeFlow() async {
final root = _repoRoot;
final dialog = _kernel()?.dialog;
final dialog = _kernel?.dialog;
if (root == null || dialog == null) return;
final dir = Directory(claudeProjectDir(root));
final sessions = await listSessions(dir);
@@ -455,15 +459,10 @@ class _ClaudePaneState extends State<ClaudePane> {
// -- helpers --------------------------------------------------------------
DaemonClient? _ipc() => _kernel()?.ipc;
DaemonClient? _ipc() => _kernel?.ipc;
KernelServices? _kernel() {
try {
return ClideKernel.of(context);
} catch (_) {
return null;
}
}
/// Cached in didChangeDependencies (T-366); see note there.
KernelServices? _kernel;
// -- build ----------------------------------------------------------------
@@ -496,7 +495,7 @@ class _ClaudePaneState extends State<ClaudePane> {
onTap: _focusComposerOnTap,
child: ConversationView(
controller: _conversation!,
foldLevel: foldLevelFromName(_kernel()?.settings.get<String>(kActivityFoldLevelKey)),
foldLevel: foldLevelFromName(_kernel?.settings.get<String>(kActivityFoldLevelKey)),
hiddenToolUseIds: _session?.promptedToolUseIds ?? const <String>{},
toolUseOutcomes: _session?.toolUseOutcomes ?? const <String, bool>{},
quietErrorToolUseIds: _session?.quietErrorToolUseIds ?? const <String>{},
lib/builtin/terminal/src/terminal_pane.dart
+16 -15
View File
@@ -33,6 +33,11 @@ class _TerminalPaneState extends State<TerminalPane> {
String? _error;
int _pid = 0;
/// Cached in didChangeDependencies — ancestor lookups are illegal in
/// dispose(), and the old lookup-and-swallow there meant pane.close
/// was never sent, leaking the backend PTY + daemon pane (T-366).
KernelServices? _kernel;
@override
void initState() {
super.initState();
@@ -45,6 +50,12 @@ class _TerminalPaneState extends State<TerminalPane> {
WidgetsBinding.instance.addPostFrameCallback((_) => _spawn());
}
@override
void didChangeDependencies() {
super.didChangeDependencies();
_kernel = ClideKernel.of(context);
}
@override
void dispose() {
_eventSub?.cancel();
@@ -53,14 +64,14 @@ class _TerminalPaneState extends State<TerminalPane> {
_paneId = null;
if (id != null) {
// Fire-and-forget. Daemon-side pane.close is idempotent.
unawaited(_kernelIpc()?.request('pane.close', args: {'id': id}));
unawaited(_kernel?.ipc.request('pane.close', args: {'id': id}));
}
super.dispose();
}
Future<void> _spawn() async {
if (!mounted) return;
final ipc = _kernelIpc();
final ipc = _kernel?.ipc;
if (ipc == null || !ipc.isConnected) {
setState(() => _error = 'Backend not connected.');
return;
@@ -92,7 +103,7 @@ class _TerminalPaneState extends State<TerminalPane> {
}
void _subscribeToPaneEvents() {
final kernel = _kernel();
final kernel = _kernel;
if (kernel == null) return;
_eventSub = kernel.events.on<DaemonEvent>().listen((event) {
if (event.subsystem != 'pane') return;
@@ -117,23 +128,13 @@ class _TerminalPaneState extends State<TerminalPane> {
void _onTerminalOutput(String text) {
final id = _paneId;
if (id == null) return;
_kernelIpc()?.request('pane.write', args: {'id': id, 'text': text});
_kernel?.ipc.request('pane.write', args: {'id': id, 'text': text});
}
void _onTerminalResize(int cols, int rows, int pixelWidth, int pixelHeight) {
final id = _paneId;
if (id == null) return;
_kernelIpc()?.request('pane.resize', args: {'id': id, 'cols': cols, 'rows': rows});
}
DaemonClient? _kernelIpc() => _kernel()?.ipc;
KernelServices? _kernel() {
try {
return ClideKernel.of(context);
} catch (_) {
return null;
}
_kernel?.ipc.request('pane.resize', args: {'id': id, 'cols': cols, 'rows': rows});
}
@override
test/builtin/terminal/terminal_pane_test.dart
+67
View File
@@ -0,0 +1,67 @@
/// TerminalPane lifecycle tests.
///
/// T-366: disposing the pane must send `pane.close` for its backend
/// pane. The pre-fix code looked the kernel up from dispose() — an
/// illegal ancestor lookup whose throw was swallowed — so the close
/// was never sent and the backend PTY + daemon pane leaked.
library;
import 'package:clide/builtin/terminal/src/terminal_pane.dart';
import 'package:clide/clide.dart';
import 'package:flutter/widgets.dart';
import 'package:flutter_test/flutter_test.dart';
import '../../helpers/kernel_fixture.dart';
import '../../helpers/widget_harness.dart';
void main() {
late KernelFixture fixture;
setUp(() async {
fixture = await KernelFixture.create();
});
tearDown(() async {
await fixture.dispose();
});
testWidgets('disposing the pane sends pane.close for the spawned pane', (tester) async {
final closed = <String>[];
fixture.ipc.setConnected(true);
fixture.ipc.stub('pane.spawn', (args) async => IpcResponse.ok(id: 'r1', data: {'id': 'pane-7', 'pid': 4321}));
fixture.ipc.stub('pane.close', (args) async {
closed.add(args['id'] as String);
return IpcResponse.ok(id: 'r2');
});
await tester.pumpWidget(harness(fixture, const TerminalPane()));
// First pump runs the post-frame spawn; second flushes its await.
await pumpAsync(tester);
expect(find.textContaining('pane-7'), findsOneWidget, reason: 'spawn should complete and surface the pane id');
// Tear the tree down — Overlay keeps its initialEntries across
// rebuilds, so swapping the harness child would NOT dispose the
// pane; unmounting the whole tree does. State.dispose() must fire
// pane.close.
await tester.pumpWidget(const SizedBox());
await pumpAsync(tester);
expect(closed, ['pane-7']);
});
testWidgets('disposing before spawn completes sends no close', (tester) async {
final closed = <String>[];
fixture.ipc.setConnected(false); // spawn bails out: no pane id
fixture.ipc.stub('pane.close', (args) async {
closed.add(args['id'] as String);
return IpcResponse.ok(id: 'r1');
});
await tester.pumpWidget(harness(fixture, const TerminalPane()));
await pumpAsync(tester);
await tester.pumpWidget(const SizedBox());
await pumpAsync(tester);
expect(closed, isEmpty);
});
}