Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions clients/chrome-extension/background/worker.ts
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 SSE onClose auth-error path sets shouldConnect=false but doesn't clear alarm

In the onClose callback at worker.ts:620-631, when an auth error is received, shouldConnect is set to false and sseConnection is nulled, but clearKeepaliveAlarm() is not called. The alarm will continue firing every 30 seconds. The alarm listener at line 964 checks shouldConnect first, so it won't attempt reconnection — the alarm is functionally inert but still wakes the worker. This is similar to the cloud-logout/self-hosted-disconnect omission reported as BUG-0004, but this path is slightly different: the user hasn't explicitly stopped, and a re-auth flow may follow. Clearing the alarm here would be correct but is a less clear-cut omission since the user may re-authenticate shortly.

(Refers to lines 620-631)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7c3b66c33d. The SSE onClose auth-error branch now void clearKeepaliveAlarm() alongside setting shouldConnect = false. Auth-required is a hard stop until the user re-signs-in, so there's no value in keeping the worker awake.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already addressed in 7c3b66c — both cloud-logout and self-hosted-disconnect handlers call await clearKeepaliveAlarm() immediately after disconnect() and before persisting setAutoConnect(false). SSE auth-error close is also covered (cleared on auth_required health transition).

Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,10 @@ function createSseConnection(mode: SseMode): SseConnection {
);
if (authError) {
shouldConnect = false;
// Auth-required is a hard stop: no automatic reconnect will
// succeed until the user re-signs-in, so let the worker idle
// out instead of waking every 30 s.
void clearKeepaliveAlarm();
setConnectionHealth("auth_required", {
lastErrorMessage: authError,
});
Expand Down Expand Up @@ -896,6 +900,54 @@ function disconnect(): void {
}
}

// ── Keep-alive (MV3 service-worker liveness) ─────────────────────────

const KEEPALIVE_ALARM_NAME = "vellum-relay-keepalive";
const KEEPALIVE_PERIOD_MIN = 0.5;

async function ensureKeepaliveAlarm(): Promise<void> {
const existing = await chrome.alarms.get(KEEPALIVE_ALARM_NAME);
if (existing) return;
await chrome.alarms.create(KEEPALIVE_ALARM_NAME, {
periodInMinutes: KEEPALIVE_PERIOD_MIN,
});
}

async function clearKeepaliveAlarm(): Promise<void> {
await chrome.alarms.clear(KEEPALIVE_ALARM_NAME);
}

chrome.alarms.onAlarm.addListener((alarm) => {
if (alarm.name !== KEEPALIVE_ALARM_NAME) return;
if (shouldConnect && !(sseConnection?.isOpen() ?? false)) {
void connect({ interactive: false }).catch((err) => {
const detail = err instanceof Error ? err.message : String(err);
console.warn(`[vellum-relay] Keepalive reconnect failed: ${detail}`);
});
}
});

// On install/update, only register the alarm if we already have an
// active auto-connect intent (e.g. an update installing over a
// connected install). For a fresh install with no prior connect,
// the alarm is created when the user first presses Connect — that
// avoids burning a wake-up every 30 s on installs that never connect.
chrome.runtime.onInstalled.addListener(() => {
void chrome.storage.local.get(AUTO_CONNECT_KEY).then((result) => {
if (result[AUTO_CONNECT_KEY] === true) {
void ensureKeepaliveAlarm();
}
});
});
Comment on lines +935 to +941
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Gate install-time keepalive alarm on connection intent

Creating the keepalive alarm unconditionally in onInstalled starts a perpetual 30s wake cycle even for users who have never connected (or intentionally remain disconnected), because bootstrap() returns early when autoConnect is false and never clears this alarm. This adds continuous background wakeups and battery/CPU overhead for idle installs; only create the alarm when autoConnect is true (or after a successful connect).

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7c3b66c33d. onInstalled now mirrors onStartup — it reads AUTO_CONNECT_KEY and only ensures the alarm if the user already had auto-connect set (e.g. an update over a connected install). For a fresh install with no prior connect, the alarm gets created on the first user-initiated Connect, so we don't burn a wake-up every 30 s on installs that never connect.

Comment on lines +935 to +941
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 onInstalled unconditionally creates keepalive alarm, wasting resources on fresh installs

The onInstalled listener at line 976 calls ensureKeepaliveAlarm() unconditionally — on fresh installs, Chrome updates, and extension updates — even when autoConnect is false. This contradicts the stated design at line 928 ("The alarm is created on first connect and cleared on pause so idle (paused) installs don't pay a wake-up cost every 30 s"). On a fresh install, autoConnect is unset and shouldConnect is false, so the alarm fires every 30 seconds doing nothing. No code path clears it until the user explicitly connects and later pauses. Compare with the onStartup listener at lines 984–990 which correctly gates on autoConnect.

Suggested change
chrome.runtime.onInstalled.addListener(() => {
void ensureKeepaliveAlarm();
});
chrome.runtime.onInstalled.addListener(() => {
void chrome.storage.local.get(AUTO_CONNECT_KEY).then((result) => {
if (result[AUTO_CONNECT_KEY] === true) {
void ensureKeepaliveAlarm();
}
});
});
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7c3b66c33d (same change as the Codex finding above). onInstalled is now gated on AUTO_CONNECT_KEY === true so a fresh install with no connection intent doesn't burn a wake-up every 30 s.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already addressed in 7c3b66conInstalled reads AUTO_CONNECT_KEY from chrome.storage.local and only calls ensureKeepaliveAlarm() when it's true, matching onStartup. Fresh installs with no prior connect won't burn wake-ups. (You can see the gated form on lines 994–1000 of the latest worker.ts.)


chrome.runtime.onStartup.addListener(() => {
void chrome.storage.local.get(AUTO_CONNECT_KEY).then((result) => {
if (result[AUTO_CONNECT_KEY] === true) {
void ensureKeepaliveAlarm();
}
});
});

// ── Extension message listener (from popup) ─────────────────────────

chrome.runtime.onMessage.addListener((message, _sender, sendResponseFn) => {
Expand All @@ -910,6 +962,7 @@ chrome.runtime.onMessage.addListener((message, _sender, sendResponseFn) => {
// was in-flight — their pause intent takes precedence.
if (shouldConnect) {
await setAutoConnect(true);
await ensureKeepaliveAlarm();
}
sendResponseFn({ ok: true });
})
Expand All @@ -922,6 +975,7 @@ chrome.runtime.onMessage.addListener((message, _sender, sendResponseFn) => {
// must not leave the flag set, otherwise the next bootstrap
// would retry a doomed connect.
await setAutoConnect(false);
await clearKeepaliveAlarm();
const serializedError = serializeWorkerError(err);
const errorMessage = serializedError.error;
// Classify the failure: auth-related errors (MissingTokenError)
Expand All @@ -947,6 +1001,7 @@ chrome.runtime.onMessage.addListener((message, _sender, sendResponseFn) => {
if (message.type === "pause" || message.type === "disconnect") {
shouldConnect = false;
setConnectionHealth("paused");
void clearKeepaliveAlarm();
// Await the storage write so MV3 can't terminate the worker before
// the autoConnect flag is persisted to false.
setAutoConnect(false)
Expand Down Expand Up @@ -1238,6 +1293,7 @@ chrome.runtime.onMessage.addListener((message, _sender, sendResponseFn) => {
disconnect();
setConnectionHealth("paused");
clearEventLog();
await clearKeepaliveAlarm();
await setAutoConnect(false);
await clearSession();
await clearSelectedAssistant();
Expand All @@ -1253,6 +1309,7 @@ chrome.runtime.onMessage.addListener((message, _sender, sendResponseFn) => {
disconnect();
setConnectionHealth("paused");
clearEventLog();
await clearKeepaliveAlarm();
await setAutoConnect(false);
await clearStoredUserMode();
sendResponseFn({ ok: true });
Expand Down Expand Up @@ -1325,6 +1382,7 @@ async function bootstrap(): Promise<void> {
const result = await chrome.storage.local.get(AUTO_CONNECT_KEY);
if (result[AUTO_CONNECT_KEY] !== true) return;
shouldConnect = true;
await ensureKeepaliveAlarm();
try {
await connect({ interactive: false });
} catch (err) {
Expand All @@ -1333,6 +1391,7 @@ async function bootstrap(): Promise<void> {
// sign in / pair to try again. Persist the error detail exactly
// once so the popup can surface it, then stop retrying.
shouldConnect = false;
void clearKeepaliveAlarm();
if (err instanceof MissingTokenError) {
console.warn(`[vellum-relay] Skipping auto-connect: ${err.message}`);
setConnectionHealth("auth_required", {
Expand Down
3 changes: 2 additions & 1 deletion clients/chrome-extension/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@
"manifest_version": 3,
"name": "Vellum Assistant",
"version": "0.8.0",
"minimum_chrome_version": "116",
"minimum_chrome_version": "120",
"description": "Bridges the Vellum assistant to your live browser session via Chrome DevTools Protocol (CDP) through chrome.debugger.",
"homepage_url": "https://www.vellum.ai",
"permissions": [
"alarms",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @noanflaherty are we as sensitive to new perms now that we are post launch?

"debugger",
"identity",
"storage",
Expand Down
47 changes: 47 additions & 0 deletions clients/chrome-extension/types/chrome-globals.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,34 @@ interface ChromeRuntimeManifest {
[key: string]: unknown;
}

interface ChromeRuntimeOnInstalledDetails {
reason: "install" | "update" | "chrome_update" | "shared_module_update";
previousVersion?: string;
id?: string;
}

interface ChromeRuntimeOnInstalledEvent {
addListener(
listener: (details: ChromeRuntimeOnInstalledDetails) => void,
): void;
removeListener(
listener: (details: ChromeRuntimeOnInstalledDetails) => void,
): void;
}

interface ChromeRuntimeOnStartupEvent {
addListener(listener: () => void): void;
removeListener(listener: () => void): void;
}

interface ChromeRuntimeNamespace {
/** The ID of the extension. */
readonly id: string;
readonly lastError: ChromeRuntimeLastError | undefined;
connectNative(application: string): ChromeRuntimePort;
onMessage: ChromeRuntimeOnMessageEvent;
onInstalled: ChromeRuntimeOnInstalledEvent;
onStartup: ChromeRuntimeOnStartupEvent;
// Generic over the response type so callers can narrow the callback
// argument without casting. Matches the de-facto shape used by the
// official @types/chrome package.
Expand All @@ -133,6 +155,30 @@ interface ChromeRuntimeNamespace {
getURL(path: string): string;
}

interface ChromeAlarm {
name: string;
scheduledTime: number;
periodInMinutes?: number;
}

interface ChromeAlarmCreateInfo {
when?: number;
delayInMinutes?: number;
periodInMinutes?: number;
}

interface ChromeAlarmsOnAlarmEvent {
addListener(listener: (alarm: ChromeAlarm) => void): void;
removeListener(listener: (alarm: ChromeAlarm) => void): void;
}

interface ChromeAlarmsNamespace {
create(name: string, alarmInfo: ChromeAlarmCreateInfo): Promise<void>;
get(name: string): Promise<ChromeAlarm | undefined>;
clear(name: string): Promise<boolean>;
onAlarm: ChromeAlarmsOnAlarmEvent;
}

interface ChromeTab {
id?: number;
windowId?: number;
Expand Down Expand Up @@ -276,6 +322,7 @@ interface ChromeActionNamespace {

interface ChromeGlobal {
action: ChromeActionNamespace;
alarms: ChromeAlarmsNamespace;
storage: ChromeStorageNamespace;
identity: ChromeIdentityNamespace;
runtime: ChromeRuntimeNamespace;
Expand Down