From 5c0b9d789c58a05b084a96e4bc030a985986e587 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Sun, 9 Aug 2026 20:54:00 +0200 Subject: [PATCH] Let a site's folder be opened while it is still being cloned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Creating a site clones wordpress-develop, which takes minutes. The window shows the site immediately and the directory exists from the first moment, but `dir:show` and `editor:open` are gated on the `sites` registry and the registry does not hear about the site until the clone finishes. So for the whole clone the app refused to open a folder it had just created, and there was no way to look at the checkout while watching it arrive (#180). Registering the site early is the obvious fix and the wrong one. `sites` is persisted, so a half-cloned directory written into it survives the crash or the quit that no unregister-on-failure path can catch — a phantom site for a directory that was never finished. It would also widen the allow-list for the recursive delete in `sites:delete` to include a tree isomorphic-git is writing into, and quietly change what `sites` means for every registry already on disk. So the second record is liveness rather than truth. `setup-tracker.js` holds the paths this process is setting up right now, keyed by the directory main computed itself and released in a `finally` however the setup ends. Nothing half-finished reaches the store, and a restart mid-clone lands back on exactly the old behaviour instead of a new state to reconcile. It sits beside the six per-site maps in main.js that already hold liveness this way. The two verbs then want opposite answers about a site being created, and get them: `isActionableSite` widens revealing and opening to include it, while `deleteRegisteredSite` refuses it outright, registered or not. Until now that refusal came for free from the path not being in `sites` yet; making the folder openable is what took the accident away, so the guard is now explicit. The menu stops offering "Delete this site" mid-clone to match — the refusal is the backstop, not the answer. `wordpress:setup` was listed as NOT_REACHABLE for wiring tests on the grounds that it clones over the network. It does not have to: the harness resolves bare packages, so isomorphic-git is stubbable and the handler runs offline. That is what lets a test be *inside* the clone, which is the only place this bug is visible. Fixes #180 for a site whose directory name was free. The window still sends its own guess of the path, so a name collision is refused until the follow-up that has it adopt the real one. Co-Authored-By: Claude Opus 5 (1M context) --- src/editor-launch.js | 8 +- src/main.js | 81 ++++++++++++-------- src/renderer/index.jsx | 9 ++- src/setup-tracker.js | 97 ++++++++++++++++++++++++ src/site-registry.js | 48 ++++++++++-- test/editor-launch.test.cjs | 39 ++++++++++ test/ipc-wiring.test.cjs | 147 +++++++++++++++++++++++++++++++++++- test/setup-tracker.test.cjs | 111 +++++++++++++++++++++++++++ test/site-registry.test.cjs | 65 ++++++++++++++++ 9 files changed, 561 insertions(+), 44 deletions(-) create mode 100644 src/setup-tracker.js create mode 100644 test/setup-tracker.test.cjs diff --git a/src/editor-launch.js b/src/editor-launch.js index 81fa6d5..e935ae8 100644 --- a/src/editor-launch.js +++ b/src/editor-launch.js @@ -29,7 +29,7 @@ const path = require('path'); const { describeRefused } = require('./safe-log'); -const { isRegisteredSite } = require('./site-registry'); +const { isActionableSite } = require('./site-registry'); // Path semantics follow the platform being asked about, not the platform the // test happens to run on: `path.isAbsolute('C:\\x')` is false under POSIX, and a @@ -283,7 +283,8 @@ const REFUSAL_REASONS = { // The `editor:open` handler's body. // // Two gates, both of which have to pass before anything is spawned. The folder -// must be one the app has on record — `isRegisteredSite` from site-registry.js, +// must be one the app has on record, or one it is creating right now — +// `isActionableSite` from site-registry.js, // the same boundary `sites:delete` uses — so "open this site" cannot become // "open this arbitrary directory". And the application must be absolute and of // the platform's shape, so an editor path that has been tampered with, or an @@ -295,12 +296,13 @@ const REFUSAL_REASONS = { // outlives the app and cannot block on a pipe nobody reads. async function openSiteInEditor(sitePath, editorPath, { sites, + pending, platform, statPath, spawn, onRefused } = {}) { - if (!isRegisteredSite(sitePath, sites)) { + if (!isActionableSite(sitePath, { sites, pending })) { if (typeof onRefused === 'function') { onRefused(REFUSAL_REASONS.UNREGISTERED_SITE, describeRefused(sitePath)); } diff --git a/src/main.js b/src/main.js index 7c2b37e..3d45b7f 100644 --- a/src/main.js +++ b/src/main.js @@ -38,6 +38,7 @@ const { buildPullRequestEntries } = require('./pr-files.cjs'); const { openAndScrape, fetchAttachment } = require('./trac-view'); const { openExternalUrl, ALLOWED_URL_SCHEMES } = require('./external-url'); const { deleteRegisteredSite, revealRegisteredSite } = require('./site-registry'); +const { createSetupTracker } = require('./setup-tracker'); const { getStore } = require('./settings-store'); const { parseTicketRef } = require('./renderer/trac-ticket.cjs'); const { parseHandle } = require('./wporg-handle.cjs'); @@ -157,6 +158,11 @@ const cancelledChildren = new WeakSet(); const runIdByDirectory = {}; /** @type {Record} */ const playgroundServers = {}; +// The sites being created right now — liveness, not truth, which is why it is +// here beside the other per-site maps and not in the store. See +// setup-tracker.js: a directory exists minutes before its clone finishes, and +// the guards need to know that without anything half-finished being persisted. +const setupTracker = createSetupTracker(); /** @type {Record} */ const wpDebugWatchers = {}; /** @type {Record} */ @@ -1061,8 +1067,15 @@ ipcMain.handle('wordpress:setup', async (event, destDir, options = {}) => { const uniqueName = findAvailableDirName(destDir, sanitizedName); const siteDir = path.join(destDir, uniqueName); await fse.ensureDir(siteDir); - event.sender.send('download:status', { phase: 'cloning', target: siteDir }); - try { + + // Tracked from here, where the directory starts existing, to the `done` + // below, where the store takes over. In between, `siteDir` is a real folder + // the app made and the registry has never heard of — so without this the + // guards refuse to open it for the whole clone (#180), and `sites:delete` + // would happily remove it if they did not. setup-tracker.js has the why; + // `track` releases the entry however this ends. + return setupTracker.track(siteDir, async () => { + event.sender.send('download:status', { phase: 'cloning', target: siteDir }); await git.clone({ http, fs, @@ -1077,37 +1090,34 @@ ipcMain.handle('wordpress:setup', async (event, destDir, options = {}) => { event.sender.send('download:progress', { target: siteDir, message: msg }); } }); - } catch (e) { - // Fallback/error - throw e; - } - await ensureAutocrlf(siteDir); + await ensureAutocrlf(siteDir); - const s = await getStore(); - const sites = s.get('sites'); - if (!sites.includes(siteDir)) { - sites.push(siteDir); - s.set('sites', sites); - const meta = s.get('siteMeta'); - const siteLabel = typeof options.siteLabel === 'string' && options.siteLabel.trim().length - ? options.siteLabel.trim() - : uniqueName; - const existingMeta = meta[siteDir] || {}; - meta[siteDir] = { - ...existingMeta, - initialized: false, - createdAt: existingMeta.createdAt || new Date().toISOString(), - label: existingMeta.label || siteLabel - }; - try { - const { trunkOid, trunkDate } = await readTrunkInfo(siteDir); - meta[siteDir].trunkOid = trunkOid; - meta[siteDir].trunkDate = trunkDate; - } catch {} - s.set('siteMeta', meta); - } - event.sender.send('download:status', { phase: 'done', target: siteDir, sitePath: siteDir }); - return siteDir; + const s = await getStore(); + const sites = s.get('sites'); + if (!sites.includes(siteDir)) { + sites.push(siteDir); + s.set('sites', sites); + const meta = s.get('siteMeta'); + const siteLabel = typeof options.siteLabel === 'string' && options.siteLabel.trim().length + ? options.siteLabel.trim() + : uniqueName; + const existingMeta = meta[siteDir] || {}; + meta[siteDir] = { + ...existingMeta, + initialized: false, + createdAt: existingMeta.createdAt || new Date().toISOString(), + label: existingMeta.label || siteLabel + }; + try { + const { trunkOid, trunkDate } = await readTrunkInfo(siteDir); + meta[siteDir].trunkOid = trunkOid; + meta[siteDir].trunkDate = trunkDate; + } catch {} + s.set('siteMeta', meta); + } + event.sender.send('download:status', { phase: 'done', target: siteDir, sitePath: siteDir }); + return siteDir; + }); }); ipcMain.handle('sites:mark-initialized', async (_e, sitePath) => { @@ -1135,6 +1145,9 @@ ipcMain.handle('sites:delete', async (_e, sitePath) => { const s = await getStore(); return deleteRegisteredSite(sitePath, { sites: s.get('sites'), + // A site whose clone is still running is refused outright, registered or + // not: `remove` would be deleting a tree isomorphic-git is writing into. + pending: setupTracker.paths(), forget: () => { s.set('sites', s.get('sites').filter((p) => p !== sitePath)); const meta = s.get('siteMeta'); @@ -1144,7 +1157,7 @@ ipcMain.handle('sites:delete', async (_e, sitePath) => { // Best-effort, as before: a site whose registry entry is gone should not be // stuck undeletable because its directory is missing or locked. remove: async (p) => { try { await fse.remove(p); } catch {} }, - onRefused: (description) => logEvent('sites', `refused to delete ${description} — not a registered site`) + onRefused: (description) => logEvent('sites', `refused to delete ${description} — not a registered site, or still being created`) }); }); @@ -1285,6 +1298,7 @@ ipcMain.handle('editor:open', async (_e, sitePath, editorPath) => { return openSiteInEditor(sitePath, target, { ...editorLaunchDeps(), sites: s.get('sites'), + pending: setupTracker.paths(), spawn, onRefused: (reason, description) => logEvent('editor', `refused to open ${description} — ${reason}`) }); @@ -1350,6 +1364,7 @@ ipcMain.handle('dir:show', async (_e, sitePath) => { const s = await getStore(); return revealRegisteredSite(sitePath, { sites: s.get('sites'), + pending: setupTracker.paths(), reveal: (target) => shell.openPath(target), onRefused: (description) => logEvent('sites', `refused to reveal ${description} — not a registered site`) }); diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index 20e2e85..4160ef9 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -3013,7 +3013,14 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // "Already up to date." in the terminal. { title: 'Update to latest trunk', onClick: startTrunkUpdate }, { title:'Forget this site', onClick:()=>confirmAnd('Remove this site from the list?', ()=>onForget(sitePath)) }, - { title:'Delete this site', onClick:()=>confirmAnd('Delete this site from disk? This cannot be undone.', ()=>onDelete(sitePath)) } + // Not while the clone is running: deleting the site would be + // removing a directory the app is still writing into. The main + // process refuses it either way (see site-registry.js) — that is + // the backstop, and not offering a control that cannot work is + // the actual answer. + ...(isPending ? [] : [ + { title:'Delete this site', onClick:()=>confirmAnd('Delete this site from disk? This cannot be undone.', ()=>onDelete(sitePath)) } + ]) ]} /> diff --git a/src/setup-tracker.js b/src/setup-tracker.js new file mode 100644 index 0000000..5cd8ed8 --- /dev/null +++ b/src/setup-tracker.js @@ -0,0 +1,97 @@ +// Which sites this process is creating right now. +// +// Creating a site clones `wordpress-develop`, which takes minutes. The window +// shows the site immediately, and it should: the directory exists from the +// first moment, and a contributor watching a clone has every reason to open the +// folder and look. But `dir:show` and `editor:open` are gated on the `sites` +// registry, and the registry does not learn about the site until the clone +// finishes — so for the whole clone the app refused to open a folder it had +// created itself (#180). +// +// The fix is not to register it early. `sites` is persisted, and a half-cloned +// directory written into it survives the crash or the quit that the +// unregister-on-failure path cannot catch — a phantom site for a directory that +// was never finished, which is the thing AGENTS.md's architecture rules single +// out. It would also widen the allow-list for the recursive delete in +// `sites:delete` to include a tree isomorphic-git is writing into. +// +// So this is the other half of the boundary, and the distinction it draws is +// **liveness against truth**. The store answers "which sites exist"; this +// answers "what is this process doing right now". They differ in lifetime, and +// that is the point: an entry here cannot outlive the process, so a restart +// mid-clone lands back on exactly today's behaviour instead of a new broken +// state that has to be reconciled. +// +// It sits beside six existing per-site maps in main.js that hold liveness the +// same way — `playgroundServers`, `runningInstalls`, `runningScripts`, +// `runIdByDirectory`, `wpDebugWatchers`, `smtpServers` — and its entries are +// the shortest-lived of them all: one handler call, released in a `finally`. +// +// Pure, so both halves are testable without an Electron process. +// +// On what the keys are, precisely, because the next widening of +// `isActionableSite` will be argued from it: a key is +// `path.join(destDir, uniqueName)`. The leaf is main's — `findAvailableDirName` +// picks a name that does not exist yet, from a string with path separators +// already stripped — but `destDir` is the renderer's, straight off the +// `wordpress:setup` invoke. So this is not "a path main computed from nothing"; +// it is "a directory main is about to create and clone into, under a parent the +// contributor chose in a file dialog". +// +// That is enough for what it is used for. Being here says the app is writing +// into that directory right now, which is a fact about this process regardless +// of who named it, and the same call would register the same path in the store +// minutes later. It would not be enough to justify widening anything +// destructive, which is why `deleteRegisteredSite` refuses these outright +// rather than treating them as a second registry. + +'use strict'; + +function createSetupTracker() { + const inFlight = new Set(); + + // True when this call is the one that claimed the path. False for a path + // already being set up — two windows can resolve the same directory name + // before either creates it, and two clones interleaving in one tree is worse + // than the second one refusing. + function begin(sitePath) { + if (typeof sitePath !== 'string' || sitePath === '') return false; + if (inFlight.has(sitePath)) return false; + inFlight.add(sitePath); + return true; + } + + function end(sitePath) { + return inFlight.delete(sitePath); + } + + function has(sitePath) { + return typeof sitePath === 'string' && sitePath !== '' && inFlight.has(sitePath); + } + + // A copy. The array is handed to the guards as their `pending` list, and a + // guard that could be widened by whoever it is guarding is not a guard. + function paths() { + return [...inFlight]; + } + + // Runs `work` with the path tracked, and releases it however that ends. The + // release is the whole reason this is a function rather than two calls: a + // clone that throws is exactly when a forgotten `end` would leave the site + // permanently undeletable, and exactly when the caller is thinking about + // something else. + async function track(sitePath, work) { + if (!begin(sitePath)) { + throw new Error(`A setup is already running for ${sitePath}`); + } + try { + return await work(); + } finally { + end(sitePath); + } + } + + return { begin, end, has, paths, track }; +} + +module.exports = { createSetupTracker }; diff --git a/src/site-registry.js b/src/site-registry.js index a0823a7..a322157 100644 --- a/src/site-registry.js +++ b/src/site-registry.js @@ -13,6 +13,13 @@ // one the app should carry out. This is the same shape as external-url.js — a // pure check, a safe log formatter, and a wrapper whose effects are injected so // both branches can be tested without an Electron process. +// +// There is a second, shorter-lived record: the sites this process is creating +// right now (setup-tracker.js). A site's directory exists from the moment the +// app makes it, minutes before the clone finishes and the store hears about it, +// and the two verbs here want opposite answers about it — open it, yes; delete +// it, absolutely not. So `pending` widens `revealRegisteredSite` and is an +// outright refusal in `deleteRegisteredSite`. The asymmetry is the point. const { describeRefused } = require('./safe-log'); @@ -27,6 +34,21 @@ function isRegisteredSite(sitePath, sites) { return sites.includes(sitePath); } +// True for a path this app is responsible for right now: one it has on record, +// or one it is creating this very moment (see setup-tracker.js for why the +// second kind is deliberately not in the store). +// +// `pending` goes through the same exact-match predicate as `sites` rather than +// any looser comparison. It is a list of directories the app is writing into, +// so a prefix match would turn "this site is being cloned" into a lever on +// everything beneath it. +// +// This widens what may be *opened*. It must never be used to widen what may be +// removed — see `deleteRegisteredSite`, which refuses a pending path outright. +function isActionableSite(sitePath, { sites, pending } = {}) { + return isRegisteredSite(sitePath, sites) || isRegisteredSite(sitePath, pending); +} + // A refused path is attacker-influenced by hypothesis, and it is about to be // written into the file contributors attach to bug reports, so it has to stay on // one line and it has to be bounded. safe-log.js is where both live, and why. @@ -39,7 +61,18 @@ function describeRefusedSite(sitePath) { // the store, `remove` is the real `fse.remove` in the app, and both are recording // stubs in the tests. A path that is not registered performs neither: no store // mutation and no removal, just a logged refusal. -async function deleteRegisteredSite(sitePath, { sites, forget, remove, onRefused } = {}) { +async function deleteRegisteredSite(sitePath, { sites, pending, forget, remove, onRefused } = {}) { + // Checked before the registry, and separately from it. A site whose clone is + // still running is the one case where `remove` would delete a tree another + // part of this process is actively writing into, so it is refused whether or + // not it is registered. Until this existed the refusal came for free from the + // path not being in `sites` yet; making the folder openable mid-clone is what + // took that accident away. + if (isRegisteredSite(sitePath, pending)) { + if (typeof onRefused === 'function') onRefused(describeRefusedSite(sitePath)); + return false; + } + if (!isRegisteredSite(sitePath, sites)) { if (typeof onRefused === 'function') onRefused(describeRefusedSite(sitePath)); return false; @@ -51,15 +84,17 @@ async function deleteRegisteredSite(sitePath, { sites, forget, remove, onRefused } // The `dir:show` handler's body. `shell.openPath` hands a local path to whatever -// the OS has registered for it, so "show this site in the file manager" gets the -// same boundary as "delete this site": only a path the app has on record. The -// reveal itself is injected, like `remove` above. +// the OS has registered for it, so "show this site in the file manager" is +// bounded the same way "delete this site" is — except that a site still being +// created counts here and does not there, since opening a folder mid-clone is +// what a contributor watching one wants and removing it is not. The reveal +// itself is injected, like `remove` above. // // `reveal` resolves to electron's own convention — the empty string on success, // an error message otherwise — and that is passed through rather than reduced to // a boolean, so the renderer can say what went wrong. -async function revealRegisteredSite(sitePath, { sites, reveal, onRefused } = {}) { - if (!isRegisteredSite(sitePath, sites)) { +async function revealRegisteredSite(sitePath, { sites, pending, reveal, onRefused } = {}) { + if (!isActionableSite(sitePath, { sites, pending })) { if (typeof onRefused === 'function') onRefused(describeRefusedSite(sitePath)); return { ok: false, reason: 'unregistered-site' }; } @@ -70,6 +105,7 @@ async function revealRegisteredSite(sitePath, { sites, reveal, onRefused } = {}) module.exports = { isRegisteredSite, + isActionableSite, describeRefusedSite, revealRegisteredSite, deleteRegisteredSite diff --git a/test/editor-launch.test.cjs b/test/editor-launch.test.cjs index adee77a..09802da 100644 --- a/test/editor-launch.test.cjs +++ b/test/editor-launch.test.cjs @@ -461,3 +461,42 @@ test('a long-lived editor answers as soon as the OS accepts it', async () => { assert.deepEqual(result, { ok: true }); assert.equal(calls[0].unrefed, true); }); + +// --- a site that is still being created ---------------------------------- +// +// The same boundary as revealing the folder, and for the same reason: the +// directory exists from the moment the app creates it, but the registry does +// not hear about it until the clone finishes minutes later, so opening the +// site in an editor was refused for the whole of it (#180). `pending` is the +// main process's own record of what it is setting up right now. + +const CLONING = '/Users/dev/sites/being-cloned'; + +test('a site still being created opens in an editor', async () => { + const { calls, refusals, options } = launchDeps({ sites: [] }); + + const result = await openSiteInEditor(CLONING, EDITOR, { ...options, pending: [CLONING] }); + + assert.deepEqual(result, { ok: true }); + assert.deepEqual(refusals, []); + assert.equal(calls.length, 1); + assert.deepEqual(calls[0].args, ['-a', EDITOR, CLONING]); +}); + +test('a pending path is matched exactly, like a registered one', async () => { + for (const near of ['/Users/dev/sites', `${CLONING}/wp-content`, `${CLONING}/`]) { + const { calls, options } = launchDeps({ sites: [] }); + + const result = await openSiteInEditor(near, EDITOR, { ...options, pending: [CLONING] }); + + assert.equal(result.ok, false, near); + assert.deepEqual(calls, []); + } +}); + +test('no pending list at all behaves exactly as before', async () => { + const { options } = launchDeps(); + + assert.deepEqual(await openSiteInEditor(SITE, EDITOR, options), { ok: true }); + assert.equal((await openSiteInEditor(CLONING, EDITOR, options)).reason, REFUSAL_REASONS.UNREGISTERED_SITE); +}); diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index 147f1a8..cf933ef 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -362,6 +362,10 @@ test('sites:delete asks site-registry whether the path may be removed', async () // callbacks: without the store's own `sites` array it would be deciding // against nothing, and without the callbacks it could not act on its answer. assert.deepEqual(options.sites, ['/sites/wp']); + // And the sites being created right now, which the module refuses outright. + // Without this the handler would be asking a question with half the facts, + // and the answer would be to delete a tree a clone is writing into. + assert.ok(Array.isArray(options.pending), 'the in-flight setups must reach the guard'); assert.equal(typeof options.forget, 'function'); assert.equal(typeof options.remove, 'function'); assert.equal(typeof options.onRefused, 'function'); @@ -1397,6 +1401,9 @@ test('editor:open asks editor-launch to open the site, with the registry as its // boundary, `statPath` is how it checks the application is still there, and // `spawn` is the effect it is being asked to guard. assert.deepEqual(options.sites, [SITE]); + // The registry is not the whole boundary: a site still being cloned is not + // in it yet, and opening that folder is the point of #180. + assert.ok(Array.isArray(options.pending), 'the in-flight setups must reach the guard'); assert.equal(typeof options.statPath, 'function'); assert.equal(typeof options.spawn, 'function'); assert.equal(options.platform, process.platform); @@ -1498,6 +1505,7 @@ test('dir:show asks site-registry whether the path may be revealed', async () => const [sitePath, options] = revealRegisteredSite.calls[0]; assert.equal(sitePath, SITE); assert.deepEqual(options.sites, [SITE]); + assert.ok(Array.isArray(options.pending), 'the in-flight setups must reach the guard'); assert.equal(typeof options.reveal, 'function'); assert.equal(typeof options.onRefused, 'function'); }); @@ -1519,6 +1527,143 @@ test('dir:show refuses a path the registry does not hold, and logs it', async () assert.deepEqual(main.calls.openPath, [SITE]); }); +// --- creating a site, and opening it while it is still being created ----- +// +// This handler was listed as NOT_REACHABLE, on the grounds that it clones +// wordpress-develop over the network. It does not have to: `resolveStubs` +// resolves bare packages through `require.resolve`, so `isomorphic-git` is +// stubbable like any other module and the whole handler runs offline. That +// matters here beyond coverage — #180 is a bug about *when* things are true +// during the clone, and only a test that can be inside the clone can see it. + +// Runs `wordpress:setup` with a stubbed clone, and calls `duringClone` at the +// moment the real clone would be running: the directory exists, nothing is in +// the store yet. `clone` can be made to fail instead. +async function runSetup({ duringClone, cloneFails = false, existing = [], extraStubs = {} } = {}) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'ipc-wiring-setup-')); + for (const name of existing) fs.mkdirSync(path.join(root, name)); + + const settings = fakeSettingsStore(); + const seen = []; + let inside; + + const clone = async ({ dir }) => { + if (duringClone) inside = await duringClone({ dir, root, main, settings }); + if (cloneFails) throw new Error('clone failed'); + }; + + const main = loadMain({ + stubs: { + ...silentLogging(), + ...settings.stubs, + 'isomorphic-git': { clone }, + './trunk-update': { ensureAutocrlf: async () => {}, readTrunkInfo: async () => ({ trunkOid: 'abc', trunkDate: '2026-01-01' }) }, + ...extraStubs + } + }); + + const event = createIpcEvent(); + const settled = await main.invokeWith('wordpress:setup', event, root, { siteName: 'demo', siteLabel: 'Demo' }) + .then((siteDir) => ({ siteDir }), (error) => ({ error })); + + for (const { channel, payload } of event.sent) if (channel === 'download:status') seen.push(payload); + return { root, main, settings, inside, statuses: seen, ...settled }; +} + +test('the folder can be revealed while it is still being cloned, without being registered', async () => { + const { root, settings, inside, siteDir } = await runSetup({ + duringClone: async ({ dir, main: m, settings: st }) => ({ + revealed: await m.invoke('dir:show', dir), + openPathCalls: [...m.calls.openPath], + registeredMidClone: structuredClone(st.values.sites) + }) + }); + + // The bug, stated: this was `{ ok: false, reason: 'unregistered-site' }`. + assert.deepEqual(inside.revealed, { ok: true }); + assert.deepEqual(inside.openPathCalls, [path.join(root, 'demo')]); + // And the reason it could not simply be registered early: nothing + // half-finished may reach the store, where it would outlive the process. + assert.deepEqual(inside.registeredMidClone, [], 'no phantom site while the clone runs'); + + assert.equal(siteDir, path.join(root, 'demo')); + assert.deepEqual(settings.values.sites, [siteDir], 'and it is registered once the clone finishes'); +}); + +test('deleting a site is refused while its clone is running, and the directory survives', async () => { + const { root, inside } = await runSetup({ + duringClone: async ({ dir, main: m }) => ({ + deleted: await m.invoke('sites:delete', dir), + stillThere: fs.existsSync(dir) + }) + }); + + assert.equal(inside.deleted, false); + assert.equal(inside.stillThere, true); + assert.equal(fs.existsSync(path.join(root, 'demo')), true, 'the finished clone is still on disk'); +}); + +test('a clone that fails leaves nothing registered and nothing in flight', async () => { + const { root, main, settings, error } = await runSetup({ cloneFails: true }); + + assert.match(String(error), /clone failed/); + assert.deepEqual(settings.values.sites, []); + assert.deepEqual(settings.values.siteMeta, {}); + // The entry is released however the setup ends, so the path is refused again + // rather than staying openable — and, more importantly, staying undeletable. + assert.equal((await main.invoke('dir:show', path.join(root, 'demo'))).ok, false); +}); + +test('a name already taken on disk is the one that opens, from the first moment', async () => { + const { root, inside, siteDir } = await runSetup({ + existing: ['demo'], + duringClone: async ({ dir, root: destDir, main: m }) => ({ + dir, + collided: await m.invoke('dir:show', path.join(destDir, 'demo')), + real: await m.invoke('dir:show', dir) + }) + }); + + assert.equal(siteDir, path.join(root, 'demo-2')); + assert.equal(inside.dir, path.join(root, 'demo-2')); + assert.deepEqual(inside.real, { ok: true }); + // The directory that merely shares the name is not the site being created, + // and is not opened on its behalf. + assert.equal(inside.collided.ok, false); +}); + +// The other half of #180, and the half the unit tests cannot see: they hand +// `pending` to the guard themselves, so they stay green if main stops sending +// it. This asserts the list main actually builds, at the one moment it matters. +test('the folder can be opened in an editor while it is still being cloned', async () => { + const openSiteInEditor = spy(async () => ({ ok: true })); + const { inside, siteDir } = await runSetup({ + extraStubs: { + './editor-launch': { + openSiteInEditor, + matchDetectedEditor: async () => ({ id: 'cursor', name: 'Cursor', path: EDITOR }) + } + }, + duringClone: async ({ dir, main: m }) => ({ + opened: await m.invoke('editor:open', dir, EDITOR), + handed: openSiteInEditor.calls.at(-1)[2] + }) + }); + + assert.deepEqual(inside.opened, { ok: true }); + assert.deepEqual(inside.handed.sites, [], 'the registry cannot know about it yet'); + assert.deepEqual(inside.handed.pending, [siteDir], 'so this is what lets the guard say yes'); +}); + +test('the cloning status names the directory the guards are keyed on', async () => { + const { statuses, siteDir } = await runSetup({ existing: ['demo'] }); + + const cloning = statuses.find((p) => p.phase === 'cloning'); + // PR 3 has the window adopt this; it is only safe if it is the same string + // the tracker holds, verbatim. + assert.equal(cloning.target, siteDir); +}); + // --- opening a pull request (#167) --------------------------------------- // Sign-in is two-legged: the handler returns as soon as there is a code to @@ -1762,6 +1907,7 @@ const WIRED = new Set([ 'editor:list', 'editor:open', 'dir:show', + 'wordpress:setup', 'provenance:set-handle', 'provenance:set-event', 'github:account', @@ -1807,7 +1953,6 @@ const NO_DELEGATION = new Map([ // Channels that do delegate, but whose call sits behind something this harness // cannot stand in for yet. Each one is a known hole, not an oversight. const NOT_REACHABLE = new Map([ - ['wordpress:setup', 'calls ensureAutocrlf and readTrunkInfo only after cloning wordpress-develop over the network'], ['trac:list-attachments', 'reads electron-store for the ticket before it can open the Trac window'] ]); diff --git a/test/setup-tracker.test.cjs b/test/setup-tracker.test.cjs new file mode 100644 index 0000000..68d8a75 --- /dev/null +++ b/test/setup-tracker.test.cjs @@ -0,0 +1,111 @@ +'use strict'; + +// The set of sites this process is creating right now. +// +// Everything here is about the release, not the record: a path that stays +// tracked after its setup ended would keep `sites:delete` refusing a site the +// contributor can see and has every right to remove. + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { createSetupTracker } = require('../src/setup-tracker.js'); + +const DIR = '/Users/dev/sites/wp'; + +test('a path is tracked while its work runs, and not before or after', async () => { + const tracker = createSetupTracker(); + assert.equal(tracker.has(DIR), false); + + let duringTheWork; + await tracker.track(DIR, async () => { + duringTheWork = tracker.has(DIR); + }); + + assert.equal(duringTheWork, true, 'the folder must be openable while it is being created'); + assert.equal(tracker.has(DIR), false, 'the entry must not outlive the setup'); +}); + +// The whole reason the module exists rather than a bare Set with two call +// sites: a clone that throws is the case where forgetting to release would +// leave the site permanently undeletable. +test('a failure releases the path and still fails', async () => { + const tracker = createSetupTracker(); + const boom = new Error('clone failed'); + + await assert.rejects( + tracker.track(DIR, async () => { throw boom; }), + (e) => e === boom + ); + + assert.equal(tracker.has(DIR), false); +}); + +test('the work’s own value is what track resolves to', async () => { + const tracker = createSetupTracker(); + + assert.equal(await tracker.track(DIR, async () => DIR), DIR); +}); + +// Two windows can compute the same directory name before either creates it, and +// a second setup into a directory the first is cloning into would interleave two +// clones in one tree. +test('a path already being set up is refused a second setup', async () => { + const tracker = createSetupTracker(); + let secondAttempt; + let ranSecondWork = false; + let stillHeldAfterRefusal; + + await tracker.track(DIR, async () => { + secondAttempt = await tracker.track(DIR, async () => { ranSecondWork = true; }).then( + () => null, + (e) => e + ); + // Asserted here, inside the first setup, rather than after it: the + // release this is about is the one a refused attempt must *not* perform. + // Checked after the outer `track` had ended, it would only be re-testing + // the outer release and would pass for a tracker whose refusal deleted + // the entry — leaving a live clone deletable. + stillHeldAfterRefusal = tracker.has(DIR); + }); + + assert.ok(secondAttempt instanceof Error, 'the second setup must not run'); + assert.equal(ranSecondWork, false, 'and must not run its work either'); + assert.equal(stillHeldAfterRefusal, true, 'the refused attempt must not release the first one’s entry'); + assert.equal(tracker.has(DIR), false, 'the first one still releases normally'); +}); + +test('paths lists what is in flight, and is a copy', () => { + const tracker = createSetupTracker(); + tracker.begin(DIR); + + const paths = tracker.paths(); + assert.deepEqual(paths, [DIR]); + + paths.push('/somewhere/else'); + assert.deepEqual(tracker.paths(), [DIR], 'a caller must not be able to widen the guard'); +}); + +test('sites being set up in parallel are tracked independently', async () => { + const tracker = createSetupTracker(); + const other = '/Users/dev/sites/other'; + let seen; + + await tracker.track(DIR, async () => { + await tracker.track(other, async () => { seen = tracker.paths().sort(); }); + assert.equal(tracker.has(other), false); + assert.equal(tracker.has(DIR), true); + }); + + assert.deepEqual(seen, [other, DIR].sort()); + assert.deepEqual(tracker.paths(), []); +}); + +test('anything that is not a usable path is not tracked', () => { + const tracker = createSetupTracker(); + + for (const bad of ['', null, undefined, 42, {}]) { + assert.equal(tracker.begin(bad), false, String(bad)); + } + assert.deepEqual(tracker.paths(), []); +}); diff --git a/test/site-registry.test.cjs b/test/site-registry.test.cjs index 3db73a7..a5af5d6 100644 --- a/test/site-registry.test.cjs +++ b/test/site-registry.test.cjs @@ -161,3 +161,68 @@ test('a reveal the OS declines is reported rather than swallowed', async () => { assert.equal(result.reason, 'open-failed'); assert.equal(result.error, 'Failed to open path'); }); + +// --- a site that is still being created ---------------------------------- +// +// Creating a site clones wordpress-develop, which takes minutes, and the +// registry does not learn about the site until that finishes. The directory +// exists the whole time and the window shows it, so refusing to open it was the +// app declining to open a folder it had just created (#180). +// +// `pending` is the other half of the boundary: paths the main process is +// setting up right now, computed by main itself and never sent by the renderer. +// It widens what may be *opened*, and it narrows what may be *deleted* — a +// recursive remove of a tree isomorphic-git is writing into is the one thing +// worse than the bug. + +const PENDING = '/Users/dev/sites/being-cloned'; + +test('a site still being created is revealed', async () => { + const rec = revealRecorder([]); + + const result = await revealRegisteredSite(PENDING, { ...rec.options, pending: [PENDING] }); + + assert.deepEqual(result, { ok: true }); + assert.deepEqual(rec.revealed, [PENDING]); + assert.deepEqual(rec.refused, []); +}); + +test('a site still being created is not deleted, registered or not', async () => { + for (const sites of [[], [PENDING]]) { + const rec = recorder(sites); + + assert.equal(await deleteRegisteredSite(PENDING, { ...rec.options, pending: [PENDING] }), false); + + assert.equal(rec.forgotten(), 0, 'the store must not be touched mid-clone'); + assert.deepEqual(rec.removed, [], 'the clone must not be removed from under itself'); + assert.equal(rec.refused.length, 1); + } +}); + +// The asymmetry is the design, so it is asserted rather than assumed: being in +// flight is a reason to open and a reason not to delete. +test('pending never widens what may be deleted', async () => { + const rec = recorder([]); + + assert.equal(await deleteRegisteredSite('/Users/dev/elsewhere', { ...rec.options, pending: [PENDING] }), false); + + assert.deepEqual(rec.removed, []); +}); + +test('a pending path is matched exactly, like a registered one', async () => { + for (const near of ['/Users/dev/sites', `${PENDING}/wp-content`, `${PENDING}/`]) { + const rec = revealRecorder([]); + + const result = await revealRegisteredSite(near, { ...rec.options, pending: [PENDING] }); + + assert.equal(result.ok, false, near); + assert.deepEqual(rec.revealed, []); + } +}); + +test('no pending list at all behaves exactly as before', async () => { + const rec = revealRecorder(); + + assert.deepEqual(await revealRegisteredSite('/Users/dev/sites/my-site', rec.options), { ok: true }); + assert.equal((await revealRegisteredSite(PENDING, rec.options)).reason, 'unregistered-site'); +});