From 1b05490e916e7ead6d527026d620fda9b625c150 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 4 Aug 2026 02:35:28 -0400 Subject: [PATCH 1/3] Onboarding/pairing reliability: credential-store race hardening, Repair button, iOS PIN fix, .syncing retirement - credentialStore: race-safe keychain material (no -U clobber), decrypt self-heal, migration abort-on-unreadable + retain/prune of file-backed keys, safeStorage write guard - new osBoundKeyMaterial module + episodeAnalytics helper + ade_account_session_unreadable event - desktop: verified brain Repair (restartBrain + IPC), Tailscale info severity, brain_repair analytics - iOS: PIN sheet isAttached fix, .connected at hello-apply, .syncing retired - ade doctor: brain-restart remedy on session failures; docs + dashboard spec updated Co-Authored-By: Claude Fable 5 --- apps/ade-cli/src/commands/doctor.test.ts | 29 + apps/ade-cli/src/commands/doctor.ts | 15 +- .../ade-cli/src/multiProjectRpcServer.test.ts | 2 + .../services/account/accountAuthService.ts | 60 +- .../accountMachinePublisherService.test.ts | 154 +++++ .../account/accountMachinePublisherService.ts | 59 +- .../src/services/account/episodeAnalytics.ts | 51 ++ .../credentials/credentialStore.test.ts | 415 ++++++++++++- .../services/credentials/credentialStore.ts | 552 +++++++++++------- .../credentials/osBoundKeyMaterial.ts | 323 ++++++++++ .../sync/brainProjectActionsSyncHandler.ts | 8 +- .../src/services/sync/syncHostService.test.ts | 16 +- .../analytics/productAnalyticsPolicy.ts | 7 +- .../analytics/productAnalyticsService.test.ts | 31 + .../src/main/services/ipc/registerIpc.ts | 32 + .../localRuntimeConnectionPool.test.ts | 88 +++ .../localRuntimeConnectionPool.ts | 268 ++++++--- .../remoteRuntime/runtimeDiscovery.ts | 11 +- .../runtime/projectRecoveryService.test.ts | 125 ++++ .../runtime/projectRecoveryService.ts | 154 ++++- apps/desktop/src/preload/global.d.ts | 7 + apps/desktop/src/preload/preload.ts | 2 + .../remoteTargets/RemoteTargetList.test.tsx | 85 ++- .../remoteTargets/RemoteTargetList.tsx | 98 +++- .../components/settings/BrainRepairButton.tsx | 39 ++ .../settings/SyncDevicesSection.test.tsx | 118 ++++ .../settings/SyncDevicesSection.tsx | 14 +- .../settings/accountDirectorySummary.test.ts | 19 - .../settings/accountDirectorySummary.ts | 7 +- .../components/settings/useSyncConnections.ts | 2 + .../src/renderer/hooks/useBrainRepair.ts | 61 ++ apps/desktop/src/shared/ipc.ts | 1 + .../src/shared/types/productAnalytics.ts | 1 + .../desktop/src/shared/types/remoteRuntime.ts | 9 +- apps/desktop/src/shared/types/sync.ts | 16 + apps/ios/ADE/Services/SyncService.swift | 134 +++-- .../Views/Components/ADEDesignSystem.swift | 10 - apps/ios/ADE/Views/Cto/CtoRootScreen.swift | 2 +- .../Views/Cto/CtoSessionDestinationView.swift | 2 +- .../ADE/Views/Deeplinks/SendToMacCard.swift | 4 +- .../Views/Files/FilesDetailComponents.swift | 4 +- .../Views/Files/FilesRootScreen+Actions.swift | 2 +- .../ios/ADE/Views/Hub/HubComposerDrawer.swift | 2 +- apps/ios/ADE/Views/Hub/HubQuickConnect.swift | 2 +- apps/ios/ADE/Views/Hub/HubScreen.swift | 6 +- .../ADE/Views/Lanes/LaneDetailScreen.swift | 2 +- .../ADE/Views/Lanes/LaneRootStateViews.swift | 1 - apps/ios/ADE/Views/LanesTabView.swift | 4 +- apps/ios/ADE/Views/PRs/PrDetailScreen.swift | 2 +- apps/ios/ADE/Views/PRs/PrsRootScreen.swift | 2 +- .../Settings/ConnectionSettingsView.swift | 2 +- .../Settings/SettingsConnectionHeader.swift | 2 +- .../ADE/Views/Settings/SettingsPinSheet.swift | 5 +- .../Views/Work/TerminalSessionScreen.swift | 4 +- .../ADE/Views/Work/WorkNewChatScreen.swift | 2 +- apps/ios/ADE/Views/Work/WorkRootScreen.swift | 2 +- .../Work/WorkSessionDestinationView.swift | 2 +- apps/ios/ADETests/ADETests.swift | 37 +- .../SyncAccountConnectRecoveryTests.swift | 19 +- .../ADETests/SyncRecoveryPolicyTests.swift | 26 +- docs/ARCHITECTURE.md | 22 +- .../onboarding-and-settings/README.md | 6 +- docs/features/remote-runtime/README.md | 52 +- .../remote-runtime/internal-architecture.md | 2 +- docs/features/storage-and-recovery/README.md | 2 +- docs/features/sync-and-multi-device/README.md | 73 ++- .../sync-and-multi-device/ios-companion.md | 84 ++- docs/logging.md | 20 + scripts/posthog/dashboard-spec.mjs | 2 + 69 files changed, 2836 insertions(+), 586 deletions(-) create mode 100644 apps/ade-cli/src/services/account/episodeAnalytics.ts create mode 100644 apps/ade-cli/src/services/credentials/osBoundKeyMaterial.ts create mode 100644 apps/desktop/src/renderer/components/settings/BrainRepairButton.tsx delete mode 100644 apps/desktop/src/renderer/components/settings/accountDirectorySummary.test.ts create mode 100644 apps/desktop/src/renderer/hooks/useBrainRepair.ts diff --git a/apps/ade-cli/src/commands/doctor.test.ts b/apps/ade-cli/src/commands/doctor.test.ts index bbec2aee2..1bb5b0046 100644 --- a/apps/ade-cli/src/commands/doctor.test.ts +++ b/apps/ade-cli/src/commands/doctor.test.ts @@ -211,6 +211,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 4f5b28fa0..ad89376e2 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, @@ -588,12 +589,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") { @@ -614,8 +623,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/services/account/accountAuthService.ts b/apps/ade-cli/src/services/account/accountAuthService.ts index ee127fbd4..f31b33eac 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/keychain/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..5bee8027a 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_keychain_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_keychain_material" }, + dedupeKey: "account-session-unreadable:no_keychain_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,80 @@ 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(); + }); +}); 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..0052891ad --- /dev/null +++ b/apps/ade-cli/src/services/account/episodeAnalytics.ts @@ -0,0 +1,51 @@ +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; + args.capture()?.({ + event: args.event, + surface: "api", + properties: input.properties, + dedupeKey: `${args.dedupePrefix}:${input.dedupeValue}`, + minimumIntervalMs: EPISODE_ANALYTICS_MINIMUM_INTERVAL_MS, + }); + }, + 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 cdab152bb..28e0dbe68 100644 --- a/apps/ade-cli/src/services/credentials/credentialStore.test.ts +++ b/apps/ade-cli/src/services/credentials/credentialStore.test.ts @@ -9,7 +9,15 @@ import { EncryptedFileCredentialStore, KeytarCredentialStore, createDefaultCredentialStore, + isFileBackedCredentialKey, } from "./credentialStore"; +import { ACCOUNT_SESSION_CREDENTIAL_KEY } from "../account/accountAuthService"; +import { BOOTSTRAP_TOKEN_KEY } from "../sync/brainProjectActionsSyncHandler"; +import { + createOsBoundKeyMaterialResolver, + resolveMacKeychainMaterialOutcome, + type MacKeychainCommands, +} from "./osBoundKeyMaterial"; let tempDir = ""; @@ -178,20 +186,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(); }); @@ -200,7 +208,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"); @@ -208,8 +216,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"); @@ -226,8 +233,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(); @@ -242,7 +248,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( @@ -255,13 +261,13 @@ new EncryptedFileCredentialStore({ secretsDir }).setSync(key, value); it("can read legacy machine-key ciphertext before rewriting with OS-bound key material", async () => { const legacy = new EncryptedFileCredentialStore({ secretsDir: tempDir, - keyMaterialProvider: () => null, + keyMaterial: { read: () => null }, }); legacy.setSync("agent.token", "legacy_secret"); 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(legacy.getSync("agent.token")).toBe("legacy_secret"); @@ -272,10 +278,74 @@ new EncryptedFileCredentialStore({ secretsDir }).setSync(key, value); 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"); @@ -287,7 +357,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(); @@ -296,7 +366,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"); }); @@ -313,6 +383,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 }); @@ -336,6 +417,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, @@ -431,3 +652,169 @@ describe("createDefaultCredentialStore", () => { expect(fs.existsSync(path.join(tempDir, "credentials.json.enc"))).toBe(true); }); }); + +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("createOsBoundKeyMaterialResolver", () => { + 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 = createOsBoundKeyMaterialResolver({ + 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 = createOsBoundKeyMaterialResolver({ + 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 = createOsBoundKeyMaterialResolver({ + 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 = createOsBoundKeyMaterialResolver({ + 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 = createOsBoundKeyMaterialResolver({ + 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 1f7962a2d..74bfc5717 100644 --- a/apps/ade-cli/src/services/credentials/credentialStore.ts +++ b/apps/ade-cli/src/services/credentials/credentialStore.ts @@ -1,8 +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 { + expectsOsBoundKeyMaterial, + invalidateDefaultOsBoundKeyMaterialCache, + readDefaultOsBoundKeyMaterial, + readDefaultOsBoundKeyMaterialAsync, +} from "./osBoundKeyMaterial"; export interface CredentialStore { get(key: string): Promise; @@ -12,6 +17,19 @@ 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" + /** macOS keychain material was expected but could not be obtained. */ + | "no_keychain_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; @@ -22,6 +40,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 = { @@ -38,8 +58,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"; @@ -47,18 +86,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; -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; @@ -500,155 +562,124 @@ 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 keychain defaults. + */ +export type CredentialKeyMaterialSource = { + read(): Buffer | null; + /** Defaults to an asynchronous wrapper around `read()`. */ + readAsync?(): 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?(): void; +}; -function readDefaultOsBoundKeyMaterial(): 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 (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(): 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 (cachedDefaultOsBoundKeyMaterial) return cachedDefaultOsBoundKeyMaterial; - if ( - lastMissingDefaultOsBoundKeyMaterialAt > 0 - && Date.now() - lastMissingDefaultOsBoundKeyMaterialAt < MACOS_KEYCHAIN_NEGATIVE_CACHE_MS - ) { - return null; - } - 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; +type CredentialDecodeAttempt = + | { ok: true; values: Record; rewriteWithCurrentKey: boolean } + | { 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 }), + rewriteWithCurrentKey: index > 0, + }; + } catch (error) { + lastError = error; } } + const reason: CredentialStoreReadFailureReason = formatIsUnsupported + ? "store_format" + : !osBound && expectsOsBoundKeyMaterial() + ? "no_keychain_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; } = {}) { @@ -656,11 +687,16 @@ export class EncryptedFileCredentialStore implements SyncCredentialStore { this.credentialsPath = args.credentialsPath ?? path.join(secretsDir, DEFAULT_CREDENTIALS_FILE); this.machineKeyPath = args.machineKeyPath ?? path.join(secretsDir, DEFAULT_MACHINE_KEY_FILE); this.lockPath = args.lockPath ?? defaultLockPath(this.credentialsPath); - this.keyMaterialProvider = args.keyMaterialProvider ?? readDefaultOsBoundKeyMaterial; - this.keyMaterialProviderAsync = args.keyMaterialProviderAsync - ?? (args.keyMaterialProvider - ? async () => args.keyMaterialProvider?.() ?? null - : readDefaultOsBoundKeyMaterialAsync); + const keyMaterial = args.keyMaterial ?? DEFAULT_KEY_MATERIAL_SOURCE; + this.readKeyMaterial = () => keyMaterial.read(); + this.readKeyMaterialAsync = async () => ( + keyMaterial.readAsync ? keyMaterial.readAsync() : keyMaterial.read() + ); + // 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?.() + : null; this.credentialChangePollIntervalMs = args.credentialChangePollIntervalMs === undefined ? CREDENTIAL_CHANGE_POLL_INTERVAL_MS : args.credentialChangePollIntervalMs; @@ -694,6 +730,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(); @@ -754,93 +795,120 @@ export class EncryptedFileCredentialStore implements SyncCredentialStore { return this.readAll({ allowRewrite: false }); } + /** + * 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); + } + private readAll(args: { allowRewrite: 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) { - try { - this.writeAll(values); - } catch { - // Preserve read compatibility if migration cannot rewrite right now. - } - } - 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. + 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) { + try { + this.writeAll(attempt.values); + } 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 key = deriveOsBoundCredentialKey(machineKey, await this.keyMaterialProviderAsync()); - if (!key.equals(machineKey)) { - try { - const values = deserializeStore(raw, key, { emptyOnDecryptFailure: false }); - this.lastReadState = "available"; - return values; - } catch { - try { - const values = deserializeStore(raw, machineKey, { emptyOnDecryptFailure: false }); - 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; + 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()); writeFileAtomic(this.credentialsPath, `${JSON.stringify(serializeStore(values, key), null, 2)}\n`); } @@ -867,7 +935,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; @@ -881,7 +949,7 @@ export class ElectronSafeStorageCredentialStore implements SyncCredentialStore { credentialsPath: this.legacyCredentialsPath, machineKeyPath: this.legacyMachineKeyPath, lockPath: this.legacyLockPath, - keyMaterialProvider: args.keyMaterialProvider, + keyMaterial: args.keyMaterial, }) : args.legacyStore; } @@ -910,6 +978,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; @@ -930,8 +1004,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); }); } @@ -985,22 +1067,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); @@ -1019,6 +1141,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..f69e15604 --- /dev/null +++ b/apps/ade-cli/src/services/credentials/osBoundKeyMaterial.ts @@ -0,0 +1,323 @@ +import crypto from "node:crypto"; +import { execFile, spawnSync } from "node:child_process"; + +/** + * OS-bound credential key material. + * + * The encrypted file store derives its key from a machine-local secret held by + * the OS keychain. This module owns everything about obtaining that secret — + * the `security` invocations, the create race, the process-wide cache, and the + * negative-cache backoff — so the credential store itself only deals with + * ciphertext. + */ + +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; +}; + +export 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(): boolean { + if (process.env.ADE_CREDENTIAL_STORE_DISABLE_OS_BINDING === "1") return true; + return process.env.VITEST === "true" || process.env.NODE_ENV === "test"; +} + +/** Is macOS keychain material expected to back this process's credential key? */ +export function expectsOsBoundKeyMaterial(): boolean { + if (process.platform !== "darwin") return false; + if (readCredentialPassphraseFromEnv()) return false; + return !osBindingDisabledByEnv(); +} + +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 createOsBoundKeyMaterialResolver(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 defaultOsBoundKeyMaterialResolver = createOsBoundKeyMaterialResolver({ + read: readOrCreateMacKeychainMaterial, + readAsync: readMacKeychainMaterialAsync, +}); + +/** + * Drops the process-wide keychain-material cache so the next read re-asks the + * keychain. Used when a decrypt fails with cached material: the other process + * may have won the create race after this one cached its own copy. + */ +export function invalidateDefaultOsBoundKeyMaterialCache(): void { + defaultOsBoundKeyMaterialResolver.invalidate(); +} + +export function readDefaultOsBoundKeyMaterial(): Buffer | null { + const envMaterial = readCredentialPassphraseFromEnv(); + if (envMaterial) return envMaterial; + if (osBindingDisabledByEnv()) return null; + return defaultOsBoundKeyMaterialResolver.read(); +} + +export async function readDefaultOsBoundKeyMaterialAsync(): Promise { + const envMaterial = readCredentialPassphraseFromEnv(); + if (envMaterial) return envMaterial; + if (osBindingDisabledByEnv()) return null; + return await defaultOsBoundKeyMaterialResolver.readAsync(); +} 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 fbdc23147..f9093dfa6 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/registerIpc.ts b/apps/desktop/src/main/services/ipc/registerIpc.ts index 21b5eaf0e..5b2486f87 100644 --- a/apps/desktop/src/main/services/ipc/registerIpc.ts +++ b/apps/desktop/src/main/services/ipc/registerIpc.ts @@ -3931,6 +3931,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 acab4c40e..544a9c398 100644 --- a/apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.ts +++ b/apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.ts @@ -405,6 +405,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 7b7186da1..cd9adfb8c 100644 --- a/apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts +++ b/apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts @@ -81,11 +81,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. @@ -97,6 +114,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; @@ -976,9 +997,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; @@ -994,27 +1031,78 @@ 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. + */ + 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, }); 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"); @@ -1026,7 +1114,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) => { @@ -1034,24 +1121,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), + }); }); }); } @@ -1131,72 +1207,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, - }); - 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 ae8b03eae..b66ae4fe4 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"; @@ -417,22 +418,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..9f92484a2 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,127 @@ 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 }); + expect(connectionPool.callSync).toHaveBeenCalledWith("ping", {}); + }); + + 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 }); + }); +}); diff --git a/apps/desktop/src/main/services/runtime/projectRecoveryService.ts b/apps/desktop/src/main/services/runtime/projectRecoveryService.ts index 91122d8b0..947ba9d8c 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,8 @@ 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; constructor(private readonly deps: ProjectRecoveryServiceDeps) { this.socketPath = deps.socketPath ?? resolveMachineAdeLayout({ ...process.env, ADE_HOME: deps.adeHome }).socketPath; @@ -367,6 +408,71 @@ 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 { + await this.deps.connectionPool.callSync("ping", {}); + } 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 outcome = await this.restartServiceAndWait(true); + 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 +550,20 @@ 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 { + 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 +690,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 eaece045b..8d72d1d15 100644 --- a/apps/desktop/src/preload/global.d.ts +++ b/apps/desktop/src/preload/global.d.ts @@ -726,6 +726,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 b0ec72010..4c86d56b7 100644 --- a/apps/desktop/src/preload/preload.ts +++ b/apps/desktop/src/preload/preload.ts @@ -3495,6 +3495,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 5e178bbf4..6a8b045cf 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 Macs 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 Macs 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 091c4bc10..c1e70d58e 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,44 @@ export function RemoteTargetList({ // This Mac'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); + const refreshPublishHealth = useCallback(() => { + const infoPromise = window.ade.app?.getInfo?.(); + if (!infoPromise) return; + void infoPromise + .then((info) => { + if (!publishHealthMountedRef.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 +1139,7 @@ export function RemoteTargetList({ display: "flex", alignItems: "center", gap: 5, + flexWrap: "wrap", color: COLORS.warning, fontFamily: SANS_FONT, fontSize: 11, @@ -1110,6 +1151,7 @@ export function RemoteTargetList({ Other devices may not reach this Mac — route publish failing for{" "} {publishHealthDisplay.minutes} min + {showRepair ? : null} ) : null} @@ -1252,6 +1294,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 f23fa2b2c..f4840ba35 100644 --- a/apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx +++ b/apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx @@ -23,6 +23,7 @@ import { useSyncConnections, type SyncConnections, } from "./SyncDevicesSection"; +import { accountDirectorySummary } from "./accountDirectorySummary"; vi.mock("qrcode.react", () => ({ QRCodeSVG: ({ value, title }: { value: string; title?: string }) => ( @@ -134,10 +135,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", () => { @@ -170,6 +183,67 @@ describe("ThisMacCard", () => { )).toBeTruthy(); }); + 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( @@ -594,3 +668,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 Mac 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 Mac 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 0d2e9fd10..f27c235fa 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, @@ -24,6 +25,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, @@ -178,6 +181,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…
; @@ -208,6 +214,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 (
@@ -247,7 +258,7 @@ export function ThisMacCard({ This Mac
-
+
{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 c76aad775..336550fa3 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 734314e43..e2e336017 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 7c6c23d2d..27d8b2b44 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 ae1bb5eb0..bb2e37cb8 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 Mac" @@ -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.") } @@ -9283,7 +9287,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 { @@ -9297,7 +9301,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" } @@ -15251,35 +15255,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, @@ -16020,23 +16057,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( @@ -16306,7 +16347,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, @@ -16443,7 +16488,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) { @@ -18647,7 +18692,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 { @@ -19385,8 +19430,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 af13c7b17..e32eec5c4 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 4486bca07..13f213f27 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 524d38236..9b86e6a06 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 3d8c871e1..b16ccfea3 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 e034a4535..c45979169 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 0a8f440a8..0e34debd9 100644 --- a/apps/ios/ADE/Views/Work/WorkRootScreen.swift +++ b/apps/ios/ADE/Views/Work/WorkRootScreen.swift @@ -268,7 +268,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 6b517537a..f7d44716a 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 719394358..6eb0be89b 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,7 +1269,9 @@ 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 { @@ -1272,15 +1279,20 @@ final class SyncRecoveryPolicyTests: XCTestCase { } } while !restoration.isWaiting { await Task.yield() } - XCTAssertEqual(service.connectionState, .syncing) + 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) @@ -1302,9 +1314,9 @@ final class SyncRecoveryPolicyTests: XCTestCase { while !restoration.isWaiting { await Task.yield() } service.teardownSocketForTesting() restoration.resume() - await postHello.value + let completed = await postHello.value - XCTAssertNotEqual(service.connectionState, .connected) + XCTAssertFalse(completed) } func testTerminalInputQueuePreservesOrderAckAndReconnectIds() throws { diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 90fefe180..5cb1499c2 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -653,7 +653,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). @@ -1012,6 +1012,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 | @@ -1020,6 +1022,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 ddc480457..6e0fa59f2 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 f0967cb80..72e3b0f85 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-Mac 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, @@ -700,7 +743,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 09afedf93..912b46299 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 15d4d0dee..70e26b184 100644 --- a/docs/features/sync-and-multi-device/README.md +++ b/docs/features/sync-and-multi-device/README.md @@ -386,7 +386,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_keychain_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 @@ -427,6 +438,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_keychain_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 @@ -518,7 +567,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 Mac's identity, pairing code, and Phone/Web device lists. @@ -539,6 +591,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 611b21b84..475d2c143 100644 --- a/docs/features/sync-and-multi-device/ios-companion.md +++ b/docs/features/sync-and-multi-device/ios-companion.md @@ -554,10 +554,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 @@ -684,8 +685,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. @@ -694,14 +693,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. @@ -712,8 +713,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 @@ -1029,11 +1032,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 @@ -1304,7 +1322,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 @@ -1348,6 +1371,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, @@ -2384,12 +2414,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 6ad5bffd9..bb5ff1bd2 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_keychain_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..28a418a7b 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", @@ -536,6 +537,7 @@ export const dashboardSpec = Object.freeze({ 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"), From 14063b76e5b57543bd05a6f2e530be39c91bb946 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 4 Aug 2026 03:35:44 -0400 Subject: [PATCH 2/3] ship: apply merge-revalidation quality fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows DPAPI cache invalidation now carries an epoch so a read that started before an invalidation cannot repopulate the cache the self-heal just cleared — the macOS resolver already guarded this, the Windows half did not. Also: require the key-binding directory on the Windows invalidate, name that parameter for what it is (keyBindingDir, the machine key's directory), name the macOS-only resolver accordingly, restore the Windows no-negative-cache and in-flight-dedup rationale lost in the merge, rename no_keychain_material to no_os_key_material (unreachable on win32 under the old meaning; never shipped), drop a dead export, and document the Windows self-heal lock-hold cost. Co-Authored-By: Claude Fable 5 --- .../services/account/accountAuthService.ts | 2 +- .../accountMachinePublisherService.test.ts | 6 +- .../credentials/credentialStore.test.ts | 31 ++--- .../services/credentials/credentialStore.ts | 51 ++++---- .../credentials/osBoundKeyMaterial.ts | 37 +++--- .../credentials/windowsDpapiMaterial.test.ts | 111 ++++++++++++++++++ .../credentials/windowsDpapiMaterial.ts | 65 ++++++---- docs/features/sync-and-multi-device/README.md | 4 +- docs/logging.md | 2 +- 9 files changed, 219 insertions(+), 90 deletions(-) create mode 100644 apps/ade-cli/src/services/credentials/windowsDpapiMaterial.test.ts diff --git a/apps/ade-cli/src/services/account/accountAuthService.ts b/apps/ade-cli/src/services/account/accountAuthService.ts index f31b33eac..48b80773b 100644 --- a/apps/ade-cli/src/services/account/accountAuthService.ts +++ b/apps/ade-cli/src/services/account/accountAuthService.ts @@ -113,7 +113,7 @@ export type AccountSessionReadState = "available" | "missing" | "unreadable"; * be reported as a product-analytics property. */ export type AccountSessionReadFailureReason = - /** Everything the credential store itself can report (decrypt/keychain/format). */ + /** 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" diff --git a/apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts b/apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts index 5bee8027a..f6ff6a75b 100644 --- a/apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts +++ b/apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts @@ -438,7 +438,7 @@ describe("account machine publisher health", () => { signedIn: sessionReadState === "available", sessionReadState, sessionReadFailureReason: sessionReadState === "unreadable" - ? ("no_keychain_material" as const) + ? ("no_os_key_material" as const) : null, }), getSnapshot: async () => snapshot(), @@ -455,8 +455,8 @@ describe("account machine publisher health", () => { expect(captureAnalytics).toHaveBeenCalledWith({ event: "ade_account_session_unreadable", surface: "api", - properties: { code: "no_keychain_material" }, - dedupeKey: "account-session-unreadable:no_keychain_material", + properties: { code: "no_os_key_material" }, + dedupeKey: "account-session-unreadable:no_os_key_material", minimumIntervalMs: 24 * 60 * 60 * 1_000, }); diff --git a/apps/ade-cli/src/services/credentials/credentialStore.test.ts b/apps/ade-cli/src/services/credentials/credentialStore.test.ts index a5198a8ba..c03643c5d 100644 --- a/apps/ade-cli/src/services/credentials/credentialStore.test.ts +++ b/apps/ade-cli/src/services/credentials/credentialStore.test.ts @@ -14,14 +14,12 @@ import { import { ACCOUNT_SESSION_CREDENTIAL_KEY } from "../account/accountAuthService"; import { BOOTSTRAP_TOKEN_KEY } from "../sync/brainProjectActionsSyncHandler"; import { - createOsBoundKeyMaterialResolver, - invalidateDefaultOsBoundKeyMaterialCache, + createMacKeychainMaterialResolver, resolveMacKeychainMaterialOutcome, resolveOsBoundKeyMaterialBinding, type MacKeychainCommands, } from "./osBoundKeyMaterial"; import { - invalidateWindowsDpapiMaterial, readOrCreateWindowsDpapiMaterial, readOrCreateWindowsDpapiMaterialAsync, resolveWindowsDpapiPowerShellPath, @@ -808,21 +806,6 @@ describe("resolveOsBoundKeyMaterialBinding", () => { })).toBe("env_passphrase"); }, ); - - it("scopes the Windows self-heal invalidation to one secrets directory", () => { - // Self-heal is only self-heal if the invalidation drops the cache that - // actually backs THIS platform's reader. DPAPI material is per directory, - // so the hook the source wires in has to take one — a global-only drop - // would evict every other store's key on every failed decrypt. - const secretsDir = path.join(tempDir, "dpapi-invalidate"); - expect(invalidateWindowsDpapiMaterial.length).toBe(1); - expect(() => invalidateWindowsDpapiMaterial(secretsDir)).not.toThrow(); - expect(() => invalidateWindowsDpapiMaterial()).not.toThrow(); - // Routed through the shared dispatch, so it is inert under the test env - // gate no matter which platform runs the suite. - expect(() => invalidateDefaultOsBoundKeyMaterialCache(secretsDir)).not.toThrow(); - expect(resolveOsBoundKeyMaterialBinding()).toBe("disabled"); - }); }); describe("resolveMacKeychainMaterialOutcome", () => { @@ -892,12 +875,12 @@ describe("resolveMacKeychainMaterialOutcome", () => { }); }); -describe("createOsBoundKeyMaterialResolver", () => { +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 = createOsBoundKeyMaterialResolver({ + const resolver = createMacKeychainMaterialResolver({ read, readAsync: async () => ({ material: null, reason: "unavailable" as const }), }); @@ -916,7 +899,7 @@ describe("createOsBoundKeyMaterialResolver", () => { 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 = createOsBoundKeyMaterialResolver({ + const resolver = createMacKeychainMaterialResolver({ read, readAsync, now: () => now, @@ -936,7 +919,7 @@ describe("createOsBoundKeyMaterialResolver", () => { 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 = createOsBoundKeyMaterialResolver({ + const resolver = createMacKeychainMaterialResolver({ read, readAsync: async () => ({ material: null, reason: "unavailable" as const }), now: () => now, @@ -960,7 +943,7 @@ describe("createOsBoundKeyMaterialResolver", () => { const read = vi.fn(() => (current ? { material: current } : { material: null, reason: "unavailable" as const })); - const resolver = createOsBoundKeyMaterialResolver({ + const resolver = createMacKeychainMaterialResolver({ read, readAsync: async () => ({ material: null, reason: "unavailable" as const }), now: () => now, @@ -978,7 +961,7 @@ describe("createOsBoundKeyMaterialResolver", () => { it("coalesces concurrent read-only resolutions into one OS call", async () => { const readAsync = vi.fn(async () => ({ material })); - const resolver = createOsBoundKeyMaterialResolver({ + const resolver = createMacKeychainMaterialResolver({ read: () => ({ material: null, reason: "unavailable" as const }), readAsync, }); diff --git a/apps/ade-cli/src/services/credentials/credentialStore.ts b/apps/ade-cli/src/services/credentials/credentialStore.ts index 8c2a01c8a..cbc270694 100644 --- a/apps/ade-cli/src/services/credentials/credentialStore.ts +++ b/apps/ade-cli/src/services/credentials/credentialStore.ts @@ -26,10 +26,12 @@ export type CredentialStoreReadFailureReason = /** The ciphertext exists but no available key decrypts it. */ | "decrypt_failure" /** - * OS-held key material was expected but could not be obtained — the macOS - * keychain item on darwin, the DPAPI-protected key file on win32. + * 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_keychain_material" + | "no_os_key_material" /** The file exists but is not a recognised credential envelope. */ | "store_format"; @@ -603,21 +605,22 @@ function isSameKeyMaterial(left: Buffer | null, right: Buffer | null): boolean { * material. Injecting the trio together makes that contract explicit; omitting * the whole object takes the process-wide OS defaults. * - * Every member receives the store's secrets directory. 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. + * 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(secretsDir: string): Buffer | null; + read(keyBindingDir: string): Buffer | null; /** Defaults to an asynchronous wrapper around `read()`. */ - readAsync?(secretsDir: string): Promise; + 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?(secretsDir: string): void; + invalidate?(keyBindingDir: string): void; }; const DEFAULT_KEY_MATERIAL_SOURCE: Required = { @@ -673,7 +676,7 @@ function decodeCredentialStore( const reason: CredentialStoreReadFailureReason = formatIsUnsupported ? "store_format" : !osBound && expectsOsBoundKeyMaterial() - ? "no_keychain_material" + ? "no_os_key_material" : "decrypt_failure"; return { ok: false, error: lastError, osBound, reason }; } @@ -722,21 +725,17 @@ export class EncryptedFileCredentialStore implements SyncCredentialStore { 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); const keyMaterial = args.keyMaterial ?? DEFAULT_KEY_MATERIAL_SOURCE; - // The key material is selected by the directory the MACHINE KEY lives in, - // not by `secretsDir`: a store given an explicit `machineKeyPath` outside - // `secretsDir` keeps its whole key derivation — machine key and Windows - // DPAPI file alike — in one directory. - this.readKeyMaterial = () => keyMaterial.read(osBindingDir); + this.readKeyMaterial = () => keyMaterial.read(keyBindingDir); this.readKeyMaterialAsync = async () => ( - keyMaterial.readAsync ? keyMaterial.readAsync(osBindingDir) : keyMaterial.read(osBindingDir) + 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?.(osBindingDir) + ? () => keyMaterial.invalidate?.(keyBindingDir) : null; this.credentialChangePollIntervalMs = args.credentialChangePollIntervalMs === undefined ? CREDENTIAL_CHANGE_POLL_INTERVAL_MS @@ -873,6 +872,18 @@ export class EncryptedFileCredentialStore implements SyncCredentialStore { // 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, diff --git a/apps/ade-cli/src/services/credentials/osBoundKeyMaterial.ts b/apps/ade-cli/src/services/credentials/osBoundKeyMaterial.ts index df3a0c607..3bf5121e3 100644 --- a/apps/ade-cli/src/services/credentials/osBoundKeyMaterial.ts +++ b/apps/ade-cli/src/services/credentials/osBoundKeyMaterial.ts @@ -16,11 +16,12 @@ import { * itself only deals with ciphertext. * * The two supported bindings are shaped differently, and the difference is why - * every entry point takes a `secretsDir`: - * - macOS holds ONE global keychain item for the machine, so `secretsDir` is - * ignored there. - * - Windows holds a DPAPI-protected key file PER secrets directory - * (`/.credential-key.dpapi`), so the directory selects which + * 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. */ @@ -71,7 +72,7 @@ export type MacKeychainCommands = { add: (secret: string) => MacKeychainAddOutcome; }; -export function readCredentialPassphraseFromEnv(): Buffer | null { +function readCredentialPassphraseFromEnv(): Buffer | null { const passphrase = process.env.ADE_CREDENTIAL_STORE_PASSPHRASE?.trim(); return passphrase ? Buffer.from(passphrase, "utf8") : null; } @@ -88,7 +89,7 @@ export type OsBoundKeyMaterialBinding = | "env_passphrase" /** Explicit opt-out, or a test process that must not touch the real OS store. */ | "disabled" - /** `/.credential-key.dpapi`, protected per directory. */ + /** `/.credential-key.dpapi`, protected per directory. */ | "windows_dpapi" /** One global `security` generic-password item per machine. */ | "macos_keychain" @@ -279,7 +280,7 @@ export type OsBoundKeyMaterialResolver = { * suppressing creation for it would starve first-run item creation forever * because the async path refreshes the miss timestamp on every read. */ -export function createOsBoundKeyMaterialResolver(args: { +export function createMacKeychainMaterialResolver(args: { read: () => OsBoundKeyMaterialResolution; readAsync: () => Promise; now?: () => number; @@ -344,7 +345,7 @@ export function createOsBoundKeyMaterialResolver(args: { }; } -const defaultOsBoundKeyMaterialResolver = createOsBoundKeyMaterialResolver({ +const defaultMacKeychainMaterialResolver = createMacKeychainMaterialResolver({ read: readOrCreateMacKeychainMaterial, readAsync: readMacKeychainMaterialAsync, }); @@ -359,13 +360,13 @@ const defaultOsBoundKeyMaterialResolver = createOsBoundKeyMaterialResolver({ * 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(secretsDir: string): void { +export function invalidateDefaultOsBoundKeyMaterialCache(keyBindingDir: string): void { switch (resolveOsBoundKeyMaterialBinding()) { case "windows_dpapi": - invalidateWindowsDpapiMaterial(secretsDir); + invalidateWindowsDpapiMaterial(keyBindingDir); return; case "macos_keychain": - defaultOsBoundKeyMaterialResolver.invalidate(); + defaultMacKeychainMaterialResolver.invalidate(); return; default: // Nothing is cached when no OS binding is in play. @@ -381,31 +382,31 @@ export function invalidateDefaultOsBoundKeyMaterialCache(secretsDir: string): vo * PowerShell timeout would read a DPAPI-bound store as empty and, on the write * path, silently re-seal it unbound. */ -export function readDefaultOsBoundKeyMaterial(secretsDir: string): Buffer | null { +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(secretsDir); + return readOrCreateWindowsDpapiMaterial(keyBindingDir); case "macos_keychain": - return defaultOsBoundKeyMaterialResolver.read(); + return defaultMacKeychainMaterialResolver.read(); default: return null; } } export async function readDefaultOsBoundKeyMaterialAsync( - secretsDir: string, + keyBindingDir: string, ): Promise { switch (resolveOsBoundKeyMaterialBinding()) { case "env_passphrase": return readCredentialPassphraseFromEnv(); case "windows_dpapi": - return await readOrCreateWindowsDpapiMaterialAsync(secretsDir); + return await readOrCreateWindowsDpapiMaterialAsync(keyBindingDir); case "macos_keychain": - return await defaultOsBoundKeyMaterialResolver.readAsync(); + 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 73e613507..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,30 +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 so the next read re-runs the - * unprotect against whatever is on disk now. + * 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; the cache write it performs is superseded by the next invalidation. + * decrypt keeps failing against material that is already known to be wrong. * - * Omit `secretsDir` to drop every entry. + * 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(secretsDir?: string): void { - if (secretsDir === undefined) { - cachedKeyMaterial.clear(); - return; - } - cachedKeyMaterial.delete(protectedKeyPath(secretsDir)); +export function invalidateWindowsDpapiMaterial(keyBindingDir: string): void { + dpapiEpoch += 1; + cachedKeyMaterial.delete(protectedKeyPath(keyBindingDir)); } function unprotectKey(keyPath: string): Buffer { @@ -260,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); @@ -284,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 { @@ -316,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/docs/features/sync-and-multi-device/README.md b/docs/features/sync-and-multi-device/README.md index 603379dde..496f59f09 100644 --- a/docs/features/sync-and-multi-device/README.md +++ b/docs/features/sync-and-multi-device/README.md @@ -403,7 +403,7 @@ Runtime support files outside `services/sync/`: 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_keychain_material`, `store_format`, `session_parse`, + (`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 @@ -467,7 +467,7 @@ Runtime support files outside `services/sync/`: 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_keychain_material`, or + `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 diff --git a/docs/logging.md b/docs/logging.md index 52a81f483..6362fdce6 100644 --- a/docs/logging.md +++ b/docs/logging.md @@ -113,7 +113,7 @@ 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_keychain_material`, `store_format`, +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. From f748d404e09ce869ac727bb9171e953277f2e392 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 4 Aug 2026 04:10:30 -0400 Subject: [PATCH 3/3] ship: address review findings on the repair and analytics paths - serialize repair() behind an in-flight restartBrain() instead of racing it into exclusive database work, with a bounded wait so the recovery screen never parks; repair still runs whatever the restart did - bound the readiness ping (RuntimeRpcClient's default is 10 minutes) and give the restart IPC a budget that covers a forced install queued behind an in-flight one - keep analytics capture best-effort so a throwing sink cannot rewrite a token_unreadable outcome as transport_error - drop stale publish-health responses with a request generation - bound two unbounded waits in SyncRecoveryPolicyTests - name relay suppression in the reliability dashboard description Co-Authored-By: Claude Fable 5 --- .../accountMachinePublisherService.test.ts | 13 +++++ .../src/services/account/episodeAnalytics.ts | 22 ++++--- .../src/main/services/ipc/ipcTimeouts.test.ts | 11 ++++ .../src/main/services/ipc/ipcTimeouts.ts | 8 +++ .../runtime/projectRecoveryService.test.ts | 57 ++++++++++++++++++- .../runtime/projectRecoveryService.ts | 35 +++++++++++- .../remoteTargets/RemoteTargetList.tsx | 6 ++ .../ADETests/SyncRecoveryPolicyTests.swift | 26 ++++++++- scripts/posthog/dashboard-spec.mjs | 2 +- 9 files changed, 167 insertions(+), 13 deletions(-) diff --git a/apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts b/apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts index f6ff6a75b..2a6c102c2 100644 --- a/apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts +++ b/apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts @@ -1493,4 +1493,17 @@ describe("createEpisodeAnalytics", () => { 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/episodeAnalytics.ts b/apps/ade-cli/src/services/account/episodeAnalytics.ts index 0052891ad..a325649be 100644 --- a/apps/ade-cli/src/services/account/episodeAnalytics.ts +++ b/apps/ade-cli/src/services/account/episodeAnalytics.ts @@ -36,13 +36,21 @@ export function createEpisodeAnalytics(args: { report(input): void { if (emitted) return; emitted = true; - args.capture()?.({ - event: args.event, - surface: "api", - properties: input.properties, - dedupeKey: `${args.dedupePrefix}:${input.dedupeValue}`, - minimumIntervalMs: EPISODE_ANALYTICS_MINIMUM_INTERVAL_MS, - }); + // `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/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/runtime/projectRecoveryService.test.ts b/apps/desktop/src/main/services/runtime/projectRecoveryService.test.ts index 9f92484a2..cf81d77ed 100644 --- a/apps/desktop/src/main/services/runtime/projectRecoveryService.test.ts +++ b/apps/desktop/src/main/services/runtime/projectRecoveryService.test.ts @@ -327,7 +327,10 @@ describe("ProjectRecoveryService.restartBrain", () => { await expect(service.restartBrain()).resolves.toBeUndefined(); expect(connectionPool.installServiceBestEffort).toHaveBeenCalledWith({ forceRestart: true }); - expect(connectionPool.callSync).toHaveBeenCalledWith("ping", {}); + // 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 = ( @@ -429,4 +432,56 @@ describe("ProjectRecoveryService.restartBrain", () => { 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 b123729ed..60fd3f31f 100644 --- a/apps/desktop/src/main/services/runtime/projectRecoveryService.ts +++ b/apps/desktop/src/main/services/runtime/projectRecoveryService.ts @@ -367,6 +367,14 @@ export class ProjectRecoveryService { 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; @@ -440,7 +448,10 @@ export class ProjectRecoveryService { return { ok: false, reason: "restart_error", detail: errorMessage(error) }; } try { - await this.deps.connectionPool.callSync("ping", {}); + // 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) }; } @@ -461,7 +472,15 @@ export class ProjectRecoveryService { */ async restartBrain(): Promise { if (this.repairInFlight) throw new Error("Recovery is already running."); - const outcome = await this.restartServiceAndWait(true); + 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": @@ -555,6 +574,18 @@ export class ProjectRecoveryService { // 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; diff --git a/apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsx b/apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsx index 6d6a3dcbe..e6fdaee2b 100644 --- a/apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsx +++ b/apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsx @@ -462,12 +462,18 @@ export function RemoteTargetList({ // 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 diff --git a/apps/ios/ADETests/SyncRecoveryPolicyTests.swift b/apps/ios/ADETests/SyncRecoveryPolicyTests.swift index 5b2856e02..b488c993c 100644 --- a/apps/ios/ADETests/SyncRecoveryPolicyTests.swift +++ b/apps/ios/ADETests/SyncRecoveryPolicyTests.swift @@ -1278,7 +1278,7 @@ final class SyncRecoveryPolicyTests: XCTestCase { await restoration.wait() } } - while !restoration.isWaiting { await Task.yield() } + await restoration.waitUntilWaiting() XCTAssertEqual(service.connectionState, .connected) XCTAssertTrue(service.isAttached) @@ -1311,7 +1311,7 @@ final class SyncRecoveryPolicyTests: XCTestCase { await restoration.wait() } } - while !restoration.isWaiting { await Task.yield() } + await restoration.waitUntilWaiting() service.teardownSocketForTesting() restoration.resume() let completed = await postHello.value @@ -1900,6 +1900,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/scripts/posthog/dashboard-spec.mjs b/scripts/posthog/dashboard-spec.mjs index 28a418a7b..f31fc990e 100644 --- a/scripts/posthog/dashboard-spec.mjs +++ b/scripts/posthog/dashboard-spec.mjs @@ -531,7 +531,7 @@ 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"),