diff --git a/manifest-beta.json b/manifest-beta.json index 61823cd..bb033a6 100644 --- a/manifest-beta.json +++ b/manifest-beta.json @@ -1,7 +1,7 @@ { "id": "remote-ssh", "name": "Remote SSH", - "version": "1.1.8-beta.3", + "version": "1.1.8-beta.4", "minAppVersion": "1.5.0", "description": "Edit remote vaults over SSH/SFTP — VS Code Remote-SSH style.", "author": "souta shimozono", diff --git a/plugin/manifest.json b/plugin/manifest.json index 61823cd..bb033a6 100644 --- a/plugin/manifest.json +++ b/plugin/manifest.json @@ -1,7 +1,7 @@ { "id": "remote-ssh", "name": "Remote SSH", - "version": "1.1.8-beta.3", + "version": "1.1.8-beta.4", "minAppVersion": "1.5.0", "description": "Edit remote vaults over SSH/SFTP — VS Code Remote-SSH style.", "author": "souta shimozono", diff --git a/plugin/package-lock.json b/plugin/package-lock.json index bb37988..958c6fd 100644 --- a/plugin/package-lock.json +++ b/plugin/package-lock.json @@ -1,12 +1,12 @@ { "name": "obsidian-remote-ssh", - "version": "1.1.8-beta.3", + "version": "1.1.8-beta.4", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "obsidian-remote-ssh", - "version": "1.1.8-beta.3", + "version": "1.1.8-beta.4", "license": "MIT", "dependencies": { "@xterm/addon-fit": "^0.11.0", diff --git a/plugin/package.json b/plugin/package.json index 5e14f7c..4f4a0fd 100644 --- a/plugin/package.json +++ b/plugin/package.json @@ -1,6 +1,6 @@ { "name": "obsidian-remote-ssh", - "version": "1.1.8-beta.3", + "version": "1.1.8-beta.4", "description": "VS Code Remote SSH-like experience for Obsidian", "main": "main.js", "scripts": { diff --git a/plugin/src/adapter/SftpDataAdapter.ts b/plugin/src/adapter/SftpDataAdapter.ts index 02a85a5..664bd48 100644 --- a/plugin/src/adapter/SftpDataAdapter.ts +++ b/plugin/src/adapter/SftpDataAdapter.ts @@ -816,7 +816,24 @@ export class SftpDataAdapter { if (cached) { const s = await this.client.stat(remote); - if (s.mtime === cached.mtime) { + // Revalidate on mtime AND size. mtime alone is not enough: SFTP reports + // it at 1-second resolution (`SftpClient`: `mtime: stats.mtime * 1000`), + // so two edits inside the same wall-clock second collapse onto the same + // value — and we would serve a cached copy of a file that no longer + // exists on the server, then let the user save it back over the real one. + // `stat` already hands us the size (it is used just below to size the + // transfer), so comparing it costs nothing and catches every same-second + // edit that changed the length. + // + // Not a theoretical race: `reflect.spec.ts` sleeps 1.1 s between remote + // edits to keep itself green — this bug wearing a workaround. Pinned by + // `e2e/cache-pressure.spec.ts`. + // + // A same-second edit that preserves the byte length is still invisible + // here; closing that needs a content hash or a server-side change + // counter. This removes the common case, cheaply. + const sizeMatches = s.size === undefined || s.size === cached.data.byteLength; + if (s.mtime === cached.mtime && sizeMatches) { this.readCache.get(remote); // bump LRU on hit return cached.data; } diff --git a/plugin/src/main.ts b/plugin/src/main.ts index c80d194..2ef65a3 100644 --- a/plugin/src/main.ts +++ b/plugin/src/main.ts @@ -59,6 +59,18 @@ import { telemetry, telemetryLogPath } from "./util/Telemetry"; /** GitHub `owner/repo` the daemon binaries are released from. */ const DAEMON_RELEASE_REPO = 'sotashimozono/obsidian-remote-ssh'; +/** + * Everything this plugin keeps OUTSIDE any vault, on every OS: + * `~/.obsidian-remote/` — the shadow `vaults/` themselves, plus the + * per-device, never-synced `state/` (the community-plugins base + * snapshots; see `ShadowVaultBootstrap.communityPluginsBasePath`). + * `os.homedir()` resolves at runtime — no hardcoded user. + */ +const shadowStateRoot = (): string => path.join(os.homedir(), '.obsidian-remote'); + +/** Where shadow vaults live: `~/.obsidian-remote/vaults/`. */ +const shadowVaultsDir = (): string => path.join(shadowStateRoot(), 'vaults'); + export default class RemoteSshPlugin extends Plugin { settings: PluginSettings = DEFAULT_SETTINGS; @@ -623,16 +635,23 @@ export default class RemoteSshPlugin extends Plugin { // #429 / #342 residual: round-trip the enabled community-plugins // list. Pull first so plugins set up on the remote load in the // shadow vault (the marketplace installer then fetches any missing - // binaries); then push the merged result so a plugin present only - // locally reaches the remote for other machines. Pushing *after* - // the pull is safe — the local list is now the union, so it can - // never drop a plugin the remote already had. Kept out of the - // verbatim shared-config set because `remote-ssh` must be - // force-preserved through the merge (a verbatim copy of a remote - // list omitting it would disable this very plugin). + // binaries); then push the converged result so a plugin enabled — + // or UNINSTALLED — only here reaches the remote for other machines. + // Kept out of the verbatim shared-config set because `remote-ssh` + // must be force-preserved through the merge (a verbatim copy of a + // remote list omitting it would disable this very plugin). + // + // Both halves take this device's BASE snapshot: the converged list + // as of the last successful round-trip HERE. It is what turns the + // old monotonic union into a real 3-way merge, so a local uninstall + // propagates instead of being resurrected by the next pull. The + // pull only reads it; the push commits it once both sides agree. + const cpBasePath = ShadowVaultBootstrap.communityPluginsBasePath( + shadowStateRoot(), profile.id, + ); try { - await ShadowVaultBootstrap.pullCommunityPlugins(da, remoteConfigDir, localConfigDir); - await ShadowVaultBootstrap.pushCommunityPlugins(da, remoteConfigDir, localConfigDir); + await ShadowVaultBootstrap.pullCommunityPlugins(da, remoteConfigDir, localConfigDir, cpBasePath); + await ShadowVaultBootstrap.pushCommunityPlugins(da, remoteConfigDir, localConfigDir, cpBasePath); } catch (e) { logger.warn( `runAutoConnect(${tag}): community-plugins round-trip failed: ${errorMessage(e)}`, @@ -1149,12 +1168,12 @@ export default class RemoteSshPlugin extends Plugin { } this.shadowSpawnInFlight = true; - // Shadow vaults live under ~/.obsidian-remote/vaults/ on every - // OS. os.homedir() resolves at runtime — no hardcoded user. - const baseDir = path.join(os.homedir(), '.obsidian-remote', 'vaults'); - + // Shadow vaults live under ~/.obsidian-remote/vaults/ on every OS, + // alongside ~/.obsidian-remote/state/ (never-synced per-device state). const registry = new ObsidianRegistry(ObsidianRegistry.defaultConfigPath()); - const bootstrap = new ShadowVaultBootstrap(baseDir, sourcePluginDir, registry); + const bootstrap = new ShadowVaultBootstrap( + shadowVaultsDir(), sourcePluginDir, registry, shadowStateRoot(), + ); const spawner = new WindowSpawner(); const manager = new ShadowVaultManager(bootstrap, spawner); @@ -1282,7 +1301,18 @@ export default class RemoteSshPlugin extends Plugin { const pull = (async () => { await client.connect(profile); await ShadowVaultBootstrap.pullSharedObsidianConfig(reader, remoteConfigDir, localConfigDir); - await ShadowVaultBootstrap.pullCommunityPlugins(reader, remoteConfigDir, localConfigDir); + // Same base as the shadow window's own round-trip will use (same + // device, same profile), so a removal another machine made is + // applied here too and the window boots on the converged list. + // `pullCommunityPlugins` never WRITES the base — only the push + // does, once both sides hold it — so this pre-spawn pull cannot + // make the real connect's push mistake this device's local + // additions for remote removals, and re-running the merge over the + // same base is idempotent: no removal is ever double-applied. + await ShadowVaultBootstrap.pullCommunityPlugins( + reader, remoteConfigDir, localConfigDir, + ShadowVaultBootstrap.communityPluginsBasePath(shadowStateRoot(), profile.id), + ); const enabledIds = ShadowVaultBootstrap.readEnabledPluginIds(localConfigDir); await ShadowVaultBootstrap.pullPluginBinaries(reader, remoteConfigDir, localConfigDir, enabledIds); })(); diff --git a/plugin/src/shadow/ShadowVaultBootstrap.ts b/plugin/src/shadow/ShadowVaultBootstrap.ts index e03ba8a..24b4f74 100644 --- a/plugin/src/shadow/ShadowVaultBootstrap.ts +++ b/plugin/src/shadow/ShadowVaultBootstrap.ts @@ -119,6 +119,14 @@ export class ShadowVaultBootstrap { /** Absolute path to THIS running plugin's directory (source for symlink/copy). */ private readonly sourcePluginDir: string, private readonly registry: ObsidianRegistry, + /** + * Root of this device's never-synced sync state (e.g. + * `~/.obsidian-remote/`), parent of the `state/` dir that holds the + * community-plugins base snapshots — see + * {@link ShadowVaultBootstrap.communityPluginsBasePath}. Defaults to + * `baseDir`'s parent, which is exactly that on every real call site. + */ + private readonly stateRoot: string = path.dirname(baseDir), ) {} bootstrap(profile: SshProfile, allProfiles: ReadonlyArray): Promise { @@ -147,6 +155,18 @@ export class ShadowVaultBootstrap { // before the `readBaseDataJson` call below side-effects state. const isFirstBootstrap = !fs.existsSync(layout.pluginDataFile); + // A first bootstrap means this device has no shadow vault for the + // profile — either it never had one, or the user deleted it (the + // documented recovery step). Either way any base snapshot left over + // from a previous incarnation is STALE, and keeping it would be + // actively destructive: the fresh shadow's list is the `["remote-ssh"]` + // seed below, so every id in that old base would look like a local + // uninstall and the first push would strip the user's whole + // enabled-plugin list off the remote. Drop it — the round-trip then + // falls back to the union (nothing lost) and re-establishes a base on + // the first successful push. + if (isFirstBootstrap) this.discardCommunityPluginsBase(profile.id); + // `community-plugins.json` always starts as `["remote-ssh"]` only. // Inheriting source's full enabled list at bootstrap time was too // surprising — the shadow window would auto-install every plugin @@ -227,6 +247,21 @@ export class ShadowVaultBootstrap { return { layout, registryId, registryCreated: created, migrated, pluginInstallMethod }; } + /** + * Delete this device's community-plugins base snapshot for `profileId` + * (see the call site in `bootstrapSync`). Best-effort — an absent file + * is the normal case, and a failure only means the next round-trip + * falls back to the union. + */ + private discardCommunityPluginsBase(profileId: string): void { + const basePath = ShadowVaultBootstrap.communityPluginsBasePath(this.stateRoot, profileId); + try { + fs.rmSync(basePath, { force: true }); + } catch (e) { + logger.warn(`ShadowVaultBootstrap: could not discard stale ${basePath} (${errorMessage(e)})`); + } + } + /** * Compute the shadow paths for a profile without doing any I/O: the * pure `--` layout, before any collision ` (n)` suffix @@ -602,31 +637,110 @@ export class ShadowVaultBootstrap { return { pushed, skipped, errored }; } - // ─── community-plugins list round-trip (#429 / #342) ───────────────────── + // ─── community-plugins list round-trip (#429 / #342 / uninstall) ───────── // // `community-plugins.json` is the *enabled community plugins* list. // It deliberately does NOT join SHARED_OBSIDIAN_CONFIG_FILES (whose // pull/push copy bytes verbatim): a remote list that omitted // `remote-ssh` would, written verbatim, disable the very plugin doing - // the sync. Instead these two methods round-trip the list with a - // forced `remote-ssh` union, so a plugin enabled on one machine - // reaches another machine's shadow vault and reopening no longer - // "loses" installed plugins. The marketplace installer re-fetches any - // binaries the list names but that aren't staged locally yet. + // the sync. Instead these two methods round-trip the list so a plugin + // enabled on one machine reaches another machine's shadow vault and + // reopening no longer "loses" installed plugins. The marketplace + // installer re-fetches any binaries the list names but that aren't + // staged locally yet. + // + // Convergence is a 3-WAY MERGE against a per-device BASE snapshot, not + // a union. The union was MONOTONIC — the list could only ever GROW — + // so an uninstall on one device never reached the remote, and the next + // connect's pull RESURRECTED the plugin locally: a fleet-wide uninstall + // was impossible. A union cannot tell "I never had it" from "I removed + // it"; only a base can. The base is the converged list as it stood at + // the END of the last successful round-trip ON THIS DEVICE (see + // {@link communityPluginsBasePath} for where it lives and why). + // + // added = local \ base -> add to remote + // removed = base \ local -> remove from remote + // added = remote \ base -> add to local + // removed = base \ remote -> remove from local + // + // Rules, all encoded in {@link mergePluginIds}: + // + // - `remote-ssh` (SELF_PLUGIN_ID) is NEVER removable from either side. + // - NO BASE YET (first run, or a shadow vault that was deleted and + // re-bootstrapped): a removal is indistinguishable from never-had, + // so we fall back to the old UNION. Safe — nothing is lost; removals + // simply don't propagate until a base exists, which the first + // successful push then writes. + // - A device that was OFFLINE while another device removed a plugin + // has a stale base, so `base \ remote` sees the removal and drops it + // locally. Correct — that is the whole point of the base. + // - TIE-BREAK — concurrent ADD on A vs REMOVE on B: **ADD WINS**. It + // falls out of the definition: an id in neither the base nor either + // side's removal set (`base \ local`, `base \ remote`) is by + // definition an addition, and additions are unioned in. Rationale: + // re-uninstalling a plugin someone else re-installed is one click; + // silently losing a plugin you just installed is invisible data loss. + // + // The base is committed by the PUSH only, once BOTH sides hold the + // converged list. `pullCommunityPlugins` deliberately never writes it: + // a pull-only caller (`preSpawnPull`) would otherwise record the pulled + // list as the base, and the push later in the real connect would then + // read `base \ remote` as a *remote removal* of everything this device + // had locally added — deleting the user's own additions. Never writing + // the base on pull also keeps the merge idempotent (a second pull over + // the same base+remote recomputes the same list), so the + // pre-spawn-pull → connect-pull → connect-push sequence cannot + // double-apply a removal. /** The plugin's own id — always kept enabled across a round-trip. */ static readonly SELF_PLUGIN_ID = 'remote-ssh'; /** - * Pull the remote enabled-plugin list into the shadow vault, merged - * with the local list and with `remote-ssh` forced on. A remote list - * that's absent or not a valid id array leaves the local list - * untouched (never clobbered). + * Absolute path to THIS DEVICE's base snapshot of the enabled-plugin + * list for `profileId`: + * + * /state//community-plugins.base.json + * + * i.e. `~/.obsidian-remote/state//…`, a SIBLING of the shadow + * `vaults/` dir — deliberately outside every vault: + * + * - It must be PER-DEVICE and must NOT sync. A base that synced would + * record another machine's view and removals would ping-pong. + * - It must not live under a vault's `/`: everything there + * is write-through-mirrored to the remote by + * `SftpDataAdapter.writeThroughConfig`, and `PathMapper` redirects + * the four core config files (plus `plugins/∗/data.json`) per + * device. A new file there would either be shared (wrong) or entangle + * with that machinery. + * - Outside the vault root entirely also keeps it invisible to Obsidian + * and out of the populate. + * + * NOT under `vaults/` itself: `findShadowByProfileId` enumerates that + * dir looking for shadow vaults, and `uniqueVaultDir` could collide with + * a profile literally named "state". + */ + static communityPluginsBasePath(stateRoot: string, profileId: string): string { + return path.join( + stateRoot, 'state', sanitiseStateKey(profileId), 'community-plugins.base.json', + ); + } + + /** + * Pull the remote enabled-plugin list into the shadow vault, 3-way + * merged against this device's base (see the section comment above) + * with `remote-ssh` forced on. A remote list that's absent or not a + * valid id array leaves the local list untouched (never clobbered) + * and — crucially — contributes NO removals: a remote we could not read + * must never look like "every plugin was uninstalled elsewhere". + * + * `basePath` is optional; omitting it (or pointing at a base that does + * not exist yet) degrades to the historical UNION behaviour. */ static async pullCommunityPlugins( reader: SharedConfigReader, remoteConfigDir: string, localConfigDir: string, + basePath?: string | null, ): Promise<{ pulled: boolean; merged: string[] }> { const basename = 'community-plugins.json'; const remoteRel = `${remoteConfigDir}/${basename}`; @@ -649,38 +763,52 @@ export class ShadowVaultBootstrap { logger.warn(`pullCommunityPlugins: ${basename} skipped (${errorMessage(e)})`); } - const merged = ShadowVaultBootstrap.mergePluginIds( - local, remote ?? [], ShadowVaultBootstrap.SELF_PLUGIN_ID, - ); + const base = ShadowVaultBootstrap.readPluginIdBase(basePath); + const merged = remote === null + // Nothing readable on the remote → it gets no say this round. Keep + // the local list as-is (only forcing `remote-ssh` on); do NOT let + // `base \ remote` infer removals from a list we never saw. + ? ShadowVaultBootstrap.mergePluginIds(local, local, null, ShadowVaultBootstrap.SELF_PLUGIN_ID) + : ShadowVaultBootstrap.mergePluginIds(local, remote, base, ShadowVaultBootstrap.SELF_PLUGIN_ID); + const changed = merged.length !== local.length || merged.some((id, i) => id !== local[i]); if (changed) ShadowVaultBootstrap.writePluginIdListAtomic(localPath, merged); - logger.info(`pullCommunityPlugins: merged [${merged.join(', ')}] (changed=${changed})`); + logger.info( + `pullCommunityPlugins: merged [${merged.join(', ')}] ` + + `(changed=${changed}, base=${base ? `[${base.join(', ')}]` : 'none'})`, + ); return { pulled: remote !== null, merged }; } /** - * Push the local enabled-plugin list to the remote, unioned with the - * remote's CURRENT list and with `remote-ssh` forced on. + * Push the local enabled-plugin list to the remote — 3-way merged + * against the remote's CURRENT list and this device's base, with + * `remote-ssh` forced on — then record the converged list as the new + * base (both sides now hold it). * - * Self-protecting against clobber: it re-reads the remote first and - * unions, so a stale/minimal local list (e.g. after a transient pull - * read-failure earlier in the same connect) can never drop a plugin - * another machine enabled. If the remote HAS the file but it can't be - * read or parsed, it aborts rather than overwrite what it couldn't - * see. A genuinely-absent remote file is seeded from local. + * Still self-protecting against clobber: it re-reads the remote first, + * so a plugin another machine enabled (absent from the base AND from + * this device's list = a remote *addition*) is preserved. If the remote + * HAS the file but it can't be read or parsed, it aborts rather than + * overwrite what it couldn't see — and leaves the base alone, so a + * pending removal is simply retried on the next connect. A genuinely + * absent remote file is seeded from local (no removals inferred: an + * absent remote is not an emptied one). */ static async pushCommunityPlugins( rw: SharedConfigReader & SharedConfigWriter, remoteConfigDir: string, localConfigDir: string, + basePath?: string | null, ): Promise<{ pushed: boolean }> { const basename = 'community-plugins.json'; const remoteRel = `${remoteConfigDir}/${basename}`; const local = ShadowVaultBootstrap.readPluginIdList(path.join(localConfigDir, basename)); - let remote: string[] = []; + /** null = the remote has no list at all (fresh remote) — NOT an empty one. */ + let remote: string[] | null = null; try { if (await rw.exists(remoteRel)) { const parsed = ShadowVaultBootstrap.parsePluginIdList(await rw.read(remoteRel)); @@ -695,13 +823,25 @@ export class ShadowVaultBootstrap { return { pushed: false }; } - const ids = ShadowVaultBootstrap.mergePluginIds(remote, local, ShadowVaultBootstrap.SELF_PLUGIN_ID); - // No-op when the remote already equals the union — avoid churn. - if (remote.length === ids.length && remote.every((id, i) => id === ids[i])) { + const base = ShadowVaultBootstrap.readPluginIdBase(basePath); + const ids = remote === null + // Seed a fresh remote from local. No base removals: "the remote has + // no list" is not "the remote deleted everything". + ? ShadowVaultBootstrap.mergePluginIds(local, local, null, ShadowVaultBootstrap.SELF_PLUGIN_ID) + : ShadowVaultBootstrap.mergePluginIds(local, remote, base, ShadowVaultBootstrap.SELF_PLUGIN_ID); + + // No-op when the remote already equals the converged list — avoid + // churn. The base is still committed: both sides DO hold `ids`, and + // without this the steady state would never record a base at all. + if (remote !== null && remote.length === ids.length && remote.every((id, i) => id === ids[i])) { + ShadowVaultBootstrap.writePluginIdBase(basePath, ids); return { pushed: false }; } try { await rw.write(remoteRel, JSON.stringify(ids) + '\n'); + // Only now do BOTH sides hold `ids` — commit the base. A write that + // throws leaves the old base, so the merge is retried next connect. + ShadowVaultBootstrap.writePluginIdBase(basePath, ids); logger.info(`pushCommunityPlugins: pushed [${ids.join(', ')}]`); return { pushed: true }; } catch (e) { @@ -887,16 +1027,95 @@ export class ShadowVaultBootstrap { } } - /** Order-preserving union of `local` + `remote`, with `required` guaranteed present. */ - private static mergePluginIds(local: string[], remote: string[], required: string): string[] { + /** + * The 3-way converged enabled-plugin list. + * + * `base` is what BOTH sides held at the end of the last successful + * round-trip on this device (null = none recorded yet). An id is + * REMOVED iff it is in the base but has since disappeared from a side + * that once had it (`base \ local` ∪ `base \ remote`); everything else + * present on either side is kept — so an id absent from the base is an + * ADDITION and always survives (the add-wins tie-break). `required` + * (`remote-ssh`) is never removable and is always present. + * + * With `base === null` the removal set is empty and this degrades + * EXACTLY to the historical order-preserving union of local + remote — + * the documented first-run fallback (nothing can be lost, but a removal + * cannot be inferred either). + * + * Order is local-first, then remote-only additions, then `required`: + * deterministic and identical in the pull and the push, so once both + * sides converge the push's equality check sees a true no-op. + */ + private static mergePluginIds( + local: string[], + remote: string[], + base: string[] | null, + required: string, + ): string[] { + const removed = new Set(); + if (base) { + const inLocal = new Set(local); + const inRemote = new Set(remote); + for (const id of base) { + if (!inLocal.has(id) || !inRemote.has(id)) removed.add(id); + } + } + // The one plugin that may never be uninstalled — it IS the sync. + removed.delete(required); + const out: string[] = []; const seen = new Set(); for (const id of [...local, ...remote, required]) { - if (!seen.has(id)) { seen.add(id); out.push(id); } + if (removed.has(id) || seen.has(id)) continue; + seen.add(id); + out.push(id); } return out; } + /** + * This device's base snapshot ([] is meaningful — "both sides were + * empty"), or null when there is no usable base: no path given, the + * file doesn't exist yet (first run / freshly re-bootstrapped shadow), + * or it is malformed. null means "fall back to the union" — never + * "everything was removed", which is why a malformed base is degraded + * rather than treated as empty. + */ + private static readPluginIdBase(basePath: string | null | undefined): string[] | null { + if (!basePath) return null; + let raw: string; + try { + raw = fs.readFileSync(basePath, 'utf-8'); + } catch (e) { + if ((e as NodeJS.ErrnoException).code !== 'ENOENT') { + logger.warn(`communityPlugins base: unreadable ${basePath} (${errorMessage(e)}); falling back to union`); + } + return null; + } + const parsed = ShadowVaultBootstrap.parsePluginIdList(raw); + if (parsed === null) { + logger.warn(`communityPlugins base: malformed ${basePath}; falling back to union`); + } + return parsed; + } + + /** + * Record the converged list as this device's new base. Best-effort: a + * failure here only costs the NEXT connect its removal inference (it + * degrades to a union), so it must never fail the round-trip that has + * already written both sides. + */ + private static writePluginIdBase(basePath: string | null | undefined, ids: string[]): void { + if (!basePath) return; + try { + fs.mkdirSync(path.dirname(basePath), { recursive: true }); + ShadowVaultBootstrap.writePluginIdListAtomic(basePath, ids); + } catch (e) { + logger.warn(`communityPlugins base: failed to record ${basePath} (${errorMessage(e)})`); + } + } + /** Atomic (tmp + rename) write of an id array as JSON. */ private static writePluginIdListAtomic(localPath: string, ids: string[]): void { const tmp = `${localPath}.${process.pid}.tmp`; @@ -1260,6 +1479,18 @@ export class ShadowVaultBootstrap { } } +/** + * Filesystem-safe form of a profile id for the per-device `state//` + * dir. Ids are normally UUIDs (already safe), but they come from + * data.json and are not validated anywhere, so a hand-edited id must not + * be able to escape `state/` via `..` or a path separator. + */ +function sanitiseStateKey(profileId: string): string { + const cleaned = (profileId ?? '').replace(/[^a-zA-Z0-9._-]/g, '_').slice(0, 64); + if (!cleaned || /^\.+$/.test(cleaned)) return 'default'; + return cleaned; +} + /** * Filesystem-safe form of a profile *name* for the friendly vault-dir * name. Obsidian shows a vault by its directory basename, so this is diff --git a/plugin/tests/SftpDataAdapter.test.ts b/plugin/tests/SftpDataAdapter.test.ts index 9bbfa53..81a1812 100644 --- a/plugin/tests/SftpDataAdapter.test.ts +++ b/plugin/tests/SftpDataAdapter.test.ts @@ -1497,6 +1497,57 @@ describe('SftpDataAdapter — writer reflect (#341)', () => { }); }); +// ─── cache revalidation: mtime alone is not enough ────────────────────────── +// +// SFTP reports mtime at 1-SECOND resolution (`SftpClient`: `stats.mtime * 1000`), +// so two edits inside the same wall-clock second land on the SAME mtime. A cache +// that revalidates on mtime alone then serves a copy of a file that no longer +// exists on the server — and the user saves it back over the real one. `stat` +// already returns the size, so comparing it closes the common case for free. +// The E2E form of this lives in `e2e/cache-pressure.spec.ts`. +describe('SftpDataAdapter — cache revalidation compares size, not just mtime', () => { + let readCache: ReadCache; + let dirCache: DirCache; + + beforeEach(() => { + readCache = new ReadCache({ maxBytes: 4096 }); + dirCache = new DirCache({ ttlMs: 10_000 }); + }); + + it('refetches when the remote size changed but the mtime did NOT', async () => { + const fake = makeFakeClient({ + files: { '/v/a.md': { data: Buffer.from('old'), mtime: 5000 } }, + }); + const adapter = new SftpDataAdapter(fake.client, '/v', readCache, dirCache, 'v'); + + expect(await adapter.read('a.md')).toBe('old'); // populates the cache + + // A same-second edit: different bytes, DIFFERENT length, mtime unchanged. + fake.files['/v/a.md'] = { data: Buffer.from('a-much-longer-body'), mtime: 5000 }; + + expect( + await adapter.read('a.md'), + 'served the STALE cached copy: the mtime matched, but the size did not', + ).toBe('a-much-longer-body'); + }); + + it('still serves from cache — no refetch — when mtime AND size both match', async () => { + const fake = makeFakeClient({ + files: { '/v/a.md': { data: Buffer.from('same'), mtime: 5000 } }, + }); + const adapter = new SftpDataAdapter(fake.client, '/v', readCache, dirCache, 'v'); + + expect(await adapter.read('a.md')).toBe('same'); + const fetches = fake.spies.readBinary.mock.calls.length; + + expect(await adapter.read('a.md')).toBe('same'); + expect( + fake.spies.readBinary.mock.calls.length, + 'a matching mtime+size must still be a cache hit — the size check must not defeat caching', + ).toBe(fetches); + }); +}); + // ─── config write-through (#342 / #429) ───────────────────────────────────── // // Obsidian loads community plugins at startup and each `onload()` calls diff --git a/plugin/tests/ShadowVaultBootstrap.test.ts b/plugin/tests/ShadowVaultBootstrap.test.ts index 9ba2f76..247110d 100644 --- a/plugin/tests/ShadowVaultBootstrap.test.ts +++ b/plugin/tests/ShadowVaultBootstrap.test.ts @@ -1217,6 +1217,271 @@ describe('ShadowVaultBootstrap community-plugins round-trip (#429 / #342)', () = }); }); +// ─── uninstall: 3-way merge against a per-device base ─────────────────── +// +// The union above is MONOTONIC: `community-plugins.json` could only ever +// GROW, so uninstalling a plugin on one device never reached the remote +// and the next pull RESURRECTED it locally — a fleet-wide uninstall was +// impossible (pinned by e2e/plugin-code-roundtrip.spec.ts test 6). +// +// The fix is a 3-way merge against a BASE: the converged list as of the +// END of the last successful round-trip ON THIS DEVICE, stored outside +// every vault at `~/.obsidian-remote/state//`. `base \ local` +// is a local uninstall, `base \ remote` a remote one; anything absent from +// the base is an addition. With no base yet, a removal is indistinguishable +// from never-had, so it degrades to the old union (nothing is lost). +describe('ShadowVaultBootstrap community-plugins 3-way merge (uninstall propagation)', () => { + let scratchRoot: string; + + beforeEach(() => { + scratchRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'cp-3way-')); + }); + afterEach(() => { + fs.rmSync(scratchRoot, { recursive: true, force: true }); + }); + + const SELF = ShadowVaultBootstrap.SELF_PLUGIN_ID; // 'remote-ssh' + const CP = '.obsidian/community-plugins.json'; + + /** A shadow config dir seeded with `local`. */ + function makeLocal(local: string[]): string { + const dir = path.join(scratchRoot, `vault-${Math.random().toString(36).slice(2)}`, '.obsidian'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'community-plugins.json'), JSON.stringify(local), 'utf-8'); + return dir; + } + const readLocal = (dir: string): string[] => + JSON.parse(fs.readFileSync(path.join(dir, 'community-plugins.json'), 'utf-8')) as string[]; + + /** The per-device base path, optionally pre-seeded (omitted = no base yet). */ + function basePathFor(seed?: string[]): string { + const p = ShadowVaultBootstrap.communityPluginsBasePath(scratchRoot, 'profile-1'); + if (seed) { + fs.mkdirSync(path.dirname(p), { recursive: true }); + fs.writeFileSync(p, JSON.stringify(seed), 'utf-8'); + } + return p; + } + const readBase = (p: string): string[] => + JSON.parse(fs.readFileSync(p, 'utf-8')) as string[]; + + /** In-memory remote. `initial === null` → the remote has no list at all. */ + function makeRemote(initial: string[] | null): { + rw: SharedConfigReader & SharedConfigWriter; + read: () => string[] | null; + } { + const store: Record = {}; + if (initial !== null) store[CP] = JSON.stringify(initial); + return { + rw: { + exists: (p) => Promise.resolve(p in store), + read: (p) => Promise.resolve(store[p]), + write: (p, c) => { store[p] = c; return Promise.resolve(); }, + }, + read: () => (CP in store ? (JSON.parse(store[CP]) as string[]) : null), + }; + } + + /** One full connect round-trip: pull, then push — exactly what main.ts does. */ + async function roundTrip( + rw: SharedConfigReader & SharedConfigWriter, + localConfigDir: string, + basePath: string, + ): Promise { + await ShadowVaultBootstrap.pullCommunityPlugins(rw, '.obsidian', localConfigDir, basePath); + await ShadowVaultBootstrap.pushCommunityPlugins(rw, '.obsidian', localConfigDir, basePath); + } + + it('falls back to a UNION when there is no base yet (first run — nothing is lost)', async () => { + // Local has `templater`, the remote has `dataview`, and this device has + // never synced. Neither absence may be read as a removal. + const localDir = makeLocal([SELF, 'templater']); + const { rw, read } = makeRemote([SELF, 'dataview']); + const basePath = basePathFor(); // no base on disk + + await roundTrip(rw, localDir, basePath); + + expect(readLocal(localDir)).toEqual(expect.arrayContaining([SELF, 'templater', 'dataview'])); + expect(read()).toEqual(expect.arrayContaining([SELF, 'templater', 'dataview'])); + // …and the round-trip has now RECORDED a base, so the next connect can + // infer removals. + expect(readBase(basePath)).toEqual(expect.arrayContaining([SELF, 'templater', 'dataview'])); + }); + + it('propagates a LOCAL removal to the remote (the uninstall the e2e pins)', async () => { + // Steady state: both sides and the base agree on [remote-ssh, dataview]. + const localDir = makeLocal([SELF, 'dataview']); + const { rw, read } = makeRemote([SELF, 'dataview']); + const basePath = basePathFor([SELF, 'dataview']); + + // The user uninstalls `dataview` in this vault. + fs.writeFileSync( + path.join(localDir, 'community-plugins.json'), JSON.stringify([SELF]), 'utf-8', + ); + + await roundTrip(rw, localDir, basePath); + + expect(read(), 'the uninstall must reach the remote — otherwise no device can ever drop it') + .toEqual([SELF]); + expect(readLocal(localDir), 'and the pull must not resurrect it locally').toEqual([SELF]); + expect(readBase(basePath)).toEqual([SELF]); + }); + + it('propagates a REMOTE removal into the local list (uninstalled on another device)', async () => { + // This device was offline while machine B uninstalled `dataview`, so its + // base is stale — which is exactly how the removal is detected. + const localDir = makeLocal([SELF, 'dataview']); + const { rw, read } = makeRemote([SELF]); + const basePath = basePathFor([SELF, 'dataview']); + + await roundTrip(rw, localDir, basePath); + + expect(readLocal(localDir), 'a plugin removed on another device must be dropped here') + .toEqual([SELF]); + expect(read()).toEqual([SELF]); + expect(readBase(basePath)).toEqual([SELF]); + }); + + it('still sends a LOCAL addition to the remote (existing behaviour preserved)', async () => { + const localDir = makeLocal([SELF, 'dataview', 'templater']); // templater just enabled + const { rw, read } = makeRemote([SELF, 'dataview']); + const basePath = basePathFor([SELF, 'dataview']); + + await roundTrip(rw, localDir, basePath); + + expect(read()).toEqual(expect.arrayContaining([SELF, 'dataview', 'templater'])); + expect(readLocal(localDir)).toEqual(expect.arrayContaining([SELF, 'dataview', 'templater'])); + }); + + it('still pulls a REMOTE addition into the local list (existing behaviour preserved)', async () => { + const localDir = makeLocal([SELF, 'dataview']); + const { rw } = makeRemote([SELF, 'dataview', 'obsidian-git']); // enabled on machine B + const basePath = basePathFor([SELF, 'dataview']); + + await roundTrip(rw, localDir, basePath); + + expect(readLocal(localDir)).toEqual(expect.arrayContaining([SELF, 'dataview', 'obsidian-git'])); + }); + + it('never lets remote-ssh be removed — from either side', async () => { + // Both sides "uninstalled" it: the local list dropped it, the remote list + // omits it, and the base says both once had it. It must still survive — + // it IS the sync. + const localDir = makeLocal(['dataview']); // no remote-ssh + const { rw, read } = makeRemote(['dataview']); // no remote-ssh + const basePath = basePathFor([SELF, 'dataview']); // base says both had it + + await roundTrip(rw, localDir, basePath); + + expect(readLocal(localDir), 'remote-ssh must never be uninstallable locally').toContain(SELF); + expect(read(), 'remote-ssh must never be uninstallable remotely').toContain(SELF); + }); + + it('resolves concurrent add-vs-remove in favour of the ADD (documented tie-break)', async () => { + // Machine B uninstalled `dataview` and pushed, so the remote no longer + // names it. Meanwhile this device installed `dataview` fresh — it is + // absent from THIS device's base, so it is an ADDITION, not a leftover. + // Add wins: losing a plugin you just installed is invisible data loss. + const localDir = makeLocal([SELF, 'dataview']); + const { rw, read } = makeRemote([SELF]); + const basePath = basePathFor([SELF]); // dataview never in this device's base + + await roundTrip(rw, localDir, basePath); + + expect(readLocal(localDir), 'a plugin this device just installed must not vanish') + .toEqual(expect.arrayContaining([SELF, 'dataview'])); + expect(read(), 'and the addition must reach the remote') + .toEqual(expect.arrayContaining([SELF, 'dataview'])); + }); + + it('does not infer removals from a remote it could not read (no base-driven wipe)', async () => { + // Transient SSH error mid-read. `base \ remote` must NOT be taken to mean + // "everything was uninstalled elsewhere". + const localDir = makeLocal([SELF, 'dataview', 'templater']); + const basePath = basePathFor([SELF, 'dataview', 'templater']); + const rw: SharedConfigReader & SharedConfigWriter = { + exists: () => Promise.resolve(true), + read: () => Promise.reject(new Error('SSH hiccup')), + write: () => Promise.reject(new Error('must not push a list built from an unread remote')), + }; + + await ShadowVaultBootstrap.pullCommunityPlugins(rw, '.obsidian', localDir, basePath); + const r = await ShadowVaultBootstrap.pushCommunityPlugins(rw, '.obsidian', localDir, basePath); + + expect(r.pushed).toBe(false); + expect(readLocal(localDir), 'the local list must survive an unreadable remote intact') + .toEqual([SELF, 'dataview', 'templater']); + expect(readBase(basePath), 'and the base must be left alone so the merge retries next connect') + .toEqual([SELF, 'dataview', 'templater']); + }); + + it('does not infer removals when the remote has no list yet (absent ≠ emptied)', async () => { + const localDir = makeLocal([SELF, 'dataview']); + const { rw, read } = makeRemote(null); // fresh remote, no file at all + const basePath = basePathFor([SELF, 'dataview']); + + await roundTrip(rw, localDir, basePath); + + expect(readLocal(localDir)).toEqual([SELF, 'dataview']); + expect(read(), 'a fresh remote is seeded from local, not emptied by the base') + .toEqual([SELF, 'dataview']); + }); + + it('is idempotent across pre-spawn pull + connect pull + connect push (no double-apply)', async () => { + // preSpawnPull pulls with the SAME base and must not commit it — otherwise + // the connect's push would read this device's local additions as remote + // removals and delete them. + const localDir = makeLocal([SELF, 'dataview', 'templater']); // templater added here + const { rw, read } = makeRemote([SELF, 'dataview', 'obsidian-git']); // git added on B + const basePath = basePathFor([SELF, 'dataview']); + + // pre-spawn: pull only — must NOT commit the base + await ShadowVaultBootstrap.pullCommunityPlugins(rw, '.obsidian', localDir, basePath); + expect(readBase(basePath), 'a pull must never commit the base').toEqual([SELF, 'dataview']); + + // …then the real connect. + await roundTrip(rw, localDir, basePath); + + const converged = [SELF, 'dataview', 'templater', 'obsidian-git']; + expect(readLocal(localDir)).toEqual(expect.arrayContaining(converged)); + expect(read(), 'the local addition must survive the pre-spawn pull + push sequence') + .toEqual(expect.arrayContaining(converged)); + + // A second connect over the now-committed base changes nothing. + await roundTrip(rw, localDir, basePath); + expect(readLocal(localDir)).toEqual(expect.arrayContaining(converged)); + expect(read()).toEqual(expect.arrayContaining(converged)); + }); + + it('discards a stale base when the shadow vault is bootstrapped fresh (deleted-vault recovery)', async () => { + // The user deletes ~/.obsidian-remote/vaults/ and reconnects. A + // leftover base would make the fresh `["remote-ssh"]` seed look like a + // mass uninstall and strip the remote — so a first bootstrap drops it. + const baseDir = path.join(scratchRoot, 'vaults'); + const sourceDir = path.join(scratchRoot, 'source-plugin'); + fs.mkdirSync(sourceDir, { recursive: true }); + const profile = makeProfile({ id: 'profile-1' }); + + const stalePath = basePathFor([SELF, 'dataview']); + expect(fs.existsSync(stalePath)).toBe(true); + + const configPath = path.join(scratchRoot, 'obsidian.json'); + fs.writeFileSync(configPath, JSON.stringify({ vaults: {} }), 'utf-8'); + const boot = new ShadowVaultBootstrap( + baseDir, sourceDir, new ObsidianRegistry(configPath), scratchRoot, + ); + const { layout } = await boot.bootstrap(profile, [profile]); + + expect(fs.existsSync(stalePath), 'a first bootstrap must discard the stale base').toBe(false); + + // …so the round-trip degrades to a union and the remote keeps dataview. + const { rw, read } = makeRemote([SELF, 'dataview']); + await roundTrip(rw, layout.configDir, stalePath); + expect(read()).toEqual(expect.arrayContaining([SELF, 'dataview'])); + expect(readLocal(layout.configDir)).toEqual(expect.arrayContaining([SELF, 'dataview'])); + }); +}); + describe('ShadowVaultBootstrap plugin-binary round-trip (#429b — BRAT / non-marketplace)', () => { function makeLocalConfigDir(): string { const dir = path.join(