diff --git a/src/backend/services/share/ShareService.test.ts b/src/backend/services/share/ShareService.test.ts index dc808f5a5..4045ac808 100644 --- a/src/backend/services/share/ShareService.test.ts +++ b/src/backend/services/share/ShareService.test.ts @@ -1519,6 +1519,202 @@ describe('ShareService', () => { expect(audiences.flat()).toContain(recipient.user.id); }); + it('tells a folder recipient when a file inside it is deleted', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const { dir, file } = await makeDirWithFile(owner.user); + + await share(owner.actor, { + uid: dir.uuid, + recipient: { email: recipient.email }, + mode: 'read', + }); + + // Nothing was shared at the file, so the revoke reports no holder + // for it — the audience has to be found through the folder. + const payload = await capturePayload( + 'outer.gui.item.removed', + recipient.user.id, + async () => { + await server.services.fs.remove(owner.user.id, { + entry: file, + }); + }, + ); + + // Named by the path they knew, masked through the folder they hold. + expect(payload?.path).toBe( + `/${owner.user.username}/${dir.uuid}/${dir.name}/${file.name}`, + ); + }); + + it('tells a recipient once when they hold both the file and its folder', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const { dir, file } = await makeDirWithFile(owner.user); + + for (const uid of [dir.uuid, file.uuid]) { + await share(owner.actor, { + uid, + recipient: { email: recipient.email }, + mode: 'read', + }); + } + + // The revoke reports them for the file, and the folder reaches it + // too — one delete must not arrive as two removals. + const audiences = await captureAudiences( + 'outer.gui.item.removed', + file.uuid, + async () => { + await server.services.fs.remove(owner.user.id, { + entry: file, + }); + }, + ); + + const told = audiences + .flat() + .filter((id) => id === recipient.user.id); + expect(told).toHaveLength(1); + }); + + it('tells a folder recipient when a file is moved out of it', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const { dir, file } = await makeDirWithFile(owner.user); + const before = file.path; + const after = `/${owner.user.username}/Trash/${file.name}`; + + await share(owner.actor, { + uid: dir.uuid, + recipient: { email: recipient.email }, + mode: 'read', + }); + + // What the GUI's Delete does. `item.moved` would name a path the + // recipient cannot see, leaving the file on their screen. + const audiences = await captureAudiences( + 'outer.gui.item.removed', + file.uuid, + async () => { + await server.clients.event.emitAndWait( + 'fs.move.node', + { + node: { ...file, path: after }, + fromPath: before, + toPath: after, + }, + {}, + ); + }, + ); + + expect(audiences.flat()).toContain(recipient.user.id); + }); + + it('names a moved-out file by where the recipient last saw it', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const { dir, file } = await makeDirWithFile(owner.user); + const before = file.path; + const after = `/${owner.user.username}/Trash/${file.name}`; + + await share(owner.actor, { + uid: dir.uuid, + recipient: { email: recipient.email }, + mode: 'read', + }); + + const payload = await capturePayload( + 'outer.gui.item.removed', + recipient.user.id, + async () => { + await server.clients.event.emitAndWait( + 'fs.move.node', + { + node: { ...file, path: after }, + fromPath: before, + toPath: after, + }, + {}, + ); + }, + ); + + // Masked through the folder they hold, not the owner's Trash. + expect(payload?.path).toBe( + `/${owner.user.username}/${dir.uuid}/${dir.name}/${file.name}`, + ); + }); + + it('tells a folder recipient when a file is moved into it', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const { dir } = await makeDirWithFile(owner.user); + const loose = await makeFile(owner.user); + const after = `${dir.path}/${loose.name}`; + + await share(owner.actor, { + uid: dir.uuid, + recipient: { email: recipient.email }, + mode: 'read', + }); + + // They never had it, so there is nothing for `item.moved` to move. + const audiences = await captureAudiences( + 'outer.gui.item.added', + loose.uuid, + async () => { + await server.clients.event.emitAndWait( + 'fs.move.node', + { + node: { ...loose, path: after }, + fromPath: loose.path, + toPath: after, + }, + {}, + ); + }, + ); + + expect(audiences.flat()).toContain(recipient.user.id); + }); + + it('still reports a move within the shared folder as a move', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const { dir, file } = await makeDirWithFile(owner.user); + const before = file.path; + const after = `${dir.path}/renamed-${file.name}`; + + await share(owner.actor, { + uid: dir.uuid, + recipient: { email: recipient.email }, + mode: 'read', + }); + + const payload = await capturePayload( + 'outer.gui.item.moved', + recipient.user.id, + async () => { + await server.clients.event.emitAndWait( + 'fs.move.node', + { + node: { ...file, path: after }, + fromPath: before, + toPath: after, + }, + {}, + ); + }, + ); + + const masked = `/${owner.user.username}/${dir.uuid}/${dir.name}`; + expect(payload?.from_path).toBe(`${masked}/${file.name}`); + expect(payload?.path).toBe(`${masked}/renamed-${file.name}`); + }); + it('tells a folder recipient when a file inside it changes', async () => { const owner = await makeUser(); const recipient = await makeUser(); @@ -1638,7 +1834,10 @@ describe('ShareService', () => { /** The one payload sent to `holder` for `event`, or undefined. */ const capturePayload = async ( - event: 'outer.gui.item.moved' | 'outer.gui.item.renamed', + event: + | 'outer.gui.item.moved' + | 'outer.gui.item.removed' + | 'outer.gui.item.renamed', holderId: number, fn: () => Promise, ) => { @@ -1703,6 +1902,63 @@ describe('ShareService', () => { expect(lookups).toBe(1); }); + it('asks once per folder when a whole subtree is deleted', async () => { + const owner = await makeUser(); + const recipient = await makeUser(); + const { dir } = await makeDirWithFile(owner.user); + + await share(owner.actor, { + uid: dir.uuid, + recipient: { email: recipient.email }, + mode: 'read', + }); + + await Promise.all( + Array.from({ length: 8 }, (_, i) => + server.services.fs.touch(owner.user.id, { + path: `${dir.path}/doomed-${i}.txt`, + }), + ), + ); + const files = await Promise.all( + Array.from({ length: 8 }, (_, i) => + server.stores.fsEntry.getEntryByPath( + `${dir.path}/doomed-${i}.txt`, + ), + ), + ); + + const seen: number[][] = []; + const listener = (_key: string, data: unknown) => { + const payload = data as { user_id_list?: number[] }; + if (!payload.user_id_list?.includes(recipient.user.id)) return; + seen.push(payload.user_id_list); + }; + server.clients.event.on('outer.gui.item.removed', listener); + const reaching = vi.spyOn(server.stores.share, 'listReaching'); + let lookups = -1; + + try { + await Promise.all( + files.map((entry) => + server.services.fs.remove(owner.user.id, { entry }), + ), + ); + for (let i = 0; i < 50 && seen.length < 8; i++) { + await new Promise((resolve) => setTimeout(resolve, 20)); + } + lookups = reaching.mock.calls.length; + } finally { + server.clients.event.off('outer.gui.item.removed', listener); + reaching.mockRestore(); + } + + // Siblings share a parent, so they share the audience lookup — + // a burst settles in one or two flushes, never one per file. + expect(seen).toHaveLength(8); + expect(lookups).toBeLessThanOrEqual(2); + }); + it('stays quiet for an event another node already handled', async () => { const owner = await makeUser(); const recipient = await makeUser(); diff --git a/src/backend/services/share/ShareService.ts b/src/backend/services/share/ShareService.ts index 7deb0b645..044a068ac 100644 --- a/src/backend/services/share/ShareService.ts +++ b/src/backend/services/share/ShareService.ts @@ -205,9 +205,11 @@ const holderPayload = ( entry: FSEntry, /** Defaults to the entry itself, for the paths with no root to mask by. */ root: FSEntry = entry, + /** Where the entry was; differs from `entry.path` once it has moved. */ + realPath: string = entry.path, ): Record => { const path = - maskedPathVia(root, entry.path) ?? maskedSelfPath(entry, entry.path); + maskedPathVia(root, realPath) ?? maskedSelfPath(entry, realPath); return { uid: entry.uuid, uuid: entry.uuid, @@ -320,17 +322,7 @@ export class ShareService extends PuterService { fromUserId?: number; }; if (!node?.uuid) return; - const notify = this.#fanOutToHolders( - node, - 'outer.gui.item.moved', - (root) => { - // Omitted when the move started outside this share. - const from = fromPath - ? maskedFormerPath(root, node, fromPath) - : null; - return from ? { from_path: from } : {}; - }, - ).catch(() => { + const notify = this.#fanOutMove(node, fromPath).catch(() => { // A stale window is better than a failed move. }); @@ -499,6 +491,7 @@ export class ShareService extends PuterService { */ async #flushRetire(entries: FSEntry[]): Promise { const ownerOf = new Map(entries.map((e) => [e.uuid, e])); + const notified = new Map>(); for (let i = 0; i < entries.length; i += RETIRE_CHUNK_SIZE) { const chunk = entries.slice(i, i + RETIRE_CHUNK_SIZE); @@ -526,8 +519,135 @@ export class ShareService extends PuterService { [...holders], holderPayload(entry), ); + notified.set(uuid, holders); } } + + await this.#fanOutRetiredToAncestors(entries, notified); + } + + /** + * Tell whoever reached these through a folder above them. Their grant is on + * that folder, not on what was deleted, so the revoke reports no holder for + * them — and the file stays in their window, 404ing when they open it. + * Coalesced by parent like creates, so a subtree stays a few queries. + */ + async #fanOutRetiredToAncestors( + entries: FSEntry[], + notified: Map>, + ): Promise { + const byParent = new Map(); + for (const entry of entries) { + const parent = pathPosix.dirname(entry.path); + byParent.set(parent, [...(byParent.get(parent) ?? []), entry]); + } + + for (const siblings of byParent.values()) { + const first = siblings[0]; + if (!first) continue; + // Ancestors only — a share on the entry itself is the revoke's to + // report, and it already has. + const groups = (await this.#reachingRoots(first)).filter( + ({ root }) => root.uuid !== first.uuid, + ); + for (const { root, holders } of groups) { + for (const entry of siblings) { + const unheard = holders.filter( + (holder) => !notified.get(entry.uuid)?.has(holder), + ); + if (!unheard.length) continue; + await this.#emitGui( + 'outer.gui.item.removed', + unheard, + holderPayload(entry, root), + ); + } + } + } + } + + /** + * A move seen from both ends, because it can take an item out of someone's + * share as easily as into it. Reaching both ends is a move, only the + * destination an arrival, only the origin a removal — that last being the + * GUI's Delete, a move to the owner's Trash where no recipient has a + * share. + */ + async #fanOutMove(entry: FSEntry, fromPath?: string): Promise { + const destination = await this.#reachingRoots(entry); + const origin = + fromPath && fromPath !== entry.path + ? await this.#reachingRoots(entry, fromPath) + : destination; + + const rootsBySide = ( + groups: Array<{ root: FSEntry; holders: number[] }>, + ) => + new Map( + groups.flatMap(({ root, holders }) => + holders.map((holder) => [holder, root] as const), + ), + ); + const originRootOf = rootsBySide(origin); + const destinationRootOf = rootsBySide(destination); + + // Keyed on both shares: the paths a holder is told depend on the one + // they saw each end through, and those need not be the same share. + const batches = new Map< + string, + { + event: HolderGuiEvent; + from?: FSEntry; + to?: FSEntry; + holders: number[]; + } + >(); + const place = ( + holder: number, + event: HolderGuiEvent, + from?: FSEntry, + to?: FSEntry, + ) => { + const key = `${event}|${from?.id ?? ''}|${to?.id ?? ''}`; + const batch = batches.get(key) ?? { event, from, to, holders: [] }; + batch.holders.push(holder); + batches.set(key, batch); + }; + + for (const [holder, to] of destinationRootOf) { + const from = originRootOf.get(holder); + place( + holder, + from ? 'outer.gui.item.moved' : 'outer.gui.item.added', + from, + to, + ); + } + for (const [holder, from] of originRootOf) { + if (destinationRootOf.has(holder)) continue; + place(holder, 'outer.gui.item.removed', from); + } + + for (const { event, from, to, holders } of batches.values()) { + if (event === 'outer.gui.item.removed') { + // Named by where they last saw it, which is all they have. + await this.#emitGui( + event, + holders, + holderPayload(entry, from as FSEntry, fromPath), + ); + continue; + } + const formerPath = + from && fromPath + ? maskedFormerPath(from, entry, fromPath) + : null; + await this.#emitGui(event, holders, { + ...holderPayload(entry, to as FSEntry), + // The GUI rewrites the item it already has by this. + ...(formerPath ? { from_path: formerPath } : {}), + }); + } } async #fanOutToHolders( @@ -550,8 +670,10 @@ export class ShareService extends PuterService { /** Holders who can reach `entry`, grouped by the share they came through. */ async #reachingRoots( entry: FSEntry, + /** Where to look from; the former path once the entry has moved. */ + realPath: string = entry.path, ): Promise> { - const { rows, nodesById } = await this.#sharesReaching(entry); + const { rows, nodesById } = await this.#sharesReaching(entry, realPath); // Deepest root wins, so a holder with nested shares is told once. const rootByHolder = new Map(); @@ -588,13 +710,16 @@ export class ShareService extends PuterService { * share table to join fsentries and match `path IN (...)` instead put an * un-indexable OR on the write path. */ - async #sharesReaching(entry: FSEntry): Promise<{ + async #sharesReaching( + entry: FSEntry, + realPath: string = entry.path, + ): Promise<{ rows: Array<{ holder_user_id: number; fsentry_id: number }>; nodesById: Map; }> { const ancestorPaths: string[] = []; for ( - let cursor = pathPosix.dirname(entry.path); + let cursor = pathPosix.dirname(realPath); cursor !== '/' && cursor !== '.'; cursor = pathPosix.dirname(cursor) ) {