Skip to content

Bound embedded surface process teardown - #184

Merged
austinywang merged 3 commits into
mainfrom
issue-9573-bounded-teardown
Aug 7, 2026
Merged

austinywang merged 3 commits into
mainfrom
issue-9573-bounded-teardown

Conversation

@austinywang

@austinywang austinywang commented Aug 7, 2026 •

Copy link
Copy Markdown

Summary

  • expose a pre-free embedded API that retires a surface from app routing and wakes its Ghostty-owned IO teardown
  • preserve a 12-second SIGHUP grace period, then escalate to SIGKILL and cap the final reap wait at 3 seconds
  • cover subprocesses that ignore SIGHUP while preserving the existing embedded action-lifetime regression
  • reapply the VT stream-boundary API required by the current cmux parent pin

Supports manaflow-ai/cmux#9573.

Verification

  • focused ignored-SIGHUP escalation test: 73/73 passed
  • subprocess stop filter: 74/74 passed
  • embedded retained-action teardown filter: 73/73 passed
  • git diff --check

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Bound embedded surface teardown to avoid hangs and provide a pre-free API to retire a surface and shut down its child process. Adds a VT stream “ground” check for safe snapshot boundaries, meeting the needs in manaflow-ai/cmux#9573.

  • New Features

    • ghostty_surface_request_process_termination() retires a surface from app routing and starts child-process shutdown; idempotent; still call ghostty_surface_free().
    • Bounded process shutdown: send SIGHUP with a 12s grace, then SIGKILL; cap final reap at 3s; handles subprocesses that ignore SIGHUP.
    • ghostty_terminal_vt_stream_is_ground() reports parser/UTF‑8 ground state to snapshot VT safely (required by the current cmux parent pin).
  • Migration

    • On embedded surface close, call ghostty_surface_request_process_termination(surface) before ghostty_surface_free(surface); safe to call multiple times.
    • If snapshotting VT output, only snapshot when ghostty_terminal_vt_stream_is_ground(terminal) returns true.

Written for commit 5b20c62. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features

    • Added an API to request child-process termination before a surface is destroyed.
    • Added an API to report whether terminal input processing is complete and has no partial sequences.
  • Bug Fixes

    • Improved process shutdown by retrying termination signals, escalating when necessary, and reporting failures if the process cannot be reaped.
    • Improved surface teardown reliability by coordinating renderer, process, and thread shutdown.

@austinywang
austinywang merged commit 7350263 into main Aug 7, 2026
100 of 101 checks passed
@coderabbitai

coderabbitai Bot commented Aug 7, 2026 •

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e622af1d-2e39-4c49-8eaf-37d5c5a84387

📥 Commits

Reviewing files that changed from the base of the PR and between 1d111a0 and 5b20c62.

📒 Files selected for processing (8)
  • include/ghostty.h
  • include/ghostty/vt/terminal.h
  • src/Surface.zig
  • src/apprt/embedded.zig
  • src/lib_vt.zig
  • src/terminal/c/main.zig
  • src/terminal/c/terminal.zig
  • src/termio/Exec.zig

📝 Walkthrough

Walkthrough

The change adds APIs for requesting surface child-process termination and querying VT stream state. Surface teardown now uses bounded POSIX process-group shutdown with SIGHUP-to-SIGKILL escalation. VT tests cover complete and incomplete parser input.

Changes

Surface process termination

Layer / File(s) Summary
Bounded POSIX process teardown
src/termio/Exec.zig
POSIX process groups receive repeated SIGHUP signals, then SIGKILL after configurable grace periods. Reaping uses bounded polling and reports timeout errors. Tests cover processes that ignore SIGHUP.
Surface termination lifecycle and C API
include/ghostty.h, src/Surface.zig, src/apprt/embedded.zig
Surface teardown requests process termination before content cleanup. The request is idempotent, removes the surface from app routing, and is exposed through ghostty_surface_request_process_termination.

VT stream ground-state API

Layer / File(s) Summary
VT ground-state query and validation
include/ghostty/vt/terminal.h, src/terminal/c/terminal.zig, src/terminal/c/main.zig, src/lib_vt.zig
The API returns true only when the VT parser is in ground state and the UTF-8 decoder has no pending bytes. Tests cover complete input, split UTF-8, incomplete CSI, and split OSC terminators.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant EmbeddedSurface
  participant CoreSurface
  participant POSIXProcessGroup
  EmbeddedSurface->>CoreSurface: request process termination
  CoreSurface->>POSIXProcessGroup: send SIGHUP
  POSIXProcessGroup-->>CoreSurface: exit or remain active
  CoreSurface->>POSIXProcessGroup: send SIGKILL after grace period
  POSIXProcessGroup-->>CoreSurface: reaped child process
  EmbeddedSurface->>EmbeddedSurface: complete surface destruction
Loading

Possibly related PRs

Suggested reviewers: mitchellh, lawrencecchen, vancluever

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-9573-bounded-teardown

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants