From b310bd11716295438b9e2401d68539f04bd81a08 Mon Sep 17 00:00:00 2001 From: rustytrees Date: Wed, 29 Jul 2026 15:46:54 -0400 Subject: [PATCH] fix(services): create working_dir and accept a keg name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent bugs, both hit by `malt services start mosquitto`. **`ServiceNotFound` on a service that `services list` shows.** Services are registered under their launchd label (`com.malt.mosquitto`) while `keg_name` holds the formula (`mosquitto`), and the lifecycle lookup queried `name` only. Every other verb takes the formula name — `install mosquitto`, `uninstall mosquitto` — so the one place that demanded the label was the one place a user would not think to look it up. `resolveLabel` now accepts either. The label still wins outright, so an exact request is never reinterpreted, and a formula registering two services says so instead of picking one. `hasService` matches the same way so `status` and `start` agree on what exists. **Registered services whose working_dir does not exist cannot start.** launchd fails the spawn with EX_CONFIG (78) *before* exec when WorkingDirectory is missing, so StandardErrorPath is never created either: the user gets a bare "errored" and an empty log directory, with nothing anywhere saying which path was wrong. `cli/install.zig` already pre-creates the log directory for exactly this reason — the working dir was the half that got missed, and nothing else in the install path creates it. mosquitto is a plain example: it declares `working_dir var/mosquitto`, has no post_install, and its install step is a bare cmake install, so that directory never exists on a fresh install. `plist_mod.validate` has already confined the path to the keg or the prefix by the time we get here, so creating it is in-bounds. Both regression tests fail against the previous code — the working_dir one with FileNotFound, which is the same shape as the real failure. --- src/core/services/supervisor.zig | 60 ++++++++++++++++++++++++++- tests/services_test.zig | 69 ++++++++++++++++++++++++++++++++ tests/supervisor_pure_test.zig | 42 +++++++++++++++++++ 3 files changed, 169 insertions(+), 2 deletions(-) diff --git a/src/core/services/supervisor.zig b/src/core/services/supervisor.zig index 65c7d661..bbc0ccb9 100644 --- a/src/core/services/supervisor.zig +++ b/src/core/services/supervisor.zig @@ -36,6 +36,8 @@ const atomic = @import("../../fs/atomic.zig"); pub const SupervisorError = error{ OsNotSupported, ServiceNotFound, + /// The keg name matches more than one registered service. + AmbiguousService, LaunchctlFailed, IoFailed, DatabaseError, @@ -50,6 +52,7 @@ pub fn describeError(err: SupervisorError) []const u8 { return switch (err) { SupervisorError.OsNotSupported => "services are only supported on macOS for now", SupervisorError.ServiceNotFound => "service not registered", + SupervisorError.AmbiguousService => "that formula registers more than one service; name the one you mean (see `services list`)", SupervisorError.LaunchctlFailed => "launchctl command failed", SupervisorError.IoFailed => "filesystem error while managing service", SupervisorError.DatabaseError => "database error while managing service", @@ -203,6 +206,16 @@ pub fn register( else => return SupervisorError.IoFailed, }; + // launchd fails the spawn with EX_CONFIG (78) when WorkingDirectory does + // not exist — before exec, so StandardErrorPath is never created and the + // user gets a bare "errored" with no log to read. `install.zig` already + // pre-creates the log directory for the same reason; the working dir was + // the half that got missed. `plist_mod.validate` above has already + // confined it to the keg or the prefix, so creating it is in-bounds. + if (spec.working_dir) |wd| { + std.Io.Dir.cwd().createDirPath(ctx.io, wd) catch {}; + } + const plist_path = std.fmt.allocPrint(allocator, "{s}/service.plist", .{dir}) catch return SupervisorError.OutOfMemory; defer allocator.free(plist_path); @@ -255,11 +268,51 @@ fn userDomain(allocator: std.mem.Allocator) ![]const u8 { return std.fmt.allocPrint(allocator, "gui/{d}", .{uid}); } +/// Resolve what the user typed to a registered service label. +/// +/// Services are stored under their launchd label (`com.malt.mosquitto`) while +/// `keg_name` holds the formula (`mosquitto`). Every other malt verb takes the +/// formula name — `install mosquitto`, `uninstall mosquitto` — so requiring the +/// label here made `services start mosquitto` fail with `ServiceNotFound` on a +/// service that is registered and visible in `services list`. +/// +/// The label still wins when both could match, so an exact request is never +/// reinterpreted. A formula that registers more than one service is ambiguous +/// by keg name and says so rather than picking one. +/// Caller owns the returned slice. +pub fn resolveLabel(allocator: std.mem.Allocator, db: *sqlite.Database, name: []const u8) SupervisorError![]const u8 { + { + var exact = db.prepare("SELECT name FROM services WHERE name = ?;") catch + return SupervisorError.DatabaseError; + defer exact.finalize(); + exact.bindText(1, name) catch return SupervisorError.DatabaseError; + if (exact.step() catch return SupervisorError.DatabaseError) { + const p = exact.columnText(0) orelse return SupervisorError.ServiceNotFound; + return allocator.dupe(u8, std.mem.sliceTo(p, 0)) catch return SupervisorError.OutOfMemory; + } + } + + var by_keg = db.prepare("SELECT name FROM services WHERE keg_name = ? ORDER BY name;") catch + return SupervisorError.DatabaseError; + defer by_keg.finalize(); + by_keg.bindText(1, name) catch return SupervisorError.DatabaseError; + if (!(by_keg.step() catch return SupervisorError.DatabaseError)) return SupervisorError.ServiceNotFound; + const first = by_keg.columnText(0) orelse return SupervisorError.ServiceNotFound; + const owned = allocator.dupe(u8, std.mem.sliceTo(first, 0)) catch return SupervisorError.OutOfMemory; + errdefer allocator.free(owned); + // A second row means the keg name does not identify one service. + if (by_keg.step() catch return SupervisorError.DatabaseError) return SupervisorError.AmbiguousService; + return owned; +} + fn lookupPlistPath(allocator: std.mem.Allocator, db: *sqlite.Database, name: []const u8) SupervisorError![]const u8 { + const label = try resolveLabel(allocator, db, name); + defer allocator.free(label); + var stmt = db.prepare("SELECT plist_path FROM services WHERE name = ?;") catch return SupervisorError.DatabaseError; defer stmt.finalize(); - stmt.bindText(1, name) catch return SupervisorError.DatabaseError; + stmt.bindText(1, label) catch return SupervisorError.DatabaseError; if (!(stmt.step() catch return SupervisorError.DatabaseError)) return SupervisorError.ServiceNotFound; const p = stmt.columnText(0) orelse return SupervisorError.ServiceNotFound; return allocator.dupe(u8, std.mem.sliceTo(p, 0)) catch return SupervisorError.OutOfMemory; @@ -377,9 +430,12 @@ pub fn queryRuntime(io: std.Io, allocator: std.mem.Allocator, label: []const u8) } pub fn hasService(db: *sqlite.Database, name: []const u8) bool { - var stmt = db.prepare("SELECT 1 FROM services WHERE name = ?;") catch return false; + // Accepts a label or a keg name, matching `resolveLabel`, so `status` and + // `start` agree on what exists. + var stmt = db.prepare("SELECT 1 FROM services WHERE name = ? OR keg_name = ?;") catch return false; defer stmt.finalize(); stmt.bindText(1, name) catch return false; + stmt.bindText(2, name) catch return false; return stmt.step() catch false; } diff --git a/tests/services_test.zig b/tests/services_test.zig index 54386343..2c88dce8 100644 --- a/tests/services_test.zig +++ b/tests/services_test.zig @@ -120,3 +120,72 @@ test "tailLog returns last N lines of a small file" { try testing.expectEqualStrings("delta\nepsilon\n", aw.written()); } + +test "resolveLabel accepts the keg name a user would actually type" { + // Services are registered under their launchd label while `keg_name` holds + // the formula, so `services start mosquitto` used to fail with + // ServiceNotFound on a service that `services list` was showing. Every + // other verb takes the formula name. + var t = try TempDb.init("resolve_keg"); + defer t.deinit(); + + try t.db.exec( + \\INSERT INTO services(name, keg_name, plist_path, auto_start, last_status) + \\VALUES ('com.malt.mosquitto', 'mosquitto', '/tmp/m.plist', 0, 'registered'); + ); + + const by_keg = try supervisor.resolveLabel(testing.allocator, &t.db, "mosquitto"); + defer testing.allocator.free(by_keg); + try testing.expectEqualStrings("com.malt.mosquitto", by_keg); + + // The label itself keeps working, and still wins outright. + const by_label = try supervisor.resolveLabel(testing.allocator, &t.db, "com.malt.mosquitto"); + defer testing.allocator.free(by_label); + try testing.expectEqualStrings("com.malt.mosquitto", by_label); + + // `status` and `start` must agree on what exists. + try testing.expect(supervisor.hasService(&t.db, "mosquitto")); + try testing.expect(supervisor.hasService(&t.db, "com.malt.mosquitto")); + + try testing.expectError( + error.ServiceNotFound, + supervisor.resolveLabel(testing.allocator, &t.db, "nope"), + ); +} + +test "resolveLabel prefers an exact label over a keg name that collides with it" { + // Pathological but cheap to be correct about: if one service's label is + // another's keg name, an exact request must not be reinterpreted. + var t = try TempDb.init("resolve_collide"); + defer t.deinit(); + + try t.db.exec( + \\INSERT INTO services(name, keg_name, plist_path, auto_start, last_status) + \\VALUES ('alpha', 'beta', '/tmp/a.plist', 0, 'registered'), + \\ ('gamma', 'alpha', '/tmp/g.plist', 0, 'registered'); + ); + + const got = try supervisor.resolveLabel(testing.allocator, &t.db, "alpha"); + defer testing.allocator.free(got); + try testing.expectEqualStrings("alpha", got); +} + +test "resolveLabel refuses to guess when one formula registers two services" { + var t = try TempDb.init("resolve_ambiguous"); + defer t.deinit(); + + try t.db.exec( + \\INSERT INTO services(name, keg_name, plist_path, auto_start, last_status) + \\VALUES ('com.malt.pg.main', 'postgresql@16', '/tmp/1.plist', 0, 'registered'), + \\ ('com.malt.pg.repl', 'postgresql@16', '/tmp/2.plist', 0, 'registered'); + ); + + try testing.expectError( + error.AmbiguousService, + supervisor.resolveLabel(testing.allocator, &t.db, "postgresql@16"), + ); + // Naming one of them exactly still works. + const exact = try supervisor.resolveLabel(testing.allocator, &t.db, "com.malt.pg.repl"); + defer testing.allocator.free(exact); + try testing.expectEqualStrings("com.malt.pg.repl", exact); +} diff --git a/tests/supervisor_pure_test.zig b/tests/supervisor_pure_test.zig index 9adb7f59..a6b96bad 100644 --- a/tests/supervisor_pure_test.zig +++ b/tests/supervisor_pure_test.zig @@ -318,3 +318,45 @@ test "tailLog reports IoFailed for a missing file" { supervisor.tailLog(std.Options.debug_io, testing.allocator, "/tmp/malt_sup_tail_missing_xyz", 1, &aw.writer), ); } + +test "register creates the working directory launchd needs before it will spawn" { + // launchd rejects a job whose WorkingDirectory is missing with EX_CONFIG + // (78) *before* exec, so StandardErrorPath is never created and the user + // sees a bare "errored" with no log to read. `cli/install.zig` already + // pre-creates the log directory for exactly this reason; the working dir + // was the half that got missed, and nothing else in the install path + // creates it — a formula like mosquitto declares `working_dir var/mosquitto` + // that its own install step never makes. + var fx = try Fixture.init("register_workdir"); + defer fx.deinit(); + const prefix = fx.base; + const cellar = fx.p("Cellar/testkeg/1.0"); + _ = c.setenv("MALT_PREFIX", prefix, 1); + defer _ = c.unsetenv("MALT_PREFIX"); + + var db = try sqlite.Database.open(":memory:"); + defer db.close(); + try schema.initSchema(&db); + + const workdir = fx.p("var/testkeg"); + // Nothing has created it yet — this is the state a fresh install leaves. + try testing.expectError( + error.FileNotFound, + std.Io.Dir.accessAbsolute(std.Options.debug_io, workdir, .{}), + ); + + const spec = plist_mod.ServiceSpec{ + .label = "com.malt.test.workdir", + .program_args = &.{fx.p("Cellar/testkeg/1.0/bin/echo")}, + .working_dir = workdir, + .env = &.{}, + .keep_alive = false, + .stdout_path = fx.p("out.log"), + .stderr_path = fx.p("err.log"), + }; + const ctx: supervisor.SupervisorCtx = .{ .allocator = testing.allocator, .io = std.Options.debug_io, .db = &db }; + try supervisor.register(ctx, spec, "testkeg", false, cellar, prefix); + + // Present after registration, so the later bootstrap can actually spawn. + try std.Io.Dir.accessAbsolute(std.Options.debug_io, workdir, .{}); +}