fix(gateway): eliminate double-kickstart and double-log in repair paths

Address review feedback from Greptile:

1. **Double-kickstart on start**: repairLaunchAgentBootstrap already starts
   the service (enable → bootstrap → kickstart). Previously, repair set
   loaded=true which caused service.restart() to fire — a redundant
   kill+restart cycle. Now repair emits success directly and returns.

2. **Double-kickstart on restart**: Same issue — repair set loaded=true
   causing the if(loaded) branch to call service.restart(). Now repair
   records the result via handledNotLoaded without setting loaded=true,
   so the service.restart() branch is skipped.

3. **Double-log in non-JSON restart**: The repair block logged the message
   AND the end-of-function handledNotLoaded?.message path logged it again.
   Removed the premature log from the repair block — the end-of-function
   path handles it.

4. **New test coverage**: Added non-JSON double-log regression test and
   explicit double-kickstart prevention test for restart path.

All 52 tests pass (26 launchd + 16 lifecycle-core + 10 config-guard).
This commit is contained in:
SPFAdvisors 2026-03-20 09:14:48 -04:00
parent 16d1462f8b
commit 6e4602a64b
3 changed files with 62 additions and 11 deletions

View File

@ -193,7 +193,8 @@ describe("config-guard gates repairNotLoaded (#43602 + #35862)", () => {
});
expect(repairNotLoaded).toHaveBeenCalledTimes(1);
expect(service.restart).toHaveBeenCalledTimes(1);
// Repair already started the service; service.restart() is NOT called
expect(service.restart).not.toHaveBeenCalled();
});
it("restart: aborts before repairNotLoaded when config is invalid", async () => {

View File

@ -205,8 +205,9 @@ describe("runServiceRestart token drift", () => {
});
expect(repairNotLoaded).toHaveBeenCalledTimes(1);
// After successful repair, start should proceed to restart the service.
expect(service.restart).toHaveBeenCalledTimes(1);
// Repair already started the service (bootstrap → kickstart), so
// service.restart() should NOT be called — avoids double-kickstart.
expect(service.restart).not.toHaveBeenCalled();
const jsonLine = runtimeLogs.find((line) => line.trim().startsWith("{"));
const payload = JSON.parse(jsonLine ?? "{}") as { result?: string };
expect(payload.result).toBe("started");
@ -307,6 +308,44 @@ describe("runServiceRestart token drift", () => {
expect(repairNotLoaded).not.toHaveBeenCalled();
});
it("restart: does not double-log repair message in non-JSON mode", async () => {
service.isLoaded.mockResolvedValue(false);
const repairNotLoaded = vi.fn().mockResolvedValue({ ok: true });
const serviceWithRepair = { ...service, repairNotLoaded };
const result = await runServiceRestart({
serviceNoun: "Gateway",
service: serviceWithRepair,
renderStartHints: () => [],
opts: { json: false },
onNotLoaded: async () => null,
});
expect(result).toBe(true);
expect(repairNotLoaded).toHaveBeenCalledTimes(1);
// The re-registered message should appear exactly once in non-JSON output
const reRegisterLogs = runtimeLogs.filter((line) => line.includes("re-registered"));
expect(reRegisterLogs).toHaveLength(1);
});
it("restart: does not call service.restart after successful repair (no double-kickstart)", async () => {
service.isLoaded.mockResolvedValue(false);
const repairNotLoaded = vi.fn().mockResolvedValue({ ok: true });
const serviceWithRepair = { ...service, repairNotLoaded };
await runServiceRestart({
serviceNoun: "Gateway",
service: serviceWithRepair,
renderStartHints: () => [],
opts: { json: true },
onNotLoaded: async () => null,
});
// repairLaunchAgentBootstrap already kickstarted the service.
// service.restart() should NOT be called — avoids redundant kill+restart.
expect(service.restart).not.toHaveBeenCalled();
});
it("restart: falls through to hints when repair returns ok:false", async () => {
service.isLoaded.mockResolvedValue(false);
const repairNotLoaded = vi.fn().mockResolvedValue({ ok: false });

View File

@ -219,12 +219,20 @@ export async function runServiceStart(params: {
try {
const repair = await params.service.repairNotLoaded({ env: process.env });
if (repair.ok) {
loaded = true;
// repairLaunchAgentBootstrap already started the service (enable →
// bootstrap → kickstart). Emit success and return — calling
// service.restart() here would be a redundant kill+restart cycle.
const message = `${params.serviceNoun} was not loaded — re-registered from existing service definition.`;
emit({
ok: true,
result: "started",
message,
service: buildDaemonServiceSnapshot(params.service, true),
});
if (!json) {
defaultRuntime.log(
`${params.serviceNoun} was not loaded — re-registered from existing service definition.`,
);
defaultRuntime.log(message);
}
return;
}
} catch {
// Best-effort repair; fall through to normal not-loaded handling.
@ -416,17 +424,20 @@ export async function runServiceRestart(params: {
// No running process to signal, but the service definition may still
// exist on disk (e.g. macOS LaunchAgent unloaded after sleep/idle).
// Re-register it so `restart` can proceed normally. See #43602.
//
// repairLaunchAgentBootstrap already starts the service (enable →
// bootstrap → kickstart), so we do NOT set `loaded = true` here —
// that would cause the `if (loaded)` branch below to call
// `service.restart()`, issuing a redundant kill+restart cycle.
// Instead we record the result via `handledNotLoaded` and let the
// end-of-function emit path handle messaging.
try {
const repair = await params.service.repairNotLoaded({ env: process.env });
if (repair.ok) {
loaded = true;
handledNotLoaded = {
result: "restarted",
message: `${params.serviceNoun} was not loaded — re-registered from existing service definition.`,
};
if (!json) {
defaultRuntime.log(handledNotLoaded.message);
}
}
} catch {
// Best-effort repair; fall through to normal not-loaded handling.