diff --git a/src/Session.zig b/src/Session.zig index 08a9cde..e572fcc 100644 --- a/src/Session.zig +++ b/src/Session.zig @@ -12,6 +12,7 @@ const glibunix = @import("glibunix"); const vt = @import("ghostty-vt"); const Pty = @import("Pty.zig"); +const scope = @import("scope.zig"); const theme = @import("theme.zig"); const Session = @This(); @@ -208,6 +209,10 @@ pub fn create( }); errdefer self.pty.deinit(); + // Out of the app's own cgroup, so that the kernel killing something this + // shell runs is not also systemd's cue to kill the app. See `scope`. + scope.adopt(self.pty.pid); + if (opts.command.len > 0) { self.startup_command = try alloc.dupe(u8, opts.command); } diff --git a/src/Snapshot.zig b/src/Snapshot.zig index 8e802fa..c4d0c5e 100644 --- a/src/Snapshot.zig +++ b/src/Snapshot.zig @@ -23,7 +23,7 @@ //! //! It lives in the *state* directory rather than beside `layouts.json` and //! `settings.json` in the config one. Those two are files a person writes; -//! this one is written behind their back on every exit, and putting churn like +//! this one is written behind their back every few seconds and on every exit, and putting churn like //! that in a config directory — which plenty of people keep in version control //! — would make every quit look like an edit. `$XDG_STATE_HOME` is where the //! spec puts exactly this: state that should carry across restarts but that @@ -276,27 +276,37 @@ pub const SaveError = error{ OutOfMemory, WriteFailed }; /// nothing about that state worth offering to restore — so the offer should not /// appear at the next launch, and the stale one it would otherwise still be /// holding should not either. -pub fn save(self: *Snapshot) SaveError!void { +/// +/// `written` is a hash of what the caller last had written, and is updated to +/// this one's. A snapshot that would write the same bytes again is skipped: the +/// window is photographed every few seconds while it is open, and nearly every +/// one of those photographs is the same as the last. +pub fn save(self: *Snapshot, written: *u64) SaveError!void { var path_buf: [max_path]u8 = undefined; const path = statePath(&path_buf) orelse return error.WriteFailed; if (!self.any()) { remove(path); + written.* = 0; return; } + const text = try self.serialize(); + defer self.arena.child_allocator.free(text); + + const hash = std.hash.Wyhash.hash(0, text); + if (hash == written.*) return; + var dir_buf: [max_path]u8 = undefined; const dir = stateDir(&dir_buf) orelse return error.WriteFailed; if (glib.mkdirWithParents(dir.ptr, 0o700) != 0) return error.WriteFailed; - const text = try self.serialize(); - defer self.arena.child_allocator.free(text); - var err: ?*glib.Error = null; if (glib.fileSetContents(path.ptr, text.ptr, @intCast(text.len), &err) == 0) { if (err) |e| e.free(); return error.WriteFailed; } + written.* = hash; } /// Delete the file. Failures are ignored on purpose: the common one is it not diff --git a/src/Window.zig b/src/Window.zig index 9f658f3..a0c5d1b 100644 --- a/src/Window.zig +++ b/src/Window.zig @@ -10,6 +10,7 @@ const adw = @import("adw"); const gdk = @import("gdk"); const gio = @import("gio"); const glib = @import("glib"); +const glibunix = @import("glibunix"); const gobject = @import("gobject"); const gtk = @import("gtk"); const vt = @import("ghostty-vt"); @@ -152,6 +153,16 @@ restore_detail: *gtk.Label, /// it — see `applyRestoreVisible`. restore_offered: bool = false, +/// The timer that photographs the window while it is open, and the hash of the +/// photograph last written — see `onAutosave`. +autosave: c_uint = 0, +snapshot_written: u64 = 0, + +/// The handlers that turn a polite kill into an ordinary quit — see +/// `onTerminateSignal`. Removed on teardown, which also hands each signal back +/// to its default action. +signal_sources: [terminate_signals.len]c_uint = @splat(0), + /// What a sidebar row is signaling. Defined with the dots themselves, since /// a row and a pane header show the same five states for the same reasons. const Attention = Pane.Attention; @@ -495,6 +506,11 @@ pub fn create(alloc: std.mem.Allocator, app: *adw.Application) !*Window { // otherwise withdraw it before it was ever made. self.offerRestore(); + self.autosave = glib.timeoutAddSeconds(autosave_interval_s, &onAutosave, self); + for (terminate_signals, &self.signal_sources) |signum, *source| { + source.* = glibunix.signalAdd(signum, &onTerminateSignal, self); + } + return self; } @@ -1383,8 +1399,9 @@ fn captureStartupTabs(ctx: ?*anyopaque) void { // ------------------------------------------------------------------------- // The session snapshot // -// Every exit photographs the window — see `Snapshot` for what is in the -// photograph and what deliberately is not — and every launch that finds one +// Every exit photographs the window, and so does a timer while it is open — +// see `Snapshot` for what is in the photograph and what deliberately is not, +// and `onAutosave` for why once is not enough — and every launch that finds one // offers it back from a banner at the foot of the sidebar. The offer is // optional in both directions: dismissing it costs nothing, and taking it is a // single click rather than a dialog. @@ -1595,16 +1612,16 @@ fn buildSnapshotTab(self: *Window, entry: Snapshot.Tab) !*Tab { /// Photograph the window for the next launch. /// -/// Called from `onDestroy`, which is the one funnel every route out of the -/// window passes through — the quit confirmation being accepted, the last tab -/// closing, the compositor closing the window — and which runs while the tabs -/// and their shells are all still alive. That last part is the reason it is -/// there and not in `quit`: a terminal's directory is read out of its live -/// child process, and after teardown there is nothing left to ask. +/// Called from `onAutosave` while the window is open, and from `onDestroy`, +/// which is the one funnel every orderly route out of the window passes through +/// — the quit confirmation being accepted, the last tab closing, the compositor +/// closing the window, a SIGTERM — and which runs while the tabs and their +/// shells are all still alive. That last part is the reason it is there and not +/// in `quit`: a terminal's directory is read out of its live child process, and +/// after teardown there is nothing left to ask. /// -/// Every failure below is a warning and a carry-on. This runs while the app is -/// already leaving, there is nowhere to report anything, and the worst case is -/// one launch that has nothing to offer. +/// Every failure below is a warning and a carry-on. There is nowhere better to +/// report anything, and the worst case is one launch that has nothing to offer. fn saveSnapshot(self: *Window) void { var snapshot: Snapshot = .init(self.alloc); defer snapshot.deinit(); @@ -1638,11 +1655,49 @@ fn saveSnapshot(self: *Window) void { }; } - snapshot.save() catch |err| { + snapshot.save(&self.snapshot_written) catch |err| { std.log.warn("could not write the session snapshot: {s}", .{@errorName(err)}); }; } +/// How often the window is photographed while it is open. +/// +/// An exit is not the only way out. The app can be killed outright — by +/// systemd tearing down the scope it runs in, by the OOM killer, by a crash — +/// and none of those reach `onDestroy`, so a snapshot taken only there is lost +/// along with the session it was meant to rescue. Photographing is cheap (a +/// `readlink` per terminal and a hash; the file is only written when something +/// actually moved), so this errs towards often. +const autosave_interval_s = 10; + +fn onAutosave(data: ?*anyopaque) callconv(.c) c_int { + const self: *Window = @ptrCast(@alignCast(data.?)); + + // While the offer stands, the file on disk is the *last* session — the one + // the banner is offering — and the window is only showing the tabs it opened + // for itself. Overwriting it now would leave a crash before you answer with + // nothing to offer but those. Anything that changes the tab set withdraws the + // offer, so this resumes the moment the window holds work of its own. + if (!self.restore_offered) self.saveSnapshot(); + return 1; +} + +/// The signals that mean "please exit": systemd stopping the unit, the terminal +/// playpen was started from going away, and Ctrl+C in that terminal. +const terminate_signals = [_]c_int{ 15, 1, 2 }; // SIGTERM, SIGHUP, SIGINT + +/// Quit as though the window had been closed, so the session is saved first. +/// +/// Left to the default action, any of these kills the process where it stands +/// and `onDestroy` never runs. Destroying the window rather than closing it: a +/// signal is not a question, and the quit confirmation has nobody to ask. +fn onTerminateSignal(data: ?*anyopaque) callconv(.c) c_int { + const self: *Window = @ptrCast(@alignCast(data.?)); + std.log.info("asked to exit by signal; saving the session and quitting", .{}); + self.window.as(gtk.Window).destroy(); + return 0; +} + /// Make `tab` the visible one. fn select(self: *Window, tab: *Tab) void { self.updating = true; @@ -2351,8 +2406,19 @@ fn onDestroy(_: *adw.ApplicationWindow, self: *Window) callconv(.c) void { if (self.closing) return; self.closing = true; + // Both hold this window as their user data. Removing the signal handlers + // also puts the default actions back, so a second SIGTERM while teardown is + // stuck behind a shell that will not exit still gets rid of the process. + if (self.autosave != 0) _ = glib.Source.remove(self.autosave); + for (self.signal_sources) |source| { + if (source != 0) _ = glib.Source.remove(source); + } + // First, while every tab is still whole and every shell still running: the - // directories in the snapshot are read out of live processes. + // directories in the snapshot are read out of live processes. Written even + // if the last autosave took the same picture, since the exit is the one + // write that has to be sure it landed. + self.snapshot_written = 0; self.saveSnapshot(); // Before anything is freed: a scheme change arriving mid-teardown would diff --git a/src/scope.zig b/src/scope.zig new file mode 100644 index 0000000..ab291e1 --- /dev/null +++ b/src/scope.zig @@ -0,0 +1,119 @@ +//! Each shell in its own systemd scope, so that the kernel's OOM killer taking +//! one process out of a tab cannot take the whole app with it. +//! +//! The failure this exists for: a desktop launcher starts playpen in a scope of +//! its own (niri's is `app-niri-playpen-.scope`), and by default every +//! process the app ever starts lands in that same scope — every shell, and +//! everything run from them. A scope's `OOMPolicy` defaults to `stop`, which +//! means that when the kernel OOM-kills *any* process in it, systemd responds by +//! stopping the unit, which is every other process in it. So a Gradle test +//! worker blowing through memory in one tab gets killed, as it should, and then +//! systemd kills playpen and every shell in every other tab for it. No crash, +//! no core dump, nothing in the app's own log: the journal just says the scope +//! "Failed with result 'oom-kill'". +//! +//! The policy cannot be changed on a scope that is already running — systemd +//! only takes `OOMPolicy` when the unit is created — so the app cannot fix the +//! scope it was launched into. What it can do is what GNOME's terminals do: +//! move each shell, right after it is forked, into a new transient scope of its +//! own with `OOMPolicy=continue`. The kernel still kills whatever it picked, and +//! that is the end of it; the shell that ran it, the other tabs and the window +//! all carry on. It also means that anything else in the system that acts on a +//! whole cgroup — `systemd-oomd`, `systemctl --user stop`, per-unit memory +//! accounting — sees one tab at a time rather than the whole app as one lump. +//! +//! Best-effort throughout. Outside a systemd user session the call fails, the +//! shell stays in the app's own cgroup, and everything else behaves exactly as +//! it did before this existed. + +const std = @import("std"); +const gio = @import("gio"); +const glib = @import("glib"); +const gobject = @import("gobject"); + +/// The session bus, fetched on first use. GIO hands every caller the same +/// shared connection, so this is only a cache of the lookup. +var bus: ?*gio.DBusConnection = null; + +/// Whether a failure has been reported already. One line in the log says that +/// shells are not being isolated; one per tab would only repeat it. +var warned = false; + +/// Move `pid` into a scope of its own. Returns immediately: the request goes out +/// asynchronously and the shell carries on starting while systemd moves it. +/// +/// That leaves a window, between the fork and the move, in which anything the +/// shell starts stays behind in the app's cgroup. It is a few milliseconds long +/// and falls while the shell is still reading its profile, which is not where +/// anything that could run out of memory gets started. A layout's startup +/// command in particular is not typed until the first prompt has been drawn. +pub fn adopt(pid: std.c.pid_t) void { + const conn = bus orelse blk: { + var err: ?*glib.Error = null; + const found = gio.busGetSync(.session, null, &err) orelse { + defer if (err) |e| e.free(); + warnOnce(if (err) |e| e.f_message else null); + return; + }; + bus = found; + break :blk found; + }; + + // The random part keeps the name unique even if a scope outlives its shell, + // which it does whenever the shell left a daemon behind — Gradle's does this + // as a matter of course — and the pid is later reused. + var name_buf: [96]u8 = undefined; + const name = std.fmt.bufPrintZ(&name_buf, "app-playpen-shell-{d}-{x:0>8}.scope", .{ + pid, + glib.randomInt(), + }) catch return; + + // `CollectMode` because a scope in which something *was* OOM-killed ends up + // "failed" when it finally empties, and would otherwise sit in + // `systemctl --user --failed` long after the tab is gone. + const params = glib.Variant.newParsed( + "(%s, 'fail', [" ++ + "('Description', <'Playpen shell'>), " ++ + "('Slice', <'app.slice'>), " ++ + "('PIDs', <[%u]>), " ++ + "('OOMPolicy', <'continue'>), " ++ + "('CollectMode', <'inactive-or-failed'>)" ++ + "], @a(sa(sv)) [])", + name.ptr, + @as(u32, @intCast(pid)), + ); + + conn.call( + "org.freedesktop.systemd1", + "/org/freedesktop/systemd1", + "org.freedesktop.systemd1.Manager", + "StartTransientUnit", + params, + null, + .{}, + -1, + null, + &onStarted, + null, + ); +} + +fn onStarted(source: ?*gobject.Object, res: *gio.AsyncResult, _: ?*anyopaque) callconv(.c) void { + const conn: *gio.DBusConnection = @ptrCast(source orelse return); + var err: ?*glib.Error = null; + const reply = conn.callFinish(res, &err) orelse { + defer if (err) |e| e.free(); + warnOnce(if (err) |e| e.f_message else null); + return; + }; + reply.unref(); +} + +fn warnOnce(message: ?[*:0]const u8) void { + if (warned) return; + warned = true; + std.log.warn( + "could not give shells their own systemd scope, so one running out of memory can take the app down with it: {s}", + .{message orelse "unknown error"}, + ); +} diff --git a/web/src/App.tsx b/web/src/App.tsx index 95e8ff9..496bee3 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -208,6 +208,10 @@ export default function App() { // a repeat request for the same file a distinct one, so re-clicking a comment // after re-folding its file opens it again. const [reveal, setReveal] = useState<{ file: string; seq: number } | null>(null); + // The file the diff last asked the rail to show, sequenced like `reveal` so + // locating the same file twice still counts. + const [locate, setLocate] = useState<{ file: string; seq: number } | null>(null); + const locateSeq = useRef(0); const revealSeq = useRef(0); const [theme, setTheme] = useState(initialTheme); const [sidebarWidth, setSidebarWidth] = useState(() => @@ -620,6 +624,11 @@ export default function App() { waitForComment(c.id, () => scrollToFile(c.file)); }, []); + // Stable, since every file header holds it and FileView is memoized. + const locateFile = useCallback((file: string) => { + setLocate({ file, seq: ++locateSeq.current }); + }, []); + const draftCount = useMemo( () => comments.filter((c) => c.status === 'draft').length, [comments], @@ -895,7 +904,13 @@ export default function App() { /> )} {payload && ( - + )}
{openCount > 0 && ( @@ -947,6 +962,7 @@ export default function App() { snapshots={snapshots} reveal={reveal} onSetViewed={setFileViewed} + onLocate={locateFile} onStartDraft={setDraft} onCancelDraft={cancelDraft} onSubmitDraft={submitDraft} diff --git a/web/src/components/DiffView.tsx b/web/src/components/DiffView.tsx index ad5027f..473f0d9 100644 --- a/web/src/components/DiffView.tsx +++ b/web/src/components/DiffView.tsx @@ -64,6 +64,8 @@ interface Props { // `source` is the file's contents at the moment it was marked, kept so a later // visit can be diffed against it. Absent when they can't be reconstructed. onSetViewed: (file: string, viewed: boolean, source?: string) => void; + // Reveals the file in the left rail's file list. + onLocate: (file: string) => void; onStartDraft: (d: DraftTarget) => void; onCancelDraft: () => void; onSubmitDraft: (body: string) => Promise; @@ -432,6 +434,7 @@ export function DiffView({ snapshots, reveal, onSetViewed, + onLocate, onStartDraft, onCancelDraft, onSubmitDraft, @@ -480,6 +483,7 @@ export function DiffView({ cardRef={cardRef(path)} bodyRef={bodyRef(path)} onSetViewed={onSetViewed} + onLocate={onLocate} onStartDraft={onStartDraft} onCancelDraft={onCancelDraft} onSubmitDraft={onSubmitDraft} @@ -537,6 +541,7 @@ const FileView = memo(function FileView({ cardRef, bodyRef, onSetViewed, + onLocate, onStartDraft, onCancelDraft, onSubmitDraft, @@ -561,6 +566,7 @@ const FileView = memo(function FileView({ cardRef: (el: HTMLElement | null) => void; bodyRef: (el: HTMLElement | null) => void; onSetViewed: Props['onSetViewed']; + onLocate: Props['onLocate']; onStartDraft: (d: DraftTarget) => void; onCancelDraft: () => void; onSubmitDraft: Props['onSubmitDraft']; @@ -749,6 +755,14 @@ const FileView = memo(function FileView({ > + {file.type !== 'modify' && ( {statusLabel(file.type)} diff --git a/web/src/components/FileList.tsx b/web/src/components/FileList.tsx index e28a538..f17d3c1 100644 --- a/web/src/components/FileList.tsx +++ b/web/src/components/FileList.tsx @@ -1,4 +1,4 @@ -import { useMemo, useState } from 'react'; +import { useEffect, useMemo, useRef, useState } from 'react'; import type { Comment, DiffFile } from '../types'; import { @@ -8,12 +8,18 @@ import { pathOf, type DirNode, type FileNode, + type TreeNode, } from '../lib/filetree'; import { Icon } from './Icon'; interface Props { files: DiffFile[]; comments: Comment[]; + // Files marked viewed; their rows gray out, as do folders with nothing left. + viewed: ReadonlySet; + // A file the diff asked to be shown here, sequenced so asking twice for the + // same file still counts. + locate: { file: string; seq: number } | null; onSelect: (path: string) => void; } @@ -28,9 +34,12 @@ function initialMode(): Mode { // FileList is the left rail: every changed file with its stats and open-comment // count, either as a GitHub-style collapsible folder tree or a flat list. // Clicking a file scrolls to it. -export function FileList({ files, comments, onSelect }: Props) { +export function FileList({ files, comments, viewed, locate, onSelect }: Props) { const [mode, setMode] = useState(initialMode); const [collapsed, setCollapsed] = useState>(new Set()); + const navRef = useRef(null); + // The located file waiting for its row to be on screen to scroll to. + const [pending, setPending] = useState(null); const openByFile = useMemo(() => { const m = new Map(); @@ -43,6 +52,7 @@ export function FileList({ files, comments, onSelect }: Props) { const tree = useMemo(() => buildTree(files, openByFile), [files, openByFile]); const rows = useMemo(() => flatten(tree, collapsed), [tree, collapsed]); + const viewedDirs = useMemo(() => fullyViewed(tree, viewed), [tree, viewed]); const allCollapsed = useMemo(() => { const dirs = dirPaths(tree); return dirs.length > 0 && dirs.every((p) => collapsed.has(p)); @@ -63,8 +73,40 @@ export function FileList({ files, comments, onSelect }: Props) { const toggleAll = () => setCollapsed(allCollapsed ? new Set() : new Set(dirPaths(tree))); + // Locating a file opens every folder above it. A folder's collapse key is its + // own path, so those are exactly the prefixes of the file's path — including + // the ones a chain-merged row swallowed, which were never keys to begin with. + useEffect(() => { + if (!locate) return; + const parts = locate.file.split('/'); + const ancestors = parts.slice(0, -1).map((_, i) => parts.slice(0, i + 1).join('/')); + setCollapsed((prev) => { + if (!ancestors.some((a) => prev.has(a))) return prev; + const next = new Set(prev); + for (const a of ancestors) next.delete(a); + return next; + }); + setPending(locate.file); + }, [locate]); + + // Then, once the render that opened those folders has the row in the DOM, + // bring it into view and pulse it so the eye finds it. + useEffect(() => { + if (pending == null) return; + const el = navRef.current?.querySelector( + `[data-path="${CSS.escape(pending)}"]`, + ); + if (!el) return; + setPending(null); + el.scrollIntoView({ behavior: 'smooth', block: 'center' }); + el.classList.remove('is-flashed'); + void el.offsetWidth; // restart the flash when the same row is located again + el.classList.add('is-flashed'); + window.setTimeout(() => el.classList.remove('is-flashed'), 1800); + }, [pending, rows, mode]); + return ( -