docs(multi-profile): RFC and design for multi-profile data isolation#16716
docs(multi-profile): RFC and design for multi-profile data isolation#16716defi-failure wants to merge 2 commits into
Conversation
RFC records intent and decisions: motivation, database-level isolation, the ProfileActivatable activation model, switch semantics, threat/concurrency model, observability contract, and non-goals. design.md is the mechanism authority: path model, profile registry, boot sequence, activation ADT, DbService, switch orchestration with convergent rollback, renderer reset, service classification, invariants, profile management (§13) and the switch-window context fence (§14). Signed-off-by: defi-failure <159208748+defi-failure@users.noreply.github.com>
OverallThe design is well-reasoned and lands the three hardest decisions on solid ground (see "What's solid" below). My review centers on one load-bearing decision — the "no restart" goal — plus two concrete correctness fixes and one rationale fix. I'd treat the first as a decision to make explicitly before merging the design, since ~half the mechanism (§8/§9/§14) exists only to serve it. 1. The load-bearing question: is a zero-restart, top-to-bottom hot-switch actually achievable? (please reconsider)The RFC treats "switch at runtime, no restart" (§2) as a given, and it's what forces the runtime machinery: §8 reverse-order deactivate + convergent rollback, §9 renderer reset, and the entire §14
Net: since the renderer half must be recreated regardless, "no restart" really means "no main-process restart" — and the price for keeping the main process alive across the switch is §8 + §14, with §14's admitted holes. That's a large, leaky cost for a modest prize. Industry check (adversarially verified sources): the apps that do full data-layer isolation either run concurrent instances (Chrome/Firefox: separate 2. What a restart-based switch unlocks — and it reuses an existing seamRelaxing "no restart" revives the option the RFC §6 rejected because it needed a restart. At preboot you can Crucially this isn't new machinery: The honest costs of restart — ~1–3s latency and killing in-flight jobs/streams — are bounded: in-flight work needs draining/aborting on a hot-switch too (§14.6 keeps the drains), and §8 step 4 already re-arms recovery on activation. Restart is abort + boot-recovery, which is simpler and more testable than drain + fence + recovery. This matters only if profile switching is rare (the RFC's own mental model — work/personal, Feishu/Slack-style — is a few switches per session, not a hot loop). If switching is genuinely high-frequency, that's the one argument for keeping the hot-switch — worth stating explicitly if so. Ask: state, in the RFC, whether "no restart" is a hard product requirement or an engineering preference. If there's no product reason that forbids a ~2s relaunch, I'd strongly recommend restart-on-switch + preboot 3. §9 renderer reset via
|
…or, precedents Revise per 0xfullex's review on #16716: - §9 rewritten to source isolation: per-profile webview partitions (reload-remount, window never destroyed) + persist-cache key namespacing; clearStorageData and switch-time clearing removed entirely - §8: flush-before-teardown in deactivate; rollback failure escalates to app.relaunch() into target (restart as the reliability floor) - §13.1: partition-dir tombstone + registry-driven persist-key GC; safeStorage namespace-isolation note - §3: registry rationale rewritten (blast radius / write churn / entity store vs settings KV); version field in type; base62 example fixed; missing-file log level split from corrupt-file - §10: WebviewService/ProxyService join the list; audit candidates noted - §14: transitional-guard provenance clarified; bare ipcMain.handle inventory corrected (~130, concentrated in ipc.ts) - RFC §2 intent/frequency, §4 storage decision + relaunch floor, §7 precedents (Chrome in-process profiles, in-place-reload apps) Signed-off-by: defi-failure <159208748+defi-failure@users.noreply.github.com>
|
@0xfullex Thanks for the review — genuinely useful, and two of the four points I'm adopting wholesale. I've revised the docs accordingly ( 1. The renderer-storage constraint, taken apart into its two populationsYou distinguished two storage populations, and they are indeed different problems — addressing them separately: (a) (b) The default session the app windows load in. You're right that a reload cannot re-home this storage — and the design no longer attempts anything of the sort. In v2 the renderer holds no business data by design; the sanctioned persistent use of the default session is CacheService's persist tier ( Multi-window semantics are exercised in the demo too: a second window shares the default session (which is why the namespacing must be app-wide, not per-window), and on switch non-main windows are closed while the main window survives as the same BrowserWindow, asserted by id. Net effect on your framing — "'no restart' really means 'no main-process restart'" — yes, exactly, and that is the claim: the renderer reloads, the window and the process survive. 2. The fence's holesConceded — and repositioned rather than defended. The ordered deactivate + drains are the primary guarantee; the fence is defense-in-depth that turns residual straddle writes into loud errors instead of silent corruption. And §8 gains an explicit terminal fallback: any switch failure beyond rollback escalates to 3. TimingThe demo's numbers — reload-switch 66–143 ms vs 4. Industry check — two additions
5. Your ask: preference or requirementA candid answer: the origin is an experience conviction — switching should feel like switching, not like the app dying. Checking it against product evidence, it holds up: in-place switching is the baseline users are calibrated to (see 4); a relaunch is a visible app death — tray vanishes, dock re-bounces, window arrangement resets, global shortcuts and the selection assistant go dark for the gap — which reads as "something heavy happened" rather than "I switched"; and per §8's identity/sync direction, switching trends toward routine — the case you noted where keeping the hot switch is justified. This is now stated explicitly in RFC §2. And feasibility still comes first: if the platform made a clean hot switch impossible, none of this would justify forcing it — the demo is my attempt to show it doesn't. 6. profiles.json rationale + safeStorageAdopted as-is — your three reasons (corruption blast radius, write-churn decoupling, entity store vs settings KV) are simply better than mine and replace the "list vs flat KV" wording in §3. Pointer + list stay co-located to preserve the atomic single-file commit in §8 step 5. §13 gains the safeStorage note (namespace-isolated, not key-isolated, consistent with §4). Demo report (10/10) |
Validated — the architecture is soundThanks for the thorough revision and especially the runnable
As an architecture the design is basically sound — the two storage populations are correctly separated, Issues found during validation (for the record / relevant if this graduates)
But the prior question is demand — and for the community edition it doesn't holdStepping back from implementation quality to the requirement itself: a well-built mechanism that shouldn't exist still shouldn't exist, so ahead of any further polish the question is who needs this, in which edition. Multi-profile is a real, structural requirement in the enterprise edition — shared/managed devices, tenant and role separation, contexts a single OS login can't cleanly partition. There the demand is genuine and this design is the right basis for it. For the community edition it's a pseudo-need, on two grounds:
DecisionArchitecture — sound. But we won't be introducing multi-profile into the community edition. The design and the demo aren't wasted — they're the right foundation for the enterprise track, where the demand is real. Rather than merge into community Thanks again — the engineering is genuinely strong; this is a scoping call about where it belongs, not how well it's done. |
What this PR does
Before this PR:
After this PR:
v2-refactor-temp/docs/multi-profile/:rfc-multi-profile.md— intent and decisions: motivation, database-level isolation, theProfileActivatableactivation model, switch semantics, threat/concurrency model, observability contract, and non-goals.design.md— the mechanism authority: path model (app-level frozen map + per-profile slot), profile registry (profiles.json), boot sequence (boot = first activation, two tiers), activation ADT with convergent rollback, per-profileDbService, switch orchestration, renderer reset, service classification, invariants table, the profile management surface (§13), and the switch-window context fence (§14).Fixes #
Why we need it and why it was done in this way
Users need multiple mutually invisible local environments in one app (work/personal separation, clean experiment environments, per-context provider/assistant/knowledge configurations). The current frozen single data root makes this impossible without restarting. This PR lands the design for review; implementation follows in separate PRs.
The following tradeoffs were made:
ProfileActivatableaxis instead of lifecycle stop/start: stop tears downonInit-registered IPC and cross-phase dependencies are invisible to stop cascades; the activation contract is repeatable, awaited, and symmetric.The following alternatives were considered:
userDataper profile (setPathis preboot-only), fine-grained renderer cache invalidation instead of window reload (leak-prone, deferred), full process relaunch on switch (violates the no-restart goal). See RFC §6.Links to places where the discussion took place: internal RFC review (Feishu)
Breaking changes
None — documentation only.
Special notes for your reviewer
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
mainfor active development,v1for v1 maintenance fixes/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note