SRCmail/docs/ELECTRON-OFFLINE-ENGINE-REVIEW.md

⚠️ 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 builds	FIXED as specified. It is an optionalDependencies entry with a guarded runtime require (lib/mail-index/binding.ts). Both docker builds 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.