> # ⚠️ SUPERSEDED — reviews a design that was not built > > This reviews `ELECTRON-OFFLINE-ENGINE-DESIGN.md`, which was **dropped**. Its findings were the > direct cause: seeing them, the human narrowed the requirement from a full offline mail replica to > *"a SQLite index we can prompt against"*, refreshed on each delivery/change event. What shipped is > `lib/mail-index/**` + `app/api/offline/{reindex,search}` — see that doc's superseded note. > > **This review did its job.** Most of its severe findings were resolved by the scope change > removing the thing they were about, which is the strongest outcome a review can have: > > | Finding | Outcome | > |---|---| > | **C1** — `@signalapp/sqlcipher` in `dependencies` breaks both Alpine `docker build`s | **FIXED as specified.** It is an `optionalDependencies` entry with a guarded runtime require (`lib/mail-index/binding.ts`). Both `docker build`s verified passing, and the require verified failing cleanly with MODULE_NOT_FOUND inside the musl image. | > | **C2** — credentials are request-scoped, so no persistent worker can hold them | **MOOT.** There is no worker. Indexing is a normal API route using the request's own `jmap_stalwart_ctx` cookie, via the existing `lib/stalwart/credentials.ts`. | > | **C3** — the OAuth-refresh mitigation is itself the bug | **MOOT, and avoided by construction.** The indexer never touches the refresh-token cookie; it only reads an already-minted auth header, so it cannot rotate a token into a response nobody reads. | > | **C4** — shared `registry.json` breaks the multi-account safety premise | **MOOT.** No registry, no epochs, no concurrent workers. | > | **H1** — a server-side engine can't read a renderer-only setting | **MOOT.** The renderer decides when to index. | > | **H2** — key handoff sequencing, and a nonce via env is readable by same-user processes | **FIXED.** The key crosses on an **inherited file descriptor**, never env, and is fetched per job and zeroed after — not held. Sequencing is moot: the key is fetched when a job runs, not at spawn. | > | **H3** — local unread-count arithmetic needs a coherence story | **MOOT.** A retrieval index does not need to stay coherent with live unread counts. | > | **H4** — no cap on concurrent multi-account sync | **MOOT.** One request, one account. | > | *medium/low:* `getSelectedStorageBackend()` is Linux-only and would crash elsewhere | **FIXED** — platform-guarded. | > | *medium/low:* `cipher_version` check would pass vacuously on zero rows | **FIXED** — the shipped assertion requires a non-empty *string*, and a test reads the raw file bytes for a plaintext canary. | > | *medium/low:* the two bindings are not "the same code either way" | **CONFIRMED true, the hard way.** `@signalapp/sqlcipher` rejects varargs params (`TypeError: Params must be either object or array`) where better-sqlite3 accepts them. Documented in `binding.ts`. | > > Reviewing this file's own accuracy: its two re-executed claims (the binding working in Electron 43, > and `PRAGMA key` being a silent no-op) both held up and both shaped the shipped code. # Adversarial review: `docs/ELECTRON-OFFLINE-ENGINE-DESIGN.md` Reviewer: independent agent, fresh context, no relation to the design's author. 2026-08-04. ## Verdict **Needs substantial rework before implementation — but narrowly scoped rework.** The delta-sync core (everything tagged `[reused]` from M, the mobile design) is genuinely sound and transfers; the reviewer attacked it directly and could not break it. The problem is that **all four genuinely-new sections have an unclosed load-bearing mechanism**, and one of them breaks a build that ships today: - §3 (binding choice) contains a packaging decision that breaks the hosted Docker image and the integration fixture. - §2 (process choice) rests on a credential claim that is only true inside an HTTP request. - §6 (key handoff) is under-specified in a way that doesn't work as sequenced, and its "unresolved implementation choice" is not security-neutral. - §5.3/§8.3 (multi-account) breaks the specific premise M's D6 fix relies on. Nothing here requires re-architecting the sync engine. Stages B-G can proceed against M as written. Stage A as currently specified would not surface most of this. All file:line citations in the design doc that were checked resolve correctly (one trivial miscount, noted at the end) — citation quality is high; the problems are in the reasoning built on top. --- ## CRITICAL ### C1 — Adding `@signalapp/sqlcipher` to `dependencies` breaks the hosted Docker build *and* the integration fixture **Where:** §3.3.1 ("entering `dependencies`"), §3.3.5, §2.4, §10.5, E2, §13 item 6. `Dockerfile:1-4` — `FROM node:24-alpine`, `RUN npm ci`. `integration/webmail.Dockerfile:12-15` — same, `FROM node:24-alpine` + `npm ci`. Verified from the published tarball that `@signalapp/sqlcipher@4.0.3`: - ships **6** prebuilds — `darwin-{arm64,x64}`, `linux-{arm64,x64}`, `win32-{arm64,x64}`. No `linuxmusl-*`. (The design doc's list is exactly right.) - ships **no build sources at all** — published `files` is `dist/*`, `prebuilds`, `README.md`. No `binding.gyp`, no `src/`, no `deps/`. - has `install: node-gyp-build`. `node-gyp-build`'s `bin.js` runs `node-gyp-build-test`; on failure it calls `build()` → spawns `node-gyp rebuild` → `process.exit(code)`. No prebuild + no `binding.gyp` ⇒ `node-gyp rebuild` fails ⇒ **`npm ci` exits nonzero**. The Linux prebuild also has a glibc ≥ 2.34 floor, so it could not load on musl even if copied. **Concrete failure:** the next `docker build` of the production image fails at line 4. `npm run test:integration` fails to build the webmail container. Neither is gated by `VNCMAIL_DESKTOP_STORE_DIR` — that env var only governs *activation*, not *installation*. §3.3.5 dismisses musl as "relevant if an Alpine-based container ever wants the engine — which, per §2.4, it must not" — that reasoning is inverted: the Alpine container doesn't want the engine, it just needs `npm install` to succeed regardless. **Fix direction:** `optionalDependencies` + a guarded runtime `require` (which also delivers E2's graceful-load-failure behavior for free), or a separate optional package, or `--omit=optional` in both Dockerfiles. Pick one and say so explicitly; add "`docker build` of both Dockerfiles still succeeds" to the Stage A verification list. ### C2 — Option A's central justification is only true inside an HTTP request; the Worker credential path does not exist **Where:** §2.1-for-1, §1.2, §2.4 (Worker), §5.3, §8.1 triggers T1/T4/T5/T11, §13 item 1. Verified in `app/api/auth/session/route.ts` and `app/api/auth/token/route.ts`: every credential read goes through `cookies()` from `next/headers` — request-scoped. `lib/oauth/cookie-config.ts:11` sets `httpOnly: true`. The cookies live in the renderer's cookie jar, not in the server. The standalone server holds no session state whatsoever; it decrypts a cookie per request and discards it. So "the credentials are already there... No new credential path, no IPC carrying secrets, no second copy" (§2.1-for-1) is materially overstated. What is actually there is *the ability to decrypt a credential presented on an inbound request* — not a resident credential. Consequences the design never addresses: 1. **T1 ("server process ready + an account's credentials resolvable") cannot fire.** At server-ready there are no cookies anywhere. Nor can T4 (network regained), T5 (`StateChange` on the engine's own socket), or T11 (resume from sleep) — none is an inbound renderer request. 2. **A `worker_threads` Worker is a separate execution context with no cookie access at all.** §2.4 mandates the Worker and routes talk to it via `postMessage`, but there's no specified point at which the Worker actually receives credentials. 3. The only workable shape is: on the first renderer request, decrypt and hand the **plaintext** credentials to the Worker, which retains them for the process lifetime. That is a new long-lived plaintext secret in a new location — the exact thing §2.1-for-1 claims doesn't happen, and the same category of thing `client.ts:6055-6059` already declined once (a resident credential copy in a process that didn't previously hold one). It also creates an invalidation problem never addressed: password change, logout elsewhere, or a cleared cookie leaves the Worker retrying stale credentials indefinitely (since `AuthenticationError` is correctly never treated as a purge signal) — against a server with failed-auth lockout, this locks the user's account. This doesn't kill Option A, but it kills the argument that Option A is free of new secret handling — which was the design's #1 stated reason for choosing it over the alternative. That comparison needs to be redone with the resident-copy cost included, not dropped. ### C3 — The proposed OAuth-refresh mitigation (E11) is not just unimplementable; it *is* the bug it's meant to prevent **Where:** E11 (failure-mode table), §1.2's note about `app/api/auth/token/route.ts:104-106`. E11's rule: *"the engine never refreshes independently. It obtains tokens only through the existing `PUT /api/auth/token` route, in-process, so there is exactly one refresher and one rotation writer — the route."* But that route: reads the refresh token from the **request's** cookie; writes the rotated token as a `Set-Cookie` on the **response**; and on a 400/401/403 from the identity provider, **deletes** the refresh-token cookie and returns 401. An in-process server-side call to that route has no cookie to send (401s immediately), and even if the engine forged one from a resident copy, the rotated token would land in a response the engine discards. Net effect: engine refreshes → identity provider rotates the token → the new token lands in a discarded response → the browser still holds the now-superseded token → the next real refresh from the browser gets rejected → the route deletes the cookie → **the user is silently logged out of that account, and the offline store's credentials are dead.** The per-slot lock the design suggests as a fallback does not help — the problem is that cookie state lives in the browser, not that the writes race each other. Separately, `PUT /api/auth/session` requires three `sec-fetch-*` headers with a comment claiming "non-browser clients cannot forge these" — a Node-side `fetch` call *can* set all three, silently turning a security control into decoration if any engine path goes through this route. Not discussed in the design at all. ### C4 — The shared registry file breaks the exact premise the multi-account safety fix relies on **Where:** §5.1 (`registry.json`, epoch ownership), §2.4 ("one worker per account"), §4.3 (mutex described as "belt-and-braces"), §7.1 (`completePendingPurges()`), §8.3 cross-account, §5.5. The mobile design's cross-account safety guarantee depends explicitly on there being **exactly one writer process-wide** — its own JMAP client is a renderer singleton, so multi-account simultaneous sync was out of scope for it, and its own adversarial review never examined concurrent multi-account execution. This design introduces multi-account-simultaneous as "a genuine capability gain" and disposes of the concurrency consequences with a one-line "the jitter matters more here" — but the epoch value (the fencing token the whole safety guarantee rests on) lives in `registry.json`, a single JSON file shared across every account. No SQLite transaction covers a plain JSON file. The argument that a real database transaction demotes the old per-account mutex to "belt-and-braces" is correct for state stored *inside* the SQLite file, and does not apply to `registry.json` at all — which names no owner thread, no lock, and no atomic-write discipline. Two concrete failures: 1. **Lost epoch bump.** Worker A read-modify-writes the registry to bump account A's epoch (purge, clear, logout). Worker B, holding a stale parse, writes its own update and clobbers A's bump. A's in-flight cycle's next commit now passes the epoch check and lands on top of a wipe — an empty record store with a live, advanced cursor and `resyncRequired: false`, exactly the unreachable-by-design state the mobile design's whole S1 fix exists to prevent. 2. **Torn read on a shared file.** Worker B is mid-write; the server's launch-time `completePendingPurges()` reads and the parse throws or yields a partial object. The documented rule ("unreadable → treated as a purge") means a transient concurrency artifact triggers a full purge-and-rebootstrap for accounts that were perfectly fine — and because the file is shared, one torn read can hit every account at once, not just one. ### Other critical-adjacent findings, condensed - **H1** — the "sync enabled" toggle lives in the renderer's local storage; the server-side engine (and its background triggers) has no way to read it, so it will materialize an encrypted store and a keychain entry for accounts that never opted in — precisely the failure the design's own lazy-materialization rule was meant to prevent. - **H2** — the key-handoff sequencing assumes accounts exist at server-spawn time; they don't (accounts are added later, by logging in). The two proposed handoff mechanisms are not equivalent: one of them passes a nonce via the spawned process's environment variables, which are readable by any other process running as the same OS user — defeating the entire point of using the OS keychain in the first place. Needs re-sequencing plus picking the other mechanism on security grounds, not "whichever is cleaner to implement." - **H3** — the "no optimistic-mutation layer exists, so nothing to keep coherent" claim is false; the webmail already does local-delta arithmetic on mailbox unread counts and totals for mark-read/move/delete actions, with a comment referencing a prior production bug from getting this exact kind of cutoff wrong. A read-only offline cache sitting underneath that arithmetic needs an explicit coherence story, which the design currently declares unnecessary. - **H4** — no cap specified on how many accounts sync simultaneously; since this is the same process serving the live webmail UI, an unbounded background sync could contend for the same rate-limited server connection as the user's foreground activity, throttling their visible mail during their own multi-account first sync. - Several medium/low findings: one proposed API call is Linux-only and would crash the app on macOS/Windows if implemented as literally described; the Linux keychain fallback behavior is described slightly wrong (Electron already fails safely there; the real hazard is a *different* API a future maintainer might reach for); the claim that two SQLite bindings are "the same code either way" doesn't hold — verified real API differences exist between them; the "single-user" safety check for the hosted-deployment gate doesn't actually verify what it claims to. --- ## What the reviewer independently re-verified (not just re-read) Re-ran two of the design's three "verified by execution" claims independently, in Electron 43.2.0 itself under the same execution mode the standalone server actually uses: 1. **`@signalapp/sqlcipher@4.0.3` in Electron 43 — fully re-confirmed by actual re-execution.** Loads with no rebuild, real SQLCipher encryption confirmed (encrypted header, no plaintext canary recoverable from raw bytes, wrong key correctly rejected). The strongest part of the original design. 2. **`node:sqlite`'s `PRAGMA key` silent no-op — fully re-confirmed by actual re-execution.** No throrw, mailbox left in cleartext, canary recoverable from raw bytes. The design is right to call this the sharpest landmine found and to mandate a positive verification check after every store open (though the exact check needs a small correction — checking for a non-empty *string* rather than a non-empty *result set*, since the no-cipher case returns zero rows, not an empty string, and a naive string comparison would pass vacuously). 3. **The Linux keychain-fallback claim — not independently confirmed, and partially contradicted** by reading Electron's own source and current documentation (no Linux desktop was available to actually execute this one). The decision made (refuse outright rather than risk a false sense of security) stays correct regardless and costs nothing, but the specific mechanism described needs correcting. ## Recommended gate Do not start implementation as currently written. Resolve in this order: 1. **C1** — decide the dependency-installation shape so the existing Docker builds keep working; add a Docker-build check to the first implementation step's own verification list. 2. **C2 + C3** — specify the credential lifecycle end to end: how a background worker actually gets credentials, where they live, how long, how invalidation reaches them, and how token refresh can work given rotation needs to land in the browser's cookie jar, not a discarded response. This may change the process-architecture verdict; re-run that comparison honestly rather than inheriting the original conclusion. 3. **C4** — name a single owner (or a real lock plus atomic write) for the shared registry file, and re-derive the multi-account safety guarantee under concurrent writers rather than citing the mobile design's single-writer proof as if it still applied. 4. **H1** — decide where the "sync enabled" setting needs to live (or how the engine learns it) so lazy materialization is actually enforceable from where the engine's triggers fire. 5. **H2** — pick the handoff mechanism that doesn't leak via process environment variables, and re-sequence it for accounts that don't exist yet at process-spawn time. 6. **H3** — add real coherence rules for the counters/totals the webmail already computes locally, or narrow the offline read path to skip anything those computations touch. 7. **H4** — state a concurrency bound and a rule that foreground user activity isn't starved by background multi-account sync. 8. The smaller medium/low findings should land in the same pass since they're cheap to fix once noticed. Everything reused from the mobile design's core sync-engine logic is safe to build against as written — the problems are entirely in the four sections that are genuinely new to this platform.