diff --git a/server/src/nest/doc-sync/doc-sync-config.service.ts b/server/src/nest/doc-sync/doc-sync-config.service.ts index 2c00fca0b..080a45083 100644 --- a/server/src/nest/doc-sync/doc-sync-config.service.ts +++ b/server/src/nest/doc-sync/doc-sync-config.service.ts @@ -411,6 +411,14 @@ export class DocSyncConfigService { updateLink(id: number, patch: Partial): LinkRow | undefined { const sets: string[] = []; const values: unknown[] = []; + // Sync automatically is the way back for an orphaned binding, but only once + // the person whose credential it runs under is on the trip again. Until + // then the switch stays off rather than handing that credential back. + const current = patch.syncEnabled === true ? this.getLink(id) : undefined; + if (current?.last_sync_state === 'orphaned') { + if (this.ownerLeft(current.connection_id)) patch = { ...patch, syncEnabled: undefined }; + else sets.push("last_sync_state = 'never'"); + } if (patch.direction !== undefined) { sets.push('direction = ?'); values.push(patch.direction); } if (patch.deletePolicy !== undefined) { sets.push('delete_policy = ?'); values.push(patch.deletePolicy); } if (patch.conflictPolicy !== undefined) { sets.push('conflict_policy = ?'); values.push(patch.conflictPolicy); } @@ -467,6 +475,29 @@ export class DocSyncConfigService { }; } + /** + * Whether a binding must not run. The sweep below marks it, but only while + * Sync automatically is on, so a paused binding whose owner left is not + * marked; every path that starts a run asks here rather than reading the + * mark alone. + */ + isOrphaned(link: LinkRow): boolean { + return link.last_sync_state === 'orphaned' || this.ownerLeft(link.connection_id); + } + + private ownerLeft(connectionId: number): boolean { + return !!this.db.connection + .prepare( + `SELECT 1 FROM document_connections c + WHERE c.id = ? + AND c.owner_user_id NOT IN ( + SELECT tm.user_id FROM trip_members tm WHERE tm.trip_id = c.trip_id + UNION SELECT t.user_id FROM trips t WHERE t.id = c.trip_id + )`, + ) + .get(connectionId); + } + /** * Bindings whose credential owner is no longer on the trip. * diff --git a/server/src/nest/doc-sync/doc-sync.controller.ts b/server/src/nest/doc-sync/doc-sync.controller.ts index 88e25fcc8..fc50ea7f8 100644 --- a/server/src/nest/doc-sync/doc-sync.controller.ts +++ b/server/src/nest/doc-sync/doc-sync.controller.ts @@ -354,10 +354,9 @@ export class DocSyncController { const link = this.config.getLink(Number(linkId)); if (!link || link.trip_id !== Number(tripId)) throw new HttpException('Link not found', 404); // An orphaned binding stays orphaned: its credential belongs to somebody who - // is no longer on this trip, and a manual run would use it anyway. The - // scheduler already refuses it (sync_enabled is 0), so without this the - // button was a way around that. - if (link.last_sync_state === 'orphaned') { + // is no longer on this trip. The run itself would stand down as well; this + // says why, instead of answering with an empty run. + if (this.config.isOrphaned(link)) { throw new HttpException({ error: 'This binding lost its owner and has to be reconnected' }, 409); } // Refused before the shelved rows are touched, so a binding an admin diff --git a/server/src/nest/doc-sync/doc-sync.mcp.ts b/server/src/nest/doc-sync/doc-sync.mcp.ts index 97ea486ca..7e0f5f0cf 100644 --- a/server/src/nest/doc-sync/doc-sync.mcp.ts +++ b/server/src/nest/doc-sync/doc-sync.mcp.ts @@ -101,7 +101,7 @@ export class DocSyncMcp { // An orphaned binding stays stopped, as it does on the REST route and for // the scheduler: its credential belongs to somebody who has left the trip, // and a run would use it anyway. - if (link.last_sync_state === 'orphaned') { + if (this.config.isOrphaned(link)) { results.push({ linkId: link.id, provider: link.provider_id, state: 'orphaned', errorCode: 'orphaned' }); continue; } diff --git a/server/src/nest/doc-sync/doc-sync.service.ts b/server/src/nest/doc-sync/doc-sync.service.ts index e88f7b7ac..e88f1ba45 100644 --- a/server/src/nest/doc-sync/doc-sync.service.ts +++ b/server/src/nest/doc-sync/doc-sync.service.ts @@ -112,6 +112,7 @@ export class DocSyncService { .prepare( `SELECT * FROM trip_document_links WHERE sync_enabled = 1 + AND last_sync_state != 'orphaned' AND failure_count < ? AND (next_attempt_at IS NULL OR next_attempt_at <= CURRENT_TIMESTAMP) AND provider_id IN (SELECT id FROM document_providers WHERE enabled = 1) @@ -176,6 +177,12 @@ export class DocSyncService { if (this.isSwitchedOff(link)) { return { state: 'disabled', pulled: 0, pushed: 0, conflicts: 0, missing: 0 }; } + // An orphaned binding runs for nobody. Its credential belongs to somebody + // who has left the trip, and a webhook or a resolved conflict reach this + // without asking beforehand. + if (this.config.isOrphaned(link)) { + return { state: 'orphaned', pulled: 0, pushed: 0, conflicts: 0, missing: 0 }; + } const conn = this.config.getConnection(link.connection_id); if (!conn) { this.recordLinkFailure(link, 'not_found'); diff --git a/server/tests/unit/nest/doc-sync/doc-sync-config.service.test.ts b/server/tests/unit/nest/doc-sync/doc-sync-config.service.test.ts index 2ca2b525c..b25f1eadb 100644 --- a/server/tests/unit/nest/doc-sync/doc-sync-config.service.test.ts +++ b/server/tests/unit/nest/doc-sync/doc-sync-config.service.test.ts @@ -645,3 +645,58 @@ describe('markOrphanedLinks', () => { expect(svc.getLink(paused.id)?.last_sync_state).toBe('never'); }); }); + +describe('isOrphaned', () => { + async function bind(ownerUserId: number): Promise { + return link((await connect({}, ownerUserId)).id); + } + + it('answers yes for a paused binding whose owner left, which the sweep never marks', async () => { + const paused = await bind(MEMBER); + svc.updateLink(paused.id, { syncEnabled: false }); + testDb.prepare('DELETE FROM trip_members WHERE trip_id = ? AND user_id = ?').run(TRIP, MEMBER); + + expect(svc.markOrphanedLinks()).toBe(0); + expect(svc.isOrphaned(svc.getLink(paused.id)!)).toBe(true); + }); + + it('answers yes for a marked binding, and no while the owner is on the trip', async () => { + const kept = await bind(OWNER); + expect(svc.isOrphaned(kept)).toBe(false); + + testDb.prepare("UPDATE trip_document_links SET last_sync_state = 'orphaned' WHERE id = ?").run(kept.id); + expect(svc.isOrphaned(svc.getLink(kept.id)!)).toBe(true); + }); + + it('leaves a connection that is gone to the run, which reports it as not found', async () => { + const bound = await bind(OWNER); + expect(svc.isOrphaned({ ...bound, connection_id: 999999 })).toBe(false); + }); +}); + +describe('switching an orphaned binding back on', () => { + async function orphan(): Promise { + const bound = link((await connect({}, MEMBER)).id); + testDb.prepare('DELETE FROM trip_members WHERE trip_id = ? AND user_id = ?').run(TRIP, MEMBER); + svc.markOrphanedLinks(); + return bound; + } + + it('keeps it off while its owner is still gone, and changes the rest of the patch', async () => { + const bound = await orphan(); + + const res = svc.updateLink(bound.id, { syncEnabled: true, direction: 'pull' }); + + expect(res).toMatchObject({ sync_enabled: 0, last_sync_state: 'orphaned', direction: 'pull' }); + }); + + it('lets it run again once its owner is back on the trip', async () => { + const bound = await orphan(); + testDb.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (?, ?)').run(TRIP, MEMBER); + + const res = svc.updateLink(bound.id, { syncEnabled: true }); + + expect(res).toMatchObject({ sync_enabled: 1, last_sync_state: 'never' }); + expect(svc.isOrphaned(res!)).toBe(false); + }); +}); diff --git a/server/tests/unit/nest/doc-sync/doc-sync.controller.test.ts b/server/tests/unit/nest/doc-sync/doc-sync.controller.test.ts index aa766c513..c732a7882 100644 --- a/server/tests/unit/nest/doc-sync/doc-sync.controller.test.ts +++ b/server/tests/unit/nest/doc-sync/doc-sync.controller.test.ts @@ -668,6 +668,23 @@ describe('a manual run', () => { expect(sync.retryShelvedItems).not.toHaveBeenCalled(); expect(sync.syncLink).not.toHaveBeenCalled(); }); + + it('refuses a paused binding whose owner left, although the sweep never marked it', async () => { + const res = await config.upsertConnection(tripId, Number(member.id), connBody({ credentials: { api_token: 'member-token' } })); + if ('error' in res) throw new Error('fixture failed'); + const created = (await controller.createLink(String(tripId), owner, linkBody(res.data.id, { syncEnabled: false }), makeReq())) as { id: number }; + sync.syncLink.mockClear(); + testDb.prepare('DELETE FROM trip_members WHERE trip_id = ? AND user_id = ?').run(tripId, member.id); + try { + const err = await thrown(() => controller.syncNow(String(tripId), String(created.id), { full: false })); + + expect(err.getStatus()).toBe(409); + expect(sync.retryShelvedItems).not.toHaveBeenCalled(); + expect(sync.syncLink).not.toHaveBeenCalled(); + } finally { + testDb.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (?, ?)').run(tripId, member.id); + } + }); }); describe('the document list', () => { diff --git a/server/tests/unit/nest/doc-sync/doc-sync.mcp.test.ts b/server/tests/unit/nest/doc-sync/doc-sync.mcp.test.ts index 5b700fc54..1430d7bee 100644 --- a/server/tests/unit/nest/doc-sync/doc-sync.mcp.test.ts +++ b/server/tests/unit/nest/doc-sync/doc-sync.mcp.test.ts @@ -46,7 +46,12 @@ interface Setup { function makeMcp(over: Partial = {}) { const setup: Setup = { access: true, links: [], status: {}, issues: [], addonOn: true, off: [], ...over }; const files = { verifyTripAccess: vi.fn(() => (setup.access ? { id: 3, user_id: 7 } : undefined)) }; - const config = { listLinks: vi.fn(() => setup.links) }; + const config = { + listLinks: vi.fn(() => setup.links), + // The real helper also asks whether the owner is still on the trip; here + // the mark stands in for both. + isOrphaned: vi.fn((l: LinkRow) => l.last_sync_state === 'orphaned'), + }; const sync = { status: vi.fn(() => setup.status), issues: vi.fn(() => setup.issues), diff --git a/server/tests/unit/nest/doc-sync/doc-sync.service.test.ts b/server/tests/unit/nest/doc-sync/doc-sync.service.test.ts index 3ec1c3182..324f4e3dc 100644 --- a/server/tests/unit/nest/doc-sync/doc-sync.service.test.ts +++ b/server/tests/unit/nest/doc-sync/doc-sync.service.test.ts @@ -394,6 +394,16 @@ describe('DocSyncService', () => { const nearly = makeLink({ failureCount: LINK_CIRCUIT_OPEN_AFTER - 1 }); expect(service.dueLinks().map((l) => l.id)).toEqual([nearly.id]); }); + + it('leaves out an orphaned binding even when Sync automatically was switched back on', () => { + // It would never run, so it would never get a last_sync_at and would take + // one of the slots on every tick. + const orphaned = makeLink({ lastSyncAt: null }); + testDb.prepare("UPDATE trip_document_links SET last_sync_state = 'orphaned' WHERE id = ?").run(orphaned.id); + const live = makeLink({ lastSyncAt: sqlTime('-1 minute') }); + + expect(service.dueLinks().map((l) => l.id)).toEqual([live.id]); + }); }); describe('syncLink', () => { @@ -1054,6 +1064,21 @@ describe('DocSyncService', () => { expect(fileRows()).toHaveLength(1); }); + it('never runs an orphaned binding, whoever asks', async () => { + // Sync automatically switched back on puts sync_enabled to 1 again, and a + // webhook or a resolved conflict calls syncLink without asking first. + const link = makeLink(); + testDb.prepare("UPDATE trip_document_links SET last_sync_state = 'orphaned' WHERE id = ?").run(link.id); + const before = linkRow(link.id); + + const res = await service.syncLink(config.getLink(link.id)); + + expect(res.state).toBe('orphaned'); + expect(provider.resolveScope).not.toHaveBeenCalled(); + expect(provider.list).not.toHaveBeenCalled(); + expect(linkRow(link.id)).toEqual(before); + }); + it('answers the same question for any caller that has to refuse up front', () => { const link = makeLink(); expect(service.isSwitchedOff(link)).toBe(false); diff --git a/wiki/Document-Sync.md b/wiki/Document-Sync.md index defad6c05..b0c64b15e 100644 --- a/wiki/Document-Sync.md +++ b/wiki/Document-Sync.md @@ -309,7 +309,7 @@ See [MCP-Addon-Tools](MCP-Addon-Tools) and [MCP-Scopes](MCP-Scopes). | **Failed** | The store could not be used. The notice line names the reason; TREK retries with the backoff described above. | | **Sign in again** | The store refused the stored credential while TREK was listing documents. More often a refused credential shows as **Failed** with *The credentials were refused.* The usual causes: a revoked Paperless-ngx token or Papra key, a deleted app password or app token, a changed DSM password, or TREK removed from the account's trusted devices in DSM. If the credential still exists in the store, **Sync now** tries again. The connection form only opens for a store the trip has not been connected to yet, so a replaced credential cannot be entered from the trip at the moment. | | **Folder is gone** | The bound tag, folder or space no longer exists. TREK does not recreate it. **Disconnect**, then pick the store again under **Add another** and bind a new, empty one; TREK's documents go up into it. | -| **Owner left the trip** | The person whose account the store was connected with is no longer on the trip. The binding stopped for good and **Sync now** is refused. Documents stay where they are on both sides. Once that person is back on the trip, switching **Sync automatically** on lets it run again. | +| **Owner left the trip** | The person whose account the store was connected with is no longer on the trip. The binding stopped and **Sync now** is refused, also for a paused binding. Documents stay where they are on both sides. Once that person is back on the trip, switching **Sync automatically** on lets it run again; until then the switch stays off. | ### Messages