diff --git a/apps/ade-cli/src/commands/doctor.test.ts b/apps/ade-cli/src/commands/doctor.test.ts index 119f2f294..14a7dae42 100644 --- a/apps/ade-cli/src/commands/doctor.test.ts +++ b/apps/ade-cli/src/commands/doctor.test.ts @@ -225,6 +225,35 @@ describe("doctor row evaluation", () => { status: "fail", detail: expect.stringMatching(/failing for 5m .* slow leg: http \(9\.2s\)/), })); + // Only the brain-session states get the restart remedy; a slow HTTP leg is + // not fixed by restarting. + expect(rows.find((row) => row.key === "publish")?.detail).not.toContain("ade brain restart"); + }); + + it("points an unreadable account session at `ade brain restart`", () => { + // Desktop's Connections panel shows a Repair (brain restart) button for + // this state; the CLI has to name the same remedy or an agent is stuck. + const warning = healthyInput(); + warning.publishHealth = createSyncAccountDirectoryHealth("token_unreadable", null, { + failingSinceMs: NOW - 30_000, + }); + const failing = healthyInput(); + failing.publishHealth = createSyncAccountDirectoryHealth("token_unreadable", null, { + failingSinceMs: NOW - 5 * 60_000, + }); + + expect(evaluateDoctorRows(warning).find((row) => row.key === "publish")).toEqual( + expect.objectContaining({ + status: "warn", + detail: expect.stringContaining("ade brain restart"), + }), + ); + expect(evaluateDoctorRows(failing).find((row) => row.key === "publish")).toEqual( + expect.objectContaining({ + status: "fail", + detail: expect.stringContaining("ade brain restart"), + }), + ); }); it("fails only the brain while dependent checks degrade when the socket is dead", () => { diff --git a/apps/ade-cli/src/commands/doctor.ts b/apps/ade-cli/src/commands/doctor.ts index 18959bf3f..bdc35185b 100644 --- a/apps/ade-cli/src/commands/doctor.ts +++ b/apps/ade-cli/src/commands/doctor.ts @@ -6,6 +6,7 @@ import { parseRuntimePublishHealth, type RuntimePublishHealth, } from "../../../desktop/src/shared/adeRuntimeProtocol"; +import { isBrainAccountSessionFailure } from "../../../desktop/src/shared/types/sync"; import type { SyncAccountDirectoryHealth, SyncRouteHealth, @@ -647,12 +648,20 @@ function publishRow( const failingForMs = health.failingSinceMs == null ? null : Math.max(0, nowMs - health.failingSinceMs); + // The Connections panel offers a "Repair" (brain restart) button for exactly + // the states `isBrainAccountSessionFailure` covers, because a replacement + // brain re-reads the account session from scratch. `ade brain restart` is the + // CLI's equivalent, so name it here rather than leaving an agent holding a + // bare `token_unreadable` with no next step. + const remedy = isBrainAccountSessionFailure(health.state) + ? " · run `ade brain restart` so the brain re-reads the account session" + : ""; if (failingForMs != null && failingForMs >= PUBLISH_FAILURE_RED_MS) { return { key: "publish", label: "Publish health", status: "fail", - detail: `failing for ${compactDuration(failingForMs)} · ${health.state}${publishLegDetail(health)}`, + detail: `failing for ${compactDuration(failingForMs)} · ${health.state}${publishLegDetail(health)}${remedy}`, }; } if (health.state === "published") { @@ -673,8 +682,8 @@ function publishRow( label: "Publish health", status: "warn", detail: failingForMs == null - ? `${health.state}${health.skipReason ? ` · ${health.skipReason}` : ""}` - : `failing for ${compactDuration(failingForMs)} · ${health.state}${publishLegDetail(health)}`, + ? `${health.state}${health.skipReason ? ` · ${health.skipReason}` : ""}${remedy}` + : `failing for ${compactDuration(failingForMs)} · ${health.state}${publishLegDetail(health)}${remedy}`, }; } diff --git a/apps/ade-cli/src/multiProjectRpcServer.test.ts b/apps/ade-cli/src/multiProjectRpcServer.test.ts index ec39b293e..bb3352d03 100644 --- a/apps/ade-cli/src/multiProjectRpcServer.test.ts +++ b/apps/ade-cli/src/multiProjectRpcServer.test.ts @@ -97,6 +97,7 @@ function makeAccountAuthServiceMock() { expiresAt: null, })), getSessionReadState: vi.fn(() => "missing" as const), + getSessionReadFailureReason: vi.fn(() => null), getAccessToken: vi.fn(async () => "test-access-token"), createToken: vi.fn(async () => ({ token: "test-refresh-token", @@ -579,6 +580,7 @@ describe("multi-project RPC server", () => { expiresAt: null, })), getSessionReadState: vi.fn(() => "missing" as const), + getSessionReadFailureReason: vi.fn(() => null), getAccessToken: vi.fn(), createToken: vi.fn(), cancelLogin: vi.fn(), diff --git a/apps/ade-cli/src/serviceManager/installWindows.test.ts b/apps/ade-cli/src/serviceManager/installWindows.test.ts index 7f9986a4f..99afa257d 100644 --- a/apps/ade-cli/src/serviceManager/installWindows.test.ts +++ b/apps/ade-cli/src/serviceManager/installWindows.test.ts @@ -37,6 +37,7 @@ import { WINDOWS_REG_COMMAND, WINDOWS_SCHTASKS_COMMAND, WINDOWS_TASK_ACTION_FIELD_SEPARATOR, + WINDOWS_TASKKILL_COMMAND, renderWindowsServiceLauncher, } from "./installWindows"; @@ -395,6 +396,53 @@ describe("Windows background service helpers", () => { ]); }); + it.each([ + { label: "an ordinary install", forceEnv: {} }, + { label: "a Repair-forced install", forceEnv: { ADE_FORCE_RUNTIME_SERVICE_RESTART: "1" } }, + ])("restarts the running supervisor on $label", async ({ forceEnv }) => { + // The desktop Repair button sets ADE_FORCE_RUNTIME_SERVICE_RESTART, and + // ONLY installLaunchd reads it — it exists to defeat launchd's "unchanged + // plist + loaded + responsive => skip" fast path. Windows honours the flag + // by construction rather than by reading it: this install has no skip path, + // so it always taskkills the supervisor tree and starts a fresh one. Assert + // that for BOTH env shapes, so a future "already installed, leave it alone" + // optimisation here cannot silently turn Repair into a no-op on Windows. + const home = makeTempHome("ade-windows-service-force-restart-"); + const launcherPath = path.join(home, "brain-service.ps1"); + fs.writeFileSync(`${launcherPath}.pid.json`, JSON.stringify(readyPidRecord), "utf8"); + const calls: Array<{ command: string; args: string[] }> = []; + const spawnSync = spawnSequence(calls, [ + { status: 3, stdout: "", stderr: "" }, // legacy task: absent + { status: 3, stdout: "", stderr: "" }, // channel task: absent + { status: 0, stdout: ` ${taskName} REG_SZ x`, stderr: "" }, // Run entry: installed + { status: 0, stdout: "", stderr: "" }, // supervisor probe: running + { status: 0, stdout: "SUCCESS", stderr: "" }, // taskkill /T /F + { status: 0, stdout: "SUCCESS: deleted", stderr: "" }, // reg delete + { status: 0, stdout: "SUCCESS: created", stderr: "" }, // reg add + { status: 0, stdout: "1234", stderr: "" }, // start task + ]); + + const result = await installWindowsService({ + ...immediateReadiness, + command: serviceCommand, + env: { USERDOMAIN: "ADEBOX", USERNAME: "arul", ...forceEnv }, + launcherPath, + serviceName, + spawnSync, + userName: taskUser, + }); + + expect(result.ok).toBe(true); + expect(calls).toContainEqual({ + command: WINDOWS_TASKKILL_COMMAND, + args: ["/PID", String(readyPidRecord.supervisorPid), "/T", "/F"], + }); + expect(calls.at(-1)).toEqual({ + command: WINDOWS_POWERSHELL_COMMAND, + args: buildWindowsStartTaskArgs(launcherPath, resolveWindowsStartTaskName(taskName)), + }); + }); + it("ends and replaces a running channel task before starting the repaired runtime", async () => { const calls: Array<{ command: string; args: string[] }> = []; const spawnSync = spawnSequence(calls, [ diff --git a/apps/ade-cli/src/services/account/accountAuthService.ts b/apps/ade-cli/src/services/account/accountAuthService.ts index ee127fbd4..48b80773b 100644 --- a/apps/ade-cli/src/services/account/accountAuthService.ts +++ b/apps/ade-cli/src/services/account/accountAuthService.ts @@ -10,7 +10,10 @@ import { shouldIgnoreDevelopmentClerkConfiguration, warnDevelopmentClerkIgnored, } from "../../../../desktop/src/shared/accountDirectory"; -import type { SyncCredentialStore } from "../credentials/credentialStore"; +import type { + CredentialStoreReadFailureReason, + SyncCredentialStore, +} from "../credentials/credentialStore"; import { runWithAbortSignal } from "../sync/abortSignal"; export const ACCOUNT_SESSION_CREDENTIAL_KEY = "account.session.v1"; @@ -105,6 +108,18 @@ export type AccountAuthStatus = { export type AccountSessionReadState = "available" | "missing" | "unreadable"; +/** + * Which read path produced an "unreadable" session. Coarse and closed so it can + * be reported as a product-analytics property. + */ +export type AccountSessionReadFailureReason = + /** Everything the credential store itself can report (decrypt/key-material/format). */ + | CredentialStoreReadFailureReason + /** The credential decrypted but the stored session record did not parse. */ + | "session_parse" + /** The credential store threw while being read. */ + | "read_error"; + export type AccountLoginStartResult = { sessionId: string; authorizeUrl: string; @@ -193,6 +208,8 @@ export type AccountAuthService = { getStatus(): AccountAuthStatus; /** Last persisted-session read result, refreshed by getStatus/getAccessToken. */ getSessionReadState(): AccountSessionReadState; + /** Why the last read was unreadable, or null when it was not. */ + getSessionReadFailureReason(): AccountSessionReadFailureReason | null; getAccessToken(options?: AccountAccessTokenOptions): Promise; createToken(): Promise; cancelLogin(sessionId: string): void; @@ -793,6 +810,14 @@ export function createAccountAuthService(args: { let envCredentialEpoch = 0; let authEpoch = 0; let sessionReadState: AccountSessionReadState = "missing"; + let sessionReadFailureReason: AccountSessionReadFailureReason | null = null; + const setSessionReadState = ( + state: AccountSessionReadState, + reason: AccountSessionReadFailureReason | null = null, + ): void => { + sessionReadState = state; + sessionReadFailureReason = state === "unreadable" ? reason : null; + }; let lastObservedSignedIn: boolean | null = null; let locallyRejectedSessionRaw: string | null = null; const signedInListeners = new Set<() => void>(); @@ -895,7 +920,7 @@ export function createAccountAuthService(args: { // rejected on every read instead of being erased. authEpoch += 1; lastObservedSignedIn = false; - sessionReadState = "missing"; + setSessionReadState("missing"); warnDevelopmentClerkIgnored(); }; @@ -915,15 +940,19 @@ export function createAccountAuthService(args: { const session = locallyRejected ? null : parseStoredSession(stored); - sessionReadState = locallyRejected - ? "missing" - : stored == null - ? args.credentialStore.getLastReadState?.() === "unreadable" - ? "unreadable" - : "missing" - : session - ? "available" - : "unreadable"; + if (locallyRejected) { + setSessionReadState("missing"); + } else if (stored == null) { + const storeUnreadable = args.credentialStore.getLastReadState?.() === "unreadable"; + setSessionReadState( + storeUnreadable ? "unreadable" : "missing", + storeUnreadable + ? args.credentialStore.getLastReadFailureReason?.() ?? null + : null, + ); + } else { + setSessionReadState(session ? "available" : "unreadable", "session_parse"); + } return { raw: stored, session }; }; @@ -945,12 +974,12 @@ export function createAccountAuthService(args: { accessToken: retry.session.accessToken, oauthConfig: retry.session.oauthConfig, })) { - sessionReadState = "missing"; + setSessionReadState("missing"); return { raw: retry.raw, session: null }; } return retry; } catch (error) { - sessionReadState = "unreadable"; + setSessionReadState("unreadable", "read_error"); logger.warn("account.session_read_failed", { error: error instanceof Error ? error.message : String(error), }); @@ -969,7 +998,7 @@ export function createAccountAuthService(args: { // peer may be rotating the credential. authEpoch += 1; lastObservedSignedIn = false; - sessionReadState = "missing"; + setSessionReadState("missing"); return false; } let deleted = false; @@ -982,7 +1011,7 @@ export function createAccountAuthService(args: { if (deleted) { authEpoch += 1; lastObservedSignedIn = false; - sessionReadState = "missing"; + setSessionReadState("missing"); } return deleted; }; @@ -2139,6 +2168,7 @@ export function createAccountAuthService(args: { pollDeviceLogin, getStatus, getSessionReadState: () => sessionReadState, + getSessionReadFailureReason: () => sessionReadFailureReason, getAccessToken, createToken, cancelLogin, diff --git a/apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts b/apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts index bab18c63d..2a6c102c2 100644 --- a/apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts +++ b/apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts @@ -12,6 +12,11 @@ import { DEFAULT_ADE_ACCOUNT_DIRECTORY_URL, DEVELOPMENT_ADE_ACCOUNT_DIRECTORY_URL, } from "../../../../desktop/src/shared/accountDirectory"; +import type { ProductAnalyticsCapture } from "../../../../desktop/src/shared/types/productAnalytics"; +import { + createEpisodeAnalytics, + EPISODE_ANALYTICS_MINIMUM_INTERVAL_MS, +} from "./episodeAnalytics"; const sharedAccountAuthService = vi.hoisted(() => ({ getStatus: vi.fn(() => ({ @@ -21,6 +26,7 @@ const sharedAccountAuthService = vi.hoisted(() => ({ })), getAccessToken: vi.fn(async () => "account-token"), getSessionReadState: vi.fn(() => "available" as const), + getSessionReadFailureReason: vi.fn(() => null), onSignedIn: vi.fn(() => () => {}), })); @@ -423,6 +429,77 @@ describe("account machine publisher health", () => { expect(captureAnalytics).toHaveBeenCalledTimes(2); }); + it("captures one account-session-unreadable event per unreadable episode", async () => { + let sessionReadState: "available" | "unreadable" = "unreadable"; + const captureAnalytics = vi.fn(); + const service = createAccountMachinePublisherService({ + getAccessToken: async () => "account-token", + getAccountStatus: () => ({ + signedIn: sessionReadState === "available", + sessionReadState, + sessionReadFailureReason: sessionReadState === "unreadable" + ? ("no_os_key_material" as const) + : null, + }), + getSnapshot: async () => snapshot(), + getMachineKey: () => "machine-studio", + directoryBaseUrl: () => "https://directory.example", + fetchImpl: vi.fn(async () => new Response(null, { status: 204 })), + captureAnalytics, + }); + + await service.publishNow(); + await service.publishNow(); + + expect(captureAnalytics).toHaveBeenCalledTimes(1); + expect(captureAnalytics).toHaveBeenCalledWith({ + event: "ade_account_session_unreadable", + surface: "api", + properties: { code: "no_os_key_material" }, + dedupeKey: "account-session-unreadable:no_os_key_material", + minimumIntervalMs: 24 * 60 * 60 * 1_000, + }); + + // A readable session ends the episode, so a genuinely new one reports again. + sessionReadState = "available"; + await service.publishNow(); + sessionReadState = "unreadable"; + await service.publishNow(); + + expect(captureAnalytics).toHaveBeenCalledTimes(2); + }); + + it("captures an account-session-unreadable event when the status read throws", async () => { + // A throwing status read is the same failure as an "unreadable" one, and + // `read_error` is a documented code for it. Reporting only the non-throwing + // path left this half of the incident class invisible. + const captureAnalytics = vi.fn(); + const service = createAccountMachinePublisherService({ + getAccessToken: async () => "account-token", + getAccountStatus: () => { + throw new Error("credential store unreadable"); + }, + getSnapshot: async () => snapshot(), + getMachineKey: () => "machine-studio", + directoryBaseUrl: () => "https://directory.example", + fetchImpl: vi.fn(async () => new Response(null, { status: 204 })), + captureAnalytics, + }); + + await service.publishNow(); + await service.publishNow(); + + expect(service.getPublisherHealth().state).toBe("token_unreadable"); + expect(captureAnalytics).toHaveBeenCalledTimes(1); + expect(captureAnalytics).toHaveBeenCalledWith({ + event: "ade_account_session_unreadable", + surface: "api", + properties: { code: "read_error" }, + dedupeKey: "account-session-unreadable:read_error", + minimumIntervalMs: 24 * 60 * 60 * 1_000, + }); + }); + it("starts a new publish-failure analytics episode after a benign skip", async () => { let clock = 0; let syncEnabled = true; @@ -1340,3 +1417,93 @@ describe("account machine registration publisher", () => { service.dispose(); }); }); + +const analyticsFor = (capture: () => ((input: ProductAnalyticsCapture) => void) | undefined) => + createEpisodeAnalytics({ + event: "ade_publish_failing", + dedupePrefix: "publish-failing", + capture, + }); + +describe("createEpisodeAnalytics", () => { + it("reports once per episode no matter how often the condition is observed", () => { + const capture = vi.fn(); + const episode = analyticsFor(() => capture); + + episode.report({ dedupeValue: 1, properties: { code: "http_error" } }); + episode.report({ dedupeValue: 1, properties: { code: "http_error" } }); + episode.report({ dedupeValue: 2, properties: { code: "token_timeout" } }); + + expect(capture).toHaveBeenCalledTimes(1); + expect(capture).toHaveBeenCalledWith({ + event: "ade_publish_failing", + surface: "api", + properties: { code: "http_error" }, + dedupeKey: "publish-failing:1", + minimumIntervalMs: EPISODE_ANALYTICS_MINIMUM_INTERVAL_MS, + }); + }); + + it("re-arms only after the condition clears", () => { + const capture = vi.fn(); + const episode = analyticsFor(() => capture); + + episode.report({ dedupeValue: "first", properties: {} }); + episode.end(); + episode.report({ dedupeValue: "second", properties: {} }); + // A repeated clear must not open a second report inside the same episode. + episode.end(); + episode.end(); + episode.report({ dedupeValue: "third", properties: {} }); + episode.report({ dedupeValue: "fourth", properties: {} }); + + expect(capture.mock.calls.map(([input]) => input.dedupeKey)).toEqual([ + "publish-failing:first", + "publish-failing:second", + "publish-failing:third", + ]); + }); + + it("reads the capture handler at report time, not at construction", () => { + // The publisher builds its episodes before its options are necessarily + // wired up; a handler snapshotted at construction would drop every event. + let capture: ((input: ProductAnalyticsCapture) => void) | undefined; + const episode = analyticsFor(() => capture); + + episode.report({ dedupeValue: "missed", properties: {} }); + const late = vi.fn(); + capture = late; + episode.end(); + episode.report({ dedupeValue: "seen", properties: {} }); + + expect(late).toHaveBeenCalledTimes(1); + expect(late.mock.calls[0]?.[0].dedupeKey).toBe("publish-failing:seen"); + }); + + it("still consumes the episode when no capture handler is configured", () => { + // An absent handler must not leave the episode armed: once the handler + // appears mid-episode it would emit for a failure already in progress. + let capture: ((input: ProductAnalyticsCapture) => void) | undefined; + const episode = analyticsFor(() => capture); + + episode.report({ dedupeValue: "in-progress", properties: {} }); + const late = vi.fn(); + capture = late; + episode.report({ dedupeValue: "in-progress", properties: {} }); + + expect(late).not.toHaveBeenCalled(); + }); + + it("swallows a throwing capture and still consumes the episode", () => { + // A synchronous capture failure escaping here would surface as the + // publisher's transport_error instead of the real token_unreadable + // outcome, hiding the repair path from the user. + const capture = vi.fn(() => { throw new Error("posthog exploded"); }); + const episode = analyticsFor(() => capture); + + expect(() => episode.report({ dedupeValue: "boom", properties: {} })).not.toThrow(); + episode.report({ dedupeValue: "boom", properties: {} }); + + expect(capture).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/ade-cli/src/services/account/accountMachinePublisherService.ts b/apps/ade-cli/src/services/account/accountMachinePublisherService.ts index 7d34811e6..813c65b1e 100644 --- a/apps/ade-cli/src/services/account/accountMachinePublisherService.ts +++ b/apps/ade-cli/src/services/account/accountMachinePublisherService.ts @@ -18,6 +18,7 @@ import { import { getSignedInAccountAccessToken, type AccountAuthStatus, + type AccountSessionReadFailureReason, type AccountSessionReadState, } from "./accountAuthService"; import { @@ -28,6 +29,7 @@ import { createMachineIdentitySigningStore, MACHINE_IDENTITY_SIGNING_FILE_NAME, } from "../sync/machineIdentitySigningStore"; +import { createEpisodeAnalytics } from "./episodeAnalytics"; export const ACCOUNT_MACHINE_HEARTBEAT_MS = 30_000; export const ACCOUNT_MACHINE_RELAY_STATE_POLL_MS = 2_000; @@ -121,6 +123,8 @@ export type AccountMachineRegistrationSnapshot = Pick< type PublisherAccountStatus = Pick & Partial> & { sessionReadState: AccountSessionReadState; + /** Which read path produced an unreadable session (analytics only). */ + sessionReadFailureReason?: AccountSessionReadFailureReason | null; }; type PublishedRelayEndpoint = Extract; @@ -330,13 +334,24 @@ export function createAccountMachinePublisherService(options: { let lastWarning: string | null = null; let transientFailureCount = 0; let successfulPublishCount = 0; - let publishFailureAnalyticsEmitted = false; let unsubscribeSignIn: (() => void) | null = null; let health = createSyncAccountDirectoryHealth( "sync_disabled", "Account-directory publishing has not started.", ); + const captureAnalytics = () => options.captureAnalytics; + const publishFailureAnalytics = createEpisodeAnalytics({ + event: "ade_publish_failing", + dedupePrefix: "publish-failing", + capture: captureAnalytics, + }); + const sessionUnreadableAnalytics = createEpisodeAnalytics({ + event: "ade_account_session_unreadable", + dedupePrefix: "account-session-unreadable", + capture: captureAnalytics, + }); + const readSigningPublicKey = (): string | null => { try { return options.getMachineIdentitySigningPublicKey?.().trim() || null; @@ -449,27 +464,37 @@ export function createAccountMachinePublisherService(options: { : null, }; if (health.failingSinceMs == null) { - publishFailureAnalyticsEmitted = false; - } else if ( - !publishFailureAnalyticsEmitted - && args.attemptAt - health.failingSinceMs >= PUBLISH_FAILURE_ANALYTICS_THRESHOLD_MS - ) { - publishFailureAnalyticsEmitted = true; - const leg = failureLegForState(state); - options.captureAnalytics?.({ - event: "ade_publish_failing", - surface: "api", + publishFailureAnalytics.end(); + } else if (args.attemptAt - health.failingSinceMs >= PUBLISH_FAILURE_ANALYTICS_THRESHOLD_MS) { + publishFailureAnalytics.report({ + dedupeValue: health.failingSinceMs, properties: { failing_minutes: Math.max(2, Math.floor((args.attemptAt - health.failingSinceMs) / 60_000)), - leg, + leg: failureLegForState(state), code: state, }, - dedupeKey: `publish-failing:${health.failingSinceMs}`, - minimumIntervalMs: 24 * 60 * 60 * 1_000, }); } }; + /** + * Reports the "app signed in, brain cannot read the session" failure once per + * episode, tagged with the read path that produced it. + */ + const observeSessionReadFailure = ( + code: AccountSessionReadFailureReason | "unknown", + ): void => { + sessionUnreadableAnalytics.report({ dedupeValue: code, properties: { code } }); + }; + + const observeSessionReadState = (status: PublisherAccountStatus | null): void => { + if (!status || status.sessionReadState !== "unreadable") { + sessionUnreadableAnalytics.end(); + return; + } + observeSessionReadFailure(status.sessionReadFailureReason ?? "unknown"); + }; + const publish = async (): Promise => { if (disposed) return; const attemptAt = now(); @@ -711,6 +736,10 @@ export function createAccountMachinePublisherService(options: { try { accountStatus = options.getAccountStatus?.() ?? null; } catch { + // A throwing status read is the same user-visible failure as an + // "unreadable" one — the brain cannot obtain the session — so it belongs + // to the same episode and reports the documented `read_error` code. + observeSessionReadFailure("read_error"); outcome("token_unreadable", { attemptAt, skipReason: "The ADE brain could not read account status.", @@ -719,6 +748,7 @@ export function createAccountMachinePublisherService(options: { }); return; } + observeSessionReadState(accountStatus); if (isPublisherSignedOut(accountStatus)) { clearRetainedRelayState(); const unreadable = accountStatus?.sessionReadState === "unreadable"; @@ -1132,6 +1162,7 @@ export function createBrainAccountMachinePublisherService(options: { userId: status.userId, source: status.source ?? null, sessionReadState: accountAuthService.getSessionReadState(), + sessionReadFailureReason: accountAuthService.getSessionReadFailureReason(), }; }, isSyncEnabled: options.isSyncEnabled, diff --git a/apps/ade-cli/src/services/account/episodeAnalytics.ts b/apps/ade-cli/src/services/account/episodeAnalytics.ts new file mode 100644 index 000000000..a325649be --- /dev/null +++ b/apps/ade-cli/src/services/account/episodeAnalytics.ts @@ -0,0 +1,59 @@ +import type { ProductAnalyticsCapture } from "../../../../desktop/src/shared/types/productAnalytics"; + +/** + * One event per failure episode, per day. Long enough that a machine stuck in + * the same failure for a week reports once a day rather than once a restart. + */ +export const EPISODE_ANALYTICS_MINIMUM_INTERVAL_MS = 24 * 60 * 60 * 1_000; + +export type EpisodeAnalytics = { + report(input: { + dedupeValue: string | number; + properties: ProductAnalyticsCapture["properties"]; + }): void; + /** The condition cleared: a genuinely new episode may report again. */ + end(): void; +}; + +/** + * Edge-triggered analytics for a failure EPISODE: at most one event while the + * condition holds, re-armed only once it clears. A brain that can never read + * the credential file the app just wrote, or that has been failing to publish + * for minutes, publishes nothing while the user only sees "onboarding is + * broken" — one event per episode makes that measurable without turning a + * persistent failure into a per-attempt firehose. + * + * `capture` is an accessor rather than the handler itself so the owning service + * keeps reading its (optionally late-bound) capture option at report time. + */ +export function createEpisodeAnalytics(args: { + event: ProductAnalyticsCapture["event"]; + dedupePrefix: string; + capture: () => ((input: ProductAnalyticsCapture) => void) | undefined; +}): EpisodeAnalytics { + let emitted = false; + return { + report(input): void { + if (emitted) return; + emitted = true; + // `emitted` is set first on purpose: a throwing capture still consumes + // the episode, so a permanently broken analytics sink cannot turn this + // into a per-attempt retry loop. Analytics must never change the health + // outcome its caller is about to record. + try { + args.capture()?.({ + event: args.event, + surface: "api", + properties: input.properties, + dedupeKey: `${args.dedupePrefix}:${input.dedupeValue}`, + minimumIntervalMs: EPISODE_ANALYTICS_MINIMUM_INTERVAL_MS, + }); + } catch { + // Best effort only. + } + }, + end(): void { + emitted = false; + }, + }; +} diff --git a/apps/ade-cli/src/services/credentials/credentialStore.test.ts b/apps/ade-cli/src/services/credentials/credentialStore.test.ts index 187ce4498..c03643c5d 100644 --- a/apps/ade-cli/src/services/credentials/credentialStore.test.ts +++ b/apps/ade-cli/src/services/credentials/credentialStore.test.ts @@ -9,7 +9,16 @@ import { EncryptedFileCredentialStore, KeytarCredentialStore, createDefaultCredentialStore, + isFileBackedCredentialKey, } from "./credentialStore"; +import { ACCOUNT_SESSION_CREDENTIAL_KEY } from "../account/accountAuthService"; +import { BOOTSTRAP_TOKEN_KEY } from "../sync/brainProjectActionsSyncHandler"; +import { + createMacKeychainMaterialResolver, + resolveMacKeychainMaterialOutcome, + resolveOsBoundKeyMaterialBinding, + type MacKeychainCommands, +} from "./osBoundKeyMaterial"; import { readOrCreateWindowsDpapiMaterial, readOrCreateWindowsDpapiMaterialAsync, @@ -269,20 +278,20 @@ new EncryptedFileCredentialStore({ secretsDir }).setSync(key, value); const osMaterial = Buffer.from("test-os-material"); const store = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: () => osMaterial, + keyMaterial: { read: () => osMaterial }, }); store.setSync("linear.token.v1", "lin_secret"); const reloaded = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: () => osMaterial, + keyMaterial: { read: () => osMaterial }, }); expect(reloaded.getSync("linear.token.v1")).toBe("lin_secret"); const unbound = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: () => null, + keyMaterial: { read: () => null }, }); expect(unbound.getSync("linear.token.v1")).toBeNull(); }); @@ -290,7 +299,7 @@ new EncryptedFileCredentialStore({ secretsDir }).setSync(key, value); it("atomically binds legacy Windows ciphertext on the first asynchronous credential read", async () => { const legacyStore = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: () => null, + keyMaterial: { read: () => null }, }); legacyStore.setSync("account.session.v1", "legacy-async-windows-session"); const credentialsPath = path.join(tempDir, "credentials.json.enc"); @@ -299,16 +308,18 @@ new EncryptedFileCredentialStore({ secretsDir }).setSync(key, value); const upgraded = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: () => { - throw new Error("async migration must not use synchronous key access"); + keyMaterial: { + read: () => { + throw new Error("async migration must not use synchronous key access"); + }, + readAsync: async () => osMaterial, }, - keyMaterialProviderAsync: async () => osMaterial, }); await expect(upgraded.get("account.session.v1")).resolves.toBe("legacy-async-windows-session"); expect(fs.readFileSync(credentialsPath, "utf8")).not.toBe(legacyCiphertext); expect(new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: () => null, + keyMaterial: { read: () => null }, }).getSync("account.session.v1")).toBeNull(); }); @@ -316,7 +327,7 @@ new EncryptedFileCredentialStore({ secretsDir }).setSync(key, value); const osMaterial = Buffer.from("test-os-material"); new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: () => osMaterial, + keyMaterial: { read: () => osMaterial }, }).setSync("github.token.v1", "ghp_async_read"); const syncProvider = vi.fn(() => { throw new Error("synchronous keychain access must not run"); @@ -324,8 +335,7 @@ new EncryptedFileCredentialStore({ secretsDir }).setSync(key, value); const asyncProvider = vi.fn(async () => osMaterial); const reader = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: syncProvider, - keyMaterialProviderAsync: asyncProvider, + keyMaterial: { read: syncProvider, readAsync: asyncProvider }, }); await expect(reader.get("github.token.v1")).resolves.toBe("ghp_async_read"); @@ -342,8 +352,7 @@ new EncryptedFileCredentialStore({ secretsDir }).setSync(key, value); }); const reader = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: syncProvider, - keyMaterialProviderAsync: asyncProvider, + keyMaterial: { read: syncProvider, readAsync: asyncProvider }, }); await expect(reader.get("github.token.v1")).resolves.toBeNull(); @@ -358,7 +367,7 @@ new EncryptedFileCredentialStore({ secretsDir }).setSync(key, value); const asyncProvider = vi.fn(async () => Buffer.from("unused")); const reader = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProviderAsync: asyncProvider, + keyMaterial: { read: () => null, readAsync: asyncProvider }, }); await expect(reader.get("github.token.v1")).rejects.toThrow( @@ -371,7 +380,7 @@ new EncryptedFileCredentialStore({ secretsDir }).setSync(key, value); it("atomically binds legacy Windows ciphertext on the first synchronous credential read", () => { const legacy = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: () => null, + keyMaterial: { read: () => null }, }); legacy.setSync("agent.token", "legacy_secret"); const credentialPath = path.join(tempDir, "credentials.json.enc"); @@ -379,17 +388,81 @@ new EncryptedFileCredentialStore({ secretsDir }).setSync(key, value); const upgraded = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: () => Buffer.from("test-os-material"), + keyMaterial: { read: () => Buffer.from("test-os-material") }, }); expect(upgraded.getSync("agent.token")).toBe("legacy_secret"); expect(fs.readFileSync(credentialPath, "utf8")).not.toBe(legacyCiphertext); expect(legacy.getSync("agent.token")).toBeNull(); }); + it("re-reads key material once and self-heals a decrypt failure caused by stale cached material", () => { + const winner = Buffer.from("os-material-winner"); + new EncryptedFileCredentialStore({ + secretsDir: tempDir, + keyMaterial: { read: () => winner }, + }).setSync("account.session.v1", "session-json"); + + // This store cached the secret it minted before the peer's item won the + // keychain race, so its first decrypt attempt fails. + let material = Buffer.from("os-material-loser"); + const provider = vi.fn(() => material); + const invalidateKeyMaterial = vi.fn(() => { + material = winner; + }); + const store = new EncryptedFileCredentialStore({ + secretsDir: tempDir, + keyMaterial: { read: provider, invalidate: invalidateKeyMaterial }, + }); + + expect(store.getSync("account.session.v1")).toBe("session-json"); + expect(store.getLastReadState()).toBe("available"); + expect(store.getLastReadFailureReason()).toBeNull(); + expect(invalidateKeyMaterial).toHaveBeenCalledTimes(1); + expect(provider).toHaveBeenCalledTimes(2); + }); + + it("does not re-ask for key material on every read while it keeps failing", () => { + new EncryptedFileCredentialStore({ + secretsDir: tempDir, + keyMaterial: { read: () => Buffer.from("os-material-A") }, + }).setSync("agent.token", "secret"); + + let material = Buffer.from("os-material-B"); + const invalidateKeyMaterial = vi.fn(() => { + // Every re-read returns a different-but-still-wrong secret, so the retry + // is genuinely attempted and genuinely fails each time. + material = Buffer.from(`os-material-B-${invalidateKeyMaterial.mock.calls.length}`); + }); + const store = new EncryptedFileCredentialStore({ + secretsDir: tempDir, + keyMaterial: { read: () => material, invalidate: invalidateKeyMaterial }, + }); + + expect(() => store.getSync("agent.token")).toThrow(); + expect(() => store.getSync("agent.token")).toThrow(); + expect(() => store.getSync("agent.token")).toThrow(); + expect(invalidateKeyMaterial).toHaveBeenCalledTimes(1); + expect(store.getLastReadState()).toBe("unreadable"); + expect(store.getLastReadFailureReason()).toBe("decrypt_failure"); + }); + + it("reports why a read was unreadable", () => { + const credentialPath = path.join(tempDir, "credentials.json.enc"); + fs.writeFileSync(credentialPath, JSON.stringify({ not: "an envelope" }), "utf8"); + const store = new EncryptedFileCredentialStore({ + secretsDir: tempDir, + keyMaterial: { read: () => null }, + }); + + expect(store.getSync("agent.token")).toBeNull(); + expect(store.getLastReadState()).toBe("unreadable"); + expect(store.getLastReadFailureReason()).toBe("store_format"); + }); + it("fails safe instead of wiping ciphertext when OS-bound key material rotates", () => { const written = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: () => Buffer.from("os-material-A"), + keyMaterial: { read: () => Buffer.from("os-material-A") }, }); written.setSync("agent.token", "secret"); @@ -401,7 +474,7 @@ new EncryptedFileCredentialStore({ secretsDir }).setSync(key, value); // (preserving the ciphertext) instead of silently rewriting an empty store. const rotated = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: () => Buffer.from("os-material-B"), + keyMaterial: { read: () => Buffer.from("os-material-B") }, }); expect(() => rotated.getSync("agent.token")).toThrow(); expect(() => rotated.setSync("agent.token", "wiped")).toThrow(); @@ -410,7 +483,7 @@ new EncryptedFileCredentialStore({ secretsDir }).setSync(key, value); expect(fs.readFileSync(cipherPath, "utf8")).toBe(before); const recovered = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: () => Buffer.from("os-material-A"), + keyMaterial: { read: () => Buffer.from("os-material-A") }, }); expect(recovered.getSync("agent.token")).toBe("secret"); }); @@ -427,6 +500,17 @@ describe("ElectronSafeStorageCredentialStore", () => { }, }; + /** Writes credentials.json.enc as an Electron-only safeStorage file. */ + const writeSharedPathSafeStorageFile = (values: Record): void => { + fs.writeFileSync( + path.join(tempDir, "credentials.json.enc"), + Buffer.concat([ + Buffer.from("ADE_SAFE_STORAGE_CREDENTIALS_V1\n"), + safeStorage.encryptString(JSON.stringify(values)), + ]), + ); + }; + it("delegates encryption to the injected safeStorage implementation", async () => { const store = new ElectronSafeStorageCredentialStore({ secretsDir: tempDir, safeStorage }); @@ -450,6 +534,146 @@ describe("ElectronSafeStorageCredentialStore", () => { expect(fs.existsSync(path.join(tempDir, ".machine-key"))).toBe(false); }); + it("leaves the brain-readable account session in the legacy file store", () => { + // The ADE brain (com.ade.runtime) and the CLI cannot read the Electron-only + // safeStorage file. Migrating the account session into it and deleting the + // file store is what left a signed-in machine unpublishable. + const legacyStore = new EncryptedFileCredentialStore({ secretsDir: tempDir }); + legacyStore.setSync(ACCOUNT_SESSION_CREDENTIAL_KEY, "session-json"); + legacyStore.setSync("sync.bootstrapToken.v1", "bootstrap-token"); + legacyStore.setSync("linear.token.v1", "lin_secret"); + + const store = new ElectronSafeStorageCredentialStore({ secretsDir: tempDir, safeStorage }); + + expect(store.getSync("linear.token.v1")).toBe("lin_secret"); + expect(fs.existsSync(path.join(tempDir, "credentials.json.enc"))).toBe(true); + expect(fs.existsSync(path.join(tempDir, ".machine-key"))).toBe(true); + + // The excluded keys stay readable through the file store the brain uses... + const brainStore = new EncryptedFileCredentialStore({ secretsDir: tempDir }); + expect(brainStore.getSync(ACCOUNT_SESSION_CREDENTIAL_KEY)).toBe("session-json"); + expect(brainStore.getSync("sync.bootstrapToken.v1")).toBe("bootstrap-token"); + + // ...and never reach the Electron-only file. + const safeFile = fs.readFileSync(path.join(tempDir, "credentials.safe.enc"), "utf8"); + expect(safeFile).not.toContain("session-json"); + expect(safeFile).not.toContain("bootstrap-token"); + }); + + it("prunes migrated duplicates out of the retained legacy file store", () => { + // A retained file store used to keep a FULL copy of every migrated key. The + // app then rotates the token through safeStorage while the brain and the + // CLI keep serving the stale file copy, and revoked secrets stay at rest. + const legacyStore = new EncryptedFileCredentialStore({ secretsDir: tempDir }); + legacyStore.setSync(ACCOUNT_SESSION_CREDENTIAL_KEY, "session-json"); + legacyStore.setSync("linear.token.v1", "lin_secret"); + legacyStore.setSync("github.token.v1", "ghp_secret"); + + const store = new ElectronSafeStorageCredentialStore({ secretsDir: tempDir, safeStorage }); + + expect(store.getSync("linear.token.v1")).toBe("lin_secret"); + expect(store.getSync("github.token.v1")).toBe("ghp_secret"); + expect(store.getSync(ACCOUNT_SESSION_CREDENTIAL_KEY)).toBeNull(); + + // The legacy file keeps ONLY what it stays authoritative for. + const brainStore = new EncryptedFileCredentialStore({ secretsDir: tempDir }); + expect(brainStore.getSync(ACCOUNT_SESSION_CREDENTIAL_KEY)).toBe("session-json"); + expect(brainStore.getSync("linear.token.v1")).toBeNull(); + expect(brainStore.getSync("github.token.v1")).toBeNull(); + expect(fs.existsSync(path.join(tempDir, ".machine-key"))).toBe(true); + + // Both stores still serve their own keys after the prune. + brainStore.setSync(ACCOUNT_SESSION_CREDENTIAL_KEY, "session-json-2"); + expect(store.getSync("linear.token.v1")).toBe("lin_secret"); + expect(brainStore.getSync(ACCOUNT_SESSION_CREDENTIAL_KEY)).toBe("session-json-2"); + }); + + it("aborts the migration instead of destroying an unreadable legacy store", () => { + // The migration reads the legacy store WITHOUT allowing a rewrite, and that + // read returns {} rather than throwing when nothing can decrypt it. Acting + // on that empty view wrote an empty safeStorage file, saw zero retained + // keys, and unlinked credentials.json.enc AND .machine-key — every + // credential on the machine, gone. + const legacyPath = path.join(tempDir, "credentials.json.enc"); + const machineKeyPath = path.join(tempDir, ".machine-key"); + const safePath = path.join(tempDir, "credentials.safe.enc"); + new EncryptedFileCredentialStore({ + secretsDir: tempDir, + keyMaterial: { read: () => Buffer.from("os-material-A") }, + }).setSync("linear.token.v1", "lin_secret"); + const ciphertextBefore = fs.readFileSync(legacyPath, "utf8"); + const machineKeyBefore = fs.readFileSync(machineKeyPath, "utf8"); + + // This process cannot obtain the OS material the ciphertext was sealed with + // (locked/denied keychain), so it falls back to the bare machine key, which + // does not decrypt either — the exact shape that reads as an empty store. + const undecryptableLegacyStore = new EncryptedFileCredentialStore({ + secretsDir: tempDir, + keyMaterial: { read: () => null }, + }); + expect(undecryptableLegacyStore.readAllForMigration()).toEqual({}); + expect(undecryptableLegacyStore.getLastReadState()).toBe("unreadable"); + const store = new ElectronSafeStorageCredentialStore({ + secretsDir: tempDir, + safeStorage, + legacyStore: undecryptableLegacyStore, + }); + + expect(store.getSync("linear.token.v1")).toBeNull(); + + // Nothing written, nothing deleted: the credentials stay recoverable. + expect(fs.existsSync(safePath)).toBe(false); + expect(fs.readFileSync(legacyPath, "utf8")).toBe(ciphertextBefore); + expect(fs.readFileSync(machineKeyPath, "utf8")).toBe(machineKeyBefore); + const recovered = new EncryptedFileCredentialStore({ + secretsDir: tempDir, + keyMaterial: { read: () => Buffer.from("os-material-A") }, + }); + expect(recovered.getSync("linear.token.v1")).toBe("lin_secret"); + }); + + it("still moves and removes a legacy file that is already safeStorage-encrypted", () => { + // Nothing in an Electron-only file is brain-readable, so retaining it would + // only leave an undecryptable file behind for the brain to trip over. + // Written the way an older app version wrote it, before the file-backed + // keys were excluded from safeStorage — today's write path refuses this. + writeSharedPathSafeStorageFile({ [ACCOUNT_SESSION_CREDENTIAL_KEY]: "session-json" }); + + const dedicatedStore = new ElectronSafeStorageCredentialStore({ secretsDir: tempDir, safeStorage }); + + expect(dedicatedStore.getSync(ACCOUNT_SESSION_CREDENTIAL_KEY)).toBe("session-json"); + expect(fs.existsSync(path.join(tempDir, "credentials.json.enc"))).toBe(false); + }); + + it("refuses to write a file-backed credential into the Electron-only file", () => { + // The migration keeps these keys in credentials.json.enc because the brain + // and the CLI cannot read safeStorage. A writer that puts one into the + // Electron-only file signs the brain out of a signed-in machine, so the + // write path must fail loudly instead of succeeding invisibly. + const store = new ElectronSafeStorageCredentialStore({ secretsDir: tempDir, safeStorage }); + store.setSync("linear.token.v1", "lin_secret"); + + expect(() => store.setSync(ACCOUNT_SESSION_CREDENTIAL_KEY, "session-json")) + .toThrow(/file-backed/); + expect(() => store.updateSync((values) => { + values[BOOTSTRAP_TOKEN_KEY] = "bootstrap-token"; + })).toThrow(/file-backed/); + + const safeFile = fs.readFileSync(path.join(tempDir, "credentials.safe.enc"), "utf8"); + expect(safeFile).not.toContain("session-json"); + expect(safeFile).not.toContain("bootstrap-token"); + expect(store.getSync("linear.token.v1")).toBe("lin_secret"); + expect(store.getSync(ACCOUNT_SESSION_CREDENTIAL_KEY)).toBeNull(); + }); + + it("keeps the account-session key excluded from safeStorage migration", () => { + expect(isFileBackedCredentialKey(ACCOUNT_SESSION_CREDENTIAL_KEY)).toBe(true); + // Asserted against the real constant: renaming it in the sync handler must + // fail here instead of silently moving the token into safeStorage. + expect(isFileBackedCredentialKey(BOOTSTRAP_TOKEN_KEY)).toBe(true); + expect(isFileBackedCredentialKey("linear.token.v1")).toBe(false); + }); + it("migrates a shared-path safeStorage file to the dedicated safeStorage file", () => { const sharedPathStore = new ElectronSafeStorageCredentialStore({ secretsDir: tempDir, @@ -545,3 +769,207 @@ describe("createDefaultCredentialStore", () => { expect(fs.existsSync(path.join(tempDir, "credentials.json.enc"))).toBe(true); }); }); + +describe("resolveOsBoundKeyMaterialBinding", () => { + // The key-material contract is platform-sensitive, and the platform that is + // NOT this machine is the one that regresses silently. Drive the decision + // directly with injected platform/env instead of spawning `security` or + // `powershell.exe`: every entry point (read, readAsync, invalidate, + // expectsOsBoundKeyMaterial) dispatches on exactly this value. + it.each([ + { platform: "win32", expected: "windows_dpapi" }, + { platform: "darwin", expected: "macos_keychain" }, + { platform: "linux", expected: "none" }, + ] as const)("binds $platform to $expected", ({ platform, expected }) => { + expect(resolveOsBoundKeyMaterialBinding(platform, {})).toBe(expected); + }); + + it.each(["win32", "darwin"] as const)( + "applies the env opt-out on %s, not just on macOS", + (platform) => { + // A test process must never reach the real keychain OR the real DPAPI + // helper; the guard used to be spelled per-platform and drifted. + expect(resolveOsBoundKeyMaterialBinding(platform, { VITEST: "true" })).toBe("disabled"); + expect(resolveOsBoundKeyMaterialBinding(platform, { NODE_ENV: "test" })).toBe("disabled"); + expect(resolveOsBoundKeyMaterialBinding(platform, { + ADE_CREDENTIAL_STORE_DISABLE_OS_BINDING: "1", + })).toBe("disabled"); + }, + ); + + it.each(["win32", "darwin", "linux"] as const)( + "lets an explicit passphrase override the OS binding on %s", + (platform) => { + expect(resolveOsBoundKeyMaterialBinding(platform, { + ADE_CREDENTIAL_STORE_PASSPHRASE: "shared-secret", + ADE_CREDENTIAL_STORE_DISABLE_OS_BINDING: "1", + })).toBe("env_passphrase"); + }, + ); +}); + +describe("resolveMacKeychainMaterialOutcome", () => { + const secretFor = (value: string) => Buffer.alloc(32, value[0]).toString("base64"); + + it("uses the existing keychain item without writing", () => { + const existing = secretFor("existing"); + const commands: MacKeychainCommands = { + find: vi.fn(() => ({ kind: "found" as const, value: existing })), + add: vi.fn(() => "created" as const), + }; + + expect(resolveMacKeychainMaterialOutcome(commands).material?.toString("base64")).toBe(existing); + expect(commands.add).not.toHaveBeenCalled(); + }); + + it("adopts the winner when the item appears between find and add", () => { + // Two first-run processes race. This one sees "not found", loses the add, + // and MUST adopt the peer's secret instead of overwriting it. + const winner = secretFor("winner"); + const find = vi.fn() + .mockReturnValueOnce({ kind: "not_found" as const }) + .mockReturnValueOnce({ kind: "found" as const, value: winner }); + const add = vi.fn(() => "exists" as const); + + const material = resolveMacKeychainMaterialOutcome({ find, add }).material; + + expect(material?.toString("base64")).toBe(winner); + expect(find).toHaveBeenCalledTimes(2); + expect(add).toHaveBeenCalledTimes(1); + }); + + it("converges on one secret when two stores race for a fresh keychain", () => { + // Shared keychain: `add` only succeeds for whoever gets there first. + let item: string | null = null; + const commandsFor = (): MacKeychainCommands => ({ + find: () => (item == null ? { kind: "not_found" } : { kind: "found", value: item }), + add: (secret) => { + if (item != null) return "exists"; + item = secret; + return "created"; + }, + }); + + const first = resolveMacKeychainMaterialOutcome(commandsFor()).material; + const second = resolveMacKeychainMaterialOutcome(commandsFor()).material; + + expect(first).not.toBeNull(); + expect(second?.toString("base64")).toBe(first?.toString("base64")); + }); + + it("fails closed instead of creating a replacement when the keychain errors", () => { + // Timeouts and locked keychains are NOT "the item is missing": minting a + // replacement here is what clobbers the peer process's secret. + const add = vi.fn(() => "created" as const); + + expect(resolveMacKeychainMaterialOutcome({ find: () => ({ kind: "error" }), add }).material).toBeNull(); + expect(add).not.toHaveBeenCalled(); + }); + + it("returns null when the add fails and the item still cannot be read", () => { + const find = vi.fn(() => ({ kind: "not_found" as const })); + const add = vi.fn(() => "error" as const); + + expect(resolveMacKeychainMaterialOutcome({ find, add }).material).toBeNull(); + expect(find).toHaveBeenCalledTimes(2); + }); +}); + +describe("createMacKeychainMaterialResolver", () => { + const material = Buffer.alloc(32, 7); + + it("caches the resolved material instead of re-asking the OS", async () => { + const read = vi.fn(() => ({ material })); + const resolver = createMacKeychainMaterialResolver({ + read, + readAsync: async () => ({ material: null, reason: "unavailable" as const }), + }); + + expect(resolver.read()).toBe(material); + expect(resolver.read()).toBe(material); + expect(await resolver.readAsync()).toBe(material); + expect(read).toHaveBeenCalledTimes(1); + }); + + it("still creates the keychain item after repeated read-only not_found misses", async () => { + // The read-only path refreshes the miss timestamp on every failed read, and + // the creating path is the only one that can mint the item. If a not_found + // miss armed the creation backoff, first-run creation would be starved + // forever on any machine whose brain polls the session faster than 30s. + let now = 1_700_000_000_000; + const read = vi.fn(() => ({ material })); + const readAsync = vi.fn(async () => ({ material: null, reason: "not_found" as const })); + const resolver = createMacKeychainMaterialResolver({ + read, + readAsync, + now: () => now, + negativeCacheMs: 30_000, + }); + + expect(await resolver.readAsync()).toBeNull(); + now += 1_000; + expect(await resolver.readAsync()).toBeNull(); + now += 1_000; + expect(await resolver.readAsync()).toBeNull(); + + expect(resolver.read()).toBe(material); + expect(read).toHaveBeenCalledTimes(1); + }); + + it("suppresses creation while the keychain itself is unavailable", () => { + let now = 1_700_000_000_000; + const read = vi.fn(() => ({ material: null, reason: "unavailable" as const })); + const resolver = createMacKeychainMaterialResolver({ + read, + readAsync: async () => ({ material: null, reason: "unavailable" as const }), + now: () => now, + negativeCacheMs: 30_000, + }); + + expect(resolver.read()).toBeNull(); + now += 10_000; + expect(resolver.read()).toBeNull(); + expect(read).toHaveBeenCalledTimes(1); + + // Once the window elapses the wedged keychain is worth one more attempt. + now += 31_000; + expect(resolver.read()).toBeNull(); + expect(read).toHaveBeenCalledTimes(2); + }); + + it("re-reads the OS after an invalidation even inside the backoff window", () => { + let now = 1_700_000_000_000; + let current: Buffer | null = null; + const read = vi.fn(() => (current + ? { material: current } + : { material: null, reason: "unavailable" as const })); + const resolver = createMacKeychainMaterialResolver({ + read, + readAsync: async () => ({ material: null, reason: "unavailable" as const }), + now: () => now, + negativeCacheMs: 30_000, + }); + + expect(resolver.read()).toBeNull(); + current = material; + now += 1_000; + expect(resolver.read()).toBeNull(); + + resolver.invalidate(); + expect(resolver.read()).toBe(material); + }); + + it("coalesces concurrent read-only resolutions into one OS call", async () => { + const readAsync = vi.fn(async () => ({ material })); + const resolver = createMacKeychainMaterialResolver({ + read: () => ({ material: null, reason: "unavailable" as const }), + readAsync, + }); + + const [first, second] = await Promise.all([resolver.readAsync(), resolver.readAsync()]); + + expect(first).toBe(material); + expect(second).toBe(material); + expect(readAsync).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/ade-cli/src/services/credentials/credentialStore.ts b/apps/ade-cli/src/services/credentials/credentialStore.ts index 73329399d..cbc270694 100644 --- a/apps/ade-cli/src/services/credentials/credentialStore.ts +++ b/apps/ade-cli/src/services/credentials/credentialStore.ts @@ -1,12 +1,13 @@ import crypto from "node:crypto"; -import { execFile, execFileSync, spawnSync } from "node:child_process"; import fs from "node:fs"; import path from "node:path"; import { resolveMachineAdeLayout } from "../projects/machineLayout"; import { - readOrCreateWindowsDpapiMaterial, - readOrCreateWindowsDpapiMaterialAsync, -} from "./windowsDpapiMaterial"; + expectsOsBoundKeyMaterial, + invalidateDefaultOsBoundKeyMaterialCache, + readDefaultOsBoundKeyMaterial, + readDefaultOsBoundKeyMaterialAsync, +} from "./osBoundKeyMaterial"; export interface CredentialStore { get(key: string): Promise; @@ -16,6 +17,24 @@ export interface CredentialStore { export type CredentialStoreReadState = "available" | "missing" | "unreadable"; +/** + * Why the last synchronous read could not produce values. Coarse by design: it + * is surfaced to product analytics so field incidence of the "brain cannot read + * the credential file the app just wrote" class becomes measurable. + */ +export type CredentialStoreReadFailureReason = + /** The ciphertext exists but no available key decrypts it. */ + | "decrypt_failure" + /** + * OS-held key material was expected but the key was derived without it. On + * darwin that is an unreadable keychain item; on win32 a DPAPI failure throws + * out of the read instead of returning null, so it never lands here. The name + * stays platform-neutral because the condition it describes is. + */ + | "no_os_key_material" + /** The file exists but is not a recognised credential envelope. */ + | "store_format"; + export type SyncCredentialStore = CredentialStore & { getSync(key: string): string | null; setSync(key: string, value: string): void; @@ -26,6 +45,8 @@ export type SyncCredentialStore = CredentialStore & { onDidChange?(listener: () => void): () => void; /** Result of the most recent synchronous credential-file read. */ getLastReadState?(): CredentialStoreReadState; + /** Why the most recent read was unreadable, or null when it was not. */ + getLastReadFailureReason?(): CredentialStoreReadFailureReason | null; }; type StoredCredentialEnvelope = { @@ -42,8 +63,27 @@ type SafeStorageLike = { decryptString(value: Buffer): string; }; +/** + * Every member is REQUIRED. An optional `getLastReadState()` would let a source + * that lacks it silently skip the unreadable-store check below and re-open the + * destroy-on-unreadable path; an optional `pruneForMigration()` would silently + * leave migrated duplicates at rest. + */ type CredentialStoreMigrationSource = { readAllForMigration(): Record; + /** + * Result of the read `readAllForMigration()` just performed. It returns `{}` + * rather than throwing for an undecryptable store, so without this the + * migration cannot tell "empty" from "unreadable" — and migrating an + * unreadable store deletes it. + */ + getLastReadState(): CredentialStoreReadState; + /** + * Rewrites the legacy file to exactly `values` WITHOUT acquiring the store's + * lock: the migration already holds that same lock file, and the file lock is + * not reentrant. + */ + pruneForMigration(values: Record): void; }; const DEFAULT_CREDENTIALS_FILE = "credentials.json.enc"; @@ -51,22 +91,41 @@ const DEFAULT_SAFE_STORAGE_CREDENTIALS_FILE = "credentials.safe.enc"; const DEFAULT_MACHINE_KEY_FILE = ".machine-key"; const STORE_AAD = Buffer.from("ade.credentials.v1"); const OS_BOUND_KEY_INFO = Buffer.from("ade.credentials.file-store.v2"); -const MACOS_KEYCHAIN_SERVICE = "com.ade.runtime.credentials.file-store-key.v1"; -const MACOS_KEYCHAIN_ACCOUNT = "machine"; const SAFE_STORAGE_FILE_MAGIC = Buffer.from("ADE_SAFE_STORAGE_CREDENTIALS_V1\n"); +/** + * Credentials that MUST stay in the shared `credentials.json.enc` file store. + * + * The Electron-only safeStorage file is unreadable by the ADE brain + * (com.ade.runtime) and by the `ade` CLI, so migrating these keys into it — and + * then deleting the file store — leaves the brain signed out on a machine whose + * app is signed in. Keep the literals in sync with: + * - ACCOUNT_SESSION_CREDENTIAL_KEY (services/account/accountAuthService.ts) + * - BOOTSTRAP_TOKEN_KEY (services/sync/brainProjectActionsSyncHandler.ts) + * They are duplicated here rather than imported to keep this module free of + * service-layer dependencies; credentialStore.test.ts asserts they match. + */ +const FILE_BACKED_CREDENTIAL_KEYS: readonly string[] = [ + "account.session.v1", + "sync.bootstrapToken.v1", +]; + +export function isFileBackedCredentialKey(key: string): boolean { + return FILE_BACKED_CREDENTIAL_KEYS.includes(key); +} + +function fileBackedCredentialWriteError(key: string): Error { + return new Error( + `${key} is file-backed; write it through the file credential store ` + + "(credentials.json.enc), not the Electron-only safeStorage file the ADE " + + "brain cannot read.", + ); +} const LOCK_TIMEOUT_MS = 15_000; const LOCK_STALE_MS = 10_000; const LOCK_RETRY_MS = 25; const CREDENTIAL_CHANGE_POLL_INTERVAL_MS = 250; -const MACOS_KEYCHAIN_READ_TIMEOUT_MS = 2_000; -const MACOS_KEYCHAIN_NEGATIVE_CACHE_MS = 30_000; -let cachedDefaultOsBoundKeyMaterial: Buffer | null = null; -// Keyed by resolved secrets directory: DPAPI material is protected per -// directory, so unlike the single macOS keychain item these cannot share a slot. -const windowsDpapiMaterialCache = new Map(); -const windowsDpapiReadInFlight = new Map>(); -let defaultOsBoundKeyMaterialReadInFlight: Promise | null = null; -let lastMissingDefaultOsBoundKeyMaterialAt = 0; +/** Bounds OS key-material re-reads when a store keeps failing to decrypt. */ +const KEY_MATERIAL_SELF_HEAL_INTERVAL_MS = 30_000; type CredentialLockMetadata = { pid?: number; @@ -528,211 +587,156 @@ async function readOrCreateMachineKeyAsync(machineKeyPath: string): Promise= 32 ? decoded : Buffer.from(raw, "utf8"); - } catch { - // Missing item or locked keychain; try to create once below. - } - - const secret = crypto.randomBytes(32).toString("base64"); - try { - const result = spawnSync("security", [ - "add-generic-password", - "-a", - MACOS_KEYCHAIN_ACCOUNT, - "-s", - MACOS_KEYCHAIN_SERVICE, - "-U", - "-w", - ], { - input: `${secret}\n`, - stdio: ["pipe", "ignore", "ignore"], - timeout: MACOS_KEYCHAIN_READ_TIMEOUT_MS, - }); - if (result.status !== 0) return null; - return Buffer.from(secret, "base64"); - } catch { - return null; - } +function isSameKeyMaterial(left: Buffer | null, right: Buffer | null): boolean { + if (!left || !right) return !left && !right; + return left.equals(right); } -async function readMacKeychainMaterialAsync(): Promise { - if (process.platform !== "darwin") return null; - return new Promise((resolve) => { - execFile( - "security", - [ - "find-generic-password", - "-a", - MACOS_KEYCHAIN_ACCOUNT, - "-s", - MACOS_KEYCHAIN_SERVICE, - "-w", - ], - { - encoding: "utf8", - timeout: MACOS_KEYCHAIN_READ_TIMEOUT_MS, - maxBuffer: 64 * 1024, - }, - (error, stdout) => { - if (error) { - resolve(null); - return; - } - const raw = stdout.trim(); - const decoded = Buffer.from(raw, "base64"); - resolve(decoded.length >= 32 ? decoded : Buffer.from(raw, "utf8")); - }, - ); - }); -} +/** + * How a credential store obtains — and gives up on — OS-bound key material. + * + * The three members are correlated: `invalidate()` must drop whatever cache + * backs `read()`/`readAsync()`, or the self-heal retry re-reads the same stale + * material. Injecting the trio together makes that contract explicit; omitting + * the whole object takes the process-wide OS defaults. + * + * Every member receives `keyBindingDir` — the directory this store's machine key + * lives in, which is where the rest of its key derivation belongs too. macOS + * ignores it (one global keychain item per machine) but Windows DPAPI material + * is protected per directory, so without it a store with a custom + * `machineKeyPath` would be handed a different store's key. Sources that do not + * care may ignore the argument. + */ +export type CredentialKeyMaterialSource = { + read(keyBindingDir: string): Buffer | null; + /** Defaults to an asynchronous wrapper around `read()`. */ + readAsync?(keyBindingDir: string): Promise; + /** + * Drops whatever cache backs the readers so a failed decrypt can be retried + * against freshly-read material. Omit to opt out of self-heal entirely. + */ + invalidate?(keyBindingDir: string): void; +}; -function readDefaultOsBoundKeyMaterial(secretsDir: string): Buffer | null { - const envMaterial = readCredentialPassphraseFromEnv(); - if (envMaterial) return envMaterial; - if (process.env.ADE_CREDENTIAL_STORE_DISABLE_OS_BINDING === "1") return null; - if (process.env.VITEST === "true" || process.env.NODE_ENV === "test") return null; - if (process.platform === "win32") { - // Windows re-spawned `powershell.exe` on every credential read, where macOS - // spawns `security` once and caches. That is a far worse trade than it - // looks: PowerShell 5.1 pays CLR load, System.Security from disk, and - // Defender's on-access scan each time. - // - // The cache must be keyed by directory, unlike macOS. Keychain material is - // one global item, but DPAPI material is protected per secrets directory - // (`/.credential-key.dpapi`), so a single shared slot would - // hand one store another store's key. - const key = path.resolve(secretsDir); - const cached = windowsDpapiMaterialCache.get(key); - if (cached) return cached; - const material = readOrCreateWindowsDpapiMaterial(secretsDir); - if (material) windowsDpapiMaterialCache.set(key, material); - return material; - } - if (cachedDefaultOsBoundKeyMaterial) return cachedDefaultOsBoundKeyMaterial; - const material = readOrCreateMacKeychainMaterial(); - if (material) { - cachedDefaultOsBoundKeyMaterial = material; - lastMissingDefaultOsBoundKeyMaterialAt = 0; - } - return material; -} +const DEFAULT_KEY_MATERIAL_SOURCE: Required = { + read: readDefaultOsBoundKeyMaterial, + readAsync: readDefaultOsBoundKeyMaterialAsync, + invalidate: invalidateDefaultOsBoundKeyMaterialCache, +}; -async function readDefaultOsBoundKeyMaterialAsync(secretsDir: string): Promise { - const envMaterial = readCredentialPassphraseFromEnv(); - if (envMaterial) return envMaterial; - if (process.env.ADE_CREDENTIAL_STORE_DISABLE_OS_BINDING === "1") return null; - if (process.env.VITEST === "true" || process.env.NODE_ENV === "test") return null; - if (process.platform === "win32") { - const key = path.resolve(secretsDir); - const cached = windowsDpapiMaterialCache.get(key); - if (cached) return cached; - // In-flight dedup matters more here than it ever did on macOS: without it, - // concurrent credential reads each spawn their own PowerShell, and that - // contention is what makes a cold start slow enough to hit the timeout. - // No negative cache -- a locked keychain is a durable state worth backing - // off from, but a DPAPI failure is usually a transient timeout, and - // suppressing retries would make one slow cold start look permanent. - const pending = windowsDpapiReadInFlight.get(key); - if (pending) return await pending; - const inFlight = readOrCreateWindowsDpapiMaterialAsync(secretsDir).then((material) => { - if (material) windowsDpapiMaterialCache.set(key, material); - return material; - }); - windowsDpapiReadInFlight.set(key, inFlight); - try { - return await inFlight; - } finally { - if (windowsDpapiReadInFlight.get(key) === inFlight) { - windowsDpapiReadInFlight.delete(key); - } - } - } - if (cachedDefaultOsBoundKeyMaterial) return cachedDefaultOsBoundKeyMaterial; - if ( - lastMissingDefaultOsBoundKeyMaterialAt > 0 - && Date.now() - lastMissingDefaultOsBoundKeyMaterialAt < MACOS_KEYCHAIN_NEGATIVE_CACHE_MS - ) { - return null; +type CredentialDecodeAttempt = + | { + ok: true; + values: Record; + /** The key the store SHOULD be sealed with, whichever one actually read it. */ + key: Buffer; + rewriteWithCurrentKey: boolean; } - if (defaultOsBoundKeyMaterialReadInFlight) { - return await defaultOsBoundKeyMaterialReadInFlight; - } - const read = readMacKeychainMaterialAsync().then((material) => { - if (material) { - cachedDefaultOsBoundKeyMaterial = material; - lastMissingDefaultOsBoundKeyMaterialAt = 0; - } else { - lastMissingDefaultOsBoundKeyMaterialAt = Date.now(); - } - return material; - }); - defaultOsBoundKeyMaterialReadInFlight = read; - try { - return await read; - } finally { - if (defaultOsBoundKeyMaterialReadInFlight === read) { - defaultOsBoundKeyMaterialReadInFlight = null; + | { ok: false; error: unknown; osBound: boolean; reason: CredentialStoreReadFailureReason }; + +/** + * One decrypt attempt for a given piece of OS key material. + * + * The os-bound key is tried first; a failure there falls back to the bare + * machine key so genuine legacy ciphertext can be rewritten. If that fallback + * also fails the ciphertext is left untouched — a rotated/foreign key must + * never cause an empty store to be written over real credentials. + */ +function decodeCredentialStore( + raw: Record | null, + machineKey: Buffer, + material: Buffer | null, +): CredentialDecodeAttempt { + const formatIsUnsupported = raw != null + && Object.keys(raw).length > 0 + && !isStoredCredentialEnvelope(raw); + const key = deriveOsBoundCredentialKey(machineKey, material); + const osBound = !key.equals(machineKey); + // The os-bound key first, then the bare machine key. Anything decrypted by a + // later candidate is genuine legacy ciphertext and may be rewritten. + const candidates = osBound ? [key, machineKey] : [machineKey]; + let lastError: unknown; + for (const [index, candidate] of candidates.entries()) { + try { + return { + ok: true, + values: deserializeStore(raw, candidate, { emptyOnDecryptFailure: false }), + key, + rewriteWithCurrentKey: index > 0, + }; + } catch (error) { + lastError = error; } } + const reason: CredentialStoreReadFailureReason = formatIsUnsupported + ? "store_format" + : !osBound && expectsOsBoundKeyMaterial() + ? "no_os_key_material" + : "decrypt_failure"; + return { ok: false, error: lastError, osBound, reason }; } -function deriveOsBoundCredentialKey(machineKey: Buffer, osMaterial: Buffer | null): Buffer { - if (!osMaterial || osMaterial.length === 0) return machineKey; - return Buffer.from(crypto.hkdfSync("sha256", osMaterial, machineKey, OS_BOUND_KEY_INFO, 32)); +/** + * The one retry a failed decrypt gets, against freshly-read OS key material. + * + * Returns null when there is nothing to gain — the OS handed back the same + * material that just failed — or when the retry failed too, so the caller keeps + * the original (already fail-closed) attempt. + */ +function retryDecodeWithRefreshedKeyMaterial(args: { + raw: Record | null; + machineKey: Buffer; + previous: Buffer | null; + refreshed: Buffer | null; +}): CredentialDecodeAttempt | null { + if (isSameKeyMaterial(args.refreshed, args.previous)) return null; + const retried = decodeCredentialStore(args.raw, args.machineKey, args.refreshed); + return retried.ok ? retried : null; } export class EncryptedFileCredentialStore implements SyncCredentialStore { private readonly credentialsPath: string; private readonly machineKeyPath: string; private readonly lockPath: string; - private readonly keyMaterialProvider: () => Buffer | null; - private readonly keyMaterialProviderAsync: () => Promise; + private readonly readKeyMaterial: () => Buffer | null; + private readonly readKeyMaterialAsync: () => Promise; private readonly credentialChangePollIntervalMs: number | null; private readonly credentialFileWatchers = new Set(); + private readonly invalidateKeyMaterial: (() => void) | null; private lastReadState: CredentialStoreReadState = "missing"; + private lastReadFailureReason: CredentialStoreReadFailureReason | null = null; + private lastKeyMaterialSelfHealAt = 0; constructor(args: { secretsDir?: string; credentialsPath?: string; machineKeyPath?: string; lockPath?: string; - keyMaterialProvider?: () => Buffer | null; - keyMaterialProviderAsync?: () => Promise; + /** Omit to use the process-wide OS-bound keychain material. */ + keyMaterial?: CredentialKeyMaterialSource; /** Set to null when tests drive checkForChangesNow() explicitly. */ credentialChangePollIntervalMs?: number | null; } = {}) { const secretsDir = args.secretsDir ?? resolveMachineAdeLayout().secretsDir; this.credentialsPath = args.credentialsPath ?? path.join(secretsDir, DEFAULT_CREDENTIALS_FILE); this.machineKeyPath = args.machineKeyPath ?? path.join(secretsDir, DEFAULT_MACHINE_KEY_FILE); - const osBindingDir = path.dirname(this.machineKeyPath); + const keyBindingDir = path.dirname(this.machineKeyPath); this.lockPath = args.lockPath ?? defaultLockPath(this.credentialsPath); - this.keyMaterialProvider = args.keyMaterialProvider - ?? (() => readDefaultOsBoundKeyMaterial(osBindingDir)); - this.keyMaterialProviderAsync = args.keyMaterialProviderAsync - ?? (args.keyMaterialProvider - ? async () => args.keyMaterialProvider?.() ?? null - : () => readDefaultOsBoundKeyMaterialAsync(osBindingDir)); + const keyMaterial = args.keyMaterial ?? DEFAULT_KEY_MATERIAL_SOURCE; + this.readKeyMaterial = () => keyMaterial.read(keyBindingDir); + this.readKeyMaterialAsync = async () => ( + keyMaterial.readAsync ? keyMaterial.readAsync(keyBindingDir) : keyMaterial.read(keyBindingDir) + ); + // An injected source owns its own cache lifetime, so self-heal is available + // only when that source supplies the matching invalidation hook. + this.invalidateKeyMaterial = keyMaterial.invalidate + ? () => keyMaterial.invalidate?.(keyBindingDir) + : null; this.credentialChangePollIntervalMs = args.credentialChangePollIntervalMs === undefined ? CREDENTIAL_CHANGE_POLL_INTERVAL_MS : args.credentialChangePollIntervalMs; @@ -759,6 +763,8 @@ export class EncryptedFileCredentialStore implements SyncCredentialStore { getSync(key: string): string | null { const normalized = normalizeKey(key); + // Locked because the read may bind legacy ciphertext to the OS-bound key, + // and that rewrite has to exclude concurrent writers. return this.withLock( () => this.readAll({ allowRewrite: false, migrateLegacy: true })[normalized] ?? null, ); @@ -768,6 +774,11 @@ export class EncryptedFileCredentialStore implements SyncCredentialStore { return this.lastReadState; } + getLastReadFailureReason(): CredentialStoreReadFailureReason | null { + // Every non-"unreadable" outcome clears this, so no state check is needed. + return this.lastReadFailureReason; + } + setSync(key: string, value: string): void { const normalized = normalizeKey(key); const nextValue = value.trim(); @@ -828,99 +839,146 @@ export class EncryptedFileCredentialStore implements SyncCredentialStore { return this.readAll({ allowRewrite: false }); } - private readAll(args: { allowRewrite: boolean; migrateLegacy?: boolean }): Record { + /** + * Rewrites this file store to exactly `values` for a migration that already + * holds this store's lock file. The lock is a `wx` create, so re-acquiring it + * from inside the migration would deadlock until the lock timeout; the caller + * owns mutual exclusion here, exactly like the migration's direct unlinks. + */ + pruneForMigration(values: Record): void { + this.writeAll(values); + } + + /** + * `migrateLegacy` binds pre-OS-bound ciphertext to the current key on a plain + * READ, not just on a write. Only callers that already hold this store's lock + * may pass it — the rewrite it performs is not itself locked. + */ + private readAll( + args: { allowRewrite: boolean; migrateLegacy?: boolean }, + ): Record { const credentialsExist = fs.existsSync(this.credentialsPath); const raw = readJsonObject(this.credentialsPath); const machineKey = readOrCreateMachineKey(this.machineKeyPath); - const key = deriveOsBoundCredentialKey(machineKey, this.keyMaterialProvider()); - if (!key.equals(machineKey)) { - try { - const values = deserializeStore(raw, key, { emptyOnDecryptFailure: false }); - this.lastReadState = credentialsExist ? "available" : "missing"; - return values; - } catch { - // Only the genuine legacy machine-key ciphertext should trigger a rewrite. - // If the legacy decrypt ALSO fails (true key rotation/corruption), propagate - // the error so the ciphertext is preserved instead of being overwritten with - // an empty store. - let values: Record; - try { - values = deserializeStore(raw, machineKey, { emptyOnDecryptFailure: false }); - } catch (error) { - this.lastReadState = "unreadable"; - throw error; - } - this.lastReadState = credentialsExist ? "available" : "missing"; - if (args.allowRewrite || args.migrateLegacy) { - this.writeAllWithKey(values, key); - } - return values; - } + const material = this.readKeyMaterial(); + let attempt = decodeCredentialStore(raw, machineKey, material); + if ( + !attempt.ok + && attempt.reason !== "store_format" + && this.beginKeyMaterialSelfHeal() + ) { + // A decrypt failure with CACHED key material is often recoverable: the + // peer process may have won the keychain create race after this process + // cached its own copy. Re-read the keychain once and retry before + // declaring the store permanently unreadable. A malformed file is not a + // key problem, so it never spends a keychain read. + // + // Known cost, accepted: on Windows an uncached key-material read is a + // synchronous PowerShell spawn budgeted at 30 s, and one that has to + // create the key spawns twice (protect, then unprotect). This path can + // take up to two uncached reads — the failing one above plus the + // refreshed one below — while holding the credential file lock, whose + // peer timeout is 15 s, so a peer process can see "Timed out waiting for + // ADE credential store lock" during a recovery. The common case is + // cheaper: the failing read is usually served from cache, which is the + // premise of the self-heal. Accepted because the alternative is no + // self-heal at all; raising the lock timeout or moving key-material reads + // outside the lock is a separate change. + const retried = retryDecodeWithRefreshedKeyMaterial({ + raw, + machineKey, + previous: material, + refreshed: this.readKeyMaterial(), + }); + if (retried) attempt = retried; } - try { - const values = deserializeStore(raw, machineKey, { emptyOnDecryptFailure: false }); - this.lastReadState = credentialsExist ? "available" : "missing"; - return values; - } catch (error) { + if (!attempt.ok) { // Preserve the historical fail-closed empty read while exposing why the // account record could not be obtained to publisher health. this.lastReadState = "unreadable"; - if (args.allowRewrite) throw error; + this.lastReadFailureReason = attempt.reason; + if (attempt.osBound || args.allowRewrite) throw attempt.error; return {}; } + this.lastReadState = credentialsExist ? "available" : "missing"; + this.lastReadFailureReason = null; + if (attempt.rewriteWithCurrentKey && (args.allowRewrite || args.migrateLegacy)) { + try { + // Seal with the key the attempt already derived, not a fresh material + // read: after a self-heal the freshly-read material is what decrypted + // this store, and re-asking the OS could disagree with it. + this.writeAllWithKey(attempt.values, attempt.key); + } catch { + // Preserve read compatibility if migration cannot rewrite right now. + } + } + return attempt.values; + } + + /** + * Bounds how often a failing store may re-ask the OS for key material, and + * drops the cache behind the readers so the next read is a fresh one. + */ + private beginKeyMaterialSelfHeal(): boolean { + if (!this.invalidateKeyMaterial) return false; + const now = Date.now(); + if ( + this.lastKeyMaterialSelfHealAt > 0 + && now - this.lastKeyMaterialSelfHealAt < KEY_MATERIAL_SELF_HEAL_INTERVAL_MS + ) { + return false; + } + this.lastKeyMaterialSelfHealAt = now; + this.invalidateKeyMaterial(); + return true; } private async readAllAsync(): Promise> { const { value: raw, exists: credentialsExist } = await readJsonObjectAsync(this.credentialsPath); if (!credentialsExist) { this.lastReadState = "missing"; + this.lastReadFailureReason = null; return {}; } if (!raw || Object.keys(raw).length === 0) { this.lastReadState = "unreadable"; + this.lastReadFailureReason = "store_format"; throw new Error("Unsupported ADE credential store format."); } const machineKey = await readOrCreateMachineKeyAsync(this.machineKeyPath); - const osMaterial = await this.keyMaterialProviderAsync(); - const key = deriveOsBoundCredentialKey(machineKey, osMaterial); - if (!key.equals(machineKey)) { - try { - const values = deserializeStore(raw, key, { emptyOnDecryptFailure: false }); - this.lastReadState = "available"; - return values; - } catch { - try { - deserializeStore(raw, machineKey, { emptyOnDecryptFailure: false }); - } catch (error) { - this.lastReadState = "unreadable"; - throw error; - } - try { - if (!osMaterial || osMaterial.length === 0) { - throw new Error("OS-bound credential material is unavailable during migration."); - } - const values = this.withLock(() => this.migrateLegacyUnderLock(osMaterial)); - this.lastReadState = "available"; - return values; - } catch (error) { - this.lastReadState = "unreadable"; - throw error; - } - } + const material = await this.readKeyMaterialAsync(); + let attempt = decodeCredentialStore(raw, machineKey, material); + if ( + !attempt.ok + && attempt.reason !== "store_format" + && this.beginKeyMaterialSelfHeal() + ) { + const retried = retryDecodeWithRefreshedKeyMaterial({ + raw, + machineKey, + previous: material, + refreshed: await this.readKeyMaterialAsync(), + }); + if (retried) attempt = retried; } - try { - const values = deserializeStore(raw, machineKey, { emptyOnDecryptFailure: false }); - this.lastReadState = "available"; - return values; - } catch { + if (!attempt.ok) { this.lastReadState = "unreadable"; + this.lastReadFailureReason = attempt.reason; + if (attempt.osBound) throw attempt.error; return {}; } + this.lastReadState = "available"; + this.lastReadFailureReason = null; + // The asynchronous path binds legacy ciphertext too. It is the brain's read + // path, and on a machine whose only reader is the brain the store would + // otherwise stay machine-key-sealed forever. + if (attempt.rewriteWithCurrentKey) this.bindLegacyCiphertextUnderLock(attempt.key); + return attempt.values; } private writeAll(values: Record): void { const machineKey = readOrCreateMachineKey(this.machineKeyPath); - const key = deriveOsBoundCredentialKey(machineKey, this.keyMaterialProvider()); + const key = deriveOsBoundCredentialKey(machineKey, this.readKeyMaterial()); this.writeAllWithKey(values, key); } @@ -928,16 +986,34 @@ export class EncryptedFileCredentialStore implements SyncCredentialStore { writeFileAtomic(this.credentialsPath, `${JSON.stringify(serializeStore(values, key), null, 2)}\n`); } - private migrateLegacyUnderLock(osMaterial: Buffer): Record { - const raw = readJsonObject(this.credentialsPath); - const machineKey = readOrCreateMachineKey(this.machineKeyPath); - const key = deriveOsBoundCredentialKey(machineKey, osMaterial); + /** + * Re-seals machine-key ciphertext with the OS-bound key, under this store's + * lock, for a caller that does NOT already hold it. + * + * The re-read inside the lock is the point: a peer may have bound the file + * while this reader waited, and re-deriving from the stale `raw` would undo + * whatever the peer wrote. `key` is passed in rather than re-derived so the + * asynchronous caller never touches the synchronous key-material reader. + * + * Best effort: a failure here only leaves the ciphertext legacy, which still + * reads, so it must never fail the read that triggered it. + */ + private bindLegacyCiphertextUnderLock(key: Buffer): void { try { - return deserializeStore(raw, key, { emptyOnDecryptFailure: false }); + this.withLock(() => { + const raw = readJsonObject(this.credentialsPath); + const machineKey = readOrCreateMachineKey(this.machineKeyPath); + try { + deserializeStore(raw, key, { emptyOnDecryptFailure: false }); + return; + } catch { + // Still legacy: fall through and bind it. + } + const values = deserializeStore(raw, machineKey, { emptyOnDecryptFailure: false }); + this.writeAllWithKey(values, key); + }); } catch { - const values = deserializeStore(raw, machineKey, { emptyOnDecryptFailure: false }); - this.writeAllWithKey(values, key); - return values; + // Preserve read compatibility if the binding cannot happen right now. } } @@ -964,7 +1040,7 @@ export class ElectronSafeStorageCredentialStore implements SyncCredentialStore { lockPath?: string; legacyLockPath?: string; legacyStore?: CredentialStoreMigrationSource | null; - keyMaterialProvider?: () => Buffer | null; + keyMaterial?: CredentialKeyMaterialSource; }) { this.safeStorage = args.safeStorage; const secretsDir = args.secretsDir ?? resolveMachineAdeLayout().secretsDir; @@ -978,7 +1054,7 @@ export class ElectronSafeStorageCredentialStore implements SyncCredentialStore { credentialsPath: this.legacyCredentialsPath, machineKeyPath: this.legacyMachineKeyPath, lockPath: this.legacyLockPath, - keyMaterialProvider: args.keyMaterialProvider, + keyMaterial: args.keyMaterial, }) : args.legacyStore; } @@ -1007,6 +1083,12 @@ export class ElectronSafeStorageCredentialStore implements SyncCredentialStore { this.deleteSync(normalized); return; } + // The migration deliberately keeps these keys out of the Electron-only + // file. A writer that puts one back in silently signs the brain and the CLI + // out of a machine whose app is signed in, so fail loudly instead. + if (isFileBackedCredentialKey(normalized)) { + throw fileBackedCredentialWriteError(normalized); + } this.withLock(() => { const values = this.readAll({ safeLockHeld: true }); values[normalized] = nextValue; @@ -1027,8 +1109,16 @@ export class ElectronSafeStorageCredentialStore implements SyncCredentialStore { updateSync(updater: (values: Record) => boolean | void): void { this.withLock(() => { const values = this.readAll({ safeLockHeld: true }); + const before = { ...values }; const shouldWrite = updater(values); - if (shouldWrite !== false) this.writeAll(values); + if (shouldWrite === false) return; + // Same guard as setSync(), scoped to what the updater actually changed so + // a pre-existing legacy entry can still be read back and rewritten as-is. + for (const [key, value] of Object.entries(values)) { + if (!isFileBackedCredentialKey(key) || before[key] === value) continue; + throw fileBackedCredentialWriteError(key); + } + this.writeAll(values); }); } @@ -1082,22 +1172,62 @@ export class ElectronSafeStorageCredentialStore implements SyncCredentialStore { } if (!raw || Object.keys(raw).length === 0) return {}; if (!isStoredCredentialEnvelope(raw)) return null; - return this.readLegacyAll(); - } - - private readLegacyAll(): Record { const legacy = this.legacyStore; - if (legacy) return legacy.readAllForMigration(); - throw new Error("Legacy credential store cannot be migrated."); + if (!legacy) throw new Error("Legacy credential store cannot be migrated."); + let values: Record; + try { + values = legacy.readAllForMigration(); + } catch { + // An undecryptable legacy store must abort the migration, never migrate + // an empty view of it. + return null; + } + // `readAllForMigration()` is a non-rewriting read, and that read returns + // `{}` instead of throwing when no available key decrypts the ciphertext. + // Migrating that empty view would write an empty safeStorage file and then + // delete credentials.json.enc AND .machine-key — destroying every + // credential on the machine. Abort instead: nothing written, nothing + // deleted, and the ciphertext stays recoverable. + if (legacy.getLastReadState() === "unreadable") return null; + return values; } private migrateLegacyStore(safeLockHeld: boolean): Record | null { const migrate = () => withOptionalCredentialFileLock(this.legacyLockPath, this.lockPath, () => { - const legacyValues = this.readLegacySafeStorageFile() ?? this.readLegacyEncryptedFileStore(); + const legacySafeStorageValues = this.readLegacySafeStorageFile(); + const legacyValues = legacySafeStorageValues ?? this.readLegacyEncryptedFileStore(); if (!legacyValues) return null; - this.writeAll(legacyValues); - this.removeLegacyFileStore(); - return legacyValues; + // Only the AES file store is shared with the ADE brain and the CLI. A + // legacy file that is ALREADY safeStorage-encrypted is Electron-only + // whether it moves or not, so it keeps the original move-and-delete path. + if (legacySafeStorageValues) { + this.writeAll(legacyValues); + this.removeLegacyFileStore(); + return legacyValues; + } + // The account session (and the sync bootstrap token) are read by the brain + // and the CLI straight from the file store. They must not be moved into + // the Electron-only safeStorage file, and while any of them are still + // there the file store (and its machine key) must survive. + const migrated: Record = {}; + const retained: Record = {}; + for (const [key, value] of Object.entries(legacyValues)) { + if (isFileBackedCredentialKey(key)) retained[key] = value; + else migrated[key] = value; + } + this.writeAll(migrated); + if (Object.keys(retained).length === 0) { + this.removeLegacyFileStore(); + return migrated; + } + // The file store survives for the retained keys, so every migrated key + // now exists in BOTH files. Leaving the duplicates behind means the brain + // and the CLI keep serving the stale file copy after the app rotates a + // token, and revoked secrets stay at rest forever. Prune the file store + // down to what it is still authoritative for. Nothing migrated means + // nothing is duplicated, so the file is left byte-identical. + if (Object.keys(migrated).length > 0) this.pruneLegacyFileStore(retained); + return migrated; }); if (safeLockHeld) return migrate(); return withCredentialFileLock(this.lockPath, migrate); @@ -1116,6 +1246,24 @@ export class ElectronSafeStorageCredentialStore implements SyncCredentialStore { ); } + /** + * Rewrites the legacy file store to only the keys it stays authoritative for. + * + * The migration holds the legacy store's own lock file (or, on the shared + * path, the one lock covering both), and that lock is not reentrant — a plain + * `legacyStore.updateSync()` here would block until the lock timeout and + * throw. The prune therefore goes through the store's non-locking migration + * seam, the same way the sibling deletes go straight to `fs`. + */ + private pruneLegacyFileStore(retained: Record): void { + try { + this.legacyStore?.pruneForMigration(retained); + } catch { + // Best effort: failing to prune only leaves the pre-existing duplicates + // behind. It must never fail the read that triggered the migration. + } + } + private removeLegacyFileStore(): void { unlinkIfExists(this.legacyMachineKeyPath); if (!isSamePath(this.legacyCredentialsPath, this.credentialsPath)) { diff --git a/apps/ade-cli/src/services/credentials/osBoundKeyMaterial.ts b/apps/ade-cli/src/services/credentials/osBoundKeyMaterial.ts new file mode 100644 index 000000000..3bf5121e3 --- /dev/null +++ b/apps/ade-cli/src/services/credentials/osBoundKeyMaterial.ts @@ -0,0 +1,413 @@ +import crypto from "node:crypto"; +import { execFile, spawnSync } from "node:child_process"; +import { + invalidateWindowsDpapiMaterial, + readOrCreateWindowsDpapiMaterial, + readOrCreateWindowsDpapiMaterialAsync, +} from "./windowsDpapiMaterial"; + +/** + * OS-bound credential key material. + * + * The encrypted file store derives its key from a machine-local secret held by + * the OS. This module owns everything about obtaining that secret — the + * platform dispatch, the `security` invocations, the create race, the + * process-wide cache, and the negative-cache backoff — so the credential store + * itself only deals with ciphertext. + * + * The two supported bindings are shaped differently, and the difference is why + * every entry point takes a `keyBindingDir` — the directory the caller keeps its + * machine key in: + * - macOS holds ONE global keychain item for the machine, so `keyBindingDir` + * is ignored there. + * - Windows holds a DPAPI-protected key file PER directory + * (`/.credential-key.dpapi`), so the directory selects which + * key is being asked for. Sharing a single slot across directories would + * hand one store another store's key. + */ + +const MACOS_KEYCHAIN_SERVICE = "com.ade.runtime.credentials.file-store-key.v1"; +const MACOS_KEYCHAIN_ACCOUNT = "machine"; +const MACOS_KEYCHAIN_READ_TIMEOUT_MS = 2_000; +const MACOS_KEYCHAIN_NEGATIVE_CACHE_MS = 30_000; +/** `security` exits 44 (errSecItemNotFound) only when the item truly does not exist. */ +const MACOS_KEYCHAIN_ITEM_NOT_FOUND_STATUS = 44; +/** `security` exits 45 (errSecDuplicateItem) when a peer already created the item. */ +const MACOS_KEYCHAIN_DUPLICATE_ITEM_STATUS = 45; + +/** + * Why a resolution produced no material. + * + * The distinction is load-bearing: `not_found` means the item has to be + * CREATED, which only the synchronous path does, so it must never suppress the + * synchronous path. `unavailable` means the keychain itself could not answer + * (locked, denied, timed out, wedged) and re-asking it on every credential read + * is what the backoff exists to prevent. + */ +export type OsBoundKeyMaterialMissReason = "not_found" | "unavailable"; + +export type OsBoundKeyMaterialResolution = + | { material: Buffer; reason?: undefined } + | { material: null; reason: OsBoundKeyMaterialMissReason }; + +type MacKeychainFindOutcome = + | { kind: "found"; value: string } + /** errSecItemNotFound: the item genuinely does not exist yet. */ + | { kind: "not_found" } + /** Timeout, locked keychain, denied access, or any other `security` failure. */ + | { kind: "error" }; + +type MacKeychainAddOutcome = "created" | "exists" | "error"; + +/** + * Injection seam for the two `security` invocations so the create race can be + * exercised without a real keychain. + */ +export type MacKeychainCommands = { + find: () => MacKeychainFindOutcome; + /** + * MUST invoke `security add-generic-password` WITHOUT `-U`: an item another + * process already created has to make this fail instead of being overwritten. + */ + add: (secret: string) => MacKeychainAddOutcome; +}; + +function readCredentialPassphraseFromEnv(): Buffer | null { + const passphrase = process.env.ADE_CREDENTIAL_STORE_PASSPHRASE?.trim(); + return passphrase ? Buffer.from(passphrase, "utf8") : null; +} + +/** Explicit opt-out, or a test process that must never touch the real keychain. */ +function osBindingDisabledByEnv(env: NodeJS.ProcessEnv): boolean { + if (env.ADE_CREDENTIAL_STORE_DISABLE_OS_BINDING === "1") return true; + return env.VITEST === "true" || env.NODE_ENV === "test"; +} + +/** Where this process's credential key material comes from. */ +export type OsBoundKeyMaterialBinding = + /** `ADE_CREDENTIAL_STORE_PASSPHRASE` overrides every OS binding. */ + | "env_passphrase" + /** Explicit opt-out, or a test process that must not touch the real OS store. */ + | "disabled" + /** `/.credential-key.dpapi`, protected per directory. */ + | "windows_dpapi" + /** One global `security` generic-password item per machine. */ + | "macos_keychain" + /** The platform has no OS binding; the bare machine key is used. */ + | "none"; + +/** + * The single decision every entry point below dispatches on. + * + * Read, read-async, invalidate and "is material expected?" MUST agree: an + * invalidation that dropped the macOS resolver's cache on Windows would leave + * the credential store's self-heal retrying against the same stale DPAPI + * material, and an `expectsOsBoundKeyMaterial()` that still said "darwin only" + * would misreport a Windows key failure as an ordinary decrypt failure. + * Deriving all four from one pure function is what keeps them in step. + * + * The env-gate applies on BOTH platforms: a test process, or an explicit + * opt-out, must never reach `security` or `powershell.exe`. + */ +export function resolveOsBoundKeyMaterialBinding( + platform: NodeJS.Platform = process.platform, + env: NodeJS.ProcessEnv = process.env, +): OsBoundKeyMaterialBinding { + if (env.ADE_CREDENTIAL_STORE_PASSPHRASE?.trim()) return "env_passphrase"; + if (osBindingDisabledByEnv(env)) return "disabled"; + if (platform === "win32") return "windows_dpapi"; + if (platform === "darwin") return "macos_keychain"; + return "none"; +} + +/** Is OS-held material expected to back this process's credential key? */ +export function expectsOsBoundKeyMaterial(): boolean { + const binding = resolveOsBoundKeyMaterialBinding(); + return binding === "windows_dpapi" || binding === "macos_keychain"; +} + +function decodeMacKeychainSecret(raw: string): Buffer { + const decoded = Buffer.from(raw, "base64"); + return decoded.length >= 32 ? decoded : Buffer.from(raw, "utf8"); +} + +function classifyMacKeychainFind(result: { + status: number | null; + signal?: NodeJS.Signals | null; + stdout?: string | null; + stderr?: string | null; + error?: unknown; +}): MacKeychainFindOutcome { + if (result.error) return { kind: "error" }; + if (result.status === 0) { + const raw = (result.stdout ?? "").trim(); + return raw.length ? { kind: "found", value: raw } : { kind: "error" }; + } + // A timeout kills `security` with a signal and no exit status. Treating that + // as "missing" is what let two first-run processes each mint their own secret. + if (result.signal) return { kind: "error" }; + if (result.status === MACOS_KEYCHAIN_ITEM_NOT_FOUND_STATUS) return { kind: "not_found" }; + return { kind: "error" }; +} + +function defaultMacKeychainCommands(): MacKeychainCommands { + const identityArgs = [ + "-a", + MACOS_KEYCHAIN_ACCOUNT, + "-s", + MACOS_KEYCHAIN_SERVICE, + ]; + return { + find: () => classifyMacKeychainFind(spawnSync( + "security", + ["find-generic-password", ...identityArgs, "-w"], + { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + timeout: MACOS_KEYCHAIN_READ_TIMEOUT_MS, + }, + )), + add: (secret) => { + // No `-U`: an existing item must fail this call so the peer's secret wins. + const result = spawnSync( + "security", + ["add-generic-password", ...identityArgs, "-w"], + { + input: `${secret}\n`, + stdio: ["pipe", "ignore", "pipe"], + timeout: MACOS_KEYCHAIN_READ_TIMEOUT_MS, + }, + ); + if (result.error || result.signal) return "error"; + if (result.status === 0) return "created"; + return result.status === MACOS_KEYCHAIN_DUPLICATE_ITEM_STATUS ? "exists" : "error"; + }, + }; +} + +/** + * Race-safe, non-destructive keychain material resolution. + * + * Two processes doing their first read concurrently must converge on ONE + * secret: whoever loses the create race adopts the winner's item instead of + * clobbering it, and any inconclusive `security` result (timeout, locked + * keychain) fails closed rather than minting a replacement. + */ +export function resolveMacKeychainMaterialOutcome( + commands: MacKeychainCommands, +): OsBoundKeyMaterialResolution { + const existing = commands.find(); + if (existing.kind === "found") return { material: decodeMacKeychainSecret(existing.value) }; + if (existing.kind === "error") return { material: null, reason: "unavailable" }; + + const secret = crypto.randomBytes(32).toString("base64"); + if (commands.add(secret) === "created") return { material: Buffer.from(secret, "base64") }; + + // The item appeared between our find and our add (or the add failed): adopt + // whatever is in the keychain now. Never overwrite it. + const winner = commands.find(); + if (winner.kind === "found") return { material: decodeMacKeychainSecret(winner.value) }; + // We could neither create nor read the item: the keychain, not its contents, + // is the problem. + return { material: null, reason: "unavailable" }; +} + +function readOrCreateMacKeychainMaterial(): OsBoundKeyMaterialResolution { + if (process.platform !== "darwin") return { material: null, reason: "unavailable" }; + return resolveMacKeychainMaterialOutcome(defaultMacKeychainCommands()); +} + +/** + * Read-only counterpart used by asynchronous reads: it never creates the item, + * so a hot async path cannot participate in the create race at all. + */ +async function readMacKeychainMaterialAsync(): Promise { + if (process.platform !== "darwin") return { material: null, reason: "unavailable" }; + return new Promise((resolve) => { + execFile( + "security", + [ + "find-generic-password", + "-a", + MACOS_KEYCHAIN_ACCOUNT, + "-s", + MACOS_KEYCHAIN_SERVICE, + "-w", + ], + { + encoding: "utf8", + timeout: MACOS_KEYCHAIN_READ_TIMEOUT_MS, + maxBuffer: 64 * 1024, + }, + (error, stdout) => { + const outcome = classifyMacKeychainFind({ + status: error ? (typeof (error as { code?: unknown }).code === "number" + ? (error as { code: number }).code + : null) : 0, + signal: (error as { signal?: NodeJS.Signals | null } | null)?.signal ?? null, + stdout, + }); + if (outcome.kind === "found") { + resolve({ material: decodeMacKeychainSecret(outcome.value) }); + return; + } + resolve({ + material: null, + reason: outcome.kind === "not_found" ? "not_found" : "unavailable", + }); + }, + ); + }); +} + +export type OsBoundKeyMaterialResolver = { + /** Creating resolution: may mint the keychain item when it does not exist. */ + read(): Buffer | null; + /** Read-only resolution: never creates the item. */ + readAsync(): Promise; + /** Drops the cache so the next resolution re-asks the OS. */ + invalidate(): void; +}; + +/** + * Caches one machine secret per process and bounds how often a failing keychain + * is re-asked. + * + * The two paths are deliberately governed by DIFFERENT backoffs. The read-only + * (async) path is the hot one and backs off on any miss. The creating (sync) + * path backs off only when the keychain was unavailable: a `not_found` miss + * recorded by the async path means the item still has to be created, and + * suppressing creation for it would starve first-run item creation forever + * because the async path refreshes the miss timestamp on every read. + */ +export function createMacKeychainMaterialResolver(args: { + read: () => OsBoundKeyMaterialResolution; + readAsync: () => Promise; + now?: () => number; + negativeCacheMs?: number; +}): OsBoundKeyMaterialResolver { + const now = args.now ?? Date.now; + const negativeCacheMs = args.negativeCacheMs ?? MACOS_KEYCHAIN_NEGATIVE_CACHE_MS; + let cached: Buffer | null = null; + let inFlight: Promise | null = null; + let lastMissAt = 0; + let lastMissReason: OsBoundKeyMaterialMissReason | null = null; + /** Bumped by invalidation so an in-flight read cannot re-cache stale material. */ + let epoch = 0; + + const withinBackoff = (): boolean => + lastMissAt > 0 && now() - lastMissAt < negativeCacheMs; + + const creationSuppressed = (): boolean => + lastMissReason === "unavailable" && withinBackoff(); + + const record = (readEpoch: number, resolution: OsBoundKeyMaterialResolution): Buffer | null => { + // An invalidation during this read means its result is already stale. + if (readEpoch !== epoch) return resolution.material; + if (resolution.material) { + cached = resolution.material; + lastMissAt = 0; + lastMissReason = null; + } else { + lastMissAt = now(); + lastMissReason = resolution.reason; + } + return resolution.material; + }; + + return { + read(): Buffer | null { + if (cached) return cached; + if (creationSuppressed()) return null; + const readEpoch = epoch; + return record(readEpoch, args.read()); + }, + async readAsync(): Promise { + if (cached) return cached; + if (withinBackoff()) return null; + if (inFlight) return await inFlight; + const readEpoch = epoch; + const read = args.readAsync().then((resolution) => record(readEpoch, resolution)); + inFlight = read; + try { + return await read; + } finally { + if (inFlight === read) inFlight = null; + } + }, + invalidate(): void { + epoch += 1; + cached = null; + lastMissAt = 0; + lastMissReason = null; + inFlight = null; + }, + }; +} + +const defaultMacKeychainMaterialResolver = createMacKeychainMaterialResolver({ + read: readOrCreateMacKeychainMaterial, + readAsync: readMacKeychainMaterialAsync, +}); + +/** + * Drops the process-wide OS-material cache so the next read re-asks the OS. + * Used when a decrypt fails with cached material: the other process may have + * won the create race after this one cached its own copy. + * + * Dispatches exactly like the readers, so the cache that actually backs + * `readDefaultOsBoundKeyMaterial` on this platform is the one that gets + * dropped: a Windows invalidation that only cleared the macOS resolver would + * leave the self-heal retry re-reading the same stale DPAPI material. + */ +export function invalidateDefaultOsBoundKeyMaterialCache(keyBindingDir: string): void { + switch (resolveOsBoundKeyMaterialBinding()) { + case "windows_dpapi": + invalidateWindowsDpapiMaterial(keyBindingDir); + return; + case "macos_keychain": + defaultMacKeychainMaterialResolver.invalidate(); + return; + default: + // Nothing is cached when no OS binding is in play. + return; + } +} + +/** + * Creating resolution for the platform's OS binding. + * + * DPAPI failures propagate rather than degrading to `null`. `null` means "no OS + * binding", which derives the bare machine key — so swallowing a transient + * PowerShell timeout would read a DPAPI-bound store as empty and, on the write + * path, silently re-seal it unbound. + */ +export function readDefaultOsBoundKeyMaterial(keyBindingDir: string): Buffer | null { + switch (resolveOsBoundKeyMaterialBinding()) { + case "env_passphrase": + return readCredentialPassphraseFromEnv(); + // Windows keeps its own per-directory cache and create race inside + // `windowsDpapiMaterial`, so it does not go through the macOS resolver. + case "windows_dpapi": + return readOrCreateWindowsDpapiMaterial(keyBindingDir); + case "macos_keychain": + return defaultMacKeychainMaterialResolver.read(); + default: + return null; + } +} + +export async function readDefaultOsBoundKeyMaterialAsync( + keyBindingDir: string, +): Promise { + switch (resolveOsBoundKeyMaterialBinding()) { + case "env_passphrase": + return readCredentialPassphraseFromEnv(); + case "windows_dpapi": + return await readOrCreateWindowsDpapiMaterialAsync(keyBindingDir); + case "macos_keychain": + return await defaultMacKeychainMaterialResolver.readAsync(); + default: + return null; + } +} diff --git a/apps/ade-cli/src/services/credentials/windowsDpapiMaterial.test.ts b/apps/ade-cli/src/services/credentials/windowsDpapiMaterial.test.ts new file mode 100644 index 000000000..1d68e82fb --- /dev/null +++ b/apps/ade-cli/src/services/credentials/windowsDpapiMaterial.test.ts @@ -0,0 +1,111 @@ +import { EventEmitter } from "node:events"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { spawn } from "node:child_process"; +import { + invalidateWindowsDpapiMaterial, + readOrCreateWindowsDpapiMaterialAsync, +} from "./windowsDpapiMaterial"; + +// The DPAPI helper is a real PowerShell child process, which no test may spawn: +// this suite runs on every platform and the module has no injection seam. Fake +// the child instead, so the cache/invalidation logic in the module's own read +// wrappers is what gets exercised. +vi.mock("node:child_process", () => ({ + spawn: vi.fn(), + spawnSync: vi.fn(), +})); + +const POWERSHELL_PATH = "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe"; + +/** Minimal stand-in for the PowerShell child the module drives. */ +class FakeDpapiChild extends EventEmitter { + readonly stdout = new EventEmitter(); + readonly stderr = Object.assign(new EventEmitter(), { resume: () => {} }); + readonly stdin = Object.assign(new EventEmitter(), { end: () => {} }); + kill = vi.fn(); + + /** Completes the unprotect with `material` as the plaintext key. */ + succeedWith(material: Buffer): void { + this.stdout.emit("data", Buffer.from(material.toString("base64"), "utf8")); + this.emit("close", 0); + } +} + +let tempDir = ""; +let pendingChildren: FakeDpapiChild[] = []; + +function writeProtectedKeyFile(dir: string): void { + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + path.join(dir, ".credential-key.dpapi"), + `ADE_WINDOWS_DPAPI_KEY_V1\n${Buffer.alloc(48, 9).toString("base64")}\n`, + ); +} + +beforeEach(() => { + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "ade-dpapi-")); + pendingChildren = []; + // `resolveWindowsDpapiPowerShellPath` validates a Windows system path; on any + // host it only has to answer consistently for the fake spawn below. + vi.spyOn(fs.realpathSync, "native").mockReturnValue(POWERSHELL_PATH); + vi.spyOn(fs, "statSync").mockReturnValue({ isFile: () => true } as fs.Stats); + vi.mocked(spawn).mockImplementation((() => { + const child = new FakeDpapiChild(); + pendingChildren.push(child); + return child; + }) as unknown as typeof spawn); +}); + +afterEach(() => { + vi.restoreAllMocks(); + fs.rmSync(tempDir, { recursive: true, force: true }); +}); + +describe("readOrCreateWindowsDpapiMaterialAsync", () => { + it("does not let a read that started before an invalidation repopulate the cache", async () => { + // Self-heal drops the cached material and retries. If a read already in + // flight when that happened were allowed to write its result back, the + // material the self-heal just rejected would be cached again — and the + // credential store's 30 s self-heal throttle means nothing would clear it + // for a full window. + const keyDir = path.join(tempDir, "dpapi-epoch"); + writeProtectedKeyFile(keyDir); + const stale = Buffer.alloc(32, 1); + const fresh = Buffer.alloc(32, 2); + + const inFlight = readOrCreateWindowsDpapiMaterialAsync(keyDir); + await vi.waitFor(() => expect(pendingChildren).toHaveLength(1)); + + invalidateWindowsDpapiMaterial(keyDir); + pendingChildren[0]!.succeedWith(stale); + // The read still answers its own callers; only the cache write is dropped. + expect(await inFlight).toEqual(stale); + + const next = readOrCreateWindowsDpapiMaterialAsync(keyDir); + await vi.waitFor(() => expect(pendingChildren).toHaveLength(2)); + pendingChildren[1]!.succeedWith(fresh); + + expect(await next).toEqual(fresh); + // And the post-invalidation read is the one that gets to stay cached. + expect(await readOrCreateWindowsDpapiMaterialAsync(keyDir)).toEqual(fresh); + expect(pendingChildren).toHaveLength(2); + }); + + it("coalesces concurrent reads of one key directory into a single helper spawn", async () => { + const keyDir = path.join(tempDir, "dpapi-dedup"); + writeProtectedKeyFile(keyDir); + const material = Buffer.alloc(32, 3); + + const first = readOrCreateWindowsDpapiMaterialAsync(keyDir); + const second = readOrCreateWindowsDpapiMaterialAsync(keyDir); + await vi.waitFor(() => expect(pendingChildren).toHaveLength(1)); + pendingChildren[0]!.succeedWith(material); + + expect(await first).toEqual(material); + expect(await second).toEqual(material); + expect(pendingChildren).toHaveLength(1); + }); +}); diff --git a/apps/ade-cli/src/services/credentials/windowsDpapiMaterial.ts b/apps/ade-cli/src/services/credentials/windowsDpapiMaterial.ts index 83e760406..dd941e594 100644 --- a/apps/ade-cli/src/services/credentials/windowsDpapiMaterial.ts +++ b/apps/ade-cli/src/services/credentials/windowsDpapiMaterial.ts @@ -22,8 +22,25 @@ const WINDOWS_DPAPI_MAX_OUTPUT_BYTES = 64 * 1024; const WINDOWS_DPAPI_POWERSHELL_KERNEL_PATH = "\\\\?\\GLOBALROOT\\SystemRoot\\System32\\WindowsPowerShell\\v1.0\\powershell.exe"; +/** + * Windows deliberately has no negative cache, unlike the macOS keychain + * resolver. A locked keychain is a durable state worth backing off from, but a + * DPAPI failure is almost always a transient PowerShell timeout — suppressing + * retries for it would turn one slow cold start into a permanent-looking + * "credentials are unavailable". + */ const cachedKeyMaterial = new Map(); +/** + * Concurrent credential reads would otherwise each spawn their own PowerShell, + * and that contention is precisely what makes a cold start slow enough to hit + * the timeout above. One in-flight read per key path serves them all. + */ const keyMaterialReadInFlight = new Map>(); +/** + * Bumped by every invalidation so a read that started before it cannot write + * its now-stale material back into the cache. + */ +let dpapiEpoch = 0; const WINDOWS_DPAPI_SCRIPT = [ "$ErrorActionPreference = 'Stop'", @@ -211,8 +228,29 @@ function runDpapiAsync(operation: "protect" | "unprotect", value: Buffer): Promi }); } -function protectedKeyPath(secretsDir: string): string { - return path.resolve(secretsDir, WINDOWS_DPAPI_KEY_FILE); +function protectedKeyPath(keyBindingDir: string): string { + return path.resolve(keyBindingDir, WINDOWS_DPAPI_KEY_FILE); +} + +/** + * Drops the in-process DPAPI material cache for one key-binding directory so + * the next read re-runs the unprotect against whatever is on disk now. + * + * The credential store's self-heal needs this on Windows for the same reason it + * needs it on macOS: a peer process can win the key-creation race after this + * process cached its own copy, and without a way to drop that copy every later + * decrypt keeps failing against material that is already known to be wrong. + * + * The in-flight promise is deliberately left alone — it is already reading, and + * dropping it would only duplicate the PowerShell spawn the dedup exists to + * avoid. Bumping the epoch is what makes that safe: the read still resolves for + * its own callers, but its cache write is discarded, so material this + * invalidation just rejected cannot reappear behind the self-heal's 30 s + * throttle. + */ +export function invalidateWindowsDpapiMaterial(keyBindingDir: string): void { + dpapiEpoch += 1; + cachedKeyMaterial.delete(protectedKeyPath(keyBindingDir)); } function unprotectKey(keyPath: string): Buffer { @@ -238,11 +276,12 @@ async function unprotectKeyAsync(keyPath: string): Promise { * key crosses the PowerShell boundary only on stdin/stdout and the persisted * blob is unusable from another Windows account. */ -export function readOrCreateWindowsDpapiMaterial(secretsDir: string): Buffer { - const keyPath = protectedKeyPath(secretsDir); +export function readOrCreateWindowsDpapiMaterial(keyBindingDir: string): Buffer { + const keyPath = protectedKeyPath(keyBindingDir); const cached = cachedKeyMaterial.get(keyPath); if (cached) return cached; + const readEpoch = dpapiEpoch; let material: Buffer; try { material = unprotectKey(keyPath); @@ -262,18 +301,21 @@ export function readOrCreateWindowsDpapiMaterial(secretsDir: string): Buffer { material = unprotectKey(keyPath); } } - cachedKeyMaterial.set(keyPath, material); + if (readEpoch === dpapiEpoch) cachedKeyMaterial.set(keyPath, material); return material; } /** Async counterpart used by brain-facing credential reads. */ -export async function readOrCreateWindowsDpapiMaterialAsync(secretsDir: string): Promise { - const keyPath = protectedKeyPath(secretsDir); +export async function readOrCreateWindowsDpapiMaterialAsync( + keyBindingDir: string, +): Promise { + const keyPath = protectedKeyPath(keyBindingDir); const cached = cachedKeyMaterial.get(keyPath); if (cached) return cached; const existing = keyMaterialReadInFlight.get(keyPath); if (existing) return await existing; + const readEpoch = dpapiEpoch; const read = (async () => { let material: Buffer; try { @@ -294,7 +336,10 @@ export async function readOrCreateWindowsDpapiMaterialAsync(secretsDir: string): material = await unprotectKeyAsync(keyPath); } } - cachedKeyMaterial.set(keyPath, material); + // An invalidation while this read was in flight means the material it + // produced is already known to be stale; hand it to this read's own callers + // but never let it repopulate the cache the self-heal just cleared. + if (readEpoch === dpapiEpoch) cachedKeyMaterial.set(keyPath, material); return material; })(); keyMaterialReadInFlight.set(keyPath, read); diff --git a/apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.ts b/apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.ts index 6cc9b8aad..8371a6daa 100644 --- a/apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.ts +++ b/apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.ts @@ -122,7 +122,13 @@ type BrainPeerState = { }; const WS_OPEN = 1; -const BOOTSTRAP_TOKEN_KEY = "sync.bootstrapToken.v1"; +/** + * Exported so the credential store's migration-exclusion list can be asserted + * against the real key instead of a bare literal: this token is read by the + * brain straight from the shared file store, and a silent rename would move it + * into the Electron-only safeStorage file. + */ +export const BOOTSTRAP_TOKEN_KEY = "sync.bootstrapToken.v1"; const BRAIN_SYNC_AUTH_TIMEOUT_MS = 15_000; const brainPeerCompressionBySocket = new WeakMap(); diff --git a/apps/ade-cli/src/services/sync/syncHostService.test.ts b/apps/ade-cli/src/services/sync/syncHostService.test.ts index 8a99fcfba..d7f4246a0 100644 --- a/apps/ade-cli/src/services/sync/syncHostService.test.ts +++ b/apps/ade-cli/src/services/sync/syncHostService.test.ts @@ -810,7 +810,7 @@ describe("brain project actions fallback handler", () => { fs.mkdirSync(secretsDir, { recursive: true }); const credentials = new EncryptedFileCredentialStore({ secretsDir, - keyMaterialProvider: () => null, + keyMaterial: { read: () => null }, }); credentials.setSync("test.bootstrap", "bootstrap-token"); const projects = Array.from({ length: 30 }, (_, index) => createDiscoveryProject({ @@ -904,7 +904,7 @@ describe("brain project actions fallback handler", () => { }, bootstrapCredentialStore: new EncryptedFileCredentialStore({ secretsDir, - keyMaterialProvider: () => null, + keyMaterial: { read: () => null }, }), pairingSecretsPath: pairing.pairingSecretsPath, pinPath: pairing.pinPath, @@ -978,7 +978,7 @@ describe("brain project actions fallback handler", () => { const { projectRoot, cleanup } = createTempProjectRoot(); const secretsDir = path.join(projectRoot, "secrets"); fs.mkdirSync(secretsDir, { recursive: true }); - const credentials = new EncryptedFileCredentialStore({ secretsDir, keyMaterialProvider: () => null }); + const credentials = new EncryptedFileCredentialStore({ secretsDir, keyMaterial: { read: () => null } }); credentials.setSync("test.bootstrap", "bootstrap-token"); let resolveCatalog!: () => void; const catalogGate = new Promise((resolve) => { resolveCatalog = resolve; }); @@ -1042,7 +1042,7 @@ describe("brain project actions fallback handler", () => { }, bootstrapCredentialStore: new EncryptedFileCredentialStore({ secretsDir, - keyMaterialProvider: () => null, + keyMaterial: { read: () => null }, }), pairingSecretsPath: pairing.pairingSecretsPath, pinPath: pairing.pinPath, @@ -1121,7 +1121,7 @@ describe("brain project actions fallback handler", () => { fs.writeFileSync(transcriptPath, ""); const credentialStore = new EncryptedFileCredentialStore({ secretsDir, - keyMaterialProvider: () => null, + keyMaterial: { read: () => null }, }); credentialStore.setSync("test.bootstrap", "bootstrap-token"); const logger = createDiscoveryLogger(); @@ -1326,7 +1326,7 @@ describe("brain project actions fallback handler", () => { fs.mkdirSync(secretsDir, { recursive: true }); const credentialStore = new EncryptedFileCredentialStore({ secretsDir, - keyMaterialProvider: () => null, + keyMaterial: { read: () => null }, }); credentialStore.setSync("test.bootstrap", "bootstrap-token"); @@ -1429,7 +1429,7 @@ describe("brain project actions fallback handler", () => { }, bootstrapCredentialStore: new EncryptedFileCredentialStore({ secretsDir, - keyMaterialProvider: () => null, + keyMaterial: { read: () => null }, }), pairingSecretsPath: pairing.pairingSecretsPath, pinPath: pairing.pinPath, @@ -1490,7 +1490,7 @@ describe("brain project actions fallback handler", () => { }, bootstrapCredentialStore: new EncryptedFileCredentialStore({ secretsDir, - keyMaterialProvider: () => null, + keyMaterial: { read: () => null }, }), pairingSecretsPath: path.join(secretsDir, "sync-paired-devices.json"), pinPath, diff --git a/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts b/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts index fd4f7b2a9..88aca333e 100644 --- a/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts +++ b/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts @@ -20,6 +20,7 @@ export const INTERNAL_ONLY_EVENTS = new Set([ "ade_update_auto_applied", "ade_update_auto_apply_cancelled", "ade_brain_recovered", "ade_publish_failing", "ade_relay_suppressed", + "ade_account_session_unreadable", ]); export const EVENT_DAILY_BUDGETS: Record = { @@ -43,6 +44,7 @@ export const EVENT_DAILY_BUDGETS: Record = { ade_brain_recovered: 10, ade_publish_failing: 10, ade_relay_suppressed: 10, + ade_account_session_unreadable: 10, }; export const EVENT_MINUTE_BUDGETS: Record = { @@ -66,6 +68,7 @@ export const EVENT_MINUTE_BUDGETS: Record = { ade_brain_recovered: 3, ade_publish_failing: 3, ade_relay_suppressed: 3, + ade_account_session_unreadable: 3, }; const STRING_PROPERTIES = new Set([ @@ -94,6 +97,7 @@ const ANALYTICS_ONLY_ACTIONS = new Set([ "maintenance_run", "header_opened", "preferences_changed", + "brain_repair", ]); const EVENT_PROPERTY_KEYS: Record> = { @@ -130,6 +134,7 @@ const EVENT_PROPERTY_KEYS: Record ade_brain_recovered: new Set(["blocked_ms", "last_command"]), ade_publish_failing: new Set(["failing_minutes", "leg", "code"]), ade_relay_suppressed: new Set(["attempt", "code"]), + ade_account_session_unreadable: new Set(["code"]), }; const SLUG_VALUE = /^[a-z0-9][a-z0-9._+-]*$/i; @@ -143,7 +148,7 @@ const SAFE_STRING_VALUES: Partial>> = { ]), feature: new Set([ "chat", "cli", "work", "lanes", "files", "git", "orchestration", "prs", - "automations", "command_palette", "storage_doctor", "attention", "updates", + "automations", "command_palette", "storage_doctor", "attention", "updates", "connections", ]), outcome: new Set([ "success", "started", "completed", "failure", "timeout", "opened", "cancelled", "approved", "denied", diff --git a/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts b/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts index a07cef47d..b0cdd4be3 100644 --- a/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts +++ b/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts @@ -156,6 +156,37 @@ describe("productAnalyticsService", () => { fs.rmSync(harness.root, { recursive: true, force: true }); }); + it("accepts the connections brain_repair fact and strips anything beyond the coarse enums", () => { + const harness = makeHarness(); + expect(harness.service.capture({ + event: "ade_feature_used", + surface: "desktop", + properties: { + feature: "connections", + action: "brain_repair", + outcome: "failed", + error_text: "ENOENT /Users/someone/.ade/secrets/credentials.json.enc", + }, + dedupeKey: "brain_repair:failed", + minimumIntervalMs: 60 * 60 * 1_000, + })).toEqual({ accepted: true, reason: "accepted" }); + expect(harness.messages[0]?.properties).toMatchObject({ + feature: "connections", + action: "brain_repair", + outcome: "failed", + }); + expect(harness.messages[0]?.properties).not.toHaveProperty("error_text"); + // Same dedupe key inside the interval: dropped, so a click-loop is bounded. + expect(harness.service.capture({ + event: "ade_feature_used", + surface: "desktop", + properties: { feature: "connections", action: "brain_repair", outcome: "failed" }, + dedupeKey: "brain_repair:failed", + minimumIntervalMs: 60 * 60 * 1_000, + })).toEqual({ accepted: false, reason: "duplicate" }); + fs.rmSync(harness.root, { recursive: true, force: true }); + }); + it("accepts only coarse transactional update telemetry properties", () => { const harness = makeHarness(); diff --git a/apps/desktop/src/main/services/ipc/ipcTimeouts.test.ts b/apps/desktop/src/main/services/ipc/ipcTimeouts.test.ts index c83cedcf1..da55d0dd4 100644 --- a/apps/desktop/src/main/services/ipc/ipcTimeouts.test.ts +++ b/apps/desktop/src/main/services/ipc/ipcTimeouts.test.ts @@ -102,6 +102,17 @@ describe("ipcInvokeTimeoutMs", () => { expect(ipcInvokeTimeoutMs(IPC.projectSwitchToPath)).toBe(285_000); }); + it("outlasts a brain restart's service install plus readiness wait", () => { + // The handler resolves only once the replacement brain answers a ping. The + // worst case is a forced install queued behind an in-flight one (60s + 60s, + // localRuntimeConnectionPool), then the socket wait and the ping, each + // bounded by BRAIN_RESTART_TIMEOUT_MS (20s, projectRecoveryService). + // Spelled out rather than imported because both budgets are module-private; + // if either grows, this expectation is the thing that should be revisited. + const WORST_CASE_MS = 60_000 + 60_000 + 20_000 + 20_000; + expect(ipcInvokeTimeoutMs(IPC.appRestartBackgroundService)).toBeGreaterThanOrEqual(WORST_CASE_MS); + }); + it("gives retryable remote runtime actions enough time to reconnect", () => { expect(ipcInvokeTimeoutMs(IPC.remoteRuntimeCallAction, [{ id: "target-1", diff --git a/apps/desktop/src/main/services/ipc/ipcTimeouts.ts b/apps/desktop/src/main/services/ipc/ipcTimeouts.ts index cda8e59ed..a5ef34cd9 100644 --- a/apps/desktop/src/main/services/ipc/ipcTimeouts.ts +++ b/apps/desktop/src/main/services/ipc/ipcTimeouts.ts @@ -107,6 +107,14 @@ export function ipcInvokeTimeoutMs(channel: string, args: readonly unknown[] = [ case IPC.agentChatHandoff: case IPC.agentChatPrepareCrossMachineHandoff: return 150_000; + // A brain restart waits through the service install, then for the + // replacement brain to rebind (20s) and answer a ping (20s). The install + // leg can be 120s rather than 60s, because a forced restart queues behind + // an in-flight non-forced install instead of joining it. The renderer must + // not give up before the main process knows the outcome, or the Repair + // button reports a failure for a restart that actually succeeded. + case IPC.appRestartBackgroundService: + return 4 * 60_000; case IPC.iosSimulatorLaunch: return 10 * 60_000; case IPC.transcriptionTranscribe: diff --git a/apps/desktop/src/main/services/ipc/registerIpc.ts b/apps/desktop/src/main/services/ipc/registerIpc.ts index 13d306adf..2f2289587 100644 --- a/apps/desktop/src/main/services/ipc/registerIpc.ts +++ b/apps/desktop/src/main/services/ipc/registerIpc.ts @@ -3937,6 +3937,38 @@ export function registerIpc({ ); }); + // Restart this machine's ADE brain (the com.ade.runtime launch agent) through + // the same `serve --install-service` boundary the launcher uses, which does a + // verified unload → reap → load handover. Direct IPC on purpose: the pool + // lives in Electron main and the daemon being restarted cannot route its own + // restart, so there is no action-domain routing and no null-service risk. + // Resolves only once the replacement brain answers a ping, and throws when it + // does not — the renderer cannot observe brain readiness itself. + ipcMain.handle(IPC.appRestartBackgroundService, async (): Promise => { + if (!projectRecoveryService) { + throw new Error("This window does not manage the ADE background service."); + } + // One coarse fact per Repair click, captured where the outcome is known. + // The 1 h dedupe bounds a frustrated click-loop to ≤24 accepted events/day. + const captureRepairOutcome = (outcome: "completed" | "failed") => { + productAnalyticsService?.capture({ + event: "ade_feature_used", + surface: "desktop", + properties: { feature: "connections", action: "brain_repair", outcome }, + projectId: null, + dedupeKey: `brain_repair:${outcome}`, + minimumIntervalMs: 60 * 60 * 1_000, + }); + }; + try { + await projectRecoveryService.restartBrain(); + } catch (error) { + captureRepairOutcome("failed"); + throw error; + } + captureRepairOutcome("completed"); + }); + ipcMain.handle(IPC.storageGetPressure, async (): Promise => { const monitor = requireAppContextValue(getCtx(), "diskPressureMonitor"); return monitor.getSnapshot({ maxAgeMs: 1_000 }); diff --git a/apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.ts b/apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.ts index 24442974c..6a3e5603f 100644 --- a/apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.ts +++ b/apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.ts @@ -514,6 +514,94 @@ describe("local runtime connection pool", () => { } }); + it("runs a forcing install instead of coalescing onto an in-flight background install", async () => { + // A background install already running cannot satisfy a Repair click: it + // may skip, or may have spawned before the user asked. The forced call must + // queue behind it and then spawn its own forcing install. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "ade-install-force-")); + const logPath = path.join(dir, "spawns.log"); + const cliPath = path.join(dir, "fake-cli.cjs"); + fs.writeFileSync( + cliPath, + [ + "const fs = require('node:fs');", + `fs.appendFileSync(${JSON.stringify(logPath)}, (process.env.ADE_FORCE_RUNTIME_SERVICE_RESTART || 'unset') + '\\n');`, + "setTimeout(() => {", + " process.stdout.write(JSON.stringify({ ok: true, path: 'com.ade.runtime', message: 'installed' }));", + "}, 150);", + ].join("\n"), + ); + const originalEnv = { + ADE_CLI_JS: process.env.ADE_CLI_JS, + ADE_RUNTIME_SOCKET_PATH: process.env.ADE_RUNTIME_SOCKET_PATH, + }; + const logger = { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + const pool = new LocalRuntimeConnectionPool("1.2.3", logger as never); + + try { + process.env.ADE_CLI_JS = cliPath; + process.env.ADE_RUNTIME_SOCKET_PATH = path.join(dir, "missing.sock"); + + const background = pool.installServiceBestEffort(); + const forced = pool.installServiceBestEffort({ forceRestart: true }); + expect(forced).not.toBe(background); + await Promise.all([background, forced]); + + expect(fs.readFileSync(logPath, "utf8").trim().split("\n")).toEqual(["unset", "1"]); + expect(pool.getStatus().serviceInstall).toMatchObject({ state: "installed", attempted: true }); + } finally { + pool.dispose(); + if (originalEnv.ADE_CLI_JS === undefined) delete process.env.ADE_CLI_JS; + else process.env.ADE_CLI_JS = originalEnv.ADE_CLI_JS; + if (originalEnv.ADE_RUNTIME_SOCKET_PATH === undefined) delete process.env.ADE_RUNTIME_SOCKET_PATH; + else process.env.ADE_RUNTIME_SOCKET_PATH = originalEnv.ADE_RUNTIME_SOCKET_PATH; + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("settles a wedged service install as failed instead of hanging forever", async () => { + // Without the timeout a stuck installer pins serviceInstallPromise, so the + // Repair button spins forever and every later install is blocked too. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "ade-install-wedge-")); + const cliPath = path.join(dir, "wedged-cli.cjs"); + fs.writeFileSync(cliPath, "setInterval(() => {}, 1000);\n"); + const originalEnv = { + ADE_CLI_JS: process.env.ADE_CLI_JS, + ADE_RUNTIME_SOCKET_PATH: process.env.ADE_RUNTIME_SOCKET_PATH, + }; + const logger = { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + const pool = new LocalRuntimeConnectionPool("1.2.3", logger as never, { + serviceInstallTimeoutMs: 250, + }); + + try { + process.env.ADE_CLI_JS = cliPath; + process.env.ADE_RUNTIME_SOCKET_PATH = path.join(dir, "missing.sock"); + + await pool.installServiceBestEffort(); + + expect(pool.getStatus().serviceInstall).toMatchObject({ + state: "failed", + attempted: true, + message: "ADE service login item installation timed out.", + }); + expect(logger.warn).toHaveBeenCalledWith( + "local_runtime.service_install_failed", + expect.objectContaining({ reason: "timeout" }), + ); + // The stuck attempt released the coalescing latch, so a retry runs. + await pool.installServiceBestEffort(); + expect(pool.getStatus().serviceInstall.state).toBe("failed"); + } finally { + pool.dispose(); + if (originalEnv.ADE_CLI_JS === undefined) delete process.env.ADE_CLI_JS; + else process.env.ADE_CLI_JS = originalEnv.ADE_CLI_JS; + if (originalEnv.ADE_RUNTIME_SOCKET_PATH === undefined) delete process.env.ADE_RUNTIME_SOCKET_PATH; + else process.env.ADE_RUNTIME_SOCKET_PATH = originalEnv.ADE_RUNTIME_SOCKET_PATH; + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + it("skips service install when a newer compatible brain is already running", async () => { const adeCliRoot = path.resolve(process.cwd(), "../ade-cli"); const cliPath = path.join(adeCliRoot, "src", "cli.ts"); diff --git a/apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts b/apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts index 975bfd024..79504a37b 100644 --- a/apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts +++ b/apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts @@ -85,11 +85,28 @@ type RuntimeServiceManagerOutput = { message: string | null; }; +/** + * Outcome of one `serve --install-service` / `serve --uninstall-service` child + * run. `timedOut` is its own variant because the child was killed before it + * could say anything: there is no exit code or output to interpret. + */ +type ServiceManagerCommandResult = + | { timedOut: true } + | { + timedOut: false; + code: number | null; + stdout: string; + stderr: string; + parsed: RuntimeServiceManagerOutput | null; + }; + type LocalRuntimeConnectionPoolOptions = { disableSync?: boolean; preferServiceRepair?: boolean; desktopBridgeAuthToken?: string | null; queryServiceStatus?: () => ServiceManagerStatusResult; + /** Test seam: bound on `serve --install-service`. Defaults to the module constant. */ + serviceInstallTimeoutMs?: number; onRuntimeStatusChange?: (status: LocalRuntimeStatus) => void; /** * Invoked when the pool enters or leaves isolated (no-sync fallback) mode. @@ -101,6 +118,10 @@ type LocalRuntimeConnectionPoolOptions = { type LocalRuntimeNodePathOptions = PackagedRuntimeNodePathOptions; const LOCAL_RUNTIME_SERVICE_UNINSTALL_TIMEOUT_MS = 20_000; +// `serve --install-service` does an unload → reap → load handover, so it is +// allowed longer than the uninstall — but never forever: a wedged installer +// used to pin `serviceInstallPromise` and block every later install. +const LOCAL_RUNTIME_SERVICE_INSTALL_TIMEOUT_MS = 60_000; const LOCAL_RUNTIME_STATUS_REFRESH_TIMEOUT_MS = 2_000; const PLACEHOLDER_RUNTIME_VERSION = "0.0.0"; const LOCAL_RUNTIME_OUTPUT_LINE_MAX_CHARS = 4_000; @@ -1040,9 +1061,25 @@ export class LocalRuntimeConnectionPool { } } + /** + * Installs (or reinstalls) the service login item, coalescing concurrent + * callers onto one child process. + * + * `forceRestart` does not coalesce onto a plain install: a background install + * may skip entirely, or may have spawned before the user asked for a restart, + * so returning its promise would report success without restarting anything. + * A forced call instead queues behind whatever is in flight and then runs its + * own forcing install, and becomes the promise later callers coalesce onto. + */ async installServiceBestEffort(options: { forceRestart?: boolean } = {}): Promise { - if (this.serviceInstallPromise) return this.serviceInstallPromise; - const install = this.runServiceInstallBestEffort(options).finally(() => { + const inFlight = this.serviceInstallPromise; + if (inFlight && !options.forceRestart) return inFlight; + const install: Promise = (inFlight + // The queued-behind install ran to settle the earlier caller; its outcome + // is that caller's to report, so failures do not skip the forced run. + ? inFlight.catch(() => {}).then(() => this.runServiceInstallBestEffort(options)) + : this.runServiceInstallBestEffort(options) + ).finally(() => { if (this.serviceInstallPromise === install) this.serviceInstallPromise = null; }); this.serviceInstallPromise = install; @@ -1058,9 +1095,69 @@ export class LocalRuntimeConnectionPool { */ async uninstallServiceBestEffort(): Promise { const cliPath = resolveCliScriptPath(); - await new Promise((resolve, reject) => { - const child = spawn(process.execPath, [cliPath, "serve", "--uninstall-service"], { - env: buildLocalRuntimeNodeEnv(this.appVersion), + let result: ServiceManagerCommandResult; + try { + result = await this.runServiceManagerCommand({ + cliPath, + flag: "--uninstall-service", + // A hung `serve --uninstall-service` (e.g. a stuck login-item removal) + // must not leave the repair flow waiting forever; the timeout kills the + // child so this throws instead of silently proceeding to exclusive + // database work. + timeoutMs: LOCAL_RUNTIME_SERVICE_UNINSTALL_TIMEOUT_MS, + }); + } catch (error) { + this.logger.warn("local_runtime.service_uninstall_failed", { + error: error instanceof Error ? error.message : String(error), + }); + throw error; + } + if (result.timedOut) { + const message = "ADE service login item removal timed out."; + this.logger.warn("local_runtime.service_uninstall_failed", { cliPath, reason: "timeout", message }); + throw new Error(message); + } + const { code, stdout: output, stderr: errorOutput, parsed } = result; + if (code !== 0 || parsed?.ok === false) { + const message = parsed?.message || errorOutput || output || "ADE service login item removal failed."; + this.logger.warn("local_runtime.service_uninstall_failed", { cliPath, exitCode: code, message }); + throw new Error(message); + } + this.serviceInstallStatus = { + state: "not_attempted", + attempted: false, + path: parsed?.path ?? cliPath, + message: parsed?.message || output || "ADE service login item was removed.", + exitCode: code, + updatedAt: new Date().toISOString(), + }; + this.logger.info("local_runtime.service_uninstall_succeeded", { cliPath, exitCode: code }); + } + + /** + * Runs one `serve ` service-manager child process — the child-process + * boundary desktop uses instead of importing ade-cli service-manager code — + * and reports what it did. Install and uninstall share every mechanic here + * (spawn, output accumulation, single-settle latch, timeout kill, output + * parse) and differ only in the policy they apply to the result. + * + * Rejects when the child cannot be spawned. Resolves `{ timedOut: true }` + * once the deadline passes, having killed the child, so neither caller can + * wait forever: a wedged installer used to pin `serviceInstallPromise` and + * block every later install. + * + * `windowsHide` lives here rather than at the call sites so neither caller + * can flash a console window on Windows by forgetting it. + */ + private async runServiceManagerCommand(options: { + cliPath: string; + flag: "--install-service" | "--uninstall-service"; + timeoutMs: number; + env?: NodeJS.ProcessEnv; + }): Promise { + return await new Promise((resolve, reject) => { + const child = spawn(process.execPath, [options.cliPath, "serve", options.flag], { + env: options.env ?? buildLocalRuntimeNodeEnv(this.appVersion), stdio: ["ignore", "pipe", "pipe"], detached: false, windowsHide: true, @@ -1068,18 +1165,12 @@ export class LocalRuntimeConnectionPool { let stdout = ""; let stderr = ""; let settled = false; - // A hung `serve --uninstall-service` (e.g. a stuck login-item removal) - // must not leave the repair flow waiting forever; time it out, kill the - // child, and reject so the caller reports a repair failure instead of - // silently proceeding to exclusive database work. const timer = setTimeout(() => { if (settled) return; settled = true; try { child.kill("SIGKILL"); } catch { /* child may already be gone */ } - const message = "ADE service login item removal timed out."; - this.logger.warn("local_runtime.service_uninstall_failed", { cliPath, reason: "timeout", message }); - reject(new Error(message)); - }, LOCAL_RUNTIME_SERVICE_UNINSTALL_TIMEOUT_MS); + resolve({ timedOut: true }); + }, options.timeoutMs); timer.unref?.(); child.stdout?.on("data", (chunk) => { stdout += chunk.toString("utf8"); @@ -1091,7 +1182,6 @@ export class LocalRuntimeConnectionPool { if (settled) return; settled = true; clearTimeout(timer); - this.logger.warn("local_runtime.service_uninstall_failed", { error: error.message }); reject(error); }); child.once("close", (code) => { @@ -1099,24 +1189,13 @@ export class LocalRuntimeConnectionPool { settled = true; clearTimeout(timer); const output = stdout.trim(); - const parsed = parseRuntimeServiceManagerOutput(output); - const failed = code !== 0 || parsed?.ok === false; - if (failed) { - const message = parsed?.message || stderr.trim() || output || "ADE service login item removal failed."; - this.logger.warn("local_runtime.service_uninstall_failed", { cliPath, exitCode: code, message }); - reject(new Error(message)); - return; - } - this.serviceInstallStatus = { - state: "not_attempted", - attempted: false, - path: parsed?.path ?? cliPath, - message: parsed?.message || output || "ADE service login item was removed.", - exitCode: code, - updatedAt: new Date().toISOString(), - }; - this.logger.info("local_runtime.service_uninstall_succeeded", { cliPath, exitCode: code }); - resolve(); + resolve({ + timedOut: false, + code, + stdout: output, + stderr: stderr.trim(), + parsed: parseRuntimeServiceManagerOutput(output), + }); }); }); } @@ -1196,73 +1275,78 @@ export class LocalRuntimeConnectionPool { exitCode: null, updatedAt: new Date().toISOString(), }; - await new Promise((resolve) => { - const child = spawn(process.execPath, [cliPath, "serve", "--install-service"], { + let result: ServiceManagerCommandResult; + try { + result = await this.runServiceManagerCommand({ + cliPath, + flag: "--install-service", + // Mirrors the uninstall guard: a wedged `serve --install-service` must + // not leave the install pending forever — that pins + // `serviceInstallPromise` and blocks every later install, and leaves + // Repair spinning. + timeoutMs: this.options.serviceInstallTimeoutMs ?? LOCAL_RUNTIME_SERVICE_INSTALL_TIMEOUT_MS, env: { ...buildLocalRuntimeNodeEnv(this.appVersion), ...(options.forceRestart ? { ADE_FORCE_RUNTIME_SERVICE_RESTART: "1" } : {}), }, - stdio: ["ignore", "pipe", "pipe"], - detached: false, - windowsHide: true, }); - let stdout = ""; - let stderr = ""; - child.stdout?.on("data", (chunk) => { - stdout += chunk.toString("utf8"); - }); - child.stderr?.on("data", (chunk) => { - stderr += chunk.toString("utf8"); - }); - child.once("error", (error) => { - this.serviceInstallStatus = { - state: "failed", - attempted: true, - path: cliPath, - message: error.message, - exitCode: null, - updatedAt: new Date().toISOString(), - }; - this.logger.warn("local_runtime.service_install_failed", { error: error.message }); - resolve(); - }); - child.once("close", (code) => { - const output = stdout.trim(); - const errorOutput = stderr.trim(); - const parsed = parseRuntimeServiceManagerOutput(output); - const failed = code !== 0 || parsed?.ok === false; - const statusPath = parsed ? parsed.path : cliPath; - const payload = { - cliPath, - servicePath: parsed?.path ?? null, - exitCode: code, - stdout: output || null, - stderr: errorOutput || null, - }; - if (!failed) { - this.serviceInstallStatus = { - state: "installed", - attempted: true, - path: statusPath, - message: parsed?.message || output || "ADE service login item is installed.", - exitCode: code, - updatedAt: new Date().toISOString(), - }; - this.logger.info("local_runtime.service_install_succeeded", payload); - } else { - this.serviceInstallStatus = { - state: "failed", - attempted: true, - path: statusPath, - message: parsed?.message || errorOutput || output || "ADE service login item installation failed.", - exitCode: code, - updatedAt: new Date().toISOString(), - }; - this.logger.warn("local_runtime.service_install_failed", payload); - } - resolve(); - }); - }); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + this.serviceInstallStatus = { + state: "failed", + attempted: true, + path: cliPath, + message, + exitCode: null, + updatedAt: new Date().toISOString(), + }; + this.logger.warn("local_runtime.service_install_failed", { error: message }); + return; + } + if (result.timedOut) { + const message = "ADE service login item installation timed out."; + this.serviceInstallStatus = { + state: "failed", + attempted: true, + path: cliPath, + message, + exitCode: null, + updatedAt: new Date().toISOString(), + }; + this.logger.warn("local_runtime.service_install_failed", { cliPath, reason: "timeout", message }); + return; + } + const { code, stdout: output, stderr: errorOutput, parsed } = result; + const failed = code !== 0 || parsed?.ok === false; + const statusPath = parsed ? parsed.path : cliPath; + const payload = { + cliPath, + servicePath: parsed?.path ?? null, + exitCode: code, + stdout: output || null, + stderr: errorOutput || null, + }; + if (!failed) { + this.serviceInstallStatus = { + state: "installed", + attempted: true, + path: statusPath, + message: parsed?.message || output || "ADE service login item is installed.", + exitCode: code, + updatedAt: new Date().toISOString(), + }; + this.logger.info("local_runtime.service_install_succeeded", payload); + } else { + this.serviceInstallStatus = { + state: "failed", + attempted: true, + path: statusPath, + message: parsed?.message || errorOutput || output || "ADE service login item installation failed.", + exitCode: code, + updatedAt: new Date().toISOString(), + }; + this.logger.warn("local_runtime.service_install_failed", payload); + } } async ensureProject( diff --git a/apps/desktop/src/main/services/remoteRuntime/runtimeDiscovery.ts b/apps/desktop/src/main/services/remoteRuntime/runtimeDiscovery.ts index 496e73cf6..76fc37700 100644 --- a/apps/desktop/src/main/services/remoteRuntime/runtimeDiscovery.ts +++ b/apps/desktop/src/main/services/remoteRuntime/runtimeDiscovery.ts @@ -16,6 +16,7 @@ import type { RemoteRuntimeDiscoveredMachine, RemoteRuntimeDiscoveryDiagnostic, RemoteRuntimeDiscoveryResult, + RemoteRuntimeDiscoverySeverity, } from "../../../shared/types/remoteRuntime"; export const ADE_SYNC_MDNS_SERVICE_TYPE = "ade-sync"; @@ -411,22 +412,28 @@ async function discoverTailscalePeers(timeoutMs = 1_200): Promise<{ /ENOENT|not found|no such file/i.test(message); let code: string; let summary: string; + let severity: RemoteRuntimeDiscoverySeverity; if (timedOut) { code = "tailscale-timeout"; summary = "Tailscale discovery timed out; LAN discovery still ran."; + severity = "warning"; } else if (notFound) { + // Tailscale is optional. Not having it installed is a fact about the + // machine, not a problem with discovery, so it must not read as a warning. code = "tailscale-unavailable"; - summary = "Tailscale CLI was not found; only LAN discovery ran."; + summary = "Tailscale not installed — LAN discovery only."; + severity = "info"; } else { code = "tailscale-status-failed"; summary = "Tailscale discovery failed; LAN discovery still ran."; + severity = "warning"; } return { machines: [], diagnostics: [ { source: "tailscale", - severity: "warning", + severity, code, message: summary, detail: message || null, diff --git a/apps/desktop/src/main/services/runtime/projectRecoveryService.test.ts b/apps/desktop/src/main/services/runtime/projectRecoveryService.test.ts index 956d37623..cf81d77ed 100644 --- a/apps/desktop/src/main/services/runtime/projectRecoveryService.test.ts +++ b/apps/desktop/src/main/services/runtime/projectRecoveryService.test.ts @@ -3,6 +3,7 @@ import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; import { expectNoJargon } from "../../../test/jargonGuard"; +import { LOCAL_RELEASE_BUILD_OUTPUT_RUNTIME_MESSAGE } from "../../../shared/runtimeErrors"; import type { AdeLastFailureReport, AdeRecoveryErrorCode } from "../../../shared/types/recovery"; import type { Logger } from "../logging/logger"; import { @@ -305,3 +306,182 @@ describe("ProjectRecoveryService.repair", () => { expect(report.nextAction).toContain("nothing has been deleted"); }); }); + +describe("ProjectRecoveryService.restartBrain", () => { + const installedPool = () => + pool(status({ + serviceInstall: { + state: "installed", + attempted: true, + path: "com.ade.runtime", + message: "ADE service login item is installed.", + exitCode: 0, + updatedAt: null, + }, + })); + + it("forces the service install and resolves once the brain answers a ping", async () => { + const connectionPool = installedPool(); + const service = createProjectRecoveryService(deps({ connectionPool })); + + await expect(service.restartBrain()).resolves.toBeUndefined(); + + expect(connectionPool.installServiceBestEffort).toHaveBeenCalledWith({ forceRestart: true }); + // The ping is explicitly bounded: the RPC client's default is 10 minutes, + // which would park this call — and any repair waiting on it — on a brain + // that binds the socket but never answers. + expect(connectionPool.callSync).toHaveBeenCalledWith("ping", {}, { timeoutMs: 20_000 }); + }); + + const installStatusPool = ( + state: "skipped" | "failed", + message: string, + ): ProjectRecoveryConnectionPool => + // installServiceBestEffort never rejects and can resolve having done + // nothing, so a silent no-op has to be caught from the status. + pool(status({ + serviceInstall: { + state, + attempted: state === "failed", + path: null, + message, + exitCode: null, + updatedAt: null, + }, + })); + + it("throws the installer's own message when the install fails", async () => { + const connectionPool = installStatusPool("failed", "launchctl bootstrap failed"); + const service = createProjectRecoveryService(deps({ connectionPool })); + + await expect(service.restartBrain()).rejects.toThrow("launchctl bootstrap failed"); + expect(connectionPool.callSync).not.toHaveBeenCalled(); + }); + + it("explains a deliberate skip in user copy instead of the installer's log line", async () => { + // Forcing past the skip would downgrade a newer running brain, so the + // remedy is to relaunch ADE — and the log line never becomes the sentence. + const connectionPool = installStatusPool( + "skipped", + "Skipped ADE service install because a newer ADE brain is already running.", + ); + const service = createProjectRecoveryService(deps({ connectionPool })); + + const rejection = await service.restartBrain().then( + () => new Error("restartBrain resolved instead of rejecting."), + (reason: unknown) => reason as Error, + ); + + expect(rejection.message).toBe( + "A newer ADE runtime is already running — quit and reopen ADE instead.", + ); + expect(connectionPool.callSync).not.toHaveBeenCalled(); + }); + + it("keeps the release-build block's own instructions, which already read as copy", async () => { + const connectionPool = installStatusPool("skipped", LOCAL_RELEASE_BUILD_OUTPUT_RUNTIME_MESSAGE); + const service = createProjectRecoveryService(deps({ connectionPool })); + + await expect(service.restartBrain()).rejects.toThrow(LOCAL_RELEASE_BUILD_OUTPUT_RUNTIME_MESSAGE); + }); + + it("throws when the replacement brain never rebinds the socket", async () => { + const connectionPool = installedPool(); + const service = createProjectRecoveryService(deps({ + connectionPool, + waitForSocketState: vi.fn(async () => false), + })); + + await expect(service.restartBrain()).rejects.toThrow("did not come back"); + expect(connectionPool.callSync).not.toHaveBeenCalled(); + }); + + it("throws when the restarted brain does not answer the ping", async () => { + const connectionPool = installedPool(); + vi.mocked(connectionPool.callSync).mockRejectedValue(new Error("socket closed")); + const service = createProjectRecoveryService(deps({ connectionPool })); + + await expect(service.restartBrain()).rejects.toThrow("socket closed"); + }); + + it("refuses to reinstall the brain while a repair owns the database, and works again after", async () => { + // repair() stops the service and then does exclusive database work; a + // renderer-triggered restart landing mid-window would put a writer back. + const connectionPool = installedPool(); + let reachedDatabaseWork = (): void => {}; + let releaseDatabaseWork = (): void => {}; + const databaseWorkStarted = new Promise((resolve) => { reachedDatabaseWork = resolve; }); + const databaseWorkBlocked = new Promise((resolve) => { releaseDatabaseWork = resolve; }); + const service = createProjectRecoveryService(deps({ + connectionPool, + quickCheck: vi.fn(async () => { + reachedDatabaseWork(); + await databaseWorkBlocked; + return { healthy: true, detail: "ok" }; + }), + })); + + const repair = service.repair(tempRoot()); + await databaseWorkStarted; + + await expect(service.restartBrain()).rejects.toThrow("Recovery is already running."); + expect(connectionPool.installServiceBestEffort).not.toHaveBeenCalled(); + + releaseDatabaseWork(); + await expect(repair).resolves.toMatchObject({ ok: true }); + await expect(service.restartBrain()).resolves.toBeUndefined(); + expect(connectionPool.installServiceBestEffort).toHaveBeenCalledWith({ forceRestart: true }); + }); + + it("waits for an in-flight restart before repair takes the database", async () => { + // The other direction of the same invariant: a restart that began before + // repair() was called must finish its install before repair stops the + // service, or the queued install rebinds a brain during exclusive DB work. + const connectionPool = installedPool(); + let releaseInstall = (): void => {}; + let installStarted = (): void => {}; + const installBlocked = new Promise((resolve) => { releaseInstall = resolve; }); + const installRunning = new Promise((resolve) => { installStarted = resolve; }); + vi.mocked(connectionPool.installServiceBestEffort).mockImplementation(async () => { + installStarted(); + await installBlocked; + }); + const quickCheck = vi.fn(async () => ({ healthy: true, detail: "ok" })); + const service = createProjectRecoveryService(deps({ connectionPool, quickCheck })); + + const restart = service.restartBrain(); + await installRunning; + const repair = service.repair(tempRoot()); + // Repair must be parked on the restart, not already owning the database. + for (let i = 0; i < 10; i += 1) await Promise.resolve(); + expect(quickCheck).not.toHaveBeenCalled(); + + releaseInstall(); + await expect(restart).resolves.toBeUndefined(); + await expect(repair).resolves.toMatchObject({ ok: true }); + expect(quickCheck).toHaveBeenCalled(); + }); + + it("still runs repair when the in-flight restart it waited on failed", async () => { + // Recovery is the user's last resort: a rejected restart must not take the + // repair run down with it. + const connectionPool = installStatusPool("failed", "launchctl bootstrap failed"); + let releaseInstall = (): void => {}; + let installStarted = (): void => {}; + const installBlocked = new Promise((resolve) => { releaseInstall = resolve; }); + const installRunning = new Promise((resolve) => { installStarted = resolve; }); + vi.mocked(connectionPool.installServiceBestEffort).mockImplementation(async () => { + installStarted(); + await installBlocked; + }); + const service = createProjectRecoveryService(deps({ connectionPool })); + + const restart = service.restartBrain(); + await installRunning; + const repair = service.repair(tempRoot()); + releaseInstall(); + + await expect(restart).rejects.toThrow("launchctl bootstrap failed"); + await expect(repair).resolves.toMatchObject({ ok: true }); + }); +}); diff --git a/apps/desktop/src/main/services/runtime/projectRecoveryService.ts b/apps/desktop/src/main/services/runtime/projectRecoveryService.ts index b85dedf8a..60fd3f31f 100644 --- a/apps/desktop/src/main/services/runtime/projectRecoveryService.ts +++ b/apps/desktop/src/main/services/runtime/projectRecoveryService.ts @@ -1,7 +1,6 @@ import fs from "node:fs"; import net from "node:net"; import path from "node:path"; -import type { LocalRuntimeStatus } from "../../../shared/types"; import type { AdeLastFailureReport, AdeRecoveryErrorCode, @@ -22,11 +21,15 @@ import { runQuickCheck, } from "../state/kvDb"; import { readVolumeSpace } from "../storage/volume"; +import { isLocalReleaseBuildOutputError } from "../../../shared/runtimeErrors"; import { clearLastFailure, readLastFailure } from "./lastFailureStore"; const MIB = 1024 * 1024; const GIB = 1024 * MIB; const FRESH_FAILURE_MS = 5 * 60 * 1_000; +// How long a restarted brain gets to rebind the machine endpoint, shared by +// `repair()`'s restart_service step and `restartBrain()`. +const BRAIN_RESTART_TIMEOUT_MS = 20_000; const STEP_LABELS: Record = { check_space: "Checking storage space", @@ -58,6 +61,27 @@ type SpaceStats = { bavail: number | bigint; bsize: number | bigint }; type QuickCheckResult = { healthy: boolean | null; detail: string }; type ChatCounts = { total: number; needingAttention: number }; +/** + * Which stage of the shared brain-restart sequence lost, if any. The sequence + * reports the stage rather than the copy, because its two callers say + * different things about the same stage: `repair()` speaks in repair steps and + * `restartBrain()` throws. + */ +type BrainRestartOutcome = + | { ok: true } + /** The endpoint never came back inside the restart budget. */ + | { ok: false; reason: "unreachable" } + | { + ok: false; + /** + * "install_skipped" is its own reason because the installer declined on + * purpose — nothing restarted and nothing is broken — so it needs copy of + * its own rather than the installer's log line. + */ + reason: "restart_error" | "install_skipped" | "ping_error"; + detail: string; + }; + export type ProjectRecoveryConnectionPool = Pick< LocalRuntimeConnectionPool, "getStatus" | "installServiceBestEffort" | "uninstallServiceBestEffort" | "callSync" | "ensureProject" | "callActionForRoot" @@ -194,6 +218,21 @@ function humanGb(bytes: number): string { return `${gb} GB`; } +/** + * User-facing copy for a restart the installer deliberately declined to run. + * + * The skips exist to protect a newer, protocol-compatible brain that is + * already running — forcing past them would downgrade it — so the honest + * remedy is to relaunch ADE, not to retry. The installer's own message is a + * log line ("Skipped ADE service install because…") and never becomes the + * sentence a person reads; the release-build block is the exception, since it + * is already written as instructions. + */ +function skippedRestartCopy(installerMessage: string): string { + if (isLocalReleaseBuildOutputError(installerMessage)) return installerMessage; + return "A newer ADE runtime is already running — quit and reopen ADE instead."; +} + function socketConnectOptions(socketPath: string): net.NetConnectOpts { if (!socketPath.startsWith("tcp://")) return { path: socketPath }; const url = new URL(socketPath); @@ -326,6 +365,16 @@ export class ProjectRecoveryService { private readonly readChatCounts: (projectRoot: string) => Promise; private readonly socketExists: (socketPath: string) => boolean; private readonly now: () => number; + /** Set for the whole of `repair()`; see `restartBrain()` for why. */ + private repairInFlight = false; + /** + * Settled-either-way handle on an in-flight `restartBrain()`. The flag above + * only closes one direction (a restart started after repair began); a repair + * started *during* a restart must wait for the install to finish, or that + * install can bind a replacement brain to the socket while repair owns the + * database. + */ + private restartInFlight: Promise | null = null; constructor(private readonly deps: ProjectRecoveryServiceDeps) { this.socketPath = deps.socketPath ?? resolveMachineAdeLayout({ ...process.env, ADE_HOME: deps.adeHome }).socketPath; @@ -367,6 +416,82 @@ export class ProjectRecoveryService { return { free: Math.min(freeBytes(projectStats), freeBytes(homeStats)), dbSize }; } + /** + * Install the service, wait for the machine endpoint to come back, then ping + * it — the one verified brain-restart sequence, shared by `repair()`'s + * restart_service/verify_endpoint steps and by `restartBrain()`. + * + * `force` is more than the install flag. A forced restart is the only caller + * that actually asked for a restart, so an install that resolves having + * skipped is a failure for it; `repair()` tolerates a skip, because a + * protocol-compatible brain that is already running satisfies its step. + */ + private async restartServiceAndWait(force: boolean): Promise { + try { + await this.deps.connectionPool.installServiceBestEffort(force ? { forceRestart: true } : {}); + if (force) { + // `installServiceBestEffort` never rejects and can resolve having + // skipped the install entirely, hence the status check. + const install = this.deps.connectionPool.getStatus().serviceInstall; + if (install.state !== "installed") { + return { + ok: false, + reason: install.state === "skipped" ? "install_skipped" : "restart_error", + detail: install.message?.trim() ?? "", + }; + } + } + if (!await this.waitForSocketState(this.socketPath, true, BRAIN_RESTART_TIMEOUT_MS)) { + return { ok: false, reason: "unreachable" }; + } + } catch (error) { + return { ok: false, reason: "restart_error", detail: errorMessage(error) }; + } + try { + // Bound the ping: RuntimeRpcClient's default is 10 minutes, and a brain + // that binds the socket but never answers would otherwise park both this + // call and any `repair()` waiting on it for that whole window. + await this.deps.connectionPool.callSync("ping", {}, { timeoutMs: BRAIN_RESTART_TIMEOUT_MS }); + } catch (error) { + return { ok: false, reason: "ping_error", detail: errorMessage(error) }; + } + return { ok: true }; + } + + /** + * Machine-scoped brain restart for the "Repair" button. Only the main + * process can observe when the replacement brain is actually answering, so + * the renderer awaits this instead of sleeping and hoping. + * + * Throws so the caller's error path fires, and rejects outright while a + * `repair()` is running: repair stops the service and then does exclusive + * database work, and reinstalling the brain underneath it would put a writer + * back on the database mid-check (see the invariant on + * `uninstallServiceBestEffort`). Repair wins; the button can be pressed again + * after it finishes. + */ + async restartBrain(): Promise { + if (this.repairInFlight) throw new Error("Recovery is already running."); + const restart = this.restartServiceAndWait(true); + // Never-rejecting handle so `repair()` can await it without inheriting a + // restart failure; the real outcome is still thrown to this caller below. + const settled = restart.then(() => undefined, () => undefined); + this.restartInFlight = settled; + void settled.then(() => { + if (this.restartInFlight === settled) this.restartInFlight = null; + }); + const outcome = await restart; + if (outcome.ok) return; + switch (outcome.reason) { + case "install_skipped": + throw new Error(skippedRestartCopy(outcome.detail)); + case "unreachable": + throw new Error("The background service did not come back after the restart."); + default: + throw new Error(outcome.detail.trim() || "ADE could not restart its background service."); + } + } + async diagnose(projectRoot: string): Promise { const normalizedRoot = path.resolve(projectRoot); const dbPath = path.join(normalizedRoot, ".ade", "ade.db"); @@ -444,6 +569,32 @@ export class ProjectRecoveryService { async repair( projectRoot: string, opts: { onStep?: (step: RepairStepResult) => void } = {}, + ): Promise { + // Held across the whole run so a renderer-triggered `restartBrain()` + // cannot reinstall the brain while the steps below own the database. + this.repairInFlight = true; + try { + // A restart that started before the flag was set is not covered by it: + // let its install finish before repair stops the service and takes the + // database. Never fails repair — this is the user's last resort, so the + // wait is bounded and repair proceeds regardless of how the restart ends. + // Repair's own stop_service step is what actually guarantees exclusivity; + // this wait only avoids racing an install that is already underway. + if (this.restartInFlight) { + await Promise.race([ + this.restartInFlight, + new Promise((resolve) => setTimeout(resolve, BRAIN_RESTART_TIMEOUT_MS)), + ]); + } + return await this.runRepair(projectRoot, opts); + } finally { + this.repairInFlight = false; + } + } + + private async runRepair( + projectRoot: string, + opts: { onStep?: (step: RepairStepResult) => void }, ): Promise { const normalizedRoot = path.resolve(projectRoot); const dbPath = path.join(normalizedRoot, ".ade", "ade.db"); @@ -570,29 +721,23 @@ export class ProjectRecoveryService { return fail("resolve_migrations", failureCode, nextAction, errorMessage(error)); } - let restarted = false; - try { - await this.deps.connectionPool.installServiceBestEffort(); - restarted = await this.waitForSocketState(this.socketPath, true, 20_000); - } catch (error) { - return fail("restart_service", "brain_not_installed", "Restart ADE, then run repair again.", errorMessage(error)); - } - if (!restarted) { - return fail( - "restart_service", - "brain_crash_looping", - "Restart ADE, then run repair again. If the service still stops, contact support.", - "The background service did not become reachable within 20 seconds.", - ); + const restart = await this.restartServiceAndWait(false); + if (!restart.ok && restart.reason !== "ping_error") { + return restart.reason === "unreachable" + ? fail( + "restart_service", + "brain_crash_looping", + "Restart ADE, then run repair again. If the service still stops, contact support.", + `The background service did not become reachable within ${Math.round(BRAIN_RESTART_TIMEOUT_MS / 1_000)} seconds.`, + ) + : fail("restart_service", "brain_not_installed", "Restart ADE, then run repair again.", restart.detail); } addStep("restart_service", "ok", "The background service restarted."); - try { - await this.deps.connectionPool.callSync("ping", {}); - addStep("verify_endpoint", "ok", "The background service answered."); - } catch (error) { - return fail("verify_endpoint", "brain_crash_looping", "Restart ADE, then run repair again.", errorMessage(error)); + if (!restart.ok) { + return fail("verify_endpoint", "brain_crash_looping", "Restart ADE, then run repair again.", restart.detail); } + addStep("verify_endpoint", "ok", "The background service answered."); try { await this.deps.connectionPool.ensureProject(normalizedRoot); diff --git a/apps/desktop/src/preload/global.d.ts b/apps/desktop/src/preload/global.d.ts index ca6b0d69d..3d645989c 100644 --- a/apps/desktop/src/preload/global.d.ts +++ b/apps/desktop/src/preload/global.d.ts @@ -733,6 +733,13 @@ declare global { ) => () => void; getResourceUsage: () => Promise; getRuntimeHealth: () => Promise; + /** + * Restarts this Mac's ADE brain (com.ade.runtime) and resolves once the + * replacement answers a ping; rejects when it does not come back. + * Native desktop only — the hosted-web adapter and browser mock cannot + * touch a launch agent, so callers must feature-detect before offering it. + */ + restartBackgroundService?: () => Promise; getLatestRelease: () => Promise; getProject: () => Promise; getWindowSession: () => Promise<{ diff --git a/apps/desktop/src/preload/preload.ts b/apps/desktop/src/preload/preload.ts index 388bfdfdd..d37fb6871 100644 --- a/apps/desktop/src/preload/preload.ts +++ b/apps/desktop/src/preload/preload.ts @@ -3500,6 +3500,8 @@ contextBridge.exposeInMainWorld("ade", { ipcRenderer.invoke(IPC.appGetResourceUsage), getRuntimeHealth: async (): Promise => ipcRenderer.invoke(IPC.appGetRuntimeHealth), + restartBackgroundService: async (): Promise => + ipcRenderer.invoke(IPC.appRestartBackgroundService), getLatestRelease: async (): Promise => ipcRenderer.invoke(IPC.appGetLatestRelease), getProject: async (): Promise => diff --git a/apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsx b/apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsx index a182a85a4..5bf59b481 100644 --- a/apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsx +++ b/apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsx @@ -44,6 +44,8 @@ const lanesMock = { const appMock = { writeClipboardText: vi.fn(), + getInfo: vi.fn(), + restartBackgroundService: vi.fn(), }; const accountMock = { @@ -63,6 +65,7 @@ function installAdeMock(): void { relayAvailable: false, }); remoteRuntimeMock.runDoctor.mockResolvedValue({ checks: [] }); + appMock.getInfo.mockResolvedValue({ localRuntime: null }); accountMock.getLocalMachineIdentity.mockResolvedValue({ machineKey: "local-mk", deviceId: "local-dev" }); accountMock.onPairMachineProgress.mockReturnValue(() => {}); accountMock.renameMachine.mockImplementation(async (machineKey: string, customName: string | null) => ({ @@ -122,6 +125,47 @@ describe("RemoteTargetList", () => { Reflect.deleteProperty(window, "ade"); }); + it("offers Repair on the publish-failing banner only for an unreadable brain session", async () => { + remoteRuntimeMock.listTargets.mockResolvedValue([]); + remoteRuntimeMock.listDiscoveredMachines.mockResolvedValue({ machines: [], diagnostics: [] }); + installAdeMock(); + appMock.restartBackgroundService.mockResolvedValue(undefined); + const publishHealth = { + state: "token_unreadable", + failingSinceMs: Date.now() - 5 * 60_000, + lastLegDurations: { snapshot: null, token: null, http: null }, + }; + appMock.getInfo.mockResolvedValue({ localRuntime: { publishHealth } }); + + render(); + const repair = await screen.findByRole("button", { name: "Repair" }); + + // The brain comes back healthy, so the banner and its button disappear. + appMock.getInfo.mockResolvedValue({ localRuntime: null }); + fireEvent.click(repair); + await waitFor(() => expect(screen.queryByRole("button", { name: "Repair" })).toBeNull()); + expect(appMock.restartBackgroundService).toHaveBeenCalledTimes(1); + }); + + it("leaves the publish-failing banner unrepairable when a restart cannot help", async () => { + remoteRuntimeMock.listTargets.mockResolvedValue([]); + remoteRuntimeMock.listDiscoveredMachines.mockResolvedValue({ machines: [], diagnostics: [] }); + installAdeMock(); + appMock.getInfo.mockResolvedValue({ + localRuntime: { + publishHealth: { + state: "http_error", + failingSinceMs: Date.now() - 5 * 60_000, + lastLegDurations: { snapshot: null, token: null, http: null }, + }, + }, + }); + + render(); + expect(await screen.findByText(/route publish failing for 5 min/)).toBeTruthy(); + expect(screen.queryByRole("button", { name: "Repair" })).toBeNull(); + }); + it("pairs a discovered ADE machine with its 6-digit code instead of creating an SSH target", async () => { remoteRuntimeMock.listTargets.mockResolvedValue([]); remoteRuntimeMock.listDiscoveredMachines.mockResolvedValue({ @@ -1115,16 +1159,16 @@ describe("RemoteTargetList", () => { expect(screen.queryByText(/token=secret/)).toBeNull(); }); - it("surfaces Tailscale discovery diagnostics separately from empty results", async () => { + it("renders an info discovery diagnostic as muted text without a warning glyph", async () => { remoteRuntimeMock.listTargets.mockResolvedValue([]); remoteRuntimeMock.listDiscoveredMachines.mockResolvedValue({ machines: [], diagnostics: [ { source: "tailscale", - severity: "warning", + severity: "info", code: "tailscale-unavailable", - message: "Tailscale CLI was not found; only LAN discovery ran.", + message: "Tailscale not installed — LAN discovery only.", detail: "ENOENT", }, ], @@ -1133,13 +1177,36 @@ describe("RemoteTargetList", () => { render(); - await waitFor(() => - expect( - screen.getByText( - "Tailscale CLI was not found; only LAN discovery ran.", - ), - ).toBeTruthy(), + const note = await waitFor(() => + screen.getByText("Tailscale not installed — LAN discovery only."), + ); + // Not having optional software installed must not wear the warning glyph. + expect(note.querySelector("svg")).toBeNull(); + expect(screen.getByText("No computers yet. Choose Add machine to connect one.")).toBeTruthy(); + }); + + it("surfaces Tailscale discovery warnings separately from empty results", async () => { + remoteRuntimeMock.listTargets.mockResolvedValue([]); + remoteRuntimeMock.listDiscoveredMachines.mockResolvedValue({ + machines: [], + diagnostics: [ + { + source: "tailscale", + severity: "warning", + code: "tailscale-status-failed", + message: "Tailscale discovery failed; LAN discovery still ran.", + detail: "boom", + }, + ], + }); + installAdeMock(); + + render(); + + const warning = await waitFor(() => + screen.getByText("Tailscale discovery failed; LAN discovery still ran."), ); + expect(warning.querySelector("svg")).not.toBeNull(); expect(screen.getByText("No computers yet. Choose Add machine to connect one.")).toBeTruthy(); }); diff --git a/apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsx b/apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsx index 67144c81a..e6fdaee2b 100644 --- a/apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsx +++ b/apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsx @@ -9,6 +9,8 @@ import { WifiHigh, } from "@phosphor-icons/react"; import { extractError } from "../../lib/format"; +import { useBrainRepair } from "../../hooks/useBrainRepair"; +import { BrainRepairButton } from "../settings/BrainRepairButton"; import { COLORS, MONO_FONT, @@ -16,6 +18,7 @@ import { outlineButton, primaryButton, } from "../lanes/laneDesignTokens"; +import { isBrainAccountSessionFailure } from "../../../shared/types"; import type { AdeAccountMachine, AdeAccountMachinesResult, @@ -23,6 +26,8 @@ import type { RemoteRuntimeConnectionStatus, RemoteRuntimeConnectResult, RemoteRuntimeDiscoveredMachine, + RemoteRuntimeDiscoveryDiagnostic, + RemoteRuntimeDiscoverySeverity, RemoteRuntimeSshHostKeyTrustStatus, RemoteRuntimeTarget, RemoteRuntimeTargetInput, @@ -130,6 +135,16 @@ function targetFormPrefill( }; } +function joinDiagnosticMessages( + diagnostics: readonly RemoteRuntimeDiscoveryDiagnostic[], + severity: RemoteRuntimeDiscoverySeverity, +): string { + return diagnostics + .filter((entry) => entry.severity === severity) + .map((entry) => entry.message) + .join(" "); +} + const SECTION_LABELS: Record = { connected: "CONNECTED", available: "AVAILABLE", @@ -173,7 +188,14 @@ export function RemoteTargetList({ const [formPrefill, setFormPrefill] = useState(null); const [error, setError] = useState(null); - const [discoveryError, setDiscoveryError] = useState(null); + // One source of truth for what discovery reported; the warning and info lines + // are derived, so they can never drift out of sync with each other. + const [discoveryDiagnostics, setDiscoveryDiagnostics] = useState< + readonly RemoteRuntimeDiscoveryDiagnostic[] + >([]); + // A failed `listDiscoveredMachines` call is not a diagnostic the discovery + // service produced, so it stays its own string rather than a synthetic entry. + const [discoveryFetchError, setDiscoveryFetchError] = useState(null); const [hostKeyTrust, setHostKeyTrust] = useState(null); const [addMode, setAddMode] = useState(null); @@ -379,18 +401,28 @@ export function RemoteTargetList({ try { const next = await window.ade.remoteRuntime.listDiscoveredMachines(); setDiscoveredMachines(next.machines); - setDiscoveryError( - next.diagnostics.length > 0 - ? next.diagnostics.map((entry) => entry.message).join(" ") - : null, - ); + setDiscoveryDiagnostics(next.diagnostics); + setDiscoveryFetchError(null); } catch (err) { - setDiscoveryError(extractError(err)); + setDiscoveryDiagnostics([]); + setDiscoveryFetchError(extractError(err)); } finally { setLoadingDiscovered(false); } }, []); + // Warnings mean discovery is degraded and get the warning treatment. Info + // diagnostics ("Tailscale isn't installed") are normal on a plain Mac, so they + // render as muted secondary text with no warning glyph. + const discoveryError = useMemo( + () => discoveryFetchError ?? (joinDiagnosticMessages(discoveryDiagnostics, "warning") || null), + [discoveryDiagnostics, discoveryFetchError], + ); + const discoveryNote = useMemo( + () => joinDiagnosticMessages(discoveryDiagnostics, "info") || null, + [discoveryDiagnostics], + ); + useEffect(() => { void loadDiscoveredMachines(); }, [loadDiscoveredMachines]); @@ -429,36 +461,50 @@ export function RemoteTargetList({ // This computer's route-publish health, refreshed periodically so a persisting // failure's "for N min" stays truthful while the panel is open. getInfo is a // cheap one-shot; there is no push event for the publisher's health. + const publishHealthMountedRef = useRef(true); + // The interval and the post-repair refresh can overlap; without a generation + // an older in-flight read can land last and restore the failing banner the + // newer read already cleared. + const publishHealthRequestRef = useRef(0); + const refreshPublishHealth = useCallback(() => { + const infoPromise = window.ade.app?.getInfo?.(); + if (!infoPromise) return; + const requestId = ++publishHealthRequestRef.current; + void infoPromise + .then((info) => { + if (!publishHealthMountedRef.current) return; + if (requestId !== publishHealthRequestRef.current) return; + const health = info.localRuntime?.publishHealth ?? null; + setLocalPublishHealth( + health + ? { state: health.state, failingSinceMs: health.failingSinceMs } + : null, + ); + }) + .catch(() => {}); + }, []); + useEffect(() => { - let cancelled = false; - const refresh = () => { - const infoPromise = window.ade.app?.getInfo?.(); - if (!infoPromise) return; - void infoPromise - .then((info) => { - if (cancelled) return; - const health = info.localRuntime?.publishHealth ?? null; - setLocalPublishHealth( - health - ? { state: health.state, failingSinceMs: health.failingSinceMs } - : null, - ); - }) - .catch(() => {}); - }; - refresh(); - const timer = window.setInterval(refresh, 30_000); + publishHealthMountedRef.current = true; + refreshPublishHealth(); + const timer = window.setInterval(refreshPublishHealth, 30_000); return () => { - cancelled = true; + publishHealthMountedRef.current = false; window.clearInterval(timer); }; - }, []); + }, [refreshPublishHealth]); const publishHealthDisplay = useMemo( () => describePublishHealth(localPublishHealth), // Re-derive on each fetch; the 30s refresh advances the "for N min" count. [localPublishHealth], ); + // Same brain-side unreadable-session failure the Connections card repairs; + // both surfaces read the one publisher health record and share the handler. + const repair = useBrainRepair(refreshPublishHealth); + const showRepair = publishHealthDisplay.kind === "failing" + && isBrainAccountSessionFailure(localPublishHealth?.state) + && repair.available; const openAddMachine = useCallback(() => { setSelectedId(null); @@ -1099,6 +1145,7 @@ export function RemoteTargetList({ display: "flex", alignItems: "center", gap: 5, + flexWrap: "wrap", color: COLORS.warning, fontFamily: SANS_FONT, fontSize: 11, @@ -1110,6 +1157,7 @@ export function RemoteTargetList({ Other devices may not reach this computer — route publish failing for{" "} {publishHealthDisplay.minutes} min + {showRepair ? : null} ) : null} @@ -1252,6 +1300,8 @@ export function RemoteTargetList({ ) : null} + {discoveryNote ?
{discoveryNote}
: null} + {loading ? (
+ + {repair.error ? ( + + Repair failed — quit and reopen ADE. + + ) : null} + + ); +} diff --git a/apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx b/apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx index e726af0d0..74b899fec 100644 --- a/apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx +++ b/apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx @@ -24,6 +24,7 @@ import { useSyncConnections, type SyncConnections, } from "./SyncDevicesSection"; +import { accountDirectorySummary } from "./accountDirectorySummary"; vi.mock("qrcode.react", () => ({ QRCodeSVG: ({ value, title }: { value: string; title?: string }) => ( @@ -135,10 +136,22 @@ function makeSync(overrides: Partial = {}): SyncConnections { saveRuntimeName: vi.fn(), forgetDevice: vi.fn(), retryInitialLoad: vi.fn(), + refresh: vi.fn(async () => {}), ...overrides, } as SyncConnections; } +function unreadableSessionStatus(): SyncRoleSnapshot { + const status = makeStatus(); + status.routeHealth.accountDirectory = { + ...status.routeHealth.accountDirectory, + state: "token_unreadable", + skipReason: "The ADE brain could not read the stored account session.", + reachableEndpointCount: 0, + }; + return status; +} + const autoConfirm = async () => true; describe("ThisMacCard", () => { @@ -187,6 +200,67 @@ describe("ThisMacCard", () => { expect(screen.queryByRole("button", { name: /Remove/i })).toBeNull(); }); + it("restarts the brain and re-reads health when repairing an unreadable session", async () => { + // The main process resolves only once the replacement brain answers, so the + // hook re-reads health the moment the call settles — no renderer-side sleep. + let releaseRestart = () => {}; + const restartBackgroundService = vi.fn( + () => new Promise((resolve) => { + releaseRestart = resolve; + }), + ); + const refresh = vi.fn(async () => {}); + (globalThis.window as any).ade = { app: { restartBackgroundService } }; + render( + , + ); + + const button = screen.getByRole("button", { name: "Repair" }); + fireEvent.click(button); + // Re-entrancy: a second click while the restart is in flight is dropped. + fireEvent.click(button); + expect(screen.getByRole("button", { name: "Repairing…" })).toBeTruthy(); + await act(async () => {}); + expect(restartBackgroundService).toHaveBeenCalledTimes(1); + expect(refresh).not.toHaveBeenCalled(); + + await act(async () => { + releaseRestart(); + }); + expect(refresh).toHaveBeenCalledTimes(1); + // Health still says unreadable, so the banner and its button stay put. + expect(screen.getByRole("button", { name: "Repair" })).toBeTruthy(); + }); + + it("shows a terse inline error and keeps Repair available when the restart fails", async () => { + const restartBackgroundService = vi.fn(async () => { + throw new Error("launchctl load failed."); + }); + (globalThis.window as any).ade = { app: { restartBackgroundService } }; + render(); + + fireEvent.click(screen.getByRole("button", { name: "Repair" })); + const failure = await screen.findByText("Repair failed — quit and reopen ADE."); + // Terse copy on screen; the technical detail rides along as the tooltip. + expect(failure.getAttribute("title")).toBe("launchctl load failed."); + expect(screen.getByRole("button", { name: "Repair" })).toBeTruthy(); + }); + + it("offers no repair for failures a brain restart cannot fix", () => { + (globalThis.window as any).ade = { app: { restartBackgroundService: vi.fn() } }; + const status = makeStatus(); + status.routeHealth.accountDirectory = { + ...status.routeHealth.accountDirectory, + state: "http_error", + skipReason: "The account directory rejected the publish.", + }; + render(); + expect(screen.queryByRole("button", { name: "Repair" })).toBeNull(); + }); + it("explains nearby fallback when signed out", () => { render(); expect( @@ -614,3 +688,47 @@ describe("useSyncConnections local scoping", () => { expect(result.current.canManageDevices).toBe(false); }); }); + +describe("accountDirectorySummary", () => { + it("reflects whether signed-out nearby pairing has a configured code", () => { + const status = { pairingPinConfigured: false } as SyncRoleSnapshot; + + expect(accountDirectorySummary(status, false)).toEqual({ + label: "Not signed in — set a pairing code so nearby devices can connect", + healthy: false, + }); + + status.pairingPinConfigured = true; + expect(accountDirectorySummary(status, false).label).toContain( + "nearby devices can still connect with the pairing code", + ); + }); + + it("names the publish failure reason instead of a bare state", () => { + const withState = ( + state: SyncRoleSnapshot["routeHealth"]["accountDirectory"]["state"], + skipReason: string | null, + ) => + accountDirectorySummary( + { + routeHealth: { + accountDirectory: { state, skipReason, reachableEndpointCount: 0 }, + }, + } as SyncRoleSnapshot, + true, + ); + + expect( + withState("token_unreadable", "The ADE brain could not read the stored account session."), + ).toEqual({ + label: + "Signed in, but this computer is not published · The ADE brain could not read the stored account session.", + healthy: false, + }); + + // No reason from the brain: the state itself is spelled out, not snake_case. + expect(withState("http_error", null).label).toBe( + "Signed in, but this computer is not published · http error", + ); + }); +}); diff --git a/apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx b/apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx index 3aeef45af..6656fbc28 100644 --- a/apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx +++ b/apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx @@ -10,6 +10,7 @@ import { import { accountDirectorySummary } from "./accountDirectorySummary"; import { QRCodeSVG } from "qrcode.react"; import { createPortal } from "react-dom"; +import { isBrainAccountSessionFailure } from "../../../shared/types"; import type { SyncDeviceRuntimeState, SyncPeerDeviceType, @@ -25,6 +26,8 @@ import { isProjectRegistrationRequiredError, } from "../../../shared/runtimeErrors"; import { openExternalUrl } from "../../lib/openExternal"; +import { useBrainRepair } from "../../hooks/useBrainRepair"; +import { BrainRepairButton } from "./BrainRepairButton"; import { useCopyToClipboard } from "../../hooks/useCopyToClipboard"; import { COLORS, @@ -190,6 +193,9 @@ export function ThisMacCard({ }) { const { status, busy, error, notice, isRemoteBound, boundMachineName } = sync; const appInfo = useAppInfoLine(); + // Restarting the brain is the fix when it cannot read the stored account + // session; re-read the snapshot once it settles so the banner clears. + const repair = useBrainRepair(sync.refresh); if (sync.loading) { return
Getting connection details…
; @@ -224,6 +230,11 @@ export function ThisMacCard({ const acceptsConnections = acceptsConnectionsState(status, host); const routeLabels = reachableRouteLabels(status); const directorySummary = accountDirectorySummary(status, accountSignedIn); + // A brain-side unreadable account session is the one directory failure a + // restart clears — same test RemoteTargetList runs on its publish health. + const showRepair = accountSignedIn + && isBrainAccountSessionFailure(status.routeHealth?.accountDirectory?.state) + && repair.available; return (
@@ -263,7 +274,7 @@ export function ThisMacCard({ {THIS_MACHINE_NAME}
-
+
{accountSignedIn ? ( {directorySummary.label} + {showRepair ? : null}
diff --git a/apps/desktop/src/renderer/components/settings/accountDirectorySummary.test.ts b/apps/desktop/src/renderer/components/settings/accountDirectorySummary.test.ts deleted file mode 100644 index dd1a4ce8a..000000000 --- a/apps/desktop/src/renderer/components/settings/accountDirectorySummary.test.ts +++ /dev/null @@ -1,19 +0,0 @@ -import { describe, expect, it } from "vitest"; -import type { SyncRoleSnapshot } from "../../../shared/types"; -import { accountDirectorySummary } from "./accountDirectorySummary"; - -describe("accountDirectorySummary", () => { - it("reflects whether signed-out nearby pairing has a configured code", () => { - const status = { pairingPinConfigured: false } as SyncRoleSnapshot; - - expect(accountDirectorySummary(status, false)).toEqual({ - label: "Not signed in — set a pairing code so nearby devices can connect", - healthy: false, - }); - - status.pairingPinConfigured = true; - expect(accountDirectorySummary(status, false).label).toContain( - "nearby devices can still connect with the pairing code", - ); - }); -}); diff --git a/apps/desktop/src/renderer/components/settings/accountDirectorySummary.ts b/apps/desktop/src/renderer/components/settings/accountDirectorySummary.ts index c51d9561c..fbe8f8816 100644 --- a/apps/desktop/src/renderer/components/settings/accountDirectorySummary.ts +++ b/apps/desktop/src/renderer/components/settings/accountDirectorySummary.ts @@ -1,9 +1,14 @@ import type { SyncRoleSnapshot } from "../../../shared/types"; +export type AccountDirectorySummary = { + label: string; + healthy: boolean; +}; + export function accountDirectorySummary( status: SyncRoleSnapshot, accountSignedIn: boolean, -): { label: string; healthy: boolean } { +): AccountDirectorySummary { if (!accountSignedIn) { return { label: status.pairingPinConfigured diff --git a/apps/desktop/src/renderer/components/settings/useSyncConnections.ts b/apps/desktop/src/renderer/components/settings/useSyncConnections.ts index edda6e757..6207b15e1 100644 --- a/apps/desktop/src/renderer/components/settings/useSyncConnections.ts +++ b/apps/desktop/src/renderer/components/settings/useSyncConnections.ts @@ -197,5 +197,7 @@ export function useSyncConnections() { saveRuntimeName, forgetDevice, retryInitialLoad, + /** One-shot re-read of the snapshots, without the initial-load spinner. */ + refresh, }; } diff --git a/apps/desktop/src/renderer/hooks/useBrainRepair.ts b/apps/desktop/src/renderer/hooks/useBrainRepair.ts new file mode 100644 index 000000000..98d89fdef --- /dev/null +++ b/apps/desktop/src/renderer/hooks/useBrainRepair.ts @@ -0,0 +1,61 @@ +import { useCallback, useState } from "react"; +import { extractError } from "../lib/format"; +import { useAsyncAction } from "./useAsyncAction"; + +export type BrainRepair = { + /** Restarts the brain; ignores clicks while a restart is in flight. */ + run: () => void; + pending: boolean; + /** False when this surface cannot restart a brain (hosted web, browser mock). */ + available: boolean; + /** Technical failure detail from the last attempt, or null. */ + error: string | null; +}; + +/** + * Restarts this Mac's ADE brain — the background `com.ade.runtime` service — + * and calls `onSettled` once the restart resolves, so the caller can re-read + * whatever health source it renders. + * + * This is the remediation for a brain that cannot decrypt the stored account + * session: the replacement process re-reads the keychain from scratch. The main + * process waits for the replacement to answer a ping before resolving — only it + * can observe brain readiness — so there is nothing to wait for here. A rejected + * restart is reported through `error`; the caller's own banner stays put either + * way, since `onSettled` runs on both paths. + */ +export function useBrainRepair(onSettled?: () => void): BrainRepair { + const [error, setError] = useState(null); + + const action = useCallback(async () => { + const invoke = window.ade?.app?.restartBackgroundService; + if (!invoke) throw new Error("Restarting the ADE background service is not available here."); + await invoke(); + }, []); + + const { run, pending } = useAsyncAction({ + action, + onSuccess: () => { + setError(null); + onSettled?.(); + }, + onError: (reason) => { + setError(extractError(reason)); + onSettled?.(); + }, + }); + + // Clear the previous failure as the retry starts, so the inline error never + // sits next to a spinner that has not failed yet. + const runRepair = useCallback(() => { + setError(null); + run(); + }, [run]); + + return { + run: runRepair, + pending, + available: typeof window.ade?.app?.restartBackgroundService === "function", + error, + }; +} diff --git a/apps/desktop/src/shared/ipc.ts b/apps/desktop/src/shared/ipc.ts index 86a0c3155..ba8243477 100644 --- a/apps/desktop/src/shared/ipc.ts +++ b/apps/desktop/src/shared/ipc.ts @@ -5,6 +5,7 @@ export const IPC = { appRuntimeStatusChanged: "ade.app.runtimeStatusChanged", appGetResourceUsage: "ade.app.getResourceUsage", appGetRuntimeHealth: "ade.app.getRuntimeHealth", + appRestartBackgroundService: "ade.app.restartBackgroundService", storageGetPressure: "ade.storage.getPressure", storageGetSnapshot: "ade.storage.getSnapshot", storageCompressNow: "ade.storage.compressNow", diff --git a/apps/desktop/src/shared/types/productAnalytics.ts b/apps/desktop/src/shared/types/productAnalytics.ts index 7b89bb19a..d4c8c47ca 100644 --- a/apps/desktop/src/shared/types/productAnalytics.ts +++ b/apps/desktop/src/shared/types/productAnalytics.ts @@ -21,6 +21,7 @@ export const PRODUCT_ANALYTICS_EVENTS = [ "ade_brain_recovered", "ade_publish_failing", "ade_relay_suppressed", + "ade_account_session_unreadable", ] as const; export type ProductAnalyticsEventName = (typeof PRODUCT_ANALYTICS_EVENTS)[number]; diff --git a/apps/desktop/src/shared/types/remoteRuntime.ts b/apps/desktop/src/shared/types/remoteRuntime.ts index 2fb29f8dc..3afc7e14f 100644 --- a/apps/desktop/src/shared/types/remoteRuntime.ts +++ b/apps/desktop/src/shared/types/remoteRuntime.ts @@ -77,9 +77,16 @@ export type RemoteRuntimeDiscoveredMachine = { lastSeenAt: number; }; +export type RemoteRuntimeDiscoverySeverity = "info" | "warning"; + export type RemoteRuntimeDiscoveryDiagnostic = { source: "bonjour" | "tailscale"; - severity: "warning"; + /** + * "info" is a normal, non-actionable observation about the environment (e.g. + * optional software like the Tailscale CLI simply isn't installed). "warning" + * means discovery is degraded in a way the user may want to look at. + */ + severity: RemoteRuntimeDiscoverySeverity; code: string; message: string; detail: string | null; diff --git a/apps/desktop/src/shared/types/sync.ts b/apps/desktop/src/shared/types/sync.ts index a75017a40..1d02498be 100644 --- a/apps/desktop/src/shared/types/sync.ts +++ b/apps/desktop/src/shared/types/sync.ts @@ -355,6 +355,22 @@ export type SyncAccountDirectoryState = | "timeout" | "transport_error"; +/** + * True for publish failures caused by the brain's own copy of the account + * session being unreadable on this Mac — it could not decrypt the stored + * session, could not read account status, or could not read/refresh the + * account token. A brain restart is the known remediation: the replacement + * process re-reads the keychain from scratch and can decrypt again. + * + * Deliberately narrow. Network, HTTP, and genuinely-signed-out states are not + * fixed by restarting, so offering a repair for them would be a lie. + */ +export function isBrainAccountSessionFailure( + state: SyncAccountDirectoryState | null | undefined, +): boolean { + return state === "token_unreadable"; +} + export type SyncAccountDirectoryLegDurations = { snapshot: number | null; token: number | null; diff --git a/apps/ios/ADE/Services/SyncService.swift b/apps/ios/ADE/Services/SyncService.swift index 024ee048f..8966d13da 100644 --- a/apps/ios/ADE/Services/SyncService.swift +++ b/apps/ios/ADE/Services/SyncService.swift @@ -22,11 +22,14 @@ enum WidgetReloadBridge { private let syncConnectLog = Logger(subsystem: "com.ade.sync", category: "connect") private let syncChatLog = Logger(subsystem: "com.ade.ios", category: "WorkChatSync") +/// Transport-level attachment to a machine. There is deliberately no separate +/// "hydrating"/"syncing" state: attachment is a fact the moment `hello_ok` is +/// applied, and per-domain hydration progress is carried by `SyncDomainStatus` +/// (`.syncingInitialData`, `.hydrating`) rather than by this enum. enum RemoteConnectionState: String { case disconnected case connecting case connected - case syncing case error /// True when the host is not reachable — either we never connected @@ -83,7 +86,7 @@ func syncConnectionHealth( return .disconnected case .connecting: return .connecting - case .connected, .syncing: + case .connected: return .connected case .error: return .unreachable @@ -2643,11 +2646,11 @@ func syncAccountMachineNavigationIsCurrent( activeHostIdentity: String?, connectionState: RemoteConnectionState ) -> Bool { - // `.syncing` is attached — it is what every connect path settles into before - // it reaches `.connected`. Demanding `.connected` exactly meant a link tapped - // mid-hydration decided we were on the wrong machine and re-paired to the - // machine we were already talking to, tearing down a healthy connection. - guard connectionState == .connected || connectionState == .syncing, + // Attachment — not hydration progress — decides whether a tapped link is + // already pointed at the machine we are talking to. `hello_ok` publishes + // `.connected` immediately, so a link tapped mid-hydration must not decide we + // are on the wrong machine and re-pair to the machine we already have. + guard connectionState == .connected, let targetDeviceId = targetDeviceId? .trimmingCharacters(in: .whitespacesAndNewlines), !targetDeviceId.isEmpty, @@ -3289,12 +3292,13 @@ final class SyncService: ObservableObject { } @Published private(set) var hostName: String? - /// Attached to a machine. `.syncing` counts: it is what every connect path - /// settles into before `.connected`, and treating it as "not attached" is - /// what made a link tapped mid-hydration re-pair to the machine we were - /// already talking to. + /// Attached to a machine — the single named success/attachment predicate for + /// the whole app. Every surface that asks "did we get on the machine?" must + /// go through this rather than re-deriving its own comparison, so that the + /// definition of attachment stays in one place if the state machine grows + /// again. Hydration progress is `SyncDomainStatus`, not this. var isAttached: Bool { - connectionState == .connected || connectionState == .syncing + connectionState == .connected } /// Human-facing name of the connected machine, or a neutral "your computer" @@ -4073,7 +4077,7 @@ final class SyncService: ObservableObject { return } - guard connectionState != .connected && connectionState != .syncing else { + guard connectionState != .connected else { lastError = "This machine connection does not support project switching. Reconnect to a current ADE machine before opening another project." setDomainStatus(SyncDomain.allCases, phase: .failed, error: lastError) return @@ -4084,7 +4088,7 @@ final class SyncService: ObservableObject { localStateRevision += 1 refreshActiveSessionsAndSnapshot() scheduleWorkspaceSnapshotWrite() - if connectionState == .connected || connectionState == .syncing { + if connectionState == .connected { startInitialHydrationTask(for: connectionGeneration) } } @@ -4657,7 +4661,7 @@ final class SyncService: ObservableObject { // Clear stale failure state from the prior project so the reconnect gap // shows active handoff progress instead of a leftover failure banner. lastError = nil - let hadLiveSocket = connectionState == .connected || connectionState == .syncing + let hadLiveSocket = connectionState == .connected if hadLiveSocket { teardownSocket(reason: "Switching project.") } @@ -9299,7 +9303,7 @@ final class SyncService: ObservableObject { /// no-op when the transport is down, and never throws — a missing GitHub /// snapshot just leaves the ADE-mapped fallback in place. func refreshLaneGithubPrItems(force: Bool = false, minInterval: TimeInterval = 20) async { - guard connectionState == .connected || connectionState == .syncing else { return } + guard connectionState == .connected else { return } if !force, let fetchedAt = laneGithubPrItemsFetchedAt, Date().timeIntervalSince(fetchedAt) < minInterval { @@ -9313,7 +9317,7 @@ final class SyncService: ObservableObject { let requestedProjectId = activeProjectId do { let snapshot = try await fetchGitHubPullRequestSnapshot(force: force) - guard connectionState == .connected || connectionState == .syncing, + guard connectionState == .connected, activeProjectId == requestedProjectId else { return } laneGithubPrItems = snapshot.repoPullRequests.filter { $0.scope == "repo" } @@ -15309,35 +15313,68 @@ final class SyncService: ObservableObject { /// already running from `applyHelloPayload`; these two network refreshes can /// interleave with it on the main actor while their awaits are in flight. private func schedulePostHelloWork(for generation: UInt64) { - guard let expectedSocket = socket else { return } - let expectedConnectionGeneration = connectionGeneration - let isCurrent = { - self.isCurrentConnectAttempt(generation) - && self.connectionGeneration == expectedConnectionGeneration - && self.socket === expectedSocket - && self.canSendLiveRequests() - } + guard let attachment = beginPostHelloAttachment(for: generation) else { return } Task { @MainActor [weak self] in guard let self else { return } await performGenerationScopedPostHelloWork( - isCurrent: isCurrent, + isCurrent: attachment.isCurrent, restore: { async let lanePresence: Void = self.restoreTrackedOpenLanesAfterReconnect() async let projectCatalog: Void = self.refreshRemoteProjectCatalog() _ = await (lanePresence, projectCatalog) }, complete: { - self.connectionState = .connected self.logProjectSwitchPhase("post_hello_ready", completed: true) self.scheduleReconnectStabilityReset( - generation: expectedConnectionGeneration, - socket: expectedSocket + generation: attachment.connectionGeneration, + socket: attachment.socket ) } ) } } + /// The socket/generation that carried `hello_ok`, plus the staleness + /// predicate the deferred post-hello completion has to re-check. + private struct PostHelloAttachment { + let socket: URLSessionWebSocketTask + let connectionGeneration: UInt64 + let isCurrent: () -> Bool + } + + /// Shared prologue for post-hello restoration — used by `schedulePostHelloWork` + /// and by the DEBUG test hook, so tests exercise the production logic. + /// + /// Attachment is a fact the moment `hello_ok` is applied, so publish + /// `.connected` here rather than at the end of the restoration that follows. + /// Deferring it left the app unattached for the length of a network round + /// trip on every connect — and permanently whenever the generation guard + /// rejected the completion — which every surface gated on `isAttached` read + /// as "not connected yet". This republish is idempotent: `applyHelloPayload` + /// already set `.connected`, and this keeps the prologue's contract standing + /// on its own for the DEBUG test hook and any future caller. The + /// reconnect-backoff reset stays behind the guarded completion: that, not + /// `connectionState`, is what must wait for a proven-usable socket. + /// + /// Returns `nil` when there is no socket to attach to. + private func beginPostHelloAttachment( + for connectAttemptGeneration: UInt64 + ) -> PostHelloAttachment? { + guard let expectedSocket = socket else { return nil } + let expectedConnectionGeneration = connectionGeneration + connectionState = .connected + return PostHelloAttachment( + socket: expectedSocket, + connectionGeneration: expectedConnectionGeneration, + isCurrent: { + self.isCurrentConnectAttempt(connectAttemptGeneration) + && self.connectionGeneration == expectedConnectionGeneration + && self.socket === expectedSocket + && self.canSendLiveRequests() + } + ) + } + private func scheduleReconnectStabilityReset( generation: UInt64, socket expectedSocket: URLSessionWebSocketTask, @@ -16078,23 +16115,27 @@ final class SyncService: ObservableObject { await scheduledTask?.value } + /// Drives the same `beginPostHelloAttachment` prologue as + /// `schedulePostHelloWork`: attachment is published up front, and the + /// generation-scoped completion (reconnect-stability reset in production) + /// only runs when the socket that carried `hello_ok` is still current. + /// Returns whether that completion ran. + @discardableResult func performPostHelloRestorationForTesting( restore: () async -> Void - ) async { - guard let expectedSocket = socket else { return } - let expectedConnectionGeneration = connectionGeneration - connectionState = .syncing + ) async -> Bool { + guard let attachment = beginPostHelloAttachment( + for: connectAttemptGeneration + ) else { return false } + var completed = false await performGenerationScopedPostHelloWork( - isCurrent: { - self.connectionGeneration == expectedConnectionGeneration - && self.socket === expectedSocket - && self.canSendLiveRequests() - }, + isCurrent: attachment.isCurrent, restore: restore, complete: { - self.connectionState = .connected + completed = true } ) + return completed } func completeTransportProbeForTesting( @@ -16364,7 +16405,11 @@ final class SyncService: ObservableObject { // changeset_batch. Setting it prematurely causes the desktop to skip // the full initial sync on reconnect (it thinks we already have the data). hostName = remoteHostName ?? activeHostProfile?.hostName - connectionState = .syncing + // Attachment is a fact at `hello_ok`. Every production call path runs + // `schedulePostHelloWork` immediately after this, which republishes + // `.connected` idempotently; publishing here means no path can leave the + // app in a transient non-attached state while hydration catches up. + connectionState = .connected publishConnectTimingMetrics( connectedHost: connectedHost, hostTransport: payload["connectionTransport"] as? String, @@ -16501,7 +16546,7 @@ final class SyncService: ObservableObject { } private func canSendLiveRequests() -> Bool { - socket != nil && (connectionState == .connected || connectionState == .syncing) + socket != nil && (connectionState == .connected) } private func receiveLoop(for task: URLSessionWebSocketTask) { @@ -18705,7 +18750,7 @@ final class SyncService: ObservableObject { private func performInitialHydration(for connectionGeneration: UInt64) async { guard isCurrentConnectionGeneration(connectionGeneration), - connectionState == .connected || connectionState == .syncing + connectionState == .connected else { return } if activeProjectId == nil { @@ -19443,8 +19488,11 @@ extension SyncService { let connection: String switch connectionState { + // Widget wire vocabulary — `ADELockScreenWidget` matches these strings. + // "syncing" stays in the vocabulary for `.connecting`; it is not a new + // value and the widget's syncing tile remains reachable. case .connected: connection = "connected" - case .syncing, .connecting: connection = "syncing" + case .connecting: connection = "syncing" default: connection = "disconnected" } diff --git a/apps/ios/ADE/Views/Components/ADEDesignSystem.swift b/apps/ios/ADE/Views/Components/ADEDesignSystem.swift index 0de52a21b..5edab4de2 100644 --- a/apps/ios/ADE/Views/Components/ADEDesignSystem.swift +++ b/apps/ios/ADE/Views/Components/ADEDesignSystem.swift @@ -588,7 +588,6 @@ struct ConnectionHealthPresentation { init( health: SyncConnectionHealth, - connectionState: RemoteConnectionState, hostName: String? ) { let truncated = Self.truncate(hostName: hostName) @@ -597,7 +596,6 @@ struct ConnectionHealthPresentation { self.truncatedHostName = truncated self.accessibilityLabel = Self.computeAccessibilityLabel( health: health, - connectionState: connectionState, truncatedHostName: truncated ) } @@ -628,7 +626,6 @@ struct ConnectionHealthPresentation { private static func computeAccessibilityLabel( health: SyncConnectionHealth, - connectionState: RemoteConnectionState, truncatedHostName: String? ) -> String { let errorSuffix: String = { @@ -649,17 +646,11 @@ struct ConnectionHealthPresentation { if health.load == .strained { return "Connected to \(name). Machine is responding slowly" } - if connectionState == .syncing { - return "Connected to \(name). Syncing changes" - } return "Connected to \(name)" } if health.load == .strained { return "Connected. Machine is responding slowly" } - if connectionState == .syncing { - return "Connected. Syncing changes" - } return "Connected" case .connecting: return "Connecting to machine" @@ -677,7 +668,6 @@ struct ADEConnectionDot: View { private var presentation: ConnectionHealthPresentation { ConnectionHealthPresentation( health: syncService.connectionHealth, - connectionState: syncService.connectionState, hostName: syncService.hostName ) } diff --git a/apps/ios/ADE/Views/Cto/CtoRootScreen.swift b/apps/ios/ADE/Views/Cto/CtoRootScreen.swift index 4088365c8..a3ce6ea46 100644 --- a/apps/ios/ADE/Views/Cto/CtoRootScreen.swift +++ b/apps/ios/ADE/Views/Cto/CtoRootScreen.swift @@ -153,7 +153,7 @@ struct CtoRootScreen: View { private var ctoLiveReloadKey: String? { guard isTabActive else { return nil } switch syncService.connectionState { - case .connected, .syncing: + case .connected: return "live-\(syncService.localStateRevision)" case .connecting, .disconnected, .error: return nil diff --git a/apps/ios/ADE/Views/Cto/CtoSessionDestinationView.swift b/apps/ios/ADE/Views/Cto/CtoSessionDestinationView.swift index 2f04d72d0..114bb4af5 100644 --- a/apps/ios/ADE/Views/Cto/CtoSessionDestinationView.swift +++ b/apps/ios/ADE/Views/Cto/CtoSessionDestinationView.swift @@ -109,7 +109,7 @@ struct CtoSessionDestinationView: View { private var isLive: Bool { let workStatus = syncService.status(for: .work) return workStatus.phase == .ready - && (syncService.connectionState == .connected || syncService.connectionState == .syncing) + && syncService.connectionState == .connected } /// Cancels any in-flight ensure and starts a fresh one. Without this guard, diff --git a/apps/ios/ADE/Views/Deeplinks/SendToMacCard.swift b/apps/ios/ADE/Views/Deeplinks/SendToMacCard.swift index 369b9f3df..d6c3a72ec 100644 --- a/apps/ios/ADE/Views/Deeplinks/SendToMacCard.swift +++ b/apps/ios/ADE/Views/Deeplinks/SendToMacCard.swift @@ -396,7 +396,7 @@ struct SendToMacCard: View { private var machineSecondaryLabel: String? { switch syncService.connectionState { - case .connected, .syncing: + case .connected: return "Connected" case .connecting: return "Connecting…" @@ -409,7 +409,7 @@ struct SendToMacCard: View { private var machineTint: Color { switch syncService.connectionState { - case .connected, .syncing: return ADEColor.success + case .connected: return ADEColor.success case .connecting: return ADEColor.warning case .error: return ADEColor.danger case .disconnected: return ADEColor.textMuted diff --git a/apps/ios/ADE/Views/Files/FilesDetailComponents.swift b/apps/ios/ADE/Views/Files/FilesDetailComponents.swift index 33175f293..0f75fe9c5 100644 --- a/apps/ios/ADE/Views/Files/FilesDetailComponents.swift +++ b/apps/ios/ADE/Views/Files/FilesDetailComponents.swift @@ -125,10 +125,10 @@ struct FilesHeaderStrip: View { private var filesBrowserStatusSuffix: String? { let phase = syncService.status(for: .files).phase let connection = syncService.connectionState - if phase == .ready && (connection == .connected || connection == .syncing) { + if phase == .ready && connection == .connected { return nil } - if phase == .hydrating || phase == .syncingInitialData || connection == .syncing { + if phase == .hydrating || phase == .syncingInitialData { return "Syncing" } if connection == .connecting { diff --git a/apps/ios/ADE/Views/Files/FilesRootScreen+Actions.swift b/apps/ios/ADE/Views/Files/FilesRootScreen+Actions.swift index 46b8f0ee7..6ed358084 100644 --- a/apps/ios/ADE/Views/Files/FilesRootScreen+Actions.swift +++ b/apps/ios/ADE/Views/Files/FilesRootScreen+Actions.swift @@ -21,7 +21,7 @@ extension FilesRootScreen { } var canUseLiveFileActions: Bool { - filesStatus.phase == .ready && (syncService.connectionState == .connected || syncService.connectionState == .syncing) + filesStatus.phase == .ready && syncService.connectionState == .connected } var needsRepairing: Bool { diff --git a/apps/ios/ADE/Views/Hub/HubComposerDrawer.swift b/apps/ios/ADE/Views/Hub/HubComposerDrawer.swift index c5cdbb8d3..cf7190ab0 100644 --- a/apps/ios/ADE/Views/Hub/HubComposerDrawer.swift +++ b/apps/ios/ADE/Views/Hub/HubComposerDrawer.swift @@ -209,7 +209,7 @@ struct HubInlineComposer: View { private var canUploadAttachments: Bool { attachmentsAvailable - && (syncService.connectionState == .connected || syncService.connectionState == .syncing) + && syncService.connectionState == .connected } private var canSend: Bool { diff --git a/apps/ios/ADE/Views/Hub/HubQuickConnect.swift b/apps/ios/ADE/Views/Hub/HubQuickConnect.swift index b6730e4b5..1bcf78f44 100644 --- a/apps/ios/ADE/Views/Hub/HubQuickConnect.swift +++ b/apps/ios/ADE/Views/Hub/HubQuickConnect.swift @@ -221,7 +221,7 @@ struct HubQuickConnectSection: View { Task { @MainActor in await syncService.reconnect(toSavedHost: host) connectingId = nil - if syncService.connectionState == .connected || syncService.connectionState == .syncing { + if syncService.connectionState == .connected { ADEHaptics.success() onConnectSuccess() } else { diff --git a/apps/ios/ADE/Views/Hub/HubScreen.swift b/apps/ios/ADE/Views/Hub/HubScreen.swift index c63115010..229fede19 100644 --- a/apps/ios/ADE/Views/Hub/HubScreen.swift +++ b/apps/ios/ADE/Views/Hub/HubScreen.swift @@ -62,7 +62,7 @@ struct HubScreen: View { } private var canShowProjects: Bool { - syncService.connectionState == .connected || syncService.connectionState == .syncing + syncService.connectionState == .connected } private var hubIsActive: Bool { @@ -417,8 +417,8 @@ struct HubScreen: View { syncService.supportsPersonalChats, syncService.canInvokeRemoteAction("personalChats.list") else { return nil } - // Connected ↔ syncing is one live state for this purpose. Keeping a stable - // host key avoids redundant refreshes while normal sync batches flow. + // Keyed on the host, not on connection or hydration progress: a stable host + // key avoids redundant refreshes while normal sync batches flow. return hubCollapseDefaultsConnectionKey ?? "machine" } diff --git a/apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift b/apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift index 09d116af6..ae71a6853 100644 --- a/apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift +++ b/apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift @@ -543,7 +543,7 @@ struct LaneDetailScreen: View { var liveActionDisabledSubtitle: String { let laneStatus = syncService.status(for: .lanes) - if syncService.connectionState == .connected || syncService.connectionState == .syncing { + if syncService.connectionState == .connected { return laneStatus.phase == .ready ? "Waiting for live lane actions." : "Waiting for lane sync." } return "Reconnect to run git actions." diff --git a/apps/ios/ADE/Views/Lanes/LaneRootStateViews.swift b/apps/ios/ADE/Views/Lanes/LaneRootStateViews.swift index 71d7f8c64..b16a7f4f5 100644 --- a/apps/ios/ADE/Views/Lanes/LaneRootStateViews.swift +++ b/apps/ios/ADE/Views/Lanes/LaneRootStateViews.swift @@ -21,7 +21,6 @@ extension LanesTabView { var showsLaneLoadingSkeletons: Bool { laneSnapshots.isEmpty && ( syncService.connectionState == .connecting - || syncService.connectionState == .syncing || laneStatus.phase == .hydrating || laneStatus.phase == .syncingInitialData ) diff --git a/apps/ios/ADE/Views/LanesTabView.swift b/apps/ios/ADE/Views/LanesTabView.swift index d2e64eaae..dd9e2d410 100644 --- a/apps/ios/ADE/Views/LanesTabView.swift +++ b/apps/ios/ADE/Views/LanesTabView.swift @@ -211,8 +211,8 @@ struct LanesTabView: View { } .onChange(of: syncService.connectionState) { oldValue, newValue in guard isActive else { return } - let wasOnline = oldValue == .connected || oldValue == .syncing - let nowOnline = newValue == .connected || newValue == .syncing + let wasOnline = oldValue == .connected + let nowOnline = newValue == .connected if wasOnline && !nowOnline { ADEHaptics.warning() } diff --git a/apps/ios/ADE/Views/PRs/PrDetailScreen.swift b/apps/ios/ADE/Views/PRs/PrDetailScreen.swift index 500199324..4b74cdd89 100644 --- a/apps/ios/ADE/Views/PRs/PrDetailScreen.swift +++ b/apps/ios/ADE/Views/PRs/PrDetailScreen.swift @@ -132,7 +132,7 @@ struct PrDetailView: View { } private var isLive: Bool { - prsStatus.phase == .ready && (syncService.connectionState == .connected || syncService.connectionState == .syncing) + prsStatus.phase == .ready && syncService.connectionState == .connected } private var canRunPrActions: Bool { diff --git a/apps/ios/ADE/Views/PRs/PrsRootScreen.swift b/apps/ios/ADE/Views/PRs/PrsRootScreen.swift index 35a4f994a..628e5bbae 100644 --- a/apps/ios/ADE/Views/PRs/PrsRootScreen.swift +++ b/apps/ios/ADE/Views/PRs/PrsRootScreen.swift @@ -81,7 +81,7 @@ struct PRsTabView: View { } private var isLive: Bool { - prsStatus.phase == .ready && (syncService.connectionState == .connected || syncService.connectionState == .syncing) + prsStatus.phase == .ready && syncService.connectionState == .connected } private var isLoadingSkeleton: Bool { diff --git a/apps/ios/ADE/Views/Settings/ConnectionSettingsView.swift b/apps/ios/ADE/Views/Settings/ConnectionSettingsView.swift index 7de2ca9c6..8fa6caae6 100644 --- a/apps/ios/ADE/Views/Settings/ConnectionSettingsView.swift +++ b/apps/ios/ADE/Views/Settings/ConnectionSettingsView.swift @@ -884,7 +884,7 @@ struct SettingsMachinesSection: View { } private var isConnected: Bool { - syncService.connectionState == .connected || syncService.connectionState == .syncing + syncService.connectionState == .connected } /// Row id of the machine currently attached, so its stale failure — and only diff --git a/apps/ios/ADE/Views/Settings/SettingsConnectionHeader.swift b/apps/ios/ADE/Views/Settings/SettingsConnectionHeader.swift index 31589b3e6..16b85402e 100644 --- a/apps/ios/ADE/Views/Settings/SettingsConnectionHeader.swift +++ b/apps/ios/ADE/Views/Settings/SettingsConnectionHeader.swift @@ -301,7 +301,7 @@ private struct SettingsConnectionQuickAction: View { var body: some View { switch connectionState { - case .connected, .syncing: + case .connected: ADEGlassActionButton( title: "Disconnect", symbol: "power", diff --git a/apps/ios/ADE/Views/Settings/SettingsPinSheet.swift b/apps/ios/ADE/Views/Settings/SettingsPinSheet.swift index bc5117320..09df4f875 100644 --- a/apps/ios/ADE/Views/Settings/SettingsPinSheet.swift +++ b/apps/ios/ADE/Views/Settings/SettingsPinSheet.swift @@ -276,7 +276,10 @@ struct SettingsPinSheet: View { } guard isSubmitting else { return } - if syncService.connectionState == .connected { + // Success is "attached to the machine". `isAttached` is the canonical + // predicate for that across the app — go through it rather than + // re-deriving a comparison here. + if syncService.isAttached { // Success beat: haptic + a brief checkmark before the sheet dismisses. ADEHaptics.success() isSubmitting = false diff --git a/apps/ios/ADE/Views/Work/TerminalSessionScreen.swift b/apps/ios/ADE/Views/Work/TerminalSessionScreen.swift index 8d7bc897e..b89b1638c 100644 --- a/apps/ios/ADE/Views/Work/TerminalSessionScreen.swift +++ b/apps/ios/ADE/Views/Work/TerminalSessionScreen.swift @@ -91,7 +91,7 @@ struct TerminalSessionScreen: View { } private func handleConnectionState(_ state: RemoteConnectionState) { - controller.handleConnectionChange(isConnected: state == .connected || state == .syncing) + controller.handleConnectionChange(isConnected: state == .connected) } private func handleKeyboardWillShow() { @@ -172,7 +172,7 @@ struct TerminalSessionScreen: View { return ADEColor.textMuted } switch syncService.connectionState { - case .connected, .syncing: + case .connected: return controller.isSubscribed ? ADEColor.success : ADEColor.warning case .connecting: return ADEColor.warning diff --git a/apps/ios/ADE/Views/Work/WorkNewChatScreen.swift b/apps/ios/ADE/Views/Work/WorkNewChatScreen.swift index e2c30928e..4e1a8071e 100644 --- a/apps/ios/ADE/Views/Work/WorkNewChatScreen.swift +++ b/apps/ios/ADE/Views/Work/WorkNewChatScreen.swift @@ -706,7 +706,7 @@ struct WorkNewChatScreen: View { private var canUploadAttachments: Bool { attachmentsAvailable - && (syncService.connectionState == .connected || syncService.connectionState == .syncing) + && syncService.connectionState == .connected } /// Fast mode only applies to in-app chat sessions on fast-tier models. The diff --git a/apps/ios/ADE/Views/Work/WorkRootScreen.swift b/apps/ios/ADE/Views/Work/WorkRootScreen.swift index 2fd864c6d..b1714e9ba 100644 --- a/apps/ios/ADE/Views/Work/WorkRootScreen.swift +++ b/apps/ios/ADE/Views/Work/WorkRootScreen.swift @@ -273,7 +273,7 @@ struct WorkRootScreen: View { } var isLive: Bool { - syncService.connectionState == .connected || syncService.connectionState == .syncing + syncService.connectionState == .connected } var isLoadingSkeleton: Bool { diff --git a/apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift b/apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift index 1e1172b19..e20e62a6d 100644 --- a/apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift +++ b/apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift @@ -811,7 +811,7 @@ struct WorkSessionDestinationView: View { } var hostReachable: Bool { - syncService.connectionState == .connected || syncService.connectionState == .syncing + syncService.connectionState == .connected } /// Live polling/load gates require BOTH the parent's "session is live" flag diff --git a/apps/ios/ADETests/ADETests.swift b/apps/ios/ADETests/ADETests.swift index 78b8e1834..9a9f13a1a 100644 --- a/apps/ios/ADETests/ADETests.swift +++ b/apps/ios/ADETests/ADETests.swift @@ -1842,14 +1842,19 @@ final class ADETests: XCTestCase { ) } - func testSyncConnectionHealthTreatsHydrationAsConnected() { + /// A retired `.syncing` state used to hold this row (hydration reported a + /// connected transport). `.connecting` is now the only transitional state + /// left, and it must stay distinct: it reports `connecting`, it must not + /// inherit load strain (strain is meaningless without a live transport), and + /// it must not surface a stale failure message. + func testSyncConnectionHealthKeepsConnectingDistinctFromConnected() { let health = syncConnectionHealth( - connectionState: .syncing, - prefersReducedSyncLoad: false, + connectionState: .connecting, + prefersReducedSyncLoad: true, lastError: "Transient sync work" ) - XCTAssertEqual(health.transport, .connected) + XCTAssertEqual(health.transport, .connecting) XCTAssertEqual(health.load, .normal) XCTAssertNil(health.lastFailureMessage) } @@ -6757,7 +6762,7 @@ final class ADETests: XCTestCase { XCTAssertEqual(attemptedAddresses, ["192.168.1.10", "192.168.1.11"]) XCTAssertEqual(winner, attempts[1]) XCTAssertEqual(failedCandidateStates, [.connecting]) - XCTAssertEqual(service.connectionState, .syncing) + XCTAssertEqual(service.connectionState, .connected) } @MainActor @@ -6792,7 +6797,7 @@ final class ADETests: XCTestCase { XCTAssertEqual(attemptedAddresses, ["100.64.0.10", "100.64.0.11"]) XCTAssertEqual(winner, attempts[1]) XCTAssertEqual(failedCandidateStates, [.error]) - XCTAssertEqual(service.connectionState, .syncing) + XCTAssertEqual(service.connectionState, .connected) } @MainActor @@ -6839,7 +6844,7 @@ final class ADETests: XCTestCase { ], ]) - XCTAssertEqual(service.connectionState, .syncing) + XCTAssertEqual(service.connectionState, .connected) XCTAssertEqual(service.hostCompatibilityMode, .limited) XCTAssertEqual(service.hostCompatibilityMissingActions, ["commandRouting"]) XCTAssertFalse(service.supportsRemoteAction("usage.getAdeStats")) @@ -6915,7 +6920,7 @@ final class ADETests: XCTestCase { ], ]) - XCTAssertEqual(service.connectionState, .syncing) + XCTAssertEqual(service.connectionState, .connected) XCTAssertEqual(service.hostCompatibilityMode, .limited) XCTAssertFalse(service.supportsRemoteAction("cto.startLinearMobileOAuth")) XCTAssertFalse(service.supportsRemoteAction("cto.setLinearToken")) @@ -6960,7 +6965,7 @@ final class ADETests: XCTestCase { ], ]) - XCTAssertEqual(service.connectionState, .syncing) + XCTAssertEqual(service.connectionState, .connected) XCTAssertEqual(service.hostCompatibilityMode, .full) XCTAssertTrue(service.supportsRemoteAction("cto.startLinearMobileOAuth")) XCTAssertTrue(service.supportsRemoteAction("cto.completeLinearMobileOAuth")) @@ -7107,7 +7112,7 @@ final class ADETests: XCTestCase { ], ]) - XCTAssertEqual(service.connectionState, .syncing) + XCTAssertEqual(service.connectionState, .connected) XCTAssertEqual(service.hostCompatibilityMode, .full) XCTAssertEqual(service.hostCompatibilityMissingActions, []) XCTAssertTrue(service.supportsRemoteAction("chat.send")) @@ -7157,7 +7162,7 @@ final class ADETests: XCTestCase { ], ]) - XCTAssertEqual(service.connectionState, .syncing) + XCTAssertEqual(service.connectionState, .connected) XCTAssertFalse(service.supportsRemoteAction("work.updateSessionMeta")) XCTAssertFalse(service.supportsChatRemoteAction("chat.cancelScheduledWork", sessionId: "chat-1")) XCTAssertFalse(service.canInvokeChatRemoteAction("chat.cancelScheduledWork", sessionId: "chat-1")) @@ -7199,7 +7204,7 @@ final class ADETests: XCTestCase { ], ]) - XCTAssertEqual(service.connectionState, .syncing) + XCTAssertEqual(service.connectionState, .connected) XCTAssertEqual(service.hostCompatibilityMode, .limited) XCTAssertEqual(service.hostCompatibilityMissingActions, ["prs.getMobileGithubDetail"]) XCTAssertFalse(service.supportsRemoteAction("prs.getMobileGithubDetail")) @@ -7574,7 +7579,7 @@ final class ADETests: XCTestCase { ], ]) - XCTAssertEqual(service.connectionState, .syncing) + XCTAssertEqual(service.connectionState, .connected) XCTAssertEqual(service.hostCompatibilityMode, .limited) XCTAssertTrue(service.supportsPersonalChats) XCTAssertTrue(service.supportsRemoteAction("personalChats.list")) @@ -12894,7 +12899,7 @@ final class ADETests: XCTestCase { ) XCTAssertFalse( laneAllowsLiveActions( - connectionState: .syncing, + connectionState: .connecting, laneStatus: SyncDomainStatus(phase: .ready, lastError: nil, lastHydratedAt: nil) ) ) @@ -12960,7 +12965,7 @@ final class ADETests: XCTestCase { XCTAssertTrue(descendants.message.contains("child lanes")) } - func testLaneAllowsDiffInspectionKeepsCachedTargetsReadableWhileOfflineOrSyncing() { + func testLaneAllowsDiffInspectionKeepsCachedTargetsReadableWhileOfflineOrConnecting() { XCTAssertTrue( laneAllowsDiffInspection( connectionState: .disconnected, @@ -12970,7 +12975,7 @@ final class ADETests: XCTestCase { ) XCTAssertTrue( laneAllowsDiffInspection( - connectionState: .syncing, + connectionState: .connecting, laneStatus: SyncDomainStatus(phase: .ready, lastError: nil, lastHydratedAt: nil), hasCachedTargets: true ) diff --git a/apps/ios/ADETests/SyncAccountConnectRecoveryTests.swift b/apps/ios/ADETests/SyncAccountConnectRecoveryTests.swift index 40949593c..1df7fc11d 100644 --- a/apps/ios/ADETests/SyncAccountConnectRecoveryTests.swift +++ b/apps/ios/ADETests/SyncAccountConnectRecoveryTests.swift @@ -191,18 +191,11 @@ final class SyncAccountConnectRecoveryTests: XCTestCase { // MARK: - syncAccountMachineNavigationIsCurrent - /// The bug: `.syncing` is an attached state that every connect path passes - /// through, but this gate demanded `.connected` exactly. A link tapped during - /// hydration therefore decided we were on the wrong machine and re-paired to - /// the machine we were already talking to, tearing down a healthy connection. - func testSyncingCountsAsAttachedToTheTargetMachine() { - XCTAssertTrue(syncAccountMachineNavigationIsCurrent( - targetDeviceId: "host-1", - activeHostIdentity: "host-1", - connectionState: .syncing - )) - } - + /// The gate is attachment, not hydration progress. `hello_ok` publishes + /// `.connected` before initial hydration finishes, so a link tapped during + /// hydration must read as "already on this machine" — the earlier bug here + /// re-paired to the machine we were already talking to, tearing down a + /// healthy connection. func testConnectedCountsAsAttachedToTheTargetMachine() { XCTAssertTrue(syncAccountMachineNavigationIsCurrent( targetDeviceId: "host-1", @@ -215,7 +208,7 @@ final class SyncAccountConnectRecoveryTests: XCTestCase { XCTAssertFalse(syncAccountMachineNavigationIsCurrent( targetDeviceId: "host-2", activeHostIdentity: "host-1", - connectionState: .syncing + connectionState: .connected )) } diff --git a/apps/ios/ADETests/SyncRecoveryPolicyTests.swift b/apps/ios/ADETests/SyncRecoveryPolicyTests.swift index b8241e5e1..56b55c413 100644 --- a/apps/ios/ADETests/SyncRecoveryPolicyTests.swift +++ b/apps/ios/ADETests/SyncRecoveryPolicyTests.swift @@ -1245,8 +1245,13 @@ final class SyncRecoveryPolicyTests: XCTestCase { XCTAssertEqual(service.nextReconnectDelayForTesting(), 1_000_000_000) } + /// `hello_ok` is the attachment barrier: `.connected` must be published as + /// soon as the payload is applied, and must still hold once post-hello work + /// is scheduled — not only after its network round trip. Holding it back + /// parked the app in a non-attached state for seconds, which the PIN sheet + /// reported as "Incorrect PIN." on a pair that had actually succeeded. @MainActor - func testSuccessfulPostHelloRestorationPublishesConnectedOnlyAfterCompletion() async throws { + func testSuccessfulPostHelloRestorationPublishesConnectedBeforeRestorationCompletes() async throws { let baseURL = FileManager.default.temporaryDirectory .appendingPathComponent(UUID().uuidString, isDirectory: true) try FileManager.default.createDirectory(at: baseURL, withIntermediateDirectories: true) @@ -1264,23 +1269,30 @@ final class SyncRecoveryPolicyTests: XCTestCase { "brain": ["deviceId": "ready-host", "deviceName": "Mac Studio"], "features": [:], ]) - XCTAssertEqual(service.connectionState, .syncing) + // Applying the payload is itself the attachment moment. + XCTAssertEqual(service.connectionState, .connected) + XCTAssertTrue(service.isAttached) let postHello = Task { @MainActor in await service.performPostHelloRestorationForTesting { await restoration.wait() } } - while !restoration.isWaiting { await Task.yield() } - XCTAssertEqual(service.connectionState, .syncing) + await restoration.waitUntilWaiting() + XCTAssertEqual(service.connectionState, .connected) + XCTAssertTrue(service.isAttached) restoration.resume() - await postHello.value + let completed = await postHello.value + XCTAssertTrue(completed) XCTAssertEqual(service.connectionState, .connected) } + /// The staleness guard still matters for the generation-scoped completion: + /// a socket torn down mid-restoration must not run the post-hello completion + /// (in production, the reconnect-backoff stability reset) for a dead socket. @MainActor - func testStalePostHelloRestorationCannotRepublishConnected() async throws { + func testStalePostHelloRestorationCannotRunCompletion() async throws { let baseURL = FileManager.default.temporaryDirectory .appendingPathComponent(UUID().uuidString, isDirectory: true) try FileManager.default.createDirectory(at: baseURL, withIntermediateDirectories: true) @@ -1299,12 +1311,12 @@ final class SyncRecoveryPolicyTests: XCTestCase { await restoration.wait() } } - while !restoration.isWaiting { await Task.yield() } + await restoration.waitUntilWaiting() service.teardownSocketForTesting() restoration.resume() - await postHello.value + let completed = await postHello.value - XCTAssertNotEqual(service.connectionState, .connected) + XCTAssertFalse(completed) } func testTerminalInputQueuePreservesOrderAckAndReconnectIds() throws { @@ -2009,6 +2021,28 @@ private final class DeferredRecoveryWork { var isWaiting: Bool { continuation != nil } + /// Bounded readiness wait. If the continuation is never installed (the work + /// body did not run), spinning on `isWaiting` would hang the whole test + /// runner; fail loudly instead. + func waitUntilWaiting( + timeout: TimeInterval = 5, + file: StaticString = #filePath, + line: UInt = #line + ) async { + let deadline = Date().addingTimeInterval(timeout) + while !isWaiting { + if Date() >= deadline { + XCTFail( + "Deferred recovery work never installed its continuation within \(timeout)s.", + file: file, + line: line + ) + return + } + await Task.yield() + } + } + func wait() async { await withCheckedContinuation { continuation in self.continuation = continuation diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0ecaee460..b658d4c74 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -673,7 +673,7 @@ Related feature docs: [Chat](./features/chat/README.md), [Agents](./features/age `apps/desktop/src/shared/ipc.ts` defines the single `IPC` const with ~550 named channel strings in a `ade..` namespace: ``` -ade.app.* # app lifecycle, clipboard text and image (writeClipboardText, writeClipboardImage, saveClipboardImageAttachment), paths, image data-URL preview (getImageDataUrl), the deeplink navigation push channel ade.app.navigate (AppNavigationRequest payloads from the ade:// protocol handler, the ade code app/navigate JSON-RPC, and the iOS deeplinks.open sync command — see features/deeplinks/README.md), the one-way zoom push channel ade.app.zoomCommand (AppZoomCommand "in"/"out"/"reset" sent from the native View menu to the renderer's window.ade.zoom.onCommand so menu/keyboard zoom shares the in-app zoom path — display %, persistence, and the macOS traffic-light inset), and the resource-pressure snapshot ade.app.getResourceUsage (async, coalesced `AppResourceUsageSnapshot` backing the TopBar pressure indicator: one bounded/timeout-guarded `ps` sample shared across windows behind a 900 ms cache + in-flight coalescing, with disjoint per-role attribution built in `services/pty/resourceUsageSampling.ts` — see features/terminals-and-sessions/pty-and-sessions.md), and the machine-level daemon-health snapshot ade.app.getRuntimeHealth (async `RuntimeHealthSnapshot` — a rolling 24 h count + p95 of slow/errored local-runtime action calls, read directly off `localRuntimeConnectionPool` with no action-domain routing, feeding the Storage > Diagnostics "slow responses" tile) +ade.app.* # app lifecycle, clipboard text and image (writeClipboardText, writeClipboardImage, saveClipboardImageAttachment), paths, image data-URL preview (getImageDataUrl), the deeplink navigation push channel ade.app.navigate (AppNavigationRequest payloads from the ade:// protocol handler, the ade code app/navigate JSON-RPC, and the iOS deeplinks.open sync command — see features/deeplinks/README.md), the one-way zoom push channel ade.app.zoomCommand (AppZoomCommand "in"/"out"/"reset" sent from the native View menu to the renderer's window.ade.zoom.onCommand so menu/keyboard zoom shares the in-app zoom path — display %, persistence, and the macOS traffic-light inset), and the resource-pressure snapshot ade.app.getResourceUsage (async, coalesced `AppResourceUsageSnapshot` backing the TopBar pressure indicator: one bounded/timeout-guarded `ps` sample shared across windows behind a 900 ms cache + in-flight coalescing, with disjoint per-role attribution built in `services/pty/resourceUsageSampling.ts` — see features/terminals-and-sessions/pty-and-sessions.md), the machine-level daemon-health snapshot ade.app.getRuntimeHealth (async `RuntimeHealthSnapshot` — a rolling 24 h count + p95 of slow/errored local-runtime action calls, read directly off `localRuntimeConnectionPool` with no action-domain routing, feeding the Storage > Diagnostics "slow responses" tile), and ade.app.restartBackgroundService (the Connections "Repair" control: restarts this machine's `com.ade.runtime` launch agent through `ProjectRecoveryService.restartBrain()` and resolves only after the replacement answers a ping, throwing otherwise. Direct IPC on purpose — the pool lives in Electron main and the daemon being restarted cannot route its own restart, so there is no action-domain routing and no null-service risk. It is optional in the preload surface: the hosted-web adapter and browser mock cannot touch a launch agent, so callers feature-detect. Each click records one `ade_feature_used` with `feature: "connections"`, `action: "brain_repair"`, and a coarse `outcome`) ade.project.* # project open/close/switch/state, unified local+remote recents (listRecent, key-based forget/reorder, setRecentPinned), in-app directory browser (browseDirectories, getDetail), git path inspection (inspectPath — ProjectPathInspection behind the renderer's worktree-open gate; promise-cached in services/projects/projectPathInspector.ts with a `fresh` bypass and invalidated on lane attach/adopt from both the in-process handler and the runtime-bridge action path), favicon resolver/override (resolveIcon, chooseIcon, removeIcon) with local-only filesystem allowlists. openRepo/switchToPath surface AdeRecoveryErrorCode-coded failures (via surfaceCodedError) so the renderer can route a failed open into the recovery screen ade.recovery.* # brain-independent project-open recovery: diagnose / repair # (projectRecoveryService against projectRecoveryConnectionPool). @@ -1032,6 +1032,8 @@ Related UI docs: [Terminals UI surfaces](./features/terminals-and-sessions/ui-su | GitHub PAT | `.ade/secrets/github/*.bin` | `safeStorage.encryptString` (OS-backed) | | API provider keys | `.ade/secrets/api-keys.json` | Plaintext `0600` | | ADE project secrets | `.ade/secrets/project-secrets.v1.enc` | AES-GCM encrypted file store, OS-bound on supported hosts | +| Machine credential store (shared) | `.ade/secrets/credentials.json.enc` + `.machine-key` | AES-GCM file store; key HKDF-derived from the machine key and a macOS-keychain secret. Read by the brain, the `ade` CLI, and desktop | +| Machine credential store (Electron) | `.ade/secrets/credentials.safe.enc` | `safeStorage.encryptString`; readable only by Electron | | Claude OAuth creds | Claude's own store | Inherited | | Codex auth tokens | Codex's own store | Inherited | | macOS Keychain entries | OS Keychain | OS-backed | @@ -1040,6 +1042,24 @@ Related UI docs: [Terminals UI surfaces](./features/terminals-and-sessions/ui-su | Sync bootstrap token | `.ade/secrets/sync-bootstrap-token` | Plaintext, never syncs | | External-ADE CLI secrets | `.ade/local.secret.yaml` | Plaintext, never syncs | +The two machine credential stores are not interchangeable. Only the AES file +store is shared across processes; a `safeStorage` file is Electron-only, so the +account session (`account.session.v1`) and the sync bootstrap token +(`sync.bootstrapToken.v1`) are pinned to the file store and the safeStorage +migration retains them there — copying everything else across, pruning the +migrated duplicates out of the file store, and deleting the legacy files only +when nothing is retained. Writing either key through the safeStorage store +throws rather than silently signing the brain out of a machine whose app is +signed in, and a legacy store that cannot be decrypted aborts the migration +instead of being replaced with an empty one. Keychain material is resolved +race-safely (`osBoundKeyMaterial.ts`): the create is non-clobbering, an +inconclusive `security` result fails closed rather than minting a replacement +secret, and a decrypt failure against cached material re-reads the keychain once +before the store is declared unreadable. A brain that still cannot read it +publishes nothing to the account directory; that state is user-repairable from +Connections and reported once per episode as `ade_account_session_unreadable` +(see [logging](./logging.md)). + ADE project-secret dotenv imports are explicit transfers, not background sync: the desktop reads a user-selected local file (1 MB cap) and sends its content to the active project runtime for parsing and atomic import. Exports are diff --git a/docs/features/onboarding-and-settings/README.md b/docs/features/onboarding-and-settings/README.md index fe5a427ec..4967a4026 100644 --- a/docs/features/onboarding-and-settings/README.md +++ b/docs/features/onboarding-and-settings/README.md @@ -499,7 +499,11 @@ Renderer — settings: loopback / relay candidates. The Phone tab explains QR + PIN and Nearby + PIN, while the Web tab is account-sign-in only. It also surfaces the bootstrap token for desktop peers, relay/discovery status, and the per-device panels - used to forget paired phones or revoke web clients. + used to forget paired phones or revoke web clients. When the account-directory + state is the brain-side unreadable session (`isBrainAccountSessionFailure`), + the card adds a **Repair** button that restarts this Mac's background service + and re-reads the snapshot once it settles — see + [Sync and multi-device](../sync-and-multi-device/README.md). - `apps/desktop/src/renderer/components/app/TopBar.tsx` and `ConnectionsPanel.tsx` — the single top-bar Connections control and its Machines, Phone, and Web tabs. The Web tab reports connected browser peers diff --git a/docs/features/remote-runtime/README.md b/docs/features/remote-runtime/README.md index 31a2115db..994f85446 100644 --- a/docs/features/remote-runtime/README.md +++ b/docs/features/remote-runtime/README.md @@ -135,13 +135,41 @@ relay payload E2E encryption is planned security work. See the trust boundary in publish health as a This-computer indicator (`remoteMachineModel.describePublishHealth`, which reads inactive states as "none" and only alarms a real failure after it has persisted ~2 minutes), and the app shell reads `lastWedge` for the - `BrainRecoveryNotice` banner. + `BrainRecoveryNotice` banner. Both `serve --install-service` and + `serve --uninstall-service` run through one shared + `runServiceManagerCommand` child-process boundary (spawn, output + accumulation, single-settle latch, timeout kill, output parse) and differ only + in the policy applied to the result. Install is bounded at 60 s and uninstall + at 20 s: a wedged installer used to pin `serviceInstallPromise` forever, which + blocked every later install and left Repair spinning. `installServiceBestEffort` + coalesces concurrent callers onto one child, but a `forceRestart: true` call + never coalesces onto a plain install — a background install may skip entirely + or may have spawned before the user asked for a restart, so returning its + promise would report success without restarting anything. A forced call queues + behind whatever is in flight, runs its own forcing install, and becomes the + promise later callers coalesce onto. - `apps/desktop/src/main/services/runtime/lastFailureStore.ts` — bounded typed project/machine failure reports used when the background service exits before desktop IPC can obtain a normal runtime error. - `apps/desktop/src/main/services/runtime/projectRecoveryService.ts` — brain-independent project diagnosis and ordered repair for storage, - database, migration, endpoint, and chat continuity failures. + database, migration, endpoint, and chat continuity failures. It also owns + `restartBrain()`, the machine-scoped restart behind the Connections **Repair** + button. Both it and `repair()`'s restart_service/verify_endpoint steps go + through one `restartServiceAndWait()` sequence — install, wait up to 20 s for + the machine endpoint to rebind, then `ping` — which reports which stage lost + rather than the copy, because its two callers phrase the same stage + differently (`repair()` speaks in repair steps, `restartBrain()` throws). + `force` is more than the install flag: a forced restart is the only caller that + actually asked for one, so an install that resolves having *skipped* is a + failure for it, and its message becomes "A newer ADE runtime is already + running — quit and reopen ADE instead." (the release-build block is passed + through verbatim, since it is already written as instructions) rather than the + installer's log line. The two are mutually exclusive: `restartBrain()` rejects + with "Recovery is already running." while a `repair()` is in flight, because + repair stops the service and then does exclusive database work, and + reinstalling the brain underneath it would put a writer back on the database + mid-check. Repair wins; the button can be pressed again afterwards. - `apps/desktop/src/main/services/runtime/machineTrustResetMigration.ts` — one-time packaged-release reset of the old machine-connection trust files. It preserves account auth, machine identity, pairing PINs, projects, and SSH @@ -162,7 +190,22 @@ relay payload E2E encryption is planned security work. See the trust boundary in (`sync_disabled`, `not_host`, `account_signed_out`, `machine_key_unavailable`, …) read as "none", and every other state is a failure that only alarms once it has persisted at least `PUBLISH_FAILING_ALARM_MS` (2 min) so a transient blip - stays quiet. + stays quiet. When that failure is specifically the brain-side unreadable + account session (`isBrainAccountSessionFailure`, i.e. `token_unreadable`), the + row also mounts the shared `BrainRepairButton` / `useBrainRepair` pair — the + same control the Connections This Mac card renders, reading the same publisher + health record, with the periodic `getInfo` poll extracted as a named + `refreshPublishHealth` so a settled restart re-reads it immediately. + Discovery diagnostics are rendered by severity rather than lumped together: + `RemoteRuntimeDiscoveryDiagnostic.severity` is `"warning"` (discovery is + degraded and worth looking at) or `"info"` (a normal, non-actionable fact + about the environment). "Tailscale not installed — LAN discovery only." is + `info`, because Tailscale is optional and not having it is not a problem; + `tailscale-timeout` and `tailscale-status-failed` remain `warning`. The panel + keeps the raw diagnostics array as its one source of truth and derives the + warning line and the muted info note from it, so the two cannot drift; a + failed `listDiscoveredMachines` call stays a separate string rather than + becoming a synthetic diagnostic. - `apps/desktop/src/renderer/components/app/projectTabGrouping.ts` — collapses the open local and remote tabs into one group per repository, joined on the normalized git origin. A project with no resolvable origin is never merged, @@ -717,7 +760,8 @@ diagnostics with `ADE_ENABLE_DESKTOP_SYNC_HOST=1`. connection closed. The destination may still finish. Check that machine before retrying; the handoff ID makes an explicit retry reconcile the same destination lane/chat rather than automatically replaying the mutation. -- "Tailscale CLI was not found / timed out / failed" warning under the discovered-machines list — surfaced from `discoverLanRuntimes` diagnostics. LAN (Bonjour) discovery still ran; install or unblock `tailscale` to add tailnet peers. +- "Tailscale discovery timed out / failed" warning under the discovered-machines list — surfaced from `discoverLanRuntimes` diagnostics. LAN (Bonjour) discovery still ran; unblock `tailscale` to add tailnet peers. "Tailscale not installed — LAN discovery only." is the `info` variant of the same diagnostic and renders as a muted note rather than a warning: Tailscale is optional, so a plain Mac without it is not in a degraded state. +- "Repair" next to the This Mac / route-publish failure — this Mac's brain cannot read the stored account session, so it never publishes to the account directory even though the app is signed in. The button restarts `com.ade.runtime` and waits for the replacement to answer; the new process re-reads the keychain from scratch. If it reports "Repair failed — quit and reopen ADE", a newer runtime is usually already running and must not be forced down. - Agent provider missing or unauthenticated — use the inline `AgentCliAuthCard` to install or authenticate that provider on the active runtime machine. - `lan :: authentication` in the route list — the host was reached and it *rejected* this desktop, so the other routes' `timeout`/`unreachable` entries are noise. The host's `hello_error` message names which of three causes it was: the pairing was removed on that machine, the saved secret no longer matches, or the two machines are signed in to different ADE accounts. The first two are reported identically (an unauthenticated caller must not be told whether a device id exists on that host) and both need a re-pair; only the account mismatch is fixed by signing in. diff --git a/docs/features/remote-runtime/internal-architecture.md b/docs/features/remote-runtime/internal-architecture.md index b2b5303f8..600629f0a 100644 --- a/docs/features/remote-runtime/internal-architecture.md +++ b/docs/features/remote-runtime/internal-architecture.md @@ -78,7 +78,7 @@ A remote target stores a primary `hostname` plus an optional `routes` array (`Re ### Discovery diagnostics -`discoverLanRuntimes` runs Bonjour and `tailscale status --json` in parallel and now returns a `RemoteRuntimeDiscoveryResult` with `{ machines, diagnostics }`. Each diagnostic carries `{ source: "bonjour" | "tailscale", code, message, detail }`. Codes today: `bonjour-discovery-failed`, `tailscale-unavailable` (CLI not installed), `tailscale-timeout`, `tailscale-status-failed`. The form surfaces these warnings inline so a missing or hung Tailscale CLI does not look like "no machines found" — the LAN side still ran. +`discoverLanRuntimes` runs Bonjour and `tailscale status --json` in parallel and now returns a `RemoteRuntimeDiscoveryResult` with `{ machines, diagnostics }`. Each diagnostic carries `{ source: "bonjour" | "tailscale", severity, code, message, detail }`. Codes today: `bonjour-discovery-failed`, `tailscale-unavailable` (CLI not installed), `tailscale-timeout`, `tailscale-status-failed`. `severity` is `"warning"` when discovery is degraded in a way the user may want to look at, and `"info"` for a normal, non-actionable observation about the environment. `tailscale-unavailable` is `info` — Tailscale is optional software, so not having it installed is a fact about the machine rather than a problem with discovery — while the timeout and failure codes stay `warning`. The form surfaces warnings inline so a hung or broken Tailscale CLI does not look like "no machines found" (the LAN side still ran), and renders info diagnostics as muted secondary text with no warning glyph. ## Bootstrap sequence diff --git a/docs/features/storage-and-recovery/README.md b/docs/features/storage-and-recovery/README.md index 6430d7587..2c689d2c5 100644 --- a/docs/features/storage-and-recovery/README.md +++ b/docs/features/storage-and-recovery/README.md @@ -19,7 +19,7 @@ | `apps/ade-cli/src/services/runtime/brainLogger.ts` | The machine-brain logger: reuses the desktop `createFileLogger` to write `~/.ade/runtime/brain.jsonl` (10 MiB `.1` rotation) and additionally mirrors timestamped `warn`/`error` lines to stderr so launchd captures them. | | `apps/ade-cli/src/commands/doctor.ts` | `ade doctor [--online]` — connects to the brain over the local socket and prints one `ok`/`warn`/`fail` row per subsystem (App version, Brain, Wedge history, Sync port, Publish health, Relay, Account); exits non-zero on any `fail`. `evaluateDoctorRows` is pure and dependency-injected so the desktop connection-doctor card and the CLI share one verdict. | | `apps/desktop/src/shared/adeRuntimeProtocol.ts` | Shared runtime-protocol contract: `RUNTIME_COMPAT_LEVEL` + `isRuntimeProtocolCompatible` (the integer compatibility-window check), and the tolerant parsers `parseRuntimePublishHealth` / `parseRuntimeLastWedge` that decode `runtimeInfo.publishHealth` and `runtimeInfo.lastWedge` for the connection pool, the doctor, and the desktop status surfaces. | -| `apps/desktop/src/main/services/runtime/projectRecoveryService.ts` | Brain-independent diagnosis and ordered repair: space, ownership, database validation, migration recovery, service restart, endpoint/project verification, and chat reconciliation. | +| `apps/desktop/src/main/services/runtime/projectRecoveryService.ts` | Brain-independent diagnosis and ordered repair: space, ownership, database validation, migration recovery, service restart, endpoint/project verification, and chat reconciliation. Also owns `restartBrain()` — the machine-scoped restart behind the Connections **Repair** button — which shares one `restartServiceAndWait()` sequence (install → wait ≤20 s for the endpoint → `ping`) with `repair()`'s restart_service/verify_endpoint steps. The two are mutually exclusive: `restartBrain()` rejects while a `repair()` is in flight, because repair stops the service and then does exclusive database work that a reinstall would put a second writer on top of. A forced restart also treats a *skipped* install as a failure ("A newer ADE runtime is already running — quit and reopen ADE instead."), where `repair()` tolerates one, since a protocol-compatible brain that is already running satisfies its step. | | `apps/desktop/src/main/services/storage/diskPressure.ts` | Samples all ADE storage roots, classifies pressure with recovery hysteresis, and gates write-producing operation classes via `canPerform(kind)`. Exports the `DiskPressureMonitor` type and refusal-message copy. | | `apps/desktop/src/main/services/storage/volume.ts` | `readVolumeSpace(dir)` (statfs free/total bytes) and `isNoSpaceError(err)` (ENOSPC/EDQUOT and disk-full message detection), shared by the pressure monitor and the database-open error classifier. | | `apps/desktop/src/main/services/storage/storageInsightsService.ts` | Builds categorized storage snapshots and preview-confirmed cleanup plans without following symlinks or deleting protected state. `proof_attachments` is a manual `review_first` cleanup target for `.ade/artifacts` and `.ade/attachments`; after bytes are removed it invokes the broker's `purgeArtifactRecordsUnder` hook so proof rows cannot outlive their files. It also runs the lane-lifecycle scan at the configured interval: safely archives excess or inactive lanes, marks old archived worktrees for review, and never removes lane files in the background. The **storage doctor** compresses history and maintains the database; filesystem candidates such as staging, backups, DerivedData, and build output remain review-first. Every run is journaled and emits one deduped `ade_feature_used` analytics event. Populates the snapshot's optional `extras` plus lifecycle policy/status and per-item ownership, age, blocked reasons, and reclaim estimates. | diff --git a/docs/features/sync-and-multi-device/README.md b/docs/features/sync-and-multi-device/README.md index 9cd254bd1..496f59f09 100644 --- a/docs/features/sync-and-multi-device/README.md +++ b/docs/features/sync-and-multi-device/README.md @@ -394,7 +394,18 @@ Runtime support files outside `services/sync/`: window. Failed publications retry after 1, 2, 5, 10, then 20 seconds so a short outage normally recovers within the lease, and a 401 forces one token refresh before the publication is classified as expired. These operational - retries and status polls are local logs, not product analytics. + retries and status polls are local logs, not product analytics. Two failure + *episodes* are the exception, and both go through + `apps/ade-cli/src/services/account/episodeAnalytics.ts` — a shared + edge-triggered helper that emits at most one event while a condition holds and + re-arms only once it clears, with a 24-hour deduplication window on top. + `ade_publish_failing` covers a publication that has been failing for at least + two minutes; `ade_account_session_unreadable` covers the brain being unable to + read the account session at all (`sessionReadState === "unreadable"`, or a + status read that threw), carrying only a coarse `code` for the read path + (`decrypt_failure`, `no_os_key_material`, `store_format`, `session_parse`, + `read_error`, `unknown`) that `accountAuthService.getSessionReadFailureReason()` + supplies. See [logging](../../logging.md). Successful account sign-in also requests an immediate publish; the brain observes both its local auth event and cross-process credential-file changes from desktop sign-in. Separately, a lightweight 2-second observer computes a @@ -435,6 +446,44 @@ Runtime support files outside `services/sync/`: `ADE_ALLOW_DEVELOPMENT_CLERK=1` is the explicit controlled-testing escape hatch. Source-checkout runtimes and non-development custom issuers keep their existing override behavior. +- `apps/ade-cli/src/services/credentials/credentialStore.ts` — the per-machine + credential store behind the account session. Two implementations share one + interface. `EncryptedFileCredentialStore` owns the AES-GCM + `.ade/secrets/credentials.json.enc` file that the brain, the `ade` CLI, and + the desktop app all read; `ElectronSafeStorageCredentialStore` owns the + Electron-only `credentials.safe.enc`. Because the brain cannot read a + safeStorage file, `FILE_BACKED_CREDENTIAL_KEYS` (`account.session.v1`, + `sync.bootstrapToken.v1`) are pinned to the file store: the safeStorage + migration copies everything *else* across, retains those keys in the file + store, prunes the now-duplicated migrated keys out of it, and keeps + `.machine-key` alive; only a legacy store with nothing retained is deleted. + `setSync`/`updateSync` on the safeStorage store throw if a caller tries to + write one of those keys back into the Electron-only file — otherwise a + machine whose app is signed in silently signs its brain out. A legacy store + that reads back `unreadable` aborts the migration outright rather than + migrating an empty view of it and then deleting the ciphertext and machine + key. Reads try the OS-bound key first and the bare machine key second (a + second-candidate hit is genuine legacy ciphertext and is rewritten); if + neither works, the store self-heals once per 30 s by dropping the cached OS + key material and retrying against a fresh read, then reports + `getLastReadState() === "unreadable"` with a coarse + `getLastReadFailureReason()` of `decrypt_failure`, `no_os_key_material`, or + `store_format`. It never writes an empty store over ciphertext it could not + decrypt. +- `apps/ade-cli/src/services/credentials/osBoundKeyMaterial.ts` — everything + about obtaining the machine-local secret the file store's key is derived + from: the `security` invocations, the process-wide cache, the negative-cache + backoff, and the create race. Resolution is race-safe and non-destructive. + `add-generic-password` runs **without** `-U`, so a process that loses the + create race adopts the winner's item instead of clobbering it, and any + inconclusive `security` result (timeout, locked keychain, denied access) fails + closed rather than minting a replacement — two first-run processes each + minting their own secret is exactly what made one of them unable to decrypt + what the other wrote. The synchronous path may create the item; the + asynchronous path is read-only and never participates in the race. The two + paths use different backoffs: the read-only path backs off on any miss, while + the creating path backs off only when the keychain was *unavailable*, so a + `not_found` miss can never starve first-run item creation. - `apps/account-directory/src/directory.ts` — the Clerk-scoped machine register/list/delete Worker routes. Machine listing selects the owner's 500 most recently seen rows before computing online-first order and exposes @@ -526,7 +575,10 @@ Desktop connection UI: message under **Technical details**: missing project registration asks the user to open a project, a non-installed local release build asks for an Applications install/relaunch, and other sync-service failures ask for an ADE - restart. The local-brain-only + restart. When the account-directory state is the one failure a restart + actually clears — `isBrainAccountSessionFailure(...)` in + `shared/types/sync.ts`, currently exactly `token_unreadable` — the This Mac + card renders a **Repair** control next to the directory summary. The local-brain-only `window.ade.sync.getLocalStatus(...)` accessor is available for the card to consume so a window bound to another machine can still show the physical computer's identity, pairing code, and Phone/Web device lists. @@ -547,6 +599,23 @@ Desktop connection UI: `connectedPeers` (via `peerToRuntimeDeviceState`) instead of the routed `listDevices()` result, which would describe the remote machine; offline-paired rows are unavailable in that mode until a local-scoped device IPC exists. + It also exposes `refresh()` — a one-shot re-read of both snapshots without the + initial-load spinner — which the Repair control uses to re-evaluate its banner + once a restart settles. +- `apps/desktop/src/renderer/hooks/useBrainRepair.ts` and + `apps/desktop/src/renderer/components/settings/BrainRepairButton.tsx` — the + shared **Repair** affordance for a brain that cannot read the stored account + session. The hook calls `window.ade.app.restartBackgroundService()` + (`ade.app.restartBackgroundService`), which restarts this Mac's + `com.ade.runtime` launch agent and resolves only once the replacement answers + a ping — readiness is observable only in the main process, so the renderer + awaits it rather than sleeping and hoping. The IPC is optional in + `global.d.ts`: the hosted-web adapter and browser mock cannot touch a launch + agent, so `repair.available` feature-detects before any surface offers the + button. A rejected restart renders "Repair failed — quit and reopen ADE." + with the technical detail in the `title`; `onSettled` runs on both paths so the + caller's banner is re-derived either way. Both the This Mac card and the + Machines panel's route-publish row mount the same hook and button. - `apps/desktop/src/shared/runtimeErrors.ts` — canonical cross-process error messages and predicates shared by the local-runtime pool, main IPC fallback, preload routing, remote-runtime connection/timeout reconciliation, and the diff --git a/docs/features/sync-and-multi-device/ios-companion.md b/docs/features/sync-and-multi-device/ios-companion.md index 392a249e4..9ca67d327 100644 --- a/docs/features/sync-and-multi-device/ios-companion.md +++ b/docs/features/sync-and-multi-device/ios-companion.md @@ -571,10 +571,11 @@ prefersReducedSyncLoad: lastError:)` and re-exposed through that used to be tangled together: - `transport: SyncTransportHealth` — `disconnected` / `connecting` / - `connected` / `unreachable`. `RemoteConnectionState.syncing` collapses - into `connected` because the connection is alive while the runtime streams a - catchup batch; only `RemoteConnectionState.error` maps to - `unreachable`. + `connected` / `unreachable`. `RemoteConnectionState` has no separate + "syncing" case: attachment is a fact the moment `hello_ok` is applied, and + per-domain hydration progress is carried by `SyncDomainStatus` + (`.syncingInitialData`, `.hydrating`) instead. Only + `RemoteConnectionState.error` maps to `unreachable`. - `load: SyncLoadHealth` — `normal` / `strained`. `strained` is set when the transport is connected but `prefersReducedSyncLoad` is on, i.e. recent request timeouts have caused the phone to back off @@ -701,8 +702,6 @@ the Tailscale app (`tailscale://`), falling back to the App Store page. - Connected, normal load → "Live · ready to sync". - Connected, strained load → "Live · machine responding slowly". -- Connected with `connectionState == .syncing` → "Live · syncing - changes". - `connecting` → "Connecting to saved machine". - `unreachable` → "Unable to reach your machine" plus the `lastFailureMessage` banner. @@ -711,14 +710,16 @@ the Tailscale app (`tailscale://`), falling back to the App Store page. `SettingsConnectionPresentation.statusLabel` returns "Connected, slow" when transport is connected and load is strained, and "Connected" -otherwise. The legacy "Syncing" label was removed — syncing is just -a connected transport doing work. +otherwise. There is no "Syncing" label: syncing is just a connected transport +doing work, and there is no transport state that means it. Accessibility: the dot's `accessibilityLabel` describes load strain -("Connected to . Machine is responding slowly"), explicit syncing -work ("Connected to . Syncing changes"), or plain "Connected to -" when neither applies; for transport `unreachable` it appends -the trimmed `lastFailureMessage`. `accessibilityHint` is "Opens +("Connected to . Machine is responding slowly") or plain "Connected to +" when it does not apply; for transport `unreachable` it appends +the trimmed `lastFailureMessage`. `ConnectionHealthPresentation` takes only the +health value and the host name — it does not see `RemoteConnectionState` at all, +so no surface can reintroduce a hydration-derived label here. +`accessibilityHint` is "Opens settings to pair or reconnect", and `accessibilityShowsLargeContentViewer()` keeps it reachable from VoiceOver and Large Content. @@ -729,8 +730,10 @@ on `RemoteModels.swift`. It surfaces only when a domain is in `.failed` phase (so cached rows may still render underneath) and offers a single "Retry" action that calls `reload(refreshRemote: true)`. The read-only header strip in `FilesHeaderStrip` also appends a -compact "Syncing" / "Connecting" / "Offline" suffix derived directly -from `SyncService.connectionState` and `status(for: .files).phase`. +compact "Syncing" / "Connecting" / "Offline" suffix: "Syncing" comes from the +Files domain phase (`.hydrating` / `.syncingInitialData`), "Connecting" from +`connectionState == .connecting`, and everything else falls through to +"Offline". Hydration progress is read from the domain, not the transport. ## Architectural pattern @@ -1046,11 +1049,26 @@ row explaining why it would not answer" is the honest steady state. A blocked Attention/notification navigation records its reason the same way, so a tap that cannot proceed explains itself rather than silently doing nothing. -`SyncService.isAttached` treats `.syncing` as attached, because that is what -every connect path settles into before reaching `.connected`. The same rule -applies in `syncAccountMachineNavigationIsCurrent`, so a deeplink tapped -mid-hydration does not conclude it is on the wrong machine and re-pair to the -machine it is already talking to. +`SyncService.isAttached` (`connectionState == .connected`) is the single named +attachment predicate for the whole app: every surface that asks "did we get on +the machine?" goes through it rather than re-deriving a comparison, so the +definition stays in one place if the state machine grows again. +`SettingsPinSheet` reads PIN-pairing success off it for exactly that reason — it +used to compare against `.connected` while the connect path was still parked in +the retired `.syncing`, so a successful pairing reported failure. + +Attachment is published at `hello_ok`, not after post-hello restoration. +`applyHelloPayload` sets `.connected`, and `beginPostHelloAttachment` — the +shared prologue for `schedulePostHelloWork` and its DEBUG test hook — +republishes it idempotently. Deferring it to the end of restoration left the app +unattached for a network round trip on every connect, and permanently whenever +the generation guard rejected the completion, which every `isAttached` surface +read as "not connected yet". Only the reconnect-backoff reset stays behind the +guarded completion: that, not `connectionState`, is what must wait for a +proven-usable socket. The same attachment test governs +`syncAccountMachineNavigationIsCurrent`, so a deeplink tapped mid-hydration does +not conclude it is on the wrong machine and re-pair to the machine it is already +talking to. ### Route ranking, route memory, and roaming @@ -1321,7 +1339,12 @@ yet arrived in the catchup batch. `SettingsPinSheet` on iOS mirrors the desktop PIN sheet and handles the entry UX. If the user misreads the digits, the runtime applies -per-IP rate limiting (5 failures → 10-minute cooldown). +per-IP rate limiting (5 failures → 10-minute cooldown). The sheet decides +success with `syncService.isAttached` rather than its own +`connectionState == .connected` comparison: the canonical attachment predicate +is the only thing that stays correct as the transport state machine changes, and +a hand-rolled comparison is exactly what made a *successful* pairing report +failure while the connect path was parked in the (now retired) `.syncing` state. ### Browser access @@ -1365,6 +1388,13 @@ same priority model with compact count/status treatments. The iOS app still updates the shared snapshot and calls `WidgetCenter.shared.reloadAllTimelines()` after snapshot writes. +The snapshot's `connection` field is a wire vocabulary the widget matches on +string, not a mirror of `RemoteConnectionState`: `.connected` serializes to +`"connected"`, `.connecting` to `"syncing"`, and everything else to +`"disconnected"`. Retiring the app-side `.syncing` case did not remove +`"syncing"` from that vocabulary, and `ADELockScreenWidget` / `ADESharedTheme` +still resolve it — do not "clean it up" to match the enum. + Agent rows mirror desktop's shared status vocabulary: blue `Working`, amber `Needs you`, emerald `Done`, red `Failed`, and neutral `Stale`. Amber is reserved for the one state asking the user to act. Syncing, offline hosts, @@ -2499,12 +2529,14 @@ different machine's cached limits. offline may still enter the existing queue with a stable `commandId`; this special case applies only after a live `chat.send` was attempted. - **Connection UI must use `SyncConnectionHealth`, not the raw state.** - `RemoteConnectionState.syncing` is just transport `connected` doing - catchup work, and `RemoteConnectionState.error` carries failure text - that should not bleed into a `disconnected` UI. New connection - affordances should render off `syncService.connectionHealth` so - load-strain and transport failure stay distinct from each other and - from background sync work. + `RemoteConnectionState` describes transport attachment only — there is + deliberately no hydrating/syncing case, and `RemoteConnectionState.error` + carries failure text that should not bleed into a `disconnected` UI. New + connection affordances should render off `syncService.connectionHealth` so + load-strain and transport failure stay distinct from each other, and read + hydration progress from `SyncDomainStatus` rather than inventing a transport + state for it. Anything asking "are we attached?" uses + `SyncService.isAttached`, not its own comparison. - **Chat streaming is push, with seq-based resume.** Once a phone sends `chat_subscribe`, the runtime fans out `chat_event` envelopes in real time from `agentChatService.subscribeToEvents`. Each event diff --git a/docs/logging.md b/docs/logging.md index 87d330f9b..6362fdce6 100644 --- a/docs/logging.md +++ b/docs/logging.md @@ -93,6 +93,7 @@ The public contract is `apps/desktop/src/shared/types/productAnalytics.ts`. The - `ade_brain_recovered` - `ade_publish_failing` - `ade_relay_suppressed` +- `ade_account_session_unreadable` The update and reliability events are low-frequency by construction: the five `ade_update_*` events fire at most once per install attempt or idle-apply cycle (daily caps 10–20, minute caps 3–6). `ade_update_install_did_not_land` is emitted once at startup when a requested install relaunched on the old version, so it is bounded by app launches that follow a failed handoff, and carries only a bounded `attempt` counter; `ade_brain_recovered` fires once per wedge recovery at brain startup; `ade_publish_failing` is edge-triggered once per sustained failure episode (first crossing of two minutes), never per attempt. @@ -107,6 +108,25 @@ daily ceilings. `ade_relay_suppressed` is the same shape for the relay leg. The relay keeps one host control socket per machine and evicts the previous holder, so two ADE brains on one machine can evict each other in a loop until relay is unusable for both. When the tunnel client exhausts its eviction budget and stops dialing, it emits one event carrying only `attempt` (the bounded eviction count) and a coarse `code` (`control_replaced`). It is keyed to the suppression *episode*, not the eviction, so a whole war collapses into one accepted event, and a 24-hour deduplication window bounds it further; a recovered control socket ends the episode so a genuinely new one still reports. The relay URL, machineKey, and raw WebSocket close reason stay in local logs and never reach the payload. Properties are closed enums and bounded numbers — `reason` is allowlisted to the abort-reason constant, `escalation_reason` to `hard_deadline` / `post_staging`, `last_command` is a closed sync-action slug, and `leg`/`code` are the coarse publish classifications. Worst-case combined volume is a handful of events on a very bad day, inside the shared ceiling. +`ade_account_session_unreadable` covers the credential-store half of the same +failure: the desktop app is signed in, but the ADE brain cannot decrypt the +shared `credentials.json.enc` and therefore never publishes the machine to the +account directory. The account-directory publisher emits it once per unreadable +*episode* (a readable status ends the episode) carrying only a coarse `code` for +the read path — `decrypt_failure`, `no_os_key_material`, `store_format`, +`session_parse`, `read_error`, or `unknown`. No paths, key material, ciphertext, +or account identifiers reach the payload, and a 24-hour deduplication window per +code bounds it further. + +Clicking "Repair" on the Connections pane's unreadable-session banner records +the existing `ade_feature_used` event at the IPC owner boundary (where the +restart outcome is known) with `feature: "connections"`, +`action: "brain_repair"`, and a coarse `outcome` (`completed` or `failed`). It +carries no error text, paths, or machine identifiers — the thrown error stays in +the renderer. A per-outcome one-hour deduplication key bounds a click-loop to at +most 24 accepted events per installation per UTC day, inside the existing +`ade_feature_used` and shared ceilings. + The default machine-wide ceiling is 200 accepted events per UTC day, shared across desktop, runtime, TUI, hosted web, and API-originated aggregates. Each event also has a tighter per-day and per-minute ceiling. Capture ingress is capped, noisy events use persisted deduplication windows, the in-memory transport queue is bounded, and the previous day's accepted/drop totals are summarized in at most two budget events per day. Persisted `usage_events` are the preferred source for meaningful user mutations. The exporter is locally at-most-once and uses a random v4 client UUID as the PostHog insert ID; non-random or malformed client IDs are regenerated at the transport boundary. Screen events are limited to project, Hub, lanes, work, PRs, settings, and onboarding arrivals; utility/detail/loading transitions are skipped. The hosted Hub uses the existing `ade_screen_viewed` event with only `screen: "hub"`, `route_kind: "web"`, and `source: "renderer_route"`. Its two-second per-screen deduplication and the existing 12-per-minute, 80-per-day screen limits bound rapid tab switching without raising the shared 200-event ceiling. Reads, renderer commits, polling, heartbeats, stream chunks, terminal bytes, progress updates, retries, and other high-frequency mechanics must not emit product events. diff --git a/scripts/posthog/dashboard-spec.mjs b/scripts/posthog/dashboard-spec.mjs index 0924b2f10..f31fc990e 100644 --- a/scripts/posthog/dashboard-spec.mjs +++ b/scripts/posthog/dashboard-spec.mjs @@ -23,6 +23,7 @@ export const EVENTS = Object.freeze({ BRAIN_RECOVERED: "ade_brain_recovered", PUBLISH_FAILING: "ade_publish_failing", RELAY_SUPPRESSED: "ade_relay_suppressed", + ACCOUNT_SESSION_UNREADABLE: "ade_account_session_unreadable", MARKETING_APP_OPENED: "ade_marketing_app_opened", MARKETING_SCREEN_VIEWED: "ade_marketing_screen_viewed", MARKETING_CTA_CLICKED: "ade_marketing_cta_clicked", @@ -530,12 +531,13 @@ export const dashboardSpec = Object.freeze({ insight( "reliability-incidents", "Reliability incidents", - "Brain wedge recoveries, sustained route-publish failures, and update-flow aborts/escalations. Coarse counts only; command names are closed action slugs and no payload content is ever attached.", + "Brain wedge recoveries, sustained route-publish failures, unreadable brain account sessions, relay suppression by a rival process, and update-flow aborts/escalations. Coarse counts only; command names are closed action slugs and no payload content is ever attached.", trends({ series: [ eventNode(EVENTS.BRAIN_RECOVERED, "Brain recovered from wedge"), eventNode(EVENTS.PUBLISH_FAILING, "Route publish failing"), eventNode(EVENTS.RELAY_SUPPRESSED, "Relay suppressed by rival process"), + eventNode(EVENTS.ACCOUNT_SESSION_UNREADABLE, "Brain account session unreadable"), eventNode(EVENTS.UPDATE_INSTALL_ABORTED, "Update install aborted"), eventNode(EVENTS.UPDATE_QUIT_ESCALATED, "Update quit escalated"), eventNode(EVENTS.UPDATE_INSTALL_DID_NOT_LAND, "Update did not land"),