From 6516b836fd75a5822c4e505d1c4fb9d5e2a2d591 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Mon, 7 Sep 2026 10:13:03 +0200 Subject: [PATCH] fix(drafts): report a mutation whose draft cleanup failed, and record a released cell only during a save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The completion listener that refreshes and closes hosted editors runs only for a tool that returned, so a draft cleanup throwing after the entity was deployed or deleted left the tab open on state that no longer exists. The cleanup cannot undo the mutation either, so both tools now report it in their result instead of throwing it as if nothing had happened. Releasing a cell recorded its value for every resource and variable, though only a settle waiting on a write in flight ever reads one — routine editor use retained large resource values and decrypted secrets for the tab's lifetime. The editors now open a window around their save, and only a release inside one is remembered; closing it drops what it held. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KcyBRxE8j3ujCYkum9KrC4 --- .../src/lib/components/ResourceEditor.svelte | 8 +++ .../src/lib/components/VariableEditor.svelte | 8 +++ .../copilot/chat/global/core.test.ts | 19 ++++++ .../components/copilot/chat/global/core.ts | 41 +++++++++-- frontend/src/lib/userDraft.svelte.ts | 68 +++++++++++-------- .../src/lib/userDraftSettleAfterWrite.test.ts | 21 ++++++ 6 files changed, 129 insertions(+), 36 deletions(-) diff --git a/frontend/src/lib/components/ResourceEditor.svelte b/frontend/src/lib/components/ResourceEditor.svelte index 56540a029f..542ac1c9c6 100644 --- a/frontend/src/lib/components/ResourceEditor.svelte +++ b/frontend/src/lib/components/ResourceEditor.svelte @@ -16,6 +16,7 @@ UserDraft, draftValuesEqual, flushDraftDelete, + beginDraftSettleWindow, settleDraftAfterWrite, type UserDraftHandle } from '$lib/userDraft.svelte' @@ -366,6 +367,11 @@ // what an earlier one wrote is deployed — a caller told nothing about it would // stay pointed at a path the item has moved off. const written: { ws: string; path: string }[] = [] + // A host left mid-write releases the cell each settle below reads; only inside + // this window is what it was holding remembered. + const closeSettleWindows = payloads.map(({ ws }) => + beginDraftSettleWindow('resource', from, { workspace: ws }) + ) try { for (const { ws, s, ini, existed } of payloads) { if (existed) { @@ -424,6 +430,8 @@ // paths, listed and editable from there — moving one onto a rename it never // wrote would be worse than not showing it here. return { written, ok: false } + } finally { + for (const close of closeSettleWindows) close() } } diff --git a/frontend/src/lib/components/VariableEditor.svelte b/frontend/src/lib/components/VariableEditor.svelte index 20e11feeca..abf8d69fc0 100644 --- a/frontend/src/lib/components/VariableEditor.svelte +++ b/frontend/src/lib/components/VariableEditor.svelte @@ -26,6 +26,7 @@ UserDraft, draftValuesEqual, flushDraftDelete, + beginDraftSettleWindow, settleDraftAfterWrite, type UserDraftHandle } from '$lib/userDraft.svelte' @@ -311,6 +312,11 @@ // is done: per workspace, each carrying its own, so a linked workspace's rename // moves the tabs acting on it and no others. const written: { ws: string; path: string }[] = [] + // A host left mid-write releases the cell each settle below reads; only inside + // this window is what it was holding remembered. + const closeSettleWindows = payloads.map(({ ws }) => + beginDraftSettleWindow('variable', from ?? '', { workspace: ws }) + ) try { for (const { ws, s, ini, existed } of payloads) { if (existed) { @@ -396,6 +402,8 @@ editVariable(actingPath) if (shownWs) selected = shownWs } + } finally { + for (const close of closeSettleWindows) close() } } diff --git a/frontend/src/lib/components/copilot/chat/global/core.test.ts b/frontend/src/lib/components/copilot/chat/global/core.test.ts index a6c1d763f8..c13f8f2e0e 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -2160,6 +2160,25 @@ describe('global AI tools', () => { expect(UserDraft.takeDraftOnlyDiscard('resource', path, { workspace: WORKSPACE })).toBe(true) }) + // The listener that refreshes hosted editors runs only for a tool that returned, + // so a throw after the entity is gone leaves a tab open on an item that no longer + // exists — and the cleanup cannot un-delete it either way. + it('reports a delete whose draft cleanup failed instead of throwing', async () => { + const path = 'f/resources/deleted-with-stuck-draft' + await callGlobalTool('write_resource', { + path, + resource_type: 'postgresql', + value: { host: 'localhost' } + }) + // The cleanup is a `value: null` draft write, so failing that write fails it. + failingWrites.add(`resource:${path}`) + + const result = await callGlobalTool('delete_workspace_item', { type: 'resource', path }) + + expect(ResourceService.deleteResource).toHaveBeenCalled() + expect(JSON.parse(result).message).toMatch(/draft could NOT be cleared/) + }) + // Nothing consumes a marker outside a session, so one can outlive the item it // was about. Recreating the item must void it, or the next action on the // recreated item reads it and sends a perfectly valid editor back to its list. diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index fd66e6d7c0..22127bce76 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -7578,7 +7578,9 @@ async function deployDraft( // fork comparisons before the fallible draft cleanup below. invalidateWorkspaceComparison(workspace) - await deleteGlobalDraft(workspace, type, path, triggerKind, { preserveLiveDraft: true }) + const draftIssue = await clearDraftAfterMutation(workspace, type, path, triggerKind, { + preserveLiveDraft: true + }) // Move the chat's mask entry to the deployed path: a draft-only item's // synthetic storage key never exists deployed, so the entry would otherwise @@ -7611,9 +7613,15 @@ async function deployDraft( return JSON.stringify( { success: true, - message: `Deployed draft ${type} "${path}" to the workspace. Draft removed.${ - deployNote ? ` ${deployNote}` : '' - }`, + message: [ + `Deployed draft ${type} "${path}" to the workspace.`, + draftIssue + ? `The draft could NOT be removed (${draftIssue}), so the item may still show unsaved changes.` + : 'Draft removed.', + deployNote + ] + .filter(Boolean) + .join(' '), type, path, triggerKind @@ -7716,6 +7724,23 @@ async function deployedItemExists( } } +/** + * Clear the draft of an item whose deployed state has already changed. A failure + * here cannot undo that change, and throwing would report the mutation as not + * having happened — to the model, and to the hosts, which hear about one only + * from a tool that returned. Reported in the result instead. + */ +async function clearDraftAfterMutation( + ...args: Parameters +): Promise { + try { + await deleteGlobalDraft(...args) + return undefined + } catch (e) { + return e instanceof Error ? e.message : String(e) + } +} + async function deleteWorkspaceItem( args: { type: WorkspaceItemType; path: string; trigger_kind?: TriggerKind }, ctx: WriteDraftCtx @@ -7759,7 +7784,7 @@ async function deleteWorkspaceItem( // are no longer trustworthy (same rule as deploy success). Before the // draft cleanup: a cleanup failure must not leave stale comparisons. invalidateWorkspaceComparison(workspace) - await deleteGlobalDraft(workspace, type, path, triggerKind) + const draftIssue = await clearDraftAfterMutation(workspace, type, path, triggerKind) // Record the deletion in the chat's modified-items mask. In a fork this leaves a // reviewable "removed" diff vs the parent that stays scoped to this chat. Keyed @@ -7779,7 +7804,11 @@ async function deleteWorkspaceItem( return JSON.stringify( { success: true, - message: `Deleted ${type} "${path}" from the workspace. Any matching draft was also cleared.`, + message: + `Deleted ${type} "${path}" from the workspace. ` + + (draftIssue + ? `Its draft could NOT be cleared (${draftIssue}), so the path may still list one.` + : 'Any matching draft was also cleared.'), type, path, triggerKind diff --git a/frontend/src/lib/userDraft.svelte.ts b/frontend/src/lib/userDraft.svelte.ts index 401b770023..9c7f545e36 100644 --- a/frontend/src/lib/userDraft.svelte.ts +++ b/frontend/src/lib/userDraft.svelte.ts @@ -142,17 +142,39 @@ function noteMarker(set: Set, key: string): void { set.delete(oldest) } } -// Only the kinds whose settle can read the record back. For the rest, recording one -// would retain a whole flow, script or app draft that nothing will ever ask about. -const RELEASE_RECORDED_KINDS: readonly UserDraftItemKind[] = ['resource', 'variable'] - /** - * What a cell held when its last holder let go: an editor stays editable while its - * save is in flight, so a tab left in that window releases a cell carrying an edit - * newer than the write, and no handle is left to read it from. Same cap and reason - * as the marker sets above. + * What a cell held when its last holder let go, recorded only while a write it + * could be newer than is in flight (see `beginDraftSettleWindow`). Outside one a + * release is an editor simply closing, and remembering its value would retain a + * whole resource — or a variable's decrypted secret — for the tab's lifetime. */ const releasedValues = new Map() +/** Keys with a write in flight, by how many are. */ +const settleWindows = new Map() + +/** + * Open the window in which a release of this cell is worth remembering: the editor + * stays editable while its save is in flight, so a tab left then hands the settle + * that follows a cell nothing holds any more. Pair with the returned disposer. + */ +export function beginDraftSettleWindow( + itemKind: UserDraftItemKind, + path: string, + opts?: UserDraftOptions +): () => void { + const mk = mapKey(resolveWorkspace(opts), itemKind, path) + settleWindows.set(mk, (settleWindows.get(mk) ?? 0) + 1) + return () => { + const left = (settleWindows.get(mk) ?? 0) - 1 + if (left > 0) { + settleWindows.set(mk, left) + return + } + settleWindows.delete(mk) + // Nothing is left to read it: the settle it was recorded for is over. + releasedValues.delete(mk) + } +} /** Read once: what it describes is settled by the caller that reads it. */ function takeReleasedValue( itemKind: UserDraftItemKind, @@ -165,15 +187,6 @@ function takeReleasedValue( return value } -function noteReleasedValue(key: string, value: unknown): void { - releasedValues.set(key, value) - while (releasedValues.size > MAX_WRITE_MARKERS) { - const oldest = releasedValues.keys().next().value - if (oldest === undefined || oldest === key) break - releasedValues.delete(oldest) - } -} - const liveEditorDrafts = new Map() /** * Map keys whose entry should start `syncSuspended` on acquire. Lets @@ -329,11 +342,9 @@ export async function settleDraftAfterWrite( const diverged = held !== undefined && !draftValuesEqual(held, written) if (savedPath === fromPath && diverged) return // The form stays editable across the delete's own request, so a keystroke made - // then queues a write behind it. Under an unchanged path that write is the - // user's and is meant to stand; under a path the item has left it would put the - // draft back where no editor is, so the old key stops accepting writes for the - // length of the delete — which is what makes one attempt final. The edit stays - // in the form, and could not have followed the rename in either case. + // then queues a write behind it: under an unchanged path that write is the + // user's and stands, but under a path the item has left it would put the draft + // back where no editor is. Suspending that key is what makes one attempt final. const moved = savedPath !== fromPath if (moved) UserDraft.stopSync(itemKind, fromPath, opts) try { @@ -346,11 +357,10 @@ export async function settleDraftAfterWrite( if (moved) UserDraft.restartSync(itemKind, fromPath, opts) } if (!moved) return - // A rename whose delete could not land — a conflict, or a failed request; no - // number of retries changes either. What is left is a draft-only item at a path - // nothing is editing. Callers still follow the rename, since the item really is - // at `savedPath` and leaving them behind would strand the editor too, so this is - // the only thing that says the leftover is there to be discarded. + // A conflict or a failed request, which no retry changes: what is left is a draft + // at a path nothing is editing. Callers still follow the rename — the item is at + // `savedPath`, and leaving them behind would strand the editor too — so this is + // the only thing that says the leftover is there. sendUserToast(`Saved, but the draft left at ${fromPath} could not be cleared`, true) } @@ -1076,9 +1086,7 @@ function releaseEntry(mk: string): void { // only here, once, at refcount 0. This is what lets multiple holders (warm // session previews + the nav editor) share the entry and drop in any order. if (entry.count <= 0) { - if (RELEASE_RECORDED_KINDS.includes(entry.itemKind)) { - noteReleasedValue(mk, snapshotDraftValue(entry.state.val)) - } + if (settleWindows.has(mk)) releasedValues.set(mk, snapshotDraftValue(entry.state.val)) // The live entry was authoritative while mounted; once gone, drop any // cached write for this key so a later read falls back to the server // rather than a value the editor may have changed in the meantime. diff --git a/frontend/src/lib/userDraftSettleAfterWrite.test.ts b/frontend/src/lib/userDraftSettleAfterWrite.test.ts index d2be6ad0dc..ac2bc645ad 100644 --- a/frontend/src/lib/userDraftSettleAfterWrite.test.ts +++ b/frontend/src/lib/userDraftSettleAfterWrite.test.ts @@ -20,11 +20,18 @@ vi.mock('./userDraftDbSyncer.svelte', () => ({ import { settleDraftAfterWrite, UserDraft } from './userDraft.svelte' +const stopSync = vi.fn() +const restartSync = vi.fn() + beforeEach(() => { settles = true toast.mockClear() discard.mockClear() + stopSync.mockClear() + restartSync.mockClear() vi.spyOn(UserDraft, 'discard').mockImplementation(discard as any) + vi.spyOn(UserDraft, 'stopSync').mockImplementation(stopSync as any) + vi.spyOn(UserDraft, 'restartSync').mockImplementation(restartSync as any) }) const OPTS = { workspace: 'ws' } @@ -102,6 +109,20 @@ describe('settleDraftAfterWrite', () => { expect(toast).not.toHaveBeenCalled() }) + // What makes the single attempt safe: the moved-from key stops accepting writes + // for the length of the delete, so a keystroke cannot queue behind it. Dropping + // the bracket would leave every other case here green. + it('suspends the key it is deleting, and only for a rename', async () => { + await settleDraftAfterWrite('variable', sent, { ...sent }, 'u/me/a', 'u/me/b', OPTS) + expect(stopSync).toHaveBeenCalledWith('variable', 'u/me/a', OPTS) + expect(restartSync).toHaveBeenCalledWith('variable', 'u/me/a', OPTS) + stopSync.mockClear() + restartSync.mockClear() + await settleDraftAfterWrite('variable', sent, { ...sent }, 'u/me/a', 'u/me/a', OPTS) + expect(stopSync).not.toHaveBeenCalled() + expect(restartSync).not.toHaveBeenCalled() + }) + it('sends it once when the key settles', async () => { await settleDraftAfterWrite('variable', sent, { ...sent }, 'u/me/a', 'u/me/b', OPTS) expect(discard).toHaveBeenCalledTimes(1)