diff --git a/apps/server/src/device/DeviceHost.ts b/apps/server/src/device/DeviceHost.ts index eafa3b6319de3..5e35458d62efc 100644 --- a/apps/server/src/device/DeviceHost.ts +++ b/apps/server/src/device/DeviceHost.ts @@ -206,7 +206,7 @@ export class DeviceHost extends Context.Service< DeviceHostError | DeviceHostTimeoutError | AgentDeviceStartError | NodeRuntimeUnavailableError >; readonly current: Effect.Effect; - readonly stopAgent: Effect.Effect; + readonly stopAgent: Effect.Effect; /** Stops helpers. Devices themselves keep running; the user owns those. */ readonly stop: Effect.Effect; } diff --git a/apps/server/src/device/DeviceService.test.ts b/apps/server/src/device/DeviceService.test.ts index f2e0bcdf0c725..db29485a16ce4 100644 --- a/apps/server/src/device/DeviceService.test.ts +++ b/apps/server/src/device/DeviceService.test.ts @@ -99,6 +99,7 @@ const fixture = Effect.fn("fixture")(function* ( readonly routesStreams?: boolean; readonly onClose?: Effect.Effect<"ok" | "missing" | "reject" | "http_error" | "malformed">; readonly onPlatformAvailability?: Effect.Effect; + readonly onStopAgent?: Effect.Effect; } = {}, ) { const settings = yield* Ref.make(DEFAULT_SERVER_SETTINGS); @@ -292,7 +293,7 @@ const fixture = Effect.fn("fixture")(function* ( ), stopAgent: Effect.sync(() => { agentStops.push("stop"); - }), + }).pipe(Effect.andThen(options.onStopAgent ?? Effect.void)), stop: Effect.sync(() => { starts.push("stop"); }), @@ -632,7 +633,28 @@ describe("device setup consent", () => { it.effect("installs agent support only after the separate agent permission", () => Effect.gen(function* () { - const { service, agentStarts, agentStops, settings } = yield* fixture(); + const stopFails = yield* Ref.make(false); + const { service, agentStarts, agentStops, settings } = yield* fixture( + Effect.void, + undefined, + false, + undefined, + false, + undefined, + { + onStopAgent: Effect.flatMap(Ref.get(stopFails), (fails) => + fails + ? Effect.fail( + new DeviceHost.DeviceHostError({ + hostId: LOCAL_DEVICE_HOST_ID, + step: "stopping agent tools", + cause: new Error("stop failed"), + }), + ) + : Effect.void, + ), + }, + ); yield* service.configure({ enabled: true }); expect(agentStarts).toEqual([]); expect(yield* service.agentReadinessIfSupported()).toBeNull(); @@ -642,8 +664,15 @@ describe("device setup consent", () => { expect((yield* Ref.get(settings)).enableAgentDeviceAccess).toBe(true); expect((yield* service.state).agentAccessEnabled).toBe(true); + yield* Ref.set(stopFails, true); + const failed = yield* Effect.flip(service.configure({ agentAccessEnabled: false })); + expect(failed._tag).toBe("DeviceHostUnavailableError"); + expect((yield* Ref.get(settings)).enableAgentDeviceAccess).toBe(true); + expect((yield* service.state).agentAccessEnabled).toBe(true); + yield* Ref.set(stopFails, false); + yield* service.configure({ agentAccessEnabled: false, onboardingCompleted: true }); - expect(agentStops).toEqual(["stop"]); + expect(agentStops).toEqual(["stop", "stop"]); expect((yield* service.state).onboardingCompleted).toBe(true); expect((yield* Ref.get(settings)).deviceOnboardingCompleted).toBe(true); }).pipe(Effect.scoped), diff --git a/apps/server/src/device/DeviceService.ts b/apps/server/src/device/DeviceService.ts index b365a6b9fe5c1..0b8f322a3f260 100644 --- a/apps/server/src/device/DeviceService.ts +++ b/apps/server/src/device/DeviceService.ts @@ -702,6 +702,25 @@ export const makeWithHosts = Effect.fn("DeviceService.makeWithHosts")(function* const nextAgentAccess = input.agentAccessEnabled ?? currentSettings.agentAccessEnabled; yield* lifecycleLock.withPermit( Effect.gen(function* () { + // Stop first so a daemon that keeps running leaves agent access on, not falsely off. + if (nextEnabled && input.agentAccessEnabled === false) { + yield* Effect.forEach( + hosts.values(), + (host) => + host.stopAgent.pipe( + Effect.mapError( + (error) => + new DeviceHostUnavailableError({ + hostId: host.id, + hostLabel: host.label, + reason: `Device support failed during ${error.step}.`, + cause: error, + }), + ), + ), + { discard: true }, + ); + } yield* settings .updateSettings({ ...(input.enabled === undefined ? {} : { enableDeviceSupport: input.enabled }), @@ -724,8 +743,6 @@ export const makeWithHosts = Effect.fn("DeviceService.makeWithHosts")(function* ); if (!nextEnabled) { yield* Effect.forEach(hosts.values(), (host) => host.stop, { discard: true }); - } else if (input.agentAccessEnabled === false) { - yield* Effect.forEach(hosts.values(), (host) => host.stopAgent, { discard: true }); } yield* publish((state) => ({ ...state, diff --git a/apps/server/src/device/LocalDeviceHost.test.ts b/apps/server/src/device/LocalDeviceHost.test.ts index e133b4f9db36c..97a4dd549eb93 100644 --- a/apps/server/src/device/LocalDeviceHost.test.ts +++ b/apps/server/src/device/LocalDeviceHost.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "@effect/vitest"; import * as NodePath from "@effect/platform-node/NodePath"; import * as HostProcess from "@t3tools/shared/HostProcess"; import * as Effect from "effect/Effect"; +import * as Exit from "effect/Exit"; import * as Layer from "effect/Layer"; import * as Option from "effect/Option"; import * as Path from "effect/Path"; @@ -41,6 +42,7 @@ const agentFixture = Effect.fn("agentFixture")(function* ( ) => Effect.Effect, avdOutput: ProcessRunner.ProcessRunOutput = processOutput(0), previousDaemon?: typeof LocalDeviceHost.__testing.AgentDeviceDaemonFile.Type, + daemonStopOutput: ProcessRunner.ProcessRunOutput = processOutput(0), ) { const fs = yield* FileSystem.FileSystem; const path = yield* Path.Path; @@ -100,9 +102,9 @@ const agentFixture = Effect.fn("agentFixture")(function* ( Effect.provideService(ProcessRunner.ProcessRunner, { run: (input) => { runs.push(input); - return input.args.includes("devices") - ? startup(stateDir) - : Effect.succeed(input.args.includes("-list-avds") ? avdOutput : processOutput(0)); + if (input.args.includes("devices")) return startup(stateDir); + if (input.args.includes("daemon")) return Effect.succeed(daemonStopOutput); + return Effect.succeed(input.args.includes("-list-avds") ? avdOutput : processOutput(0)); }, }), Effect.provideService( @@ -338,6 +340,29 @@ describe("AgentDevice startup", () => { }).pipe(Effect.scoped, Effect.provide(NodeServices.layer)), ); + // regression: #35639 turning agent access off reported success while the daemon kept running + it.effect("reports an agent-device daemon that fails to stop", () => + Effect.gen(function* () { + for (const failedStop of [ + processOutput(1, "", "stop failed"), + processOutput(0, "", "", true), + ]) { + const { host } = yield* agentFixture( + () => Effect.die("A session that never readies agent tools must not start the daemon"), + processOutput(0), + { + httpPort: 12345, + token: "previous-token", + pid: process.pid, + version: AGENT_DEVICE_VERSION, + }, + failedStop, + ); + expect(Exit.isFailure(yield* Effect.exit(host.stopAgent))).toBe(true); + } + }).pipe(Effect.scoped, Effect.provide(NodeServices.layer)), + ); + it.effect("drops a dead daemon's record without running agent-device when the host stops", () => Effect.gen(function* () { // Above the Linux and macOS PID limits, so never a live process. diff --git a/apps/server/src/device/LocalDeviceHost.ts b/apps/server/src/device/LocalDeviceHost.ts index c5097509dcade..a068df36a2137 100644 --- a/apps/server/src/device/LocalDeviceHost.ts +++ b/apps/server/src/device/LocalDeviceHost.ts @@ -642,11 +642,19 @@ export const make = Effect.fn("LocalDeviceHost.make")(function* () { ...(Option.isSome(recorded) && recorded.value.version ? [recorded.value.version] : []), AGENT_DEVICE_VERSION, ]); - if (!entryPath) return; + if (!entryPath) { + // A live recorded daemon outlives its deleted CLI, so nothing here can stop it. + if (Option.isNone(recorded) || !recorded.value.pid) return; + return yield* new DeviceHost.DeviceHostError({ + hostId, + step: "stopping agent tools", + cause: new Error("agent-device is not installed, so its running daemon cannot be stopped"), + }); + } const nodePath = yield* resolveNodeExecutable("Local device support", hostEnvironment).pipe( Effect.provideService(HostProcess.Platform, hostPlatform), ); - yield* runner.run({ + const result = yield* runner.run({ command: nodePath, args: [ entryPath, @@ -659,10 +667,21 @@ export const make = Effect.fn("LocalDeviceHost.make")(function* () { timeout: Duration.seconds(10), timeoutBehavior: "timedOutResult", }); + if (result.timedOut || result.code !== 0) { + return yield* new DeviceHost.DeviceHostError({ + hostId, + step: "stopping agent tools", + cause: result, + }); + } }).pipe( Effect.provideService(FileSystem.FileSystem, fs), Effect.provideService(Path.Path, path), - Effect.ignore, + Effect.mapError((cause) => + cause._tag === "DeviceHostError" + ? cause + : new DeviceHost.DeviceHostError({ hostId, step: "stopping agent tools", cause }), + ), ); const ensureHubReady = Effect.fn("LocalDeviceHost.ensureHubReady")(function* ( @@ -872,7 +891,7 @@ export const make = Effect.fn("LocalDeviceHost.make")(function* () { const running = yield* Ref.getAndSet(runningRef, null); yield* stopHub(running?.hub); yield* fs.remove(hubStatePath(), { force: true }).pipe(Effect.ignore); - yield* stopAgentDeviceDaemon; + yield* stopAgentDeviceDaemon.pipe(Effect.ignore); }), ); diff --git a/apps/server/src/device/SshDeviceHost.ts b/apps/server/src/device/SshDeviceHost.ts index d769a85ddb080..9a4ecc98af0b2 100644 --- a/apps/server/src/device/SshDeviceHost.ts +++ b/apps/server/src/device/SshDeviceHost.ts @@ -530,7 +530,7 @@ export const make = Effect.fn("SshDeviceHost.make")(function* ( connectionScope = null; if (previousScope) yield* Scope.close(previousScope, Exit.void); if (!enabled) yield* provide(bootstrap(config, owner, "stop-agent")); - return yield* connect().pipe( + const connected = connect().pipe( Effect.onError(() => Effect.gen(function* () { const failedScope = connectionScope; @@ -541,6 +541,8 @@ export const make = Effect.fn("SshDeviceHost.make")(function* ( }), ), ); + // The daemon is already stopped; a failed hub reconnect is retried by the next ensureReady. + return yield* enabled ? connected : connected.pipe(Effect.orElseSucceed(() => null)); }), ); yield* Effect.addFinalizer(() => stop); @@ -580,7 +582,7 @@ export const make = Effect.fn("SshDeviceHost.make")(function* ( ), ), ), - stopAgent: changeAgent(false).pipe(Effect.asVoid, Effect.ignore), + stopAgent: changeAgent(false).pipe(Effect.asVoid), stop, platformAvailability, } satisfies DeviceHost.DeviceHost["Service"]; diff --git a/apps/server/src/device/sshDeviceScript.ts b/apps/server/src/device/sshDeviceScript.ts index 2e3e895293fa3..a5714232ba5ec 100644 --- a/apps/server/src/device/sshDeviceScript.ts +++ b/apps/server/src/device/sshDeviceScript.ts @@ -164,7 +164,8 @@ async function install(name, version, entry) { stopHub(hub); fs.rmSync(hubFile, { force: true }); } - stopAgent(); + const hadDaemon = Boolean(read(daemonFile)); + if (!stopAgent() && hadDaemon && mode === 'stop-agent') throw Error('Could not stop the agent-device daemon in ' + state); return; } // A start without agent tools means the server holds no claims, so a daemon still here is