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)