Skip to content

Avoid search-thread deadlocks during surface teardown #55

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
miracle2k wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Avoid search-thread deadlocks during surface teardown #55

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 96 additions & 53 deletions src/Surface.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ pub const min_window_height_cells: u32 = 4;
/// given time. `activate_key_table` calls after this are ignored.
const max_active_key_tables = 8;

/// Search callbacks run on the search thread. They should preserve ordering in
/// normal operation, but must never wait forever because surface teardown joins
/// that thread synchronously from the app thread.
const search_callback_push_timeout_ns = 50 * std.time.ns_per_ms;

/// Unique ID used to identify this surface for IPC purposes. It is
/// exposed to the commands running in surfaces as the environment variable
/// GHOSTTY_SURFACE_ID. It must not be zero as zero is used to incicate a null
Expand Down Expand Up @@ -173,6 +178,12 @@ command_timer: ?std.time.Instant = null,
/// Search state
search: ?Search = null,

/// True while this surface is synchronously joining its search thread during
/// surface teardown. Search callbacks run on the search thread and normally
/// may block while delivering UI state, but they must become best-effort during
/// teardown because the app thread can be the thread waiting in join().
search_tearing_down: std.atomic.Value(bool) = .init(false),

/// Used to rate limit BEL handling.
last_bell_time: ?std.time.Instant = null,

Expand Down Expand Up @@ -823,7 +834,10 @@ pub fn init(
}

pub fn deinit(self: *Surface) void {
// Stop search thread
// Stop search thread. After this point the app thread may be blocked in
// join(), so search callbacks must not wait for app/renderer mailboxes to
// drain or teardown can deadlock.
self.search_tearing_down.store(true, .release);
if (self.search) |*s| s.deinit();

// Stop rendering thread
Expand Down Expand Up @@ -1448,11 +1462,76 @@ fn searchCallback(event: terminal.search.Thread.Event, ud: ?*anyopaque) void {
};
}

fn pushSearchRendererViewportMatches(
self: *Surface,
arena_: ArenaAllocator,
matches: []const terminal.highlight.Flattened,
timeout: rendererpkg.Thread.Mailbox.Timeout,
) void {
var arena = arena_;
const pushed = self.renderer_thread.mailbox.push(
.{ .search_viewport_matches = .{
.arena = arena,
.matches = matches,
} },
timeout,
);
if (pushed == 0) arena.deinit();
}

fn pushSearchRendererSelectedMatch(
self: *Surface,
arena_: ArenaAllocator,
match: terminal.highlight.Flattened,
timeout: rendererpkg.Thread.Mailbox.Timeout,
) void {
var arena = arena_;
const pushed = self.renderer_thread.mailbox.push(
.{ .search_selected_match = .{
.arena = arena,
.match = match,
} },
timeout,
);
if (pushed == 0) arena.deinit();
}

fn pushSearchRendererMessage(
self: *Surface,
message: rendererpkg.Message,
timeout: rendererpkg.Thread.Mailbox.Timeout,
) void {
_ = self.renderer_thread.mailbox.push(message, timeout);
}
Comment on lines +1499 to +1505
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 pushSearchRendererMessage silently discards non-arena message drops

Unlike pushSearchRendererViewportMatches and pushSearchRendererSelectedMatch, this helper ignores the push return value without comment. Currently it is only called with { .search_selected_match = null } — a message with no owned arena — so there is nothing to free on a failed push. However, the asymmetry with the other two helpers (which explicitly handle the pushed == 0 case) makes it easy to accidentally pass an arena-bearing message here in the future. A brief inline comment explaining that this path is intentionally arena-free would prevent misuse.


fn pushSearchSurfaceMessage(
self: *Surface,
message: Message,
timeout: App.Mailbox.Queue.Timeout,
) void {
_ = self.surfaceMailbox().push(message, timeout);
}

fn searchCallback_(
self: *Surface,
event: terminal.search.Thread.Event,
) !void {
// NOTE: This runs on the search thread.
// Search notifications normally preserve ordering by briefly waiting behind
// older renderer/app messages. They must never wait forever: surface teardown
// joins this thread synchronously, and a callback that entered just before
// teardown could otherwise block on a full mailbox whose app-thread consumer
// is already waiting in join(). During teardown these UI updates are purely
// best-effort; the surface is closing and does not need them.
const is_tearing_down = self.search_tearing_down.load(.acquire);
const renderer_timeout: rendererpkg.Thread.Mailbox.Timeout = if (is_tearing_down)
.{ .instant = {} }
else
.{ .ns = search_callback_push_timeout_ns };
const surface_timeout: App.Mailbox.Queue.Timeout = if (is_tearing_down)
.{ .instant = {} }
else
.{ .ns = search_callback_push_timeout_ns };

switch (event) {
.viewport_matches => |matches_unowned| {
Expand All @@ -1463,14 +1542,9 @@ fn searchCallback_(
const matches = try alloc.dupe(terminal.highlight.Flattened, matches_unowned);
for (matches) |*m| m.* = try m.clone(alloc);

_ = self.renderer_thread.mailbox.push(
.{ .search_viewport_matches = .{
.arena = arena,
.matches = matches,
} },
.forever,
);
try self.renderer_thread.wakeup.notify();
self.pushSearchRendererViewportMatches(arena, matches, renderer_timeout);
self.renderer_thread.wakeup.notify() catch |err|
log.warn("error notifying renderer thread after search viewport update err={}", .{err});
Comment on lines +1546 to +1547
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unconditional wakeup after a potentially-dropped message

wakeup.notify() is called regardless of whether the preceding push succeeded. When the push times out or returns 0 (message dropped), the renderer thread is woken up with no new message in the mailbox. The renderer will drain its queue and go back to sleep harmlessly, but the wakeup signal is semantically misleading — its contract is "a new message is waiting." Under heavy load this path can fire repeatedly without effect. Consider only calling wakeup.notify() when the push returned a non-zero count.

},
Comment on lines 1537 to 1548
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Dangling errdefer after arena ownership transfer by value

arena is passed by value to pushSearchRendererViewportMatches. Inside that helper, when the push fails, the helper's copy calls arena.deinit(), freeing the underlying memory pool. The caller's arena variable (still on the stack with errdefer arena.deinit() attached) now holds dangling internal pointers. If a try-able call were ever added after the push helper — or if the helper's ownership model changes — the errdefer would trigger a double-free. Today it is safe because no errors propagate after the push, but it is a fragile ownership pattern. The same applies to the selected_match branch. Consider removing or cancelling the errdefer after transferring ownership, or returning the arena back from the helper so the caller retains exclusive ownership.


.selected_match => |selected_| {
Expand All @@ -1481,67 +1555,36 @@ fn searchCallback_(
const alloc = arena.allocator();
const match = try sel.highlight.clone(alloc);

_ = self.renderer_thread.mailbox.push(
.{ .search_selected_match = .{
.arena = arena,
.match = match,
} },
.forever,
);
self.pushSearchRendererSelectedMatch(arena, match, renderer_timeout);

// Send the selected index to the surface mailbox
_ = self.surfaceMailbox().push(
.{ .search_selected = sel.idx },
.forever,
);
self.pushSearchSurfaceMessage(.{ .search_selected = sel.idx }, surface_timeout);
} else {
// Reset our selected match
_ = self.renderer_thread.mailbox.push(
.{ .search_selected_match = null },
.forever,
);
self.pushSearchRendererMessage(.{ .search_selected_match = null }, renderer_timeout);

// Reset the selected index
_ = self.surfaceMailbox().push(
.{ .search_selected = null },
.forever,
);
self.pushSearchSurfaceMessage(.{ .search_selected = null }, surface_timeout);
}

try self.renderer_thread.wakeup.notify();
self.renderer_thread.wakeup.notify() catch |err|
log.warn("error notifying renderer thread after search selection update err={}", .{err});
},

.total_matches => |total| {
_ = self.surfaceMailbox().push(
.{ .search_total = total },
.forever,
);
self.pushSearchSurfaceMessage(.{ .search_total = total }, surface_timeout);
},

// When we quit, tell our renderer to reset any search state.
.quit => {
_ = self.renderer_thread.mailbox.push(
.{ .search_selected_match = null },
.forever,
);
_ = self.renderer_thread.mailbox.push(
.{ .search_viewport_matches = .{
.arena = .init(self.alloc),
.matches = &.{},
} },
.forever,
);
try self.renderer_thread.wakeup.notify();
self.pushSearchRendererMessage(.{ .search_selected_match = null }, renderer_timeout);
self.pushSearchRendererViewportMatches(.init(self.alloc), &.{}, renderer_timeout);
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the final search reset outside teardown

When search is closed normally (not during surface teardown), is_tearing_down is false, so this quit reset is still sent with the 50 ms .ns timeout; if the renderer mailbox remains full longer than that, BlockingQueue.push returns 0 and pushSearchRendererViewportMatches drops the reset. Because .quit is the last search callback, there is no later update to clear renderer.search_matches/search_selected_match, so stale search highlights can remain visible after the user closes search under renderer backpressure. The teardown path can be best-effort, but the non-teardown quit path needs a reliable way to clear this state.

Useful? React with 👍 / 👎.

self.renderer_thread.wakeup.notify() catch |err|
log.warn("error notifying renderer thread after search quit err={}", .{err});

// Reset search totals in the surface
_ = self.surfaceMailbox().push(
.{ .search_total = null },
.forever,
);
_ = self.surfaceMailbox().push(
.{ .search_selected = null },
.forever,
);
self.pushSearchSurfaceMessage(.{ .search_total = null }, surface_timeout);
self.pushSearchSurfaceMessage(.{ .search_selected = null }, surface_timeout);
},

// Unhandled, so far.
Expand Down