From 8fca64957b8e5fc7348378f116179af56df3151d Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Fri, 31 Jul 2026 15:31:58 -0400 Subject: [PATCH 1/2] cli: report ssh terminfo cache failures A state directory with the wrong permissions left the terminfo cache failing with errors that named no path, so there was nothing to act on: $ ghostty +ssh-cache --add=user@host Error: Unable to add 'user@host' to cache. Error: error.AccessDenied Every +ssh-cache failure now names its cache file, and +ssh no longer swallows cache-related errors. Error messages in these actions are also lowercased after the "Error: " prefix and append the error with ": {t}" rather than a second "Error: ". Ref: https://github.com/ghostty-org/ghostty/issues/9393#issuecomment-5145799368 --- src/cli/ssh.zig | 37 ++++++++++++++--- src/cli/ssh_cache.zig | 96 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 110 insertions(+), 23 deletions(-) diff --git a/src/cli/ssh.zig b/src/cli/ssh.zig index 3b34109d1..5dc115652 100644 --- a/src/cli/ssh.zig +++ b/src/cli/ssh.zig @@ -245,14 +245,24 @@ fn runInner( const cache: ?DiskCache = if (opts.cache) cache: { const path = DiskCache.defaultPath(alloc, "ghostty") catch |err| { - warnPrint(stderr, "ghostty terminfo cache unavailable: {}", .{err}); + warnPrint(stderr, "ghostty terminfo cache unavailable: {t}", .{err}); break :session .{ .term = "xterm-256color" }; }; break :cache .{ .path = path }; } else null; if (cache) |c| { - if (c.contains(alloc, dest) catch false) { + const cached = c.contains(alloc, dest) catch |err| cached: { + // An invalid key is a bad destination, not a bad cache. + if (err != error.InvalidCacheKey) warnPrint( + stderr, + "unable to add '{s}' to the cache: {t}", + .{ c.path, err }, + ); + break :cached false; + }; + + if (cached) { verbosePrint(opts, stderr, "dest: {s} (cached, skipping install)", .{dest}); break :session .{ .term = "xterm-ghostty" }; } else { @@ -266,7 +276,7 @@ fn runInner( stderr.flush() catch {}; installRemoteTerminfo(alloc, opts, stderr) catch |err| { - warnPrint(stderr, "failed to install terminfo: {}", .{err}); + warnPrint(stderr, "failed to install terminfo: {t}", .{err}); break :session .{ .term = "xterm-256color" }; }; break :session .{ @@ -297,7 +307,7 @@ fn runInner( verbosePrint(opts, stderr, "exec: {f}", .{Joined{ .items = argv }}); const exit_code = childExec(argv) catch |err| { - try stderr.print("Error: failed to run {s}: {}\n", .{ argv[0], err }); + try stderr.print("Error: failed to run {s}: {t}\n", .{ argv[0], err }); return 1; }; verbosePrint(opts, stderr, "exit: {d}", .{exit_code}); @@ -309,8 +319,23 @@ fn runInner( .real, ).toSeconds())) |_| { verbosePrint(opts, stderr, "cache: wrote {s}", .{entry.dest}); - } else |err| { - log.debug("cache add failed for '{s}': {}", .{ entry.dest, err }); + } else |err| switch (err) { + // An uncacheable destination was already reported above, and a + // lock held by a concurrent `+ssh` resolves itself. + error.InvalidCacheKey, + error.CacheLocked, + => verbosePrint( + opts, + stderr, + "cache: skipped {s}: {t}", + .{ entry.dest, err }, + ), + + else => warnPrint( + stderr, + "unable to add '{s}' to the cache '{s}': {t}", + .{ entry.dest, entry.cache.path, err }, + ), } }; diff --git a/src/cli/ssh_cache.zig b/src/cli/ssh_cache.zig index b1b53657b..f509f69d4 100644 --- a/src/cli/ssh_cache.zig +++ b/src/cli/ssh_cache.zig @@ -133,7 +133,24 @@ pub fn run(alloc_gpa: Allocator) !u8 { }; } - const result = runInner(alloc, opts, query, stdout, stderr); + // Setup our disk cache to the standard location + const cache_path = DiskCache.defaultPath(alloc, "ghostty") catch |err| { + try stderr.print( + "Error: unable to determine the cache path: {t}\n", + .{err}, + ); + stderr.flush() catch {}; + return 1; + }; + + const result = runInner( + alloc, + opts, + query, + .{ .path = cache_path }, + stdout, + stderr, + ); // Flushing *shouldn't* fail but... stdout.flush() catch {}; @@ -145,6 +162,7 @@ pub fn runInner( alloc: Allocator, opts: Options, query: ?[]const u8, + cache: DiskCache, stdout: *std.Io.Writer, stderr: *std.Io.Writer, ) !u8 { @@ -165,12 +183,14 @@ pub fn runInner( return 2; } - // Setup our disk cache to the standard location - const cache_path = try DiskCache.defaultPath(alloc, "ghostty"); - const cache: DiskCache = .{ .path = cache_path }; - if (opts.clear) { - try cache.clear(); + cache.clear() catch |err| { + try stderr.print( + "Error: unable to clear cache '{s}': {t}\n", + .{ cache.path, err }, + ); + return 1; + }; return 0; } @@ -182,15 +202,15 @@ pub fn runInner( ) catch |err| switch (err) { error.InvalidCacheKey => { try stderr.print( - "Error: Invalid destination '{s}' (expected hostname or user@hostname)\n", + "Error: invalid destination '{s}' (expected hostname or user@hostname)\n", .{dest}, ); return 2; }, else => { try stderr.print( - "Error: Unable to add '{s}' to cache. Error: {}\n", - .{ dest, err }, + "Error: unable to add '{s}' to cache '{s}': {t}\n", + .{ dest, cache.path, err }, ); return 1; }, @@ -202,15 +222,15 @@ pub fn runInner( const removed = cache.remove(alloc, dest) catch |err| switch (err) { error.InvalidCacheKey => { try stderr.print( - "Error: Invalid destination '{s}' (expected hostname or user@hostname)\n", + "Error: invalid destination '{s}' (expected hostname or user@hostname)\n", .{dest}, ); return 2; }, else => { try stderr.print( - "Error: Unable to remove '{s}' from cache. Error: {}\n", - .{ dest, err }, + "Error: unable to remove '{s}' from cache '{s}': {t}\n", + .{ dest, cache.path, err }, ); return 1; }, @@ -233,14 +253,23 @@ pub fn runInner( return 2; } const pruned = cache.prune(alloc, max_age_s) catch |err| { - try stderr.print("Error: Unable to prune cache. Error: {}\n", .{err}); + try stderr.print( + "Error: unable to prune cache '{s}': {t}\n", + .{ cache.path, err }, + ); return 1; }; try stdout.print("Pruned cache entries: {d}\n", .{pruned}); return 0; } - var entries = try cache.list(alloc); + var entries = cache.list(alloc) catch |err| { + try stderr.print( + "Error: unable to read cache '{s}': {t}\n", + .{ cache.path, err }, + ); + return 1; + }; defer DiskCache.deinitEntries(alloc, &entries); // A positional query filters the listing: an exact `user@host` match, @@ -248,7 +277,7 @@ pub fn runInner( if (query) |q| { if (!DiskCache.isValidCacheKey(q)) { try stderr.print( - "Error: Invalid destination '{s}' (expected hostname or user@hostname)\n", + "Error: invalid destination '{s}' (expected hostname or user@hostname)\n", .{q}, ); return 2; @@ -474,10 +503,11 @@ test "runInner rejects multiple actions" { defer stderr.deinit(); // The check runs before any cache access, so it never touches disk. + const cache: DiskCache = .{ .path = "/nonexistent/ssh_cache" }; const code = try runInner(alloc, .{ .add = "example.com", .remove = "other.com", - }, null, &stdout.writer, &stderr.writer); + }, null, cache, &stdout.writer, &stderr.writer); try testing.expectEqual(@as(u8, 2), code); try testing.expectEqualStrings("", stdout.written()); @@ -487,7 +517,39 @@ test "runInner rejects multiple actions" { stderr.clearRetainingCapacity(); const code2 = try runInner(alloc, .{ .clear = true, - }, "example.com", &stdout.writer, &stderr.writer); + }, "example.com", cache, &stdout.writer, &stderr.writer); try testing.expectEqual(@as(u8, 2), code2); try testing.expect(std.mem.indexOf(u8, stderr.written(), "only one") != null); } + +test "runInner fails cleanly when the cache is unusable" { + const testing = std.testing; + const alloc = testing.allocator; + const io = testing.io; + + // A directory where the cache file belongs fails every operation. + var tmp = testing.tmpDir(.{}); + defer tmp.cleanup(); + try tmp.dir.createDirPath(io, "ssh_cache"); + const tmp_path = try tmp.dir.realPathFileAlloc(io, ".", alloc); + defer alloc.free(tmp_path); + const cache_path = try std.fs.path.join(alloc, &.{ tmp_path, "ssh_cache" }); + defer alloc.free(cache_path); + const cache: DiskCache = .{ .path = cache_path }; + + var stdout: std.Io.Writer.Allocating = .init(alloc); + defer stdout.deinit(); + var stderr: std.Io.Writer.Allocating = .init(alloc); + defer stderr.deinit(); + + const opts: []const Options = &.{ + .{}, // the default listing + .{ .add = "example.com" }, + .{ .remove = "example.com" }, + .{ .prune = .{ .duration = std.time.ns_per_s } }, + }; + for (opts) |o| { + const code = try runInner(alloc, o, null, cache, &stdout.writer, &stderr.writer); + try testing.expectEqual(@as(u8, 1), code); + } +} From ca3dc9eeac7893d6ac4507d60761226cb16fd09f Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Fri, 31 Jul 2026 16:41:25 -0400 Subject: [PATCH 2/2] cli: classify ssh cache errors in DiskCache --- src/cli/ssh-cache/DiskCache.zig | 19 +++++++++++++++++ src/cli/ssh.zig | 37 +++++++++++++++------------------ 2 files changed, 36 insertions(+), 20 deletions(-) diff --git a/src/cli/ssh-cache/DiskCache.zig b/src/cli/ssh-cache/DiskCache.zig index b700c443d..29f97efb1 100644 --- a/src/cli/ssh-cache/DiskCache.zig +++ b/src/cli/ssh-cache/DiskCache.zig @@ -393,6 +393,16 @@ fn readEntries( return entries; } +/// Whether an error from a cache operation reflects a problem with the cache +/// itself. A destination we can't key on and a lock held by another process +/// both leave the cache healthy, so neither is worth reporting. +pub fn isFailure(err: anyerror) bool { + return switch (err) { + error.InvalidCacheKey, error.CacheLocked => false, + else => true, + }; +} + // Supports both standalone hostnames and user@hostname format pub fn isValidCacheKey(key: []const u8) bool { if (key.len == 0) return false; @@ -704,6 +714,15 @@ test "disk cache add survives allocation failure" { } } +test isFailure { + const testing = std.testing; + + try testing.expect(!isFailure(error.InvalidCacheKey)); + try testing.expect(!isFailure(error.CacheLocked)); + try testing.expect(isFailure(error.AccessDenied)); + try testing.expect(isFailure(error.IsDir)); +} + test isValidHost { const testing = std.testing; diff --git a/src/cli/ssh.zig b/src/cli/ssh.zig index 5dc115652..f20188d91 100644 --- a/src/cli/ssh.zig +++ b/src/cli/ssh.zig @@ -253,10 +253,9 @@ fn runInner( if (cache) |c| { const cached = c.contains(alloc, dest) catch |err| cached: { - // An invalid key is a bad destination, not a bad cache. - if (err != error.InvalidCacheKey) warnPrint( + if (DiskCache.isFailure(err)) warnPrint( stderr, - "unable to add '{s}' to the cache: {t}", + "unable to read the cache '{s}': {t}", .{ c.path, err }, ); break :cached false; @@ -319,23 +318,21 @@ fn runInner( .real, ).toSeconds())) |_| { verbosePrint(opts, stderr, "cache: wrote {s}", .{entry.dest}); - } else |err| switch (err) { - // An uncacheable destination was already reported above, and a - // lock held by a concurrent `+ssh` resolves itself. - error.InvalidCacheKey, - error.CacheLocked, - => verbosePrint( - opts, - stderr, - "cache: skipped {s}: {t}", - .{ entry.dest, err }, - ), - - else => warnPrint( - stderr, - "unable to add '{s}' to the cache '{s}': {t}", - .{ entry.dest, entry.cache.path, err }, - ), + } else |err| { + if (DiskCache.isFailure(err)) { + warnPrint( + stderr, + "unable to add '{s}' to the cache '{s}': {t}", + .{ entry.dest, entry.cache.path, err }, + ); + } else { + verbosePrint( + opts, + stderr, + "cache: skipped {s}: {t}", + .{ entry.dest, err }, + ); + } } };