From 398295689b586d56c5bcdd0663dfc87411c65798 Mon Sep 17 00:00:00 2001 From: Hannes Hertach Date: Wed, 15 Jul 2026 17:42:12 +0200 Subject: [PATCH 1/3] fix(sheet): serialize reads behind in-flight saves to prevent drift misfires Debounced saves could overlap: a new saveSheets would run its checkForDrift GET while the previous saveSheets' POST was still on the wire, so the server correctly returned pre-write state that then failed the body-diff against the locally-updated _lastJsonString. That produced spurious "Content changed" dialogs and, via the non-dirty onStale branch, silent sheet resets. Add a _pendingSave promise to StaleCheck. saveSheets now wraps its POST + markSynced in staleCheck.runSave(...) so at most one save is in flight at a time. checkForDrift and getData (source/config paths) await this before issuing their GET, so no read ever races an in-flight write on the same document. Version reads skip the wait since version keys are immutable. Deadlock-free: POSTs only fire from saveSheets after its own checkForDrift completes, so a POST is always preceded by a read; the read serialisation therefore also bounds POST concurrency to one. Co-Authored-By: Claude Opus 4.7 (1M context) --- blocks/sheet/utils/index.js | 7 +++++ blocks/sheet/utils/utils.js | 60 ++++++++++++++++++++++++++++++++----- 2 files changed, 59 insertions(+), 8 deletions(-) diff --git a/blocks/sheet/utils/index.js b/blocks/sheet/utils/index.js index d39f3da30..04ac61ace 100644 --- a/blocks/sheet/utils/index.js +++ b/blocks/sheet/utils/index.js @@ -94,6 +94,13 @@ export async function getData(input) { const { config, source, versions } = await getNx2Api(); const { org, site, path, view, versionId } = input; + // A specific version snapshot is immutable and lives on a different key, so + // it never needs to wait. For the live source/config file, hold off until any + // in-flight save on this document has settled — otherwise reloadSheet (or + // any other load path) would replay pre-write state that our own POST is + // about to invalidate. + if (!versionId) await staleCheck.awaitPendingSave(); + let resp; let isVersion = false; if (versionId) { diff --git a/blocks/sheet/utils/utils.js b/blocks/sheet/utils/utils.js index f9f2bc89b..8a7253283 100644 --- a/blocks/sheet/utils/utils.js +++ b/blocks/sheet/utils/utils.js @@ -12,6 +12,14 @@ class StaleCheck { this._hasLocalEdits = false; this._saveBlocked = false; this._onStale = null; + // Promise that resolves when the current save (POST + markSynced) is done. + // Reads gate on this so we never issue a GET while our own write is in + // flight — otherwise the server correctly returns a state that predates + // the write, and drift detection misfires against the just-updated + // _lastJsonString. Deadlock is impossible because a POST only fires from + // saveSheets after checkForDrift completes, so at most one save can be + // pending at a time. + this._pendingSave = null; } // `details` is the pathDetails object ({ org, site, path, view }). @@ -30,6 +38,9 @@ class StaleCheck { this._hasLocalEdits = false; this._saveBlocked = false; this._onStale = null; + // Drop the reference so a load for a different file doesn't wait on the + // previous file's in-flight save. The fetch itself is not cancelled. + this._pendingSave = null; } markSynced(json) { @@ -51,10 +62,41 @@ class StaleCheck { return this._saveBlocked; } + // Waits for any in-flight save. Loops so a new save that started while we + // were waiting also gets awaited before returning. + async awaitPendingSave() { + while (this._pendingSave) { + // eslint-disable-next-line no-await-in-loop + await this._pendingSave; + } + } + + // Serialise a save operation. Any concurrent save queues behind the current + // one; any read that calls awaitPendingSave() waits until the save (POST + + // markSynced) has fully settled, so the read never observes a pre-write + // state that its own markSynced has already invalidated locally. + async runSave(saveOp) { + await this.awaitPendingSave(); + let resolve; + const p = new Promise((r) => { resolve = r; }); + this._pendingSave = p; + try { + return await saveOp(); + } finally { + resolve(); + if (this._pendingSave === p) this._pendingSave = null; + } + } + // Returns true if drift was detected (caller should not write). // Skips while blocked so a stale dialog doesn't keep re-firing on subsequent edits/polls. async checkForDrift() { if (this._saveBlocked) return true; + // Never read while our own save is in flight — the server hasn't seen + // the write yet, so the GET would return the previous body and drift + // would misfire against the locally-updated _lastJsonString once the + // POST returns. + await this.awaitPendingSave(); try { const { config, source } = await getNx2Api(); const { org, site, path, view } = this._doc; @@ -87,14 +129,16 @@ export const saveSheets = async (sheets) => { const { hash } = window.location; const pathname = hash.replace('#', ''); - const dasSave = await saveToDa(pathname, sheets); - if (!dasSave.ok) { - // eslint-disable-next-line no-console - console.error('Error saving sheet', dasSave); - return false; - } - staleCheck.markSynced(convertedJson); - return true; + return staleCheck.runSave(async () => { + const dasSave = await saveToDa(pathname, sheets); + if (!dasSave.ok) { + // eslint-disable-next-line no-console + console.error('Error saving sheet', dasSave); + return false; + } + staleCheck.markSynced(convertedJson); + return true; + }); }; const debouncedSaveSheets = debounce(saveSheets, DEBOUNCE_TIME); From ddabb7cf7301074e45681e80c95ff4228729a7df Mon Sep 17 00:00:00 2001 From: Hannes Hertach Date: Wed, 15 Jul 2026 18:40:52 +0200 Subject: [PATCH 2/3] fix(sheet): capture one jexcel snapshot per save so markSynced matches POST body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit saveSheets converted the sheet once at the top (used for markSynced) and saveToDa converted the live jexcel again to build the POST body. Because the two calls straddle awaits on checkForDrift and runSave, the user keeps typing in between, and the POST ends up persisting a newer snapshot than markSynced records. The next drift-check GET reads back the POSTed (newer) bytes, compares against the recorded (older) _lastJsonString, and fires a spurious onStale — dialog on dirty, silent reloadSheet on non-dirty. Thread the pre-converted snapshot through saveToDa to saveJson so the POST body and _lastJsonString come from the same jexcel state. Co-Authored-By: Claude Opus 4.7 (1M context) --- blocks/edit/utils/helpers.js | 4 ++-- blocks/sheet/utils/index.js | 6 +----- blocks/sheet/utils/utils.js | 25 +++++-------------------- 3 files changed, 8 insertions(+), 27 deletions(-) diff --git a/blocks/edit/utils/helpers.js b/blocks/edit/utils/helpers.js index 7fda25a2e..469556059 100644 --- a/blocks/edit/utils/helpers.js +++ b/blocks/edit/utils/helpers.js @@ -163,12 +163,12 @@ async function saveJson(path, sheets, jsonToSave, type = 'sheet') { return source.save(path, { body }); } -export async function saveToDa(pathname, sheet) { +export async function saveToDa(pathname, sheet, jsonToSave) { const suffix = sheet ? '.json' : '.html'; const fullPath = `${pathname}${suffix}`; if (!sheet) return saveHtml(fullPath); - return saveJson(fullPath, sheet); + return saveJson(fullPath, sheet, jsonToSave); } export function saveDaConfig(pathname, sheet) { diff --git a/blocks/sheet/utils/index.js b/blocks/sheet/utils/index.js index 04ac61ace..489caaa02 100644 --- a/blocks/sheet/utils/index.js +++ b/blocks/sheet/utils/index.js @@ -94,11 +94,7 @@ export async function getData(input) { const { config, source, versions } = await getNx2Api(); const { org, site, path, view, versionId } = input; - // A specific version snapshot is immutable and lives on a different key, so - // it never needs to wait. For the live source/config file, hold off until any - // in-flight save on this document has settled — otherwise reloadSheet (or - // any other load path) would replay pre-write state that our own POST is - // about to invalidate. + // Version snapshots are immutable; only live-file reads need to wait for our own writes. if (!versionId) await staleCheck.awaitPendingSave(); let resp; diff --git a/blocks/sheet/utils/utils.js b/blocks/sheet/utils/utils.js index 8a7253283..ab254d639 100644 --- a/blocks/sheet/utils/utils.js +++ b/blocks/sheet/utils/utils.js @@ -12,13 +12,7 @@ class StaleCheck { this._hasLocalEdits = false; this._saveBlocked = false; this._onStale = null; - // Promise that resolves when the current save (POST + markSynced) is done. - // Reads gate on this so we never issue a GET while our own write is in - // flight — otherwise the server correctly returns a state that predates - // the write, and drift detection misfires against the just-updated - // _lastJsonString. Deadlock is impossible because a POST only fires from - // saveSheets after checkForDrift completes, so at most one save can be - // pending at a time. + // Set while a POST is in flight so reads (drift + reload) queue behind it. this._pendingSave = null; } @@ -38,8 +32,7 @@ class StaleCheck { this._hasLocalEdits = false; this._saveBlocked = false; this._onStale = null; - // Drop the reference so a load for a different file doesn't wait on the - // previous file's in-flight save. The fetch itself is not cancelled. + // Detach so a new file's load isn't gated on the previous file's save. this._pendingSave = null; } @@ -62,8 +55,7 @@ class StaleCheck { return this._saveBlocked; } - // Waits for any in-flight save. Loops so a new save that started while we - // were waiting also gets awaited before returning. + // Loops so a save that started mid-wait is also awaited. async awaitPendingSave() { while (this._pendingSave) { // eslint-disable-next-line no-await-in-loop @@ -71,10 +63,6 @@ class StaleCheck { } } - // Serialise a save operation. Any concurrent save queues behind the current - // one; any read that calls awaitPendingSave() waits until the save (POST + - // markSynced) has fully settled, so the read never observes a pre-write - // state that its own markSynced has already invalidated locally. async runSave(saveOp) { await this.awaitPendingSave(); let resolve; @@ -92,10 +80,6 @@ class StaleCheck { // Skips while blocked so a stale dialog doesn't keep re-firing on subsequent edits/polls. async checkForDrift() { if (this._saveBlocked) return true; - // Never read while our own save is in flight — the server hasn't seen - // the write yet, so the GET would return the previous body and drift - // would misfire against the locally-updated _lastJsonString once the - // POST returns. await this.awaitPendingSave(); try { const { config, source } = await getNx2Api(); @@ -130,7 +114,8 @@ export const saveSheets = async (sheets) => { const { hash } = window.location; const pathname = hash.replace('#', ''); return staleCheck.runSave(async () => { - const dasSave = await saveToDa(pathname, sheets); + // Reuse the snapshot so the POST body and markSynced record the same bytes. + const dasSave = await saveToDa(pathname, sheets, convertedJson); if (!dasSave.ok) { // eslint-disable-next-line no-console console.error('Error saving sheet', dasSave); From fdde055e0459152f5e1c0ebc1eb7db058116c611 Mon Sep 17 00:00:00 2001 From: Hannes Hertach Date: Wed, 15 Jul 2026 19:07:47 +0200 Subject: [PATCH 3/3] fix(sheet): use ETag for drift detection instead of body comparison Replace _lastJsonString / full-body JSON comparison in StaleCheck with _lastEtag / ETag comparison. Track the ETag returned by POST responses and the initial GET, and compare against the drift-check GET's ETag. Eliminates the double-convertSheets snapshot race entirely: there is no longer any recorded JSON string that needs to match the POST body, so saveToDa no longer needs the jsonToSave passthrough parameter and the sheet.js 'success' listener no longer needs to re-run convertSheets on the live jexcel to update the baseline. da-title.handleAction reads the ETag directly off the POST/config response and calls markSynced inside runSave. Also more robust: ETag comparison is not fooled by JSON key-ordering, whitespace, or server-side field additions the way body-string comparison was. da-admin returns ETag on both PUT and GET responses and exposes it via Access-Control-Expose-Headers, so the browser can read it cross-origin. Co-Authored-By: Claude Opus 4.7 (1M context) --- blocks/edit/da-title/da-title.js | 44 ++++++++++++-------- blocks/edit/utils/helpers.js | 4 +- blocks/sheet/sheet.js | 8 +--- blocks/sheet/utils/index.js | 2 +- blocks/sheet/utils/utils.js | 21 +++++----- test/unit/blocks/sheet/utils-utils.test.js | 48 +++++++++++++--------- 6 files changed, 69 insertions(+), 58 deletions(-) diff --git a/blocks/edit/da-title/da-title.js b/blocks/edit/da-title/da-title.js index 9fabeb439..79052b3f0 100644 --- a/blocks/edit/da-title/da-title.js +++ b/blocks/edit/da-title/da-title.js @@ -226,31 +226,39 @@ export default class DaTitle extends LitElement { // Bail before writing if the remote drifted under us — protects against // last-write-wins. Drift triggers the stale-content dialog via onStale. + // The POST itself runs inside staleCheck.runSave so it shares the same + // serialisation gate as the debounced saveSheets flow — otherwise a + // Preview/Publish click could race a background autosave on the same file. if (view === 'sheet' || view === 'config') { const { staleCheck } = await import('../../sheet/utils/utils.js'); if (await staleCheck.checkForDrift()) { this._isSending = false; return; } - } - // Only save to DA if it is a sheet or config - if (view === 'sheet') { - const sheetPath = fullpath.replace('.json', ''); - const dasSave = await saveToDa(sheetPath, this.sheet); - if (!dasSave.ok) return; - } - if (view === 'config') { - const daConfigResp = await saveDaConfig(fullpath, this.sheet); - if (!daConfigResp.ok) { - // eslint-disable-next-line no-console - console.log('Saving configuration failed because:', daConfigResp.status, await daConfigResp.text()); - return; - } - } - if (view === 'sheet' || view === 'config') { - // Tell anything listening save was successful - this.handleSuccess('save'); + const savedOk = await staleCheck.runSave(async () => { + let resp; + if (view === 'sheet') { + const sheetPath = fullpath.replace('.json', ''); + resp = await saveToDa(sheetPath, this.sheet); + } else { + resp = await saveDaConfig(fullpath, this.sheet); + } + if (!resp.ok) { + if (view === 'config') { + // eslint-disable-next-line no-console + console.log('Saving configuration failed because:', resp.status, await resp.text()); + } + return false; + } + // markSynced inside runSave so _lastEtag is updated before pendingSave + // resolves — a concurrent read that unblocks on pendingSave otherwise + // sees a stale baseline etag. + staleCheck.markSynced(resp.headers.get('etag')); + this.handleSuccess('save'); + return true; + }); + if (!savedOk) return; } // AEM Actions diff --git a/blocks/edit/utils/helpers.js b/blocks/edit/utils/helpers.js index 469556059..7fda25a2e 100644 --- a/blocks/edit/utils/helpers.js +++ b/blocks/edit/utils/helpers.js @@ -163,12 +163,12 @@ async function saveJson(path, sheets, jsonToSave, type = 'sheet') { return source.save(path, { body }); } -export async function saveToDa(pathname, sheet, jsonToSave) { +export async function saveToDa(pathname, sheet) { const suffix = sheet ? '.json' : '.html'; const fullPath = `${pathname}${suffix}`; if (!sheet) return saveHtml(fullPath); - return saveJson(fullPath, sheet, jsonToSave); + return saveJson(fullPath, sheet); } export function saveDaConfig(pathname, sheet) { diff --git a/blocks/sheet/sheet.js b/blocks/sheet/sheet.js index 49bae136e..08ee89196 100644 --- a/blocks/sheet/sheet.js +++ b/blocks/sheet/sheet.js @@ -4,7 +4,6 @@ import { getNx } from '../../scripts/utils.js'; import '../edit/da-title/da-title.js'; import { getData } from './utils/index.js'; import { staleCheck, showDaDialog, restoreVersion } from './utils/utils.js'; -import { convertSheets } from '../edit/utils/helpers.js'; const { default: getStyle } = await import(`${getNx()}/utils/styles.js`); @@ -122,7 +121,7 @@ async function reloadSheet(daTitle, daSheet) { async function setSheet(details, daTitle, daSheet) { // Drop any open stale-content dialog so its Cancel can't act on the new path's staleCheck. document.body.querySelectorAll(':scope > da-dialog').forEach((d) => d.remove()); - // Full reset before the load — getData calls markSynced which sets _lastJsonString. + // Full reset before the load — getData calls markSynced which sets _lastEtag. // start() below only wires up the interval without resetting state. staleCheck.stop(); @@ -186,11 +185,6 @@ export default async function init(el) { el.append(daTitle, versionWrapper); - daTitle.addEventListener('success', (e) => { - if (e.detail.action !== 'save') return; - staleCheck.markSynced(convertSheets(daSheet.jexcel)); - }); - window.addEventListener('hashchange', async () => { details = getPathDetails(); setSheet(details, daTitle, daSheet); diff --git a/blocks/sheet/utils/index.js b/blocks/sheet/utils/index.js index 489caaa02..3ccaace1a 100644 --- a/blocks/sheet/utils/index.js +++ b/blocks/sheet/utils/index.js @@ -123,7 +123,7 @@ export async function getData(input) { const json = await resp.json(); if (!isVersion) { - staleCheck.markSynced(json); + staleCheck.markSynced(resp.headers.get('etag')); const sheetPanes = document.querySelector('da-sheet-panes'); if (sheetPanes) sheetPanes.data = json; } diff --git a/blocks/sheet/utils/utils.js b/blocks/sheet/utils/utils.js index ab254d639..3d42e25c2 100644 --- a/blocks/sheet/utils/utils.js +++ b/blocks/sheet/utils/utils.js @@ -8,7 +8,7 @@ class StaleCheck { constructor() { this._intervalId = null; this._doc = null; - this._lastJsonString = null; + this._lastEtag = null; this._hasLocalEdits = false; this._saveBlocked = false; this._onStale = null; @@ -28,7 +28,7 @@ class StaleCheck { if (this._intervalId) clearInterval(this._intervalId); this._intervalId = null; this._doc = null; - this._lastJsonString = null; + this._lastEtag = null; this._hasLocalEdits = false; this._saveBlocked = false; this._onStale = null; @@ -36,8 +36,8 @@ class StaleCheck { this._pendingSave = null; } - markSynced(json) { - this._lastJsonString = JSON.stringify(json); + markSynced(etag) { + this._lastEtag = etag; this._hasLocalEdits = false; // Clear the post-Cancel block: a fresh sync (load or save) is the recovery path. this._saveBlocked = false; @@ -89,9 +89,10 @@ class StaleCheck { ? await config.get({ org, site }) : await source.get({ org, site, path }); if (!resp.ok) return false; + // No baseline (never synced) or missing header: nothing to compare against. + const etag = resp.headers.get('etag'); + if (!etag || !this._lastEtag || etag === this._lastEtag) return false; const json = await resp.json(); - const text = JSON.stringify(json); - if (text === this._lastJsonString) return false; this._onStale({ json, dirty: this._hasLocalEdits }); return true; } catch { @@ -104,8 +105,7 @@ class StaleCheck { export const staleCheck = new StaleCheck(); export const saveSheets = async (sheets) => { - const convertedJson = convertSheets(sheets); - document.querySelector('da-sheet-panes').data = convertedJson; + document.querySelector('da-sheet-panes').data = convertSheets(sheets); // Bail before writing if the remote moved out from under us — protects against // last-write-wins between concurrent editors. Drift triggers the onStale flow. @@ -114,14 +114,13 @@ export const saveSheets = async (sheets) => { const { hash } = window.location; const pathname = hash.replace('#', ''); return staleCheck.runSave(async () => { - // Reuse the snapshot so the POST body and markSynced record the same bytes. - const dasSave = await saveToDa(pathname, sheets, convertedJson); + const dasSave = await saveToDa(pathname, sheets); if (!dasSave.ok) { // eslint-disable-next-line no-console console.error('Error saving sheet', dasSave); return false; } - staleCheck.markSynced(convertedJson); + staleCheck.markSynced(dasSave.headers.get('etag')); return true; }); }; diff --git a/test/unit/blocks/sheet/utils-utils.test.js b/test/unit/blocks/sheet/utils-utils.test.js index 0f39b59df..b7d58f699 100644 --- a/test/unit/blocks/sheet/utils-utils.test.js +++ b/test/unit/blocks/sheet/utils-utils.test.js @@ -67,44 +67,54 @@ describe('sheet/utils utils', () => { }); }); - describe('staleCheck after restore', () => { + describe('etag drift detection', () => { const DETAILS = { org: 'org', site: 'repo', path: '/sheet', view: 'sheet' }; const serverJson = { ':type': 'sheet', ':sheetname': 'data', data: [{ key: 'a' }] }; - beforeEach(() => { - staleCheck.start({ details: DETAILS, onStale: () => {} }); - staleCheck.markSynced(serverJson); - }); - afterEach(() => { staleCheck.stop(); }); - it('markSynced with jspreadsheet-format array causes saveSheets to falsely bail', async () => { - // Simulates the old (buggy) restore handler calling markSynced with wrong-format data - const jspsheetData = [{ sheetName: 'data', data: [['key'], ['a']], columns: [] }]; - staleCheck.markSynced(jspsheetData); + it('bails saveSheets when server etag differs from the recorded baseline', async () => { + let onStaleFired = false; + staleCheck.start({ details: DETAILS, onStale: () => { onStaleFired = true; } }); + staleCheck.markSynced('"baseline"'); window.location.hash = '#/org/repo/sheet'; - window.fetch = async () => new Response(JSON.stringify(serverJson), { status: 200 }); + window.fetch = wrap(async (url, opts) => { + if (opts?.method === 'POST' || opts?.method === 'PUT') { + return new Response('', { status: 200, headers: { ETag: '"remote"' } }); + } + return new Response(JSON.stringify(serverJson), { + status: 200, + headers: { ETag: '"remote"', 'Content-Type': 'application/json' }, + }); + }); const sheets = [buildSheet('data', [['key'], ['a']])]; const result = await saveSheets(sheets); - expect(result).to.be.false; // drift falsely detected — save bailed + expect(result).to.be.false; + expect(onStaleFired).to.be.true; }); - it('Preserves correct baseline so saveSheets proceeds when server is unchanged', async () => { - // Restore handler does NOT call markSynced with wrong-format data — baseline stays intact. + it('proceeds when server etag matches the recorded baseline', async () => { + staleCheck.start({ details: DETAILS, onStale: () => {} }); + staleCheck.markSynced('"baseline"'); window.location.hash = '#/org/repo/sheet'; - window.fetch = async (url, opts) => { - if (opts?.method === 'PUT') return new Response('', { status: 200 }); - return new Response(JSON.stringify(serverJson), { status: 200 }); - }; + window.fetch = wrap(async (url, opts) => { + if (opts?.method === 'POST' || opts?.method === 'PUT') { + return new Response('', { status: 200, headers: { ETag: '"next"' } }); + } + return new Response(JSON.stringify(serverJson), { + status: 200, + headers: { ETag: '"baseline"', 'Content-Type': 'application/json' }, + }); + }); const sheets = [buildSheet('data', [['key'], ['a']])]; const result = await saveSheets(sheets); - expect(result).to.be.true; // no false drift — save went through + expect(result).to.be.true; }); });