sessionPersistence config is never read

The browser.sessionPersistence.enabled and browser.sessionPersistence.intervalMs config fields are declared in types.browser.ts but are never consumed here (or anywhere else in the codebase). This means:

• Setting browser.sessionPersistence.enabled: false will have no effect — persistence is always started unconditionally.
• Setting browser.sessionPersistence.intervalMs will also be silently ignored — startPeriodicSave will always use the 60-second default.

The profile object passed into this closure comes from resolved config. The raw config (and its sessionPersistence key) needs to be threaded through to startSessionPersistence, and both the enabled guard and intervalMs should be respected before calling startPeriodicSave.

// Example fix sketch
const persistenceCfg = opts.getState().resolved.sessionPersistence;
if (persistenceCfg?.enabled === false) return;
const stopSave = startPeriodicSave(
  ...,
  persistenceCfg?.intervalMs,  // pass through configured interval
);

This is a comment left during a code review.
Path: src/browser/server-context.availability.ts
Line: 83-93

Comment:
**`sessionPersistence` config is never read**

The `browser.sessionPersistence.enabled` and `browser.sessionPersistence.intervalMs` config fields are declared in `types.browser.ts` but are never consumed here (or anywhere else in the codebase). This means:

- Setting `browser.sessionPersistence.enabled: false` will have **no effect** — persistence is always started unconditionally.
- Setting `browser.sessionPersistence.intervalMs` will also be silently ignored — `startPeriodicSave` will always use the 60-second default.

The `profile` object passed into this closure comes from resolved config. The raw config (and its `sessionPersistence` key) needs to be threaded through to `startSessionPersistence`, and both the `enabled` guard and `intervalMs` should be respected before calling `startPeriodicSave`.

```
// Example fix sketch
const persistenceCfg = opts.getState().resolved.sessionPersistence;
if (persistenceCfg?.enabled === false) return;
const stopSave = startPeriodicSave(
  ...,
  persistenceCfg?.intervalMs,  // pass through configured interval
);
```

How can I resolve this? If you propose a fix, please make it concise.

Concurrent saves possible with setInterval + async callback

setInterval fires every intervalMs regardless of whether the previous async callback has finished. If a CDP call is slow (e.g. the browser is under load, or the network is congested), two invocations of the callback can run concurrently and both attempt to write to the same .tmp file simultaneously — defeating the atomicity guarantee.

For example, if save #1 is still awaiting getAllCookiesViaCdp when the interval fires again, save #2 will call fs.writeFileSync(tmpPath, ...) over the partial write of save #1, then fs.renameSync before save #1 finishes its own rename — resulting in a corrupted or wrong final file.

Consider tracking an in-flight save and skipping the tick if one is already running:

let saving = false;
const timer = setInterval(async () => {
  if (saving) return;
  saving = true;
  try {
    const wsUrl = await getCdpWsUrl();
    if (!wsUrl) return;
    const result = await saveBrowserState(wsUrl, userDataDir);
    if (result.cookieCount > 0) {
      log.info(`🍪 saved ${result.cookieCount} cookies`);
    }
  } catch (err) {
    log.warn(`cookie periodic save failed: ${String(err)}`);
  } finally {
    saving = false;
  }
}, intervalMs);

This is a comment left during a code review.
Path: src/browser/session-persistence.ts
Line: 127-142

Comment:
**Concurrent saves possible with `setInterval` + async callback**

`setInterval` fires every `intervalMs` regardless of whether the previous async callback has finished. If a CDP call is slow (e.g. the browser is under load, or the network is congested), two invocations of the callback can run concurrently and both attempt to write to the same `.tmp` file simultaneously — defeating the atomicity guarantee.

For example, if save #1 is still awaiting `getAllCookiesViaCdp` when the interval fires again, save #2 will call `fs.writeFileSync(tmpPath, ...)` over the partial write of save #1, then `fs.renameSync` before save #1 finishes its own rename — resulting in a corrupted or wrong final file.

Consider tracking an in-flight save and skipping the tick if one is already running:

```typescript
let saving = false;
const timer = setInterval(async () => {
  if (saving) return;
  saving = true;
  try {
    const wsUrl = await getCdpWsUrl();
    if (!wsUrl) return;
    const result = await saveBrowserState(wsUrl, userDataDir);
    if (result.cookieCount > 0) {
      log.info(`🍪 saved ${result.cookieCount} cookies`);
    }
  } catch (err) {
    log.warn(`cookie periodic save failed: ${String(err)}`);
  } finally {
    saving = false;
  }
}, intervalMs);
```

How can I resolve this? If you propose a fix, please make it concise.

Misleading return value when all cookies are expired

When a state file exists with cookies but all of them have expired, validCookies.length will be 0. The function skips setCookiesViaCdp (correct), but still returns { cookieCount: 0, savedAt: state.savedAt } (non-null).

The caller in chrome.ts does if (restored) { log.info(...restored ${restored.cookieCount} cookies...) } — so it will log "🍪 restored 0 cookies (saved ...)" which is confusing and implies a restore happened when it didn't.

Consider returning null in this case, or adjusting the log in chrome.ts to only log when cookieCount > 0:

  if (validCookies.length === 0) {
    return null;
  }

This is a comment left during a code review.
Path: src/browser/session-persistence.ts
Line: 97-105

Comment:
**Misleading return value when all cookies are expired**

When a state file exists with cookies but all of them have expired, `validCookies.length` will be `0`. The function skips `setCookiesViaCdp` (correct), but still returns `{ cookieCount: 0, savedAt: state.savedAt }` (non-null).

The caller in `chrome.ts` does `if (restored) { log.info(...restored ${restored.cookieCount} cookies...) }` — so it will log `"🍪 restored 0 cookies (saved ...)"` which is confusing and implies a restore happened when it didn't.

Consider returning `null` in this case, or adjusting the log in `chrome.ts` to only log when `cookieCount > 0`:

```typescript
  if (validCookies.length === 0) {
    return null;
  }
```

How can I resolve this? If you propose a fix, please make it concise.

Bound pre-shutdown cookie save with a timeout

The shutdown path now awaits saveBrowserState before sending SIGTERM, but this call can wait indefinitely if CDP accepts the socket and never replies to Network.getAllCookies (there is no per-command timeout in the CDP helper). In that state, stopOpenClawChrome can hang and never reach the existing CHROME_STOP_TIMEOUT_MS kill logic, so restarts/shutdowns can stall exactly when the browser is unhealthy.

Useful? React with 👍 / 👎.

Wire sessionPersistence config into runtime behavior

The new browser.sessionPersistence settings are declared but not honored in the runtime path: browser attach always starts persistence, and the periodic saver is started with default parameters. I checked usage with repo search (sessionPersistence only appears in the type declaration), so enabled: false and custom intervalMs values currently have no effect despite being exposed as config knobs.

Useful? React with 👍 / 👎.

Scope timer cleanup to the exiting browser process

The new exit handler clears session persistence before verifying which process exited. In restart paths we can launch a replacement browser and start a new periodic-save timer before the old process emits exit; when that delayed old exit arrives, this unconditional cleanup stops the new timer, so periodic snapshots silently stop for the active browser. Tie cleanup to the PID check (or otherwise associate timer ownership) so an old process cannot clear state for a new one.

Useful? React with 👍 / 👎.

Validate session persistence interval before using it

The config value is passed through without bounds checks, then fed into setInterval. In Node, out-of-range delays (for example 3000000000) overflow and are coerced to 1ms, which turns cookie persistence into a near-tight loop and can create heavy CDP/disk load. Clamp/sanitize sessionPersistence.intervalMs to a safe positive range when resolving config.

Useful? React with 👍 / 👎.

Honor disabled session persistence in startup restore path

Cookie restore now runs unconditionally at browser launch, so browser.sessionPersistence.enabled: false still loads persisted login state if the state file exists. That makes the opt-out ineffective for users who disable persistence (for privacy/debugging) and can produce unexpected re-auth behavior. Gate this launch restore behind the resolved session-persistence flag, consistent with the periodic saver toggle.

Useful? React with 👍 / 👎.