diff --git a/src/edge.js b/src/edge.js index 70df856..9e2e194 100644 --- a/src/edge.js +++ b/src/edge.js @@ -138,17 +138,17 @@ export async function handleApiRequest(request, env) { const timingBeforeDaAdminHead = Date.now(); const initialReq = await env.daadmin.fetch(docName, opts); - // this seems to be required by CloudFlare to consider the request as completed - await initialReq.text(); - timingDaAdminHeadDuration = Date.now() - timingBeforeDaAdminHead; - if (!initialReq.ok && initialReq.status !== 404) { + if (!initialReq.ok) { // eslint-disable-next-line no-console console.log(`[worker] Unable to get resource ${docName}: ${initialReq.status} - ${initialReq.statusText}`); return new Response('unable to get resource', { status: initialReq.status }); } + // this seems to be required by CloudFlare to consider the request as completed + await initialReq.text(); + const daActions = initialReq.headers.get('X-da-actions') ?? ''; [, authActions] = daActions.split('='); } catch (err) { diff --git a/src/shareddoc.js b/src/shareddoc.js index ccc0de9..7f73714 100644 --- a/src/shareddoc.js +++ b/src/shareddoc.js @@ -16,7 +16,7 @@ import * as awarenessProtocol from 'y-protocols/awareness.js'; import * as encoding from 'lib0/encoding.js'; import * as decoding from 'lib0/decoding.js'; import debounce from 'lodash/debounce.js'; -import { aem2doc, doc2aem, EMPTY_DOC } from './collab.js'; +import { aem2doc, doc2aem } from './collab.js'; const wsReadyStateConnecting = 0; const wsReadyStateOpen = 1; @@ -191,12 +191,12 @@ export const persistence = { closeConn: closeConn.bind(this), /** - * Get the document from da-admin. If da-admin doesn't have the doc, a new empty doc is - * returned. + * Get the document from da-admin. * @param {string} docName - The document name * @param {string} auth - The authorization header * @param {object} daadmin - The da-admin worker service binding * @returns {Promise} - The content of the document + * @throws {Error} - If the document cannot be retrieved (including 404) */ get: async (docName, auth, daadmin) => { const initalOpts = {}; @@ -206,8 +206,6 @@ export const persistence = { const initialReq = await daadmin.fetch(docName, initalOpts); if (initialReq.ok) { return initialReq.text(); - } else if (initialReq.status === 404) { - return null; } else { // eslint-disable-next-line no-console console.error(`[docroom] Unable to get resource from da-admin: ${initialReq.status} - ${initialReq.statusText}`); @@ -236,17 +234,22 @@ export const persistence = { console.log('[docroom] All connections are read only, not storing'); return { ok: true }; } + + const headers = { + 'If-Match': '*', + 'X-DA-Initiator': 'collab', + }; + const auth = keys .filter((con) => con.readOnly !== true) .map((con) => con.auth); if (auth.length > 0) { - opts.headers = new Headers({ - Authorization: [...new Set(auth)].join(','), - 'X-DA-Initiator': 'collab', - }); + headers.Authorization = [...new Set(auth)].join(','); } + opts.headers = new Headers(headers); + if (blob.size < 84) { // eslint-disable-next-line no-console console.warn('[docroom] Writting back an empty document', ydoc.name, blob.size); @@ -284,7 +287,20 @@ export const persistence = { const { ok, status, statusText } = await persistence.put(ydoc, content); if (!ok) { - closeAll = (status === 401 || status === 403); + if (status === 412) { + // Document doesn't exist - clean up cached state + if (ydoc.storage) { + try { + await ydoc.storage.deleteAll(); + // eslint-disable-next-line no-console + console.log('[docroom] Cleaned worker storage after 412 (document deleted)'); + } catch (storageErr) { + // eslint-disable-next-line no-console + console.error('[docroom] Failed to clean storage', storageErr); + } + } + } + closeAll = (status === 401 || status === 403 || status === 412); throw new Error(`${status} - ${statusText}`); } @@ -312,31 +328,26 @@ export const persistence = { */ bindState: async (docName, ydoc, conn, storage) => { let timingReadStateDuration; - let timingDaAdminGetDuration; + + // Store storage reference for later use in persistence.update + // eslint-disable-next-line no-param-reassign + ydoc.storage = storage; let current; let restored = false; // True if restored from worker storage - try { - let newDoc = false; - const timingBeforeDaAdminGet = Date.now(); - current = await persistence.get(docName, conn.auth, ydoc.daadmin); - timingDaAdminGetDuration = Date.now() - timingBeforeDaAdminGet; + // Get document from da-admin (throws on error including 404) + const timingBeforeDaAdminGet = Date.now(); + current = await persistence.get(docName, conn.auth, ydoc.daadmin); + const timingDaAdminGetDuration = Date.now() - timingBeforeDaAdminGet; + + // Read the stored state from internal worker storage (errors are non-fatal) + try { const timingBeforeReadState = Date.now(); - // Read the stored state from internal worker storage const stored = await readState(docName, storage); timingReadStateDuration = Date.now() - timingBeforeReadState; - if (current === null) { - if (!stored) { - // This is a new document, it wasn't present in local storage - newDoc = true; - } - // if stored has a value, the document previously existed but was deleted - - current = EMPTY_DOC; - await storage.deleteAll(); - } else if (stored && stored.length > 0) { + if (stored && stored.length > 0) { Y.applyUpdate(ydoc, stored); // Check if the state from the worker storage is the same as the current state in da-admin. @@ -350,12 +361,6 @@ export const persistence = { console.log('[docroom] Restored from worker persistence', docName); } } - - if (newDoc === true) { - // There is no stored state and the document is empty, which means - // we have a new doc here, which doesn't need to be restored from da-admin - restored = true; - } } catch (error) { // eslint-disable-next-line no-console console.error('[docroom] Problem restoring state from worker storage', error); diff --git a/test/edge.test.js b/test/edge.test.js index d397c34..9bb90f5 100644 --- a/test/edge.test.js +++ b/test/edge.test.js @@ -294,6 +294,47 @@ describe('Worker test suite', () => { assert.equal(400, resp.status, 'Expected a document name'); }); + it('Test DocRoom fetch fails when document deleted after auth', async () => { + const savedNWSP = DocRoom.newWebSocketPair; + const savedBS = persistence.bindState; + + try { + // Mock bindState to throw 404 error (simulating document deleted between auth and bindState) + persistence.bindState = async () => { + throw new Error('unable to get resource - status: 404'); + }; + + const wsp0 = {}; + const wsp1 = { + accept() {}, + addEventListener() {}, + close() {} + }; + DocRoom.newWebSocketPair = () => [wsp0, wsp1]; + + const daadmin = { fetch: async () => ({ ok: true }) }; + const dr = new DocRoom({ storage: null }, { daadmin }); + const headers = new Map(); + headers.set('Upgrade', 'websocket'); + headers.set('X-collab-room', 'http://foo.bar/test.html'); + headers.set('X-auth-actions', 'read=allow,write=allow'); + + const req = { + headers, + url: 'http://localhost:4711/' + }; + + const resp = await dr.fetch(req, {}, 306); + + // Should return 500 error when bindState fails + assert.equal(500, resp.status); + assert.equal('internal server error', await resp.text()); + } finally { + DocRoom.newWebSocketPair = savedNWSP; + persistence.bindState = savedBS; + } + }); + it('Test DocRoom fetch WebSocket setup exception', async () => { const savedNWSP = DocRoom.newWebSocketPair; const savedBS = persistence.bindState; @@ -479,6 +520,20 @@ describe('Worker test suite', () => { assert.equal(404, res.status); }); + it('Test handleApiRequest document not found (404)', async () => { + const req = { + url: 'http://do.re.mi/https://admin.da.live/nonexistent.html', + } + + const mockFetch = async (url, opts) => new Response(null, {status: 404}); + const daadmin = { fetch: mockFetch }; + const env = { daadmin }; + + const res = await handleApiRequest(req, env); + assert.equal(404, res.status); + assert.equal('unable to get resource', await res.text()); + }); + it('Test handleApiRequest not authorized', async () => { const req = { url: 'http://do.re.mi/https://admin.da.live/hihi.html', diff --git a/test/shareddoc.test.js b/test/shareddoc.test.js index b9b1dfe..b873b81 100644 --- a/test/shareddoc.test.js +++ b/test/shareddoc.test.js @@ -170,8 +170,12 @@ describe('Collab Test Suite', () => { assert.equal(opts.headers.get('authorization'), 'auth'); return { ok: false, text: async () => { throw new Error(); }, status: 404, statusText: 'Not Found' }; }; - const result = await persistence.get('foo', 'auth', daadmin); - assert.equal(result, null); + try { + await persistence.get('foo', 'auth', daadmin); + assert.fail('Should have thrown an error'); + } catch (error) { + assert(error.toString().includes('unable to get resource - status: 404')); + } }); it('Test persistence get throws', async () => { @@ -196,7 +200,7 @@ describe('Collab Test Suite', () => { daadmin.fetch = async (url, opts) => { assert.equal(url, 'foo'); assert.equal(opts.method, 'PUT'); - assert(opts.headers === undefined); + assert.equal(opts.headers.get('If-Match'), '*', 'Should include If-Match: * header'); assert.equal(await opts.body.get('data').text(), 'test'); return { ok: true, status: 200, statusText: 'OK - Stored'}; }; @@ -215,6 +219,7 @@ describe('Collab Test Suite', () => { assert.equal(opts.method, 'PUT'); assert.equal('myauth', opts.headers.get('Authorization')); assert.equal('collab', opts.headers.get('X-DA-Initiator')); + assert.equal('*', opts.headers.get('If-Match')); assert.equal(await opts.body.get('data').text(), 'test'); return { ok: true, status: 200, statusText: 'OK - Stored too'}; }; @@ -231,7 +236,7 @@ describe('Collab Test Suite', () => { daadmin.fetch = async (url, opts) => { assert.equal(url, 'foo'); assert.equal(opts.method, 'PUT'); - assert(opts.headers === undefined); + assert.equal(opts.headers.get('If-Match'), '*'); assert.equal(await opts.body.get('data').text(), 'test'); return { ok: true, status: 200, statusText: 'OK'}; }; @@ -246,6 +251,7 @@ describe('Collab Test Suite', () => { assert.equal(opts.method, 'PUT'); assert.equal(opts.headers.get('authorization'), 'auth'); assert.equal(opts.headers.get('X-DA-Initiator'), 'collab'); + assert.equal(opts.headers.get('If-Match'), '*'); assert.equal(await opts.body.get('data').text(), 'test'); return { ok: true, status: 200, statusText: 'okidoki'}; }; @@ -375,6 +381,225 @@ describe('Collab Test Suite', () => { await testCloseAllOnAuthFailure(403); }); + it('Test persistence update closes all and cleans storage on 412', async () => { + const docName = 'https://admin.da.live/source/foo.html'; + const ydoc = new WSSharedDoc(docName); + + const storageDeleteAllCalled = []; + ydoc.storage = { + deleteAll: async () => storageDeleteAllCalled.push('deleteAll'), + }; + + const closeCalled = []; + const conn1 = { close: () => closeCalled.push('close1'), readOnly: false }; + const conn2 = { close: () => closeCalled.push('close2'), readOnly: false }; + ydoc.conns.set(conn1, new Set(['client1'])); + ydoc.conns.set(conn2, new Set(['client2'])); + + // Register the doc in the global map + const docs = setYDoc(docName, ydoc); + assert(docs.has(docName), 'Precondition: doc should be in global map'); + + ydoc.daadmin = { + fetch: async () => ({ ok: false, status: 412, statusText: 'Precondition Failed' }), + }; + + aem2doc('

test content

', ydoc); + + const result = await persistence.update(ydoc, '

old content

'); + + // Should have cleaned storage + assert.equal(storageDeleteAllCalled.length, 1, 'Should have called storage.deleteAll'); + + // Should have closed all connections + assert.equal(closeCalled.length, 2, 'Should have closed both connections'); + + // Connections should be removed from ydoc.conns + assert.equal(ydoc.conns.size, 0, 'All connections should be removed from ydoc.conns'); + + // Doc should be removed from global docs map when last connection closes + assert(!docs.has(docName), 'Doc should be removed from global docs map'); + + // Should return the original content (update failed) + assert.equal(result, '

old content

'); + }); + + it('Test 412 cleanup allows fresh connection attempt', async () => { + const docName = 'https://admin.da.live/source/bar.html'; + + // First connection and 412 scenario + const ydoc = new WSSharedDoc(docName); + ydoc.storage = { + deleteAll: async () => {}, + }; + + const conn1 = { close: () => {}, readOnly: false }; + ydoc.conns.set(conn1, new Set(['client1'])); + + const docs = setYDoc(docName, ydoc); + assert(docs.has(docName), 'Precondition'); + + ydoc.daadmin = { + fetch: async () => ({ ok: false, status: 412, statusText: 'Precondition Failed' }), + }; + + aem2doc('

content

', ydoc); + + // Trigger 412 - should close all connections and remove from global map + await persistence.update(ydoc, '

old

'); + + assert.equal(ydoc.conns.size, 0, 'All connections should be closed'); + assert(!docs.has(docName), 'Doc should be removed from global map after last connection closes'); + + // Now simulate a fresh connection attempt + // This should create a NEW ydoc since the old one was removed from the global map + const conn2 = { close: () => {}, readOnly: false }; + const newYdoc = docs.get(docName); + assert.equal(newYdoc, undefined, 'Old ydoc should not be in map'); + }); + + it('Test ydoc error map is set on 412', async () => { + const docName = 'https://admin.da.live/source/baz.html'; + const ydoc = new WSSharedDoc(docName); + ydoc.storage = { deleteAll: async () => {} }; + + const conn1 = { close: () => {}, readOnly: false }; + ydoc.conns.set(conn1, new Set()); + setYDoc(docName, ydoc); + + ydoc.daadmin = { + fetch: async () => ({ ok: false, status: 412, statusText: 'Precondition Failed' }), + }; + + aem2doc('

content

', ydoc); + + // Before 412, error map should be empty + const errorMap = ydoc.getMap('error'); + assert.equal(errorMap.size, 0, 'Precondition: error map should be empty'); + + await persistence.update(ydoc, '

old

'); + + // After 412, error map should contain error details + assert(errorMap.size > 0, 'Error map should have entries'); + assert(errorMap.has('timestamp'), 'Should have timestamp'); + assert(errorMap.has('message'), 'Should have message'); + assert(errorMap.has('stack'), 'Should have stack'); + assert(errorMap.get('message').includes('412'), 'Error message should mention 412'); + }); + + it('Test update handlers stop after 412 cleanup', async () => { + const mockdebounce = (f) => { + let debounced = async () => await f(); + debounced.cancel = () => {}; + return debounced; + }; + const pss = await esmock( + '../src/shareddoc.js', { + 'lodash/debounce.js': { + default: mockdebounce + } + }); + + const docName = 'https://admin.da.live/source/qux.html'; + const ydoc = new pss.WSSharedDoc(docName); + ydoc.storage = { deleteAll: async () => {} }; + + const conn1 = { close: () => {}, readOnly: false }; + ydoc.conns.set(conn1, new Set()); + + const docs = pss.setYDoc(docName, ydoc); + assert(docs.has(docName), 'Precondition'); + + // Mock da-admin to return 412 + ydoc.daadmin = { + fetch: async () => ({ ok: false, status: 412, statusText: 'Precondition Failed' }), + }; + + const updateHandlers = []; + const originalOn = ydoc.on.bind(ydoc); + ydoc.on = (event, handler) => { + if (event === 'update') { + updateHandlers.push(handler); + } + return originalOn(event, handler); + }; + + // Set up bindState which registers update handlers + const storage = { + list: async () => new Map(), + deleteAll: async () => {}, + put: async () => {} + }; + pss.persistence.get = async () => '

initial

'; + + await pss.persistence.bindState(docName, ydoc, conn1, storage); + + assert.equal(updateHandlers.length, 2, 'Should have two update handlers registered'); + + // Modify document + aem2doc('

modified

', ydoc); + + // Trigger 412 which closes all connections and removes from global map + await pss.persistence.update(ydoc, '

initial

'); + + assert(!docs.has(docName), 'Doc should be removed from global map'); + + // Now try to call the update handlers - they should not execute because ydoc is no longer in global map + const putCalls = []; + pss.persistence.put = async () => { + putCalls.push('put'); + return { ok: true }; + }; + + // Simulate another update after 412 + aem2doc('

another change

', ydoc); + + // Call the debounced update handler + if (updateHandlers[1]) { + await updateHandlers[1](); + } + + // Put should NOT have been called because ydoc is not in global map anymore + assert.equal(putCalls.length, 0, 'PUT should not be called after doc removed from global map'); + }); + + it('Test 412 closes all clients including readonly', async () => { + const docName = 'https://admin.da.live/source/multi.html'; + const ydoc = new WSSharedDoc(docName); + ydoc.storage = { deleteAll: async () => {} }; + + const closeCalled = []; + const conn1 = { close: () => closeCalled.push('conn1'), readOnly: false, auth: 'auth1' }; + const conn2 = { close: () => closeCalled.push('conn2'), readOnly: false, auth: 'auth2' }; + const conn3 = { close: () => closeCalled.push('conn3'), readOnly: true, auth: 'auth3' }; + + ydoc.conns.set(conn1, new Set(['client1'])); + ydoc.conns.set(conn2, new Set(['client2'])); + ydoc.conns.set(conn3, new Set(['client3'])); + + const docs = setYDoc(docName, ydoc); + + ydoc.daadmin = { + fetch: async () => ({ ok: false, status: 412, statusText: 'Precondition Failed' }), + }; + + aem2doc('

content

', ydoc); + + await persistence.update(ydoc, '

old

'); + + // All connections should be closed (including readonly) + assert.equal(closeCalled.length, 3, 'Should have closed all 3 connections'); + assert(closeCalled.includes('conn1'), 'Should close conn1'); + assert(closeCalled.includes('conn2'), 'Should close conn2'); + assert(closeCalled.includes('conn3'), 'Should close readonly conn3'); + + // All connections removed from ydoc + assert.equal(ydoc.conns.size, 0, 'All connections should be removed'); + + // Doc removed from global map + assert(!docs.has(docName), 'Doc should be removed from global map'); + }); + it('Test invalidateFromAdmin', async () => { const docName = 'http://blah.di.blah/a/ha.html'; @@ -488,55 +713,6 @@ describe('Collab Test Suite', () => { assert.equal(testYDoc, aem2DocCalled[1]); }); - it('Test bindState gets empty doc on da-admin 404', async() => { - const mockdebounce = (f) => async () => await f(); - const pss = await esmock( - '../src/shareddoc.js', { - 'lodash/debounce.js': { - default: mockdebounce - } - }); - - const docName = 'http://foobar.com/mydoc.html'; - - const ydocUpdateCB = []; - const testYDoc = new Y.Doc(); - testYDoc.on = (ev, f) => { if (ev === 'update') ydocUpdateCB.push(f); } - pss.setYDoc(docName, testYDoc); - - const called = []; - const mockStorage = { - deleteAll: async () => called.push('deleteAll'), - list: async () => new Map(), - }; - - // When da-admin returns 404, get returns null - pss.persistence.get = async () => null; - const updateCalled = []; - pss.persistence.update = async (_, cur) => updateCalled.push(cur); - - const savedSetTimeout = globalThis.setTimeout; - const setTimeoutCalls = []; - try { - globalThis.setTimeout = () => setTimeoutCalls.push('setTimeout'); - - await pss.persistence.bindState(docName, testYDoc, {}, mockStorage); - } finally { - globalThis.setTimeout = savedSetTimeout; - } - - assert.deepStrictEqual(['deleteAll'], called); - assert.equal(0, setTimeoutCalls.length, - 'Should not have called setTimeout as there is no document to restore from da-admin'); - - assert.equal(0, updateCalled.length, 'Precondition'); - assert.equal(2, ydocUpdateCB.length); - - await ydocUpdateCB[0](); - await ydocUpdateCB[1](); - assert.deepStrictEqual([EMPTY_DOC], updateCalled); - }); - it('Test bindstate read from worker storage', async () => { const docName = 'https://admin.da.live/source/foo/bar.html'; @@ -661,87 +837,6 @@ describe('Collab Test Suite', () => { } }); - it('test bind to new doc doesnt set empty server content', async () => { - const docName = 'https://admin.da.live/source/foo.html'; - - const serviceBinding = { - fetch: async (u) => { - if (u === docName) { - return { status: 404 }; - } - } - }; - - const ydoc = new Y.Doc(); - ydoc.daadmin = serviceBinding; - setYDoc(docName, ydoc); - const conn = {}; - const storage = { - deleteAll: async () => {}, - list: async () => new Map(), - }; - - const setTimeoutCalled = []; - const savedSetTimeout = globalThis.setTimeout; - try { - globalThis.setTimeout = (f) => { - // Restore the global function - globalThis.setTimeout = savedSetTimeout; - setTimeoutCalled.push('setTimeout'); - f(); - }; - - await persistence.bindState(docName, ydoc, conn, storage); - assert.equal(0, setTimeoutCalled.length, 'SetTimeout should not have been called'); - } finally { - globalThis.setTimeout = savedSetTimeout; - } - }); - - it('test bind to empty doc that was stored before updates ydoc', async () => { - const docName = 'https://admin.da.live/source/foo.html'; - - const serviceBinding = { - fetch: async (u) => { - if (u === docName) { - return { status: 404 }; - } - } - }; - - const ydoc = new Y.Doc(); - ydoc.daadmin = serviceBinding; - setYDoc(docName, ydoc); - const conn = {}; - - const deleteAllCalled = []; - const stored = new Map(); - stored.set('docstore', new Uint8Array([254, 255])); - stored.set('chunks', 17); // should be ignored - stored.set('doc', docName); - const storage = { - deleteAll: async () => deleteAllCalled.push(true), - list: async () => stored, - }; - - const setTimeoutCalled = []; - const savedSetTimeout = globalThis.setTimeout; - try { - globalThis.setTimeout = (f) => { - // Restore the global function - globalThis.setTimeout = savedSetTimeout; - setTimeoutCalled.push('setTimeout'); - f(); - }; - - await persistence.bindState(docName, ydoc, conn, storage); - assert.deepStrictEqual([true], deleteAllCalled); - assert.equal(1, setTimeoutCalled.length, 'SetTimeout should have been called to update the doc'); - } finally { - globalThis.setTimeout = savedSetTimeout; - } - }); - it('test persist state in worker storage on update', async () => { const docName = 'https://admin.da.live/source/foo/bar.html';