mirrors/ghostty
1
0
Fork 0
mirror of https://github.com/ghostty-org/ghostty.git synced 2026-08-01 13:19:09 +00:00
Code Issues Packages Projects Releases Wiki Activity

cli: report ssh terminfo cache failures (#13533)

Browse Source 
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
This commit is contained in:
Mitchell Hashimoto
2026-07-31 19:59:13 -07:00
committed by GitHub
parent 154ddc2a2f ca3dc9eeac
commit 08f039fbb3
3 changed files with 125 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
src/cli/ssh-cache/DiskCache.zig
View File

@@ -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;

32
src/cli/ssh.zig
View File

@@ -245,14 +245,23 @@ 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: {
if (DiskCache.isFailure(err)) warnPrint(
stderr,
"unable to read the cache '{s}': {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 +275,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 +306,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});
@@ -310,7 +319,20 @@ fn runInner(
).toSeconds())) |_| {
verbosePrint(opts, stderr, "cache: wrote {s}", .{entry.dest});
} else |err| {
log.debug("cache add failed for '{s}': {}", .{ entry.dest, 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 },
);
}
}
};

96
src/cli/ssh_cache.zig
View File

@@ -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);
}
}