diff --git a/BACKLOG.md b/BACKLOG.md index 22f9e67d3..4fcc5d78c 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -180,7 +180,7 @@ Description formats: | 269 | Tech Debt | Split `SceneProperties` into `InstantSceneProperties` + `TimeRangeSceneProperties` (LinkML refactor) — introduce two distinct LinkML classes (`InstantScene` and `TimeRangeScene`) with a `kind` discriminator, replacing the current XOR-rule + TS-predicate discrimination layered on top of a single `SceneProperties` class. The XOR rule lives in three places today after #263 (LinkML rule, `flavourCheck` in `validate.ts`, `isTimeRangeScene` predicate in `types.ts`); two classes with a discriminator make the flavour-coupling structural, eliminate the cross-field rule, give LinkML→Pydantic→TS clean discriminated unions for free, and remove the "narrow at the predicate" rule from contributor cognition. Considered and deferred during #263 research (research.md R2) because the renaming cost was disproportionate to a two-line XOR rule. Once a third Scene flavour arrives (e.g. live-link / streamed Scenes), the discriminator approach becomes strictly cheaper. Probably belongs in the v3 schema cycle when a breaking change is allowed. Estimate 5–8 dev-days. (follow-up to #263 review decision; depends on #263 shipping + likely sequenced post-v4.0.0 per Article XIV) | 2 | 1 | 3 | 6 | Medium | blocked | | 2026-05-19 | 2026-05-26 | | ~~263~~ | ~~Enhancement~~ | ~~[Storyboard time-range Scenes — `time_range != null` interpolation (pan/zoom + time-slider scrub)](specs/263-time-range-scenes/spec.md) — lift the v1 single-instant constraint and make `time_range != null` a first-class capture + playback mode. Capture extends #216 with a "range" affordance recording `[t_start, t_end]` + a `viewport_end` snapshot. Playback extends #217: during `executeTransition` into a time-range Scene the engine simultaneously interpolates the viewport `viewport_start → viewport_end` AND advances the time-slider `t_start → t_end` over the captured wall-clock duration, so feature visibility / track positions / chart cursors move in lock-step. Reverse playback reverses both axes. Schema work bumps `geojson.yaml` (`SceneProperties.time_range` already exists; add `viewport_end`); generate Pydantic/TS/JSON Schema; Article II adherence tests + new golden fixtures for both Scene flavours. Linear interpolation only in MVP; ease-in/ease-out and edit-time range adjustment deferred. **Lands before #264** per interview sequencing. (split from #229 via `/interview 229`; requires #215–#218 MVP; [issue #630](https://github.com/debrief/debrief-future/issues/630))~~ | ~~4~~ | ~~4~~ | ~~3~~ | ~~11~~ | ~~Medium~~ | ~~complete~~ | ~~E13~~ | ~~2026-05-19~~ | ~~2026-05-19~~ | | ~~264~~ | ~~Feature~~ | ~~[Air-gapped briefing zip — self-contained distraction-free Storyboard renderer (SPA)](specs/264-briefing-zip-renderer/spec.md) — ship the briefing renderer as a self-contained zip that opens in any modern browser by double-clicking `index.html`: no server, no extension host, no network calls. Contents = bundled SPA + `features.geojson` (which already contains the `StoryboardFeature` / `SceneFeature` entries — Storyboards live inside the plot's GeoJSON per `storyboard.yaml`, not as a separate document) + `item.json` + Scene-thumbnail assets + pre-fetched basemap tiles. Build command takes a Storyboard ID (one plot may contain multiple Storyboards). Single Storyboard per zip, read-only, with Present (chrome-hidden) / Minimal (transport + scrub) toggle. Reuses the shared playback engine from #217 + #258 displayMode capture. New VS Code command "Export Storyboard as briefing zip…" on the Storyboard overflow menu. New SPA at `apps/briefing-renderer/`. **Blocked on #263** (time-range Scenes) per interview sequencing — briefing renderer should ship knowing time-range is in play to avoid rework on the shared engine. (split from #229 via `/interview 229`; requires #215–#218 MVP, #174 thumbnails, #258 displayMode capture; [issue #631](https://github.com/debrief/debrief-future/issues/631))~~ | ~~4~~ | ~~5~~ | ~~3~~ | ~~12~~ | ~~Medium~~ | ~~complete~~ | ~~E13~~ | ~~2026-05-19~~ | ~~2026-05-20~~ | -| 268 | Tech Debt | VS Code save atomicity (broader) — `apps/vscode/src/commands/saveSession.ts:163–208` currently performs three separate write operations (sidecar at 163, FeatureCollection at 178, thumbnails at 184–203) with non-blocking error handling at lines 180 and 192–202. After #249/spec-261 lands FR-019, the FC↔sidecar pair on the *migrated-state* path is sequenced FC-first/sidecar-second to close the silent-failure gap for that specific path — but the broader flow (thumbnails, STAC assets, sidecar, FC) is still independently committable. A real production failure mid-sequence could leave any subset of files updated. This ticket scopes a broader transactional-save model: either (a) a save-orchestrator that stages all writes to a tmp directory and atomically moves them into place, or (b) an explicit "last good known state" recovery file so a half-completed save can be rolled back on the next open. Estimate 5–8 dev-days; medium-to-high complexity (touches every save consumer). Trigger: a user reports a partial-save corruption, OR a security/audit review flags the non-atomic save flow. (follow-up to #249; cross-references docs/project_notes/decisions.md re: writer abstraction; potentially extends Constitution Article IV.4) | - | - | - | - | High | approved | | 2026-05-19 | 2026-05-31 | +| ~~268~~ | ~~Tech Debt~~ | ~~[VS Code save atomicity (broader)](specs/268-save-atomicity/spec.md) — after the #249/spec-261 sidecar retirement, `apps/vscode/src/commands/saveSession.ts` performs **two** independent, non-transactional write phases (was three — the `.debrief-session` sidecar write is gone): (1) `storeFeatureCollection()` (line 124) writes `features.geojson`, with view-state (viewport, time window/playhead, selection) folded in as `SystemState` features + per-feature `visible` flags by `applyStateToFeatures` first (line 123); (2) `storeThumbnails()` (line 142) drives `StacWriter.writePlotThumbnailPair`, which writes the large + small thumbnail PNGs and updates the STAC item JSON. The two phases are independently committable and ordered badly for failure: the FC write hard-fails correctly (error surfaced, early `return`, dirty flag retained — catch at lines 125–130), but the dirty flag is then cleared and "Plot saved" is shown (lines 133–134) *before* the thumbnail/STAC write runs — so a `StacWriterError` there (surfaced at 147–148; non-`StacWriterError` capture failures are non-blocking at line 150) leaves a plot that looks saved (clean dirty flag, success toast) but carries stale/missing thumbnails, and a crash between phases leaves `features.geojson` updated against an old STAC item. A real production failure between the phases leaves a partial on-disk state. This ticket scopes a transactional-save model: either (a) a save-orchestrator that stages all writes to a tmp location and atomically moves them into place, or (b) an explicit "last good known state" recovery file so a half-completed save can be rolled back on the next open. Note the host-agnostic `StacWriter` spans the FS adaptor *and* the web-shell IndexedDB store, so "atomic move" needs a writer-level transaction/commit abstraction, not a bare `fs.rename` — which is what keeps this medium-to-high despite dropping to two writes. Estimate ~5–7 dev-days (marginally less than the original 5–8 now the sidecar write is gone; the bulk is the cross-host transaction model, not the write count); medium-to-high complexity. Trigger: a user reports a partial-save corruption, OR a security/audit review flags the non-atomic save flow. (rescoped 2026-06-01 after the spec-261 sidecar retirement landed; follow-up to #249/spec-261; cross-references docs/project_notes/decisions.md re: writer abstraction; potentially extends Constitution Article IV.4)~~ | ~~-~~ | ~~-~~ | ~~-~~ | ~~-~~ | ~~High~~ | ~~complete~~ | | ~~2026-05-19~~ | ~~2026-06-01~~ | | ~~267~~ | ~~Tech Debt~~ | ~~[Out-of-window `current_time` policy — #249/spec-261 introduces an optional `current_time` field on the LinkML `SystemStateProperties` `temporal` variant (the saved playhead position) and ships strict-on-import validation: if a plot's saved `current_time` falls outside its `[start_time, end_time]` window, the load fails with `SystemStateLoadError(kind='cross-field-invariant')`. This is the right Article XIV.4 default for v4.x pre-release. But if analyst feedback later shows that malformed plots arrive often enough to be a friction (e.g. a colleague trims the analytical window after scrubbing, leaving the playhead orphaned), the team may want a tolerant alternative: clamp to the nearest window edge with a non-modal toast, OR offer "open ignoring saved playhead" as a fallback action. This ticket revisits the policy if/when the strict-on-import behaviour proves user-hostile. Likely depends on real-world usage data from #249 ship. Estimate 1–2 dev-days. Trigger: analyst feedback that plots are failing to load due to out-of-window playheads, OR a deliberate UX decision post-v4.0.0 to add tolerant import paths (Article XIV trigger point). (follow-up to #249; depends on real-world data)](specs/267-tolerant-playhead-import/spec.md)~~ | ~~2~~ | ~~1~~ | ~~4~~ | ~~7~~ | ~~Low~~ | ~~complete~~ | | ~~2026-05-19~~ | ~~2026-05-29~~ | | ~~266~~ | ~~Tech Debt~~ | ~~Spatial-shape cleanup follow-up to #249 — #249/spec-261 unifies the LinkML schema's spatial-viewport representation onto `ViewportPolygon`, removing the parallel `bbox: float[4]` + `zoom: float` + `center: float[2]` fields from `SystemStateProperties` (per Article XIV.1 — verified zero runtime blast radius today). If any docs, ADRs, ARCHITECTURE.md sections, code comments, or external references still mention the `bbox`/`center` shape after #249 ships, they need updating. Likely affected: `docs/project_notes/decisions.md` (any ADR referencing the spatial SystemState shape), `ARCHITECTURE.md` schema-extension passages, MCP envelope docs if any, and any spec for #215 / #237 that referenced the bbox shape. Estimate 0.5–1 dev-day. Trigger: #249 has shipped and a grep for `SystemStateProperties.*bbox` / `state_type.*spatial.*bbox` over the repo turns up stale references. (follow-up to #249; cross-references shared/schemas/src/linkml/geojson.yaml post-#249)~~ | ~~-~~ | ~~-~~ | ~~-~~ | ~~-~~ | ~~Low~~ | ~~complete~~ | | ~~2026-05-19~~ | ~~2026-06-01~~ | | 265 | Research Spike | [MP4 / GIF export of a Storyboard traversal — encoding-pipeline trade-off spike](https://github.com/debrief/debrief-future/issues/632) — time-boxed (1–2 days) research evaluating at least three encoding-pipeline candidates: (1) client-side `MediaRecorder` + `ffmpeg.wasm` (~20MB bundle bloat, fully offline, ships inside the briefing zip too), (2) service-side Python `services/export/` + system `ffmpeg` (clean Article IV.1, faster, but new system dep + doesn't ship inside the air-gapped briefing zip), (3) hybrid using WebCodecs API with `ffmpeg.wasm` GIF fallback. Deliverable = `docs/project_notes/storyboard-video-export-research.md` with bundle-size measurements, encoding-time benchmarks against a 10-Scene 1080p Storyboard, output-quality samples under `evidence/`, Article I compatibility analysis, briefing-zip (#264) integration story per option, recommendation + a follow-up implementation backlog item. **No shipping code from this ticket.** (split from #229 via `/interview 229`; independent of #263/#264 but recommendation should consider both) | 2 | 2 | 4 | 8 | Low | approved | E13 | 2026-05-19 | 2026-05-19 | diff --git a/CLAUDE.md b/CLAUDE.md index ed006537d..073323f6b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -218,6 +218,7 @@ Only updated when a feature introduces a technology not already listed here. - Python 3.11 (services, schema-build tooling, + LinkML ≥ 1.7.0 (schema source + (223-linkml-stac-catalog) - TypeScript 5.x (strict mode) — no new technology; amends spec-261's `@debrief/session-state` `SystemState` load layer (`validate.ts`, reconciliation, `load.ts`) + host notification surfaces. No schema change, no new dependency. (267-tolerant-playhead-import) - TypeScript 5.x (strict), Node 20 runtime (VS Code extension host); React 18.x (renderer + panel) + `@debrief/components` (StoryboardPanel), `@debrief/stac-writer` (web-shell reads), `@debrief/schemas` (LinkML types — consumed, not changed), JSZip 3.10.1 (already present), VS Code Extension API ^1.85.0, `react-leaflet`/Leaflet (renderer map), Vite 5.x (web-shell + renderer) (273-storyboard-preview-button) +- TypeScript 5.x (strict; Article XV). No Python change. + `@debrief/stac-writer` (interface + core), `node:fs`/`node:crypto` (fs adaptor — existing), `idb` (web-shell adaptor — existing), `@debrief/schemas` (`FeatureCollection`, `StacItem`), VS Code Extension API (`window.showWarningMessage`). **No new runtime dependencies.** (268-save-atomicity) ## Before Pushing @@ -301,5 +302,6 @@ Each wrapper: Note: `vitest` does not catch TypeScript type errors — only `tsc` (run during typecheck) does. The `pnpm build` step also runs `tsc`, but typecheck is the explicit CI gate. ## Recent Changes +- 268-save-atomicity: Added TypeScript 5.x (strict; Article XV). No Python change. + `@debrief/stac-writer` (interface + core), `node:fs`/`node:crypto` (fs adaptor — existing), `idb` (web-shell adaptor — existing), `@debrief/schemas` (`FeatureCollection`, `StacItem`), VS Code Extension API (`window.showWarningMessage`). **No new runtime dependencies.** - 267-tolerant-playhead-import: Relaxes spec-261 FR-018 strict-on-import for one recoverable case — orphaned playhead (out-of-window `current_time`) clamps to nearest window edge + non-blocking notification; incoherent window (`start>end`) still hard-fails. No schema change, no new dependency. - 273-storyboard-preview-button: Added TypeScript 5.x (strict), Node 20 runtime (VS Code extension host); React 18.x (renderer + panel) + `@debrief/components` (StoryboardPanel), `@debrief/stac-writer` (web-shell reads), `@debrief/schemas` (LinkML types — consumed, not changed), JSZip 3.10.1 (already present), VS Code Extension API ^1.85.0, `react-leaflet`/Leaflet (renderer map), Vite 5.x (web-shell + renderer) diff --git a/apps/vscode/src/commands/openPlot.ts b/apps/vscode/src/commands/openPlot.ts index 5a1c210ee..fdfa845e2 100644 --- a/apps/vscode/src/commands/openPlot.ts +++ b/apps/vscode/src/commands/openPlot.ts @@ -8,6 +8,7 @@ import { createLogService, createSnapshotService, createTimeInstant, type Result import { hydrateStoreFromFeatures, SystemStateLoadError, type PlayheadClampDiagnostic } from '../services/systemStateBridge'; import { notifyPlayheadClamps } from '../services/playheadClampNotice'; import type { StacWriter } from '@debrief/stac-writer'; +import { reconcileBeforeOpen } from './reconcileOnOpen'; import type { ConfigService } from '../services/configService'; import type { StacService } from '../services/stacService'; import type { CalcService } from '../services/calcService'; @@ -134,6 +135,12 @@ export function createOpenPlotCommand( return; } + // #268 — reconcile any interrupted save BEFORE reading the plot, so the + // loads below observe a single coherent on-disk state (FR-007/FR-008). + await reconcileBeforeOpen(getStacWriter, store.path, itemPath, (message) => { + void vscode.window.showWarningMessage(message); + }); + // Load plot const plot = await vscode.window.withProgress( { diff --git a/apps/vscode/src/commands/reconcileOnOpen.ts b/apps/vscode/src/commands/reconcileOnOpen.ts new file mode 100644 index 000000000..1bada55ee --- /dev/null +++ b/apps/vscode/src/commands/reconcileOnOpen.ts @@ -0,0 +1,42 @@ +/** + * Reconcile-on-open hook for the atomic plot save (#268, US3). + * + * Extracted from `openPlot.ts` so it can be exercised in a Node test + * environment without pulling in the webview/Leaflet dependency graph. + */ + +import type { StacWriter, ReconcilePlotSaveResult } from '@debrief/stac-writer'; + +/** + * Heal an interrupted save before the plot is read. `reconcilePlotSave` can + * change what is on disk (roll a committed-but-unapplied save forward, or + * discard pre-commit temps), so this MUST run before `loadPlot` / + * `loadPlotData`. When it acts, a non-blocking notice is shown (FR-008). + * Reconcile must never block opening — any failure is logged and swallowed. + */ +export async function reconcileBeforeOpen( + getStacWriter: ((storePath: string) => StacWriter) | undefined, + storePath: string, + itemPath: string, + showWarning: (message: string) => void, +): Promise { + if (!getStacWriter) { + return null; + } + try { + const writer = getStacWriter(storePath); + const result = await writer.reconcilePlotSave({ + ctx: { kind: 'fs', nowMs: () => Date.now(), randomId: () => '' }, + stacItemPath: itemPath, + }); + if (result.recovered) { + showWarning( + 'Recovered an interrupted save — opened the last good version of this plot.', + ); + } + return result; + } catch (err) { + console.warn('[debrief] openPlot: reconcilePlotSave failed', err); + return null; + } +} diff --git a/apps/vscode/src/commands/saveSession.ts b/apps/vscode/src/commands/saveSession.ts index a44c9fa17..e3877e868 100644 --- a/apps/vscode/src/commands/saveSession.ts +++ b/apps/vscode/src/commands/saveSession.ts @@ -17,6 +17,7 @@ import * as vscode from 'vscode'; import * as path from 'path'; import * as fs from 'fs'; +import type { RawGeoJSONFeatureCollection } from '@debrief/schemas'; import type { StacWriter } from '@debrief/stac-writer'; import { StacWriterError } from '@debrief/stac-writer'; import type { SessionManager } from '../services/sessionManager'; @@ -50,33 +51,6 @@ export function storeFeatureCollection( fs.writeFileSync(featuresPath, `${JSON.stringify(fc, null, 2)}\n`); } -/** - * Write plot thumbnails by delegating to the host-agnostic StacWriter. - * - * Spec 242 closure of Article IV.1 — saveSession is a frontend command, so - * it must orchestrate persistence through the writer interface rather than - * touching the filesystem itself. The earlier (spec 241) `plotThumbnailWriter.ts` - * shim has been deleted; the FS-backed adaptor (`stacWriterFs`) now owns the - * write path and produces the spec-241 STAC 1.1 shape authoritatively. - */ -async function storeThumbnails( - writer: StacWriter, - plotUri: string, - largePngBase64: string, - smallPngBase64: string, -): Promise { - const parsed = parseStacUri(plotUri); - if (!parsed) { - return; - } - await writer.writePlotThumbnailPair({ - ctx: { kind: 'fs', nowMs: () => Date.now(), randomId: () => '' }, - stacItemPath: parsed.itemPath, - largePngBase64, - smallPngBase64, - }); -} - /** * Create the save session command handler. * @@ -115,20 +89,21 @@ export function createSaveSessionCommand( return; } - // Feature 261 (FR-009/FR-010): write exactly features.geojson. The current - // view-state (viewport, time window/playhead, selection) is upserted as - // SystemState features and per-feature visibility is set, then the whole - // FeatureCollection is written. NO `.debrief-session` sidecar is written. + // Feature 261 (FR-009/FR-010): the FeatureCollection IS the plot. The + // current view-state (viewport, time window/playhead, selection) is + // upserted as SystemState features and per-feature visibility is set; the + // whole collection is then committed atomically. NO `.debrief-session` + // sidecar. // // FR-013/FR-021: visibility transitions are recorded on the affected - // feature's own provenance, bounded to this saved state — passing the actor - // makes `applyStateToFeatures` append a visibility-change LogEntry to every - // feature whose visibility differs from the on-disk FeatureCollection. + // feature's own provenance — passing the actor makes `applyStateToFeatures` + // append a visibility-change LogEntry to every feature whose visibility + // differs from the on-disk FeatureCollection. + let features: ReturnType; try { - const features = applyStateToFeatures(mapPanel.getCurrentFeatures(), state, { + features = applyStateToFeatures(mapPanel.getCurrentFeatures(), state, { actor: sessionManager.actor, }); - storeFeatureCollection(storePath, plotUri, features); } catch (err) { void vscode.window.showErrorMessage( `Failed to save plot: ${err instanceof Error ? err.message : String(err)}`, @@ -136,27 +111,73 @@ export function createSaveSessionCommand( return; } - // The plot is now persisted; clear the dirty flag. - session.getState().markClean(); - void vscode.window.showInformationMessage('Plot saved'); - - // Capture thumbnails after successful save (#174) - if (getStacWriter) { + // Legacy fallback when no writer is wired (e.g. a minimal host). This is + // the pre-#268 non-atomic path and has no thumbnail/STAC-asset write. + if (!getStacWriter) { try { - const { largePngBase64, smallPngBase64 } = await mapPanel.requestThumbnailCapture(5000); - if (largePngBase64 && smallPngBase64) { - const writer = getStacWriter(storePath); - await storeThumbnails(writer, plotUri, largePngBase64, smallPngBase64); - } + storeFeatureCollection(storePath, plotUri, features); } catch (err) { - // Article I.3 — service-write failures must surface; capture - // failures (non-StacWriterError) remain best-effort. - if (err instanceof StacWriterError) { - void vscode.window.showErrorMessage(`Thumbnail save failed: ${err.message}`); - } else { - console.warn('[debrief] Thumbnail capture failed (non-blocking):', err); - } + void vscode.window.showErrorMessage( + `Failed to save plot: ${err instanceof Error ? err.message : String(err)}`, + ); + return; } + session.getState().markClean(); + void vscode.window.showInformationMessage('Plot saved'); + return; } + + // #268 — capture thumbnails BEFORE the commit so they ride in the same + // atomic save unit. Thumbnail *capture* stays best-effort: a capture + // failure simply omits `thumbnails` from the commit (the spec's "capture + // is best-effort and may legitimately be skipped" edge case). A capture + // *write* that begins is part of the atomic unit and obeys atomicity. + let thumbnails: { largePngBase64: string; smallPngBase64: string } | undefined; + try { + const captured = await mapPanel.requestThumbnailCapture(5000); + if (captured.largePngBase64 && captured.smallPngBase64) { + thumbnails = { + largePngBase64: captured.largePngBase64, + smallPngBase64: captured.smallPngBase64, + }; + } + } catch (err) { + console.warn('[debrief] Thumbnail capture failed (non-blocking):', err); + } + + // #268 / FR-002/FR-004 — commit the whole save unit (feature collection + + // the item-metadata it implies + thumbnails) atomically through the shared + // persistence boundary. The raw `fs.writeFileSync` of features.geojson is + // gone; the FC write is now on the boundary (Article IV.2). + // `features` is the loose host boundary shape (SystemState features folded + // in); commitPlotSave only serialises the collection, so we cross the + // RFC-7946 parse boundary here. + const featureCollection: RawGeoJSONFeatureCollection = { + type: 'FeatureCollection', + features: features as RawGeoJSONFeatureCollection['features'], + }; + + try { + const writer = getStacWriter(storePath); + await writer.commitPlotSave({ + ctx: { kind: 'fs', nowMs: () => Date.now(), randomId: () => '' }, + stacItemPath: parsed.itemPath, + featureCollection, + thumbnails, + }); + } catch (err) { + // FR-005/FR-006 — a save that cannot fully commit is reported as a + // failure; the dirty flag is left set so the analyst can retry, and the + // previously-persisted version is untouched. We do NOT markClean here. + void vscode.window.showErrorMessage( + `Failed to save plot: ${err instanceof StacWriterError || err instanceof Error ? err.message : String(err)}`, + ); + return; + } + + // FR-005 — only now, after every write of the save unit has committed, is + // the dirty flag cleared and success shown. + session.getState().markClean(); + void vscode.window.showInformationMessage('Plot saved'); }; } diff --git a/apps/vscode/src/services/saveJournal.ts b/apps/vscode/src/services/saveJournal.ts new file mode 100644 index 000000000..f1b1d17e6 --- /dev/null +++ b/apps/vscode/src/services/saveJournal.ts @@ -0,0 +1,99 @@ +/** + * Save journal — the write-ahead intent record that marks the filesystem + * commit point for an atomic plot save (#268, ADR-039). + * + * `commitPlotSave` (stacWriterFs) stages every artefact as a temp file, then + * atomically writes ONE journal listing the pending `temp → final` renames. + * The atomic creation of that journal IS the commit point: `reconcilePlotSave` + * rolls **back** before the journal exists (delete temps, keep originals) and + * **forward** after it (re-apply the pending renames idempotently). The journal + * lives at `/.save-journal.json` and is deleted on success. + * + * It never crosses the public `StacWriter` interface (FS-only; the browser host + * relies on IndexedDB transaction atomicity). It is validated on read through + * {@link parseSaveJournal} — no `any`, no unchecked casts (Article XV.5). + */ + +/** Filename of the journal within an item directory. */ +export const SAVE_JOURNAL_FILENAME = '.save-journal.json'; + +/** Current journal format version (forward-compat guard). */ +export const SAVE_JOURNAL_VERSION = 1 as const; + +/** A single pending `temp → final` rename, item-dir-relative. */ +export interface SaveJournalRename { + /** Item-dir-relative path of the staged temp file. */ + readonly temp: string; + /** Item-dir-relative path of the final destination. */ + readonly final: string; +} + +/** + * The write-ahead intent record. Persisted atomically; its existence means a + * save reached the commit point and `reconcilePlotSave` must roll forward. + */ +export interface SaveJournal { + readonly version: typeof SAVE_JOURNAL_VERSION; + /** Item this save belongs to (catalog-relative, for diagnostics). */ + readonly stacItemPath: string; + /** `ctx.nowMs()` captured at the commit-point write. */ + readonly createdAtMs: number; + /** Pending `temp → final` renames, in apply order. */ + readonly renames: ReadonlyArray; +} + +/** Narrow `unknown` to a plain (non-array, non-null) object. */ +function isPlainObject(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +function parseRename(value: unknown): SaveJournalRename | null { + if (!isPlainObject(value)) { + return null; + } + const { temp, final } = value; + if (typeof temp !== 'string' || temp.length === 0) { + return null; + } + if (typeof final !== 'string' || final.length === 0) { + return null; + } + return { temp, final }; +} + +/** + * Validate untyped JSON as a {@link SaveJournal}. Returns the typed record, or + * `null` when the input is missing, malformed, or a future/unknown version — + * the caller then treats it as "no usable journal" and rolls back safely. + */ +export function parseSaveJournal(value: unknown): SaveJournal | null { + if (!isPlainObject(value)) { + return null; + } + if (value.version !== SAVE_JOURNAL_VERSION) { + return null; + } + if (typeof value.stacItemPath !== 'string' || value.stacItemPath.length === 0) { + return null; + } + if (typeof value.createdAtMs !== 'number' || !Number.isFinite(value.createdAtMs)) { + return null; + } + if (!Array.isArray(value.renames)) { + return null; + } + const renames: SaveJournalRename[] = []; + for (const entry of value.renames) { + const rename = parseRename(entry); + if (rename === null) { + return null; + } + renames.push(rename); + } + return { + version: SAVE_JOURNAL_VERSION, + stacItemPath: value.stacItemPath, + createdAtMs: value.createdAtMs, + renames, + }; +} diff --git a/apps/vscode/src/services/stacWriterFs.ts b/apps/vscode/src/services/stacWriterFs.ts index 3d3b4dde6..106ad5f26 100644 --- a/apps/vscode/src/services/stacWriterFs.ts +++ b/apps/vscode/src/services/stacWriterFs.ts @@ -28,12 +28,16 @@ import * as path from 'path'; import * as crypto from 'crypto'; import type { CapabilityReport, + CommitPlotSaveInput, + CommitPlotSaveResult, DeleteAssetInput, DeleteAssetResult, DeleteItemInput, DeleteItemResult, PatchItemInput, PatchItemResult, + ReconcilePlotSaveInput, + ReconcilePlotSaveResult, StacWriter, StoreContext, WriteAssetInput, @@ -46,6 +50,13 @@ import type { WriteSceneThumbnailPairResult, } from '@debrief/stac-writer'; import { StacWriterError, pathGuard, validateSceneId } from '@debrief/stac-writer'; +import { + SAVE_JOURNAL_FILENAME, + SAVE_JOURNAL_VERSION, + parseSaveJournal, + type SaveJournal, + type SaveJournalRename, +} from './saveJournal'; import { writeSceneThumbnail, @@ -234,6 +245,192 @@ export function createStacWriterFs(opts: StacWriterFsOptions): StacWriter { }; }, + // eslint-disable-next-line @typescript-eslint/require-await -- StacWriter interface mandates Promise return; this adaptor wraps synchronous Node fs. + async commitPlotSave( + input: CommitPlotSaveInput, + ): Promise { + pathGuard('commitPlotSave.stacItemPath', input.stacItemPath); + if (input.featureCollection.type !== 'FeatureCollection') { + throw new StacWriterError( + 'validation-failed', + 'commitPlotSave: featureCollection.type must be "FeatureCollection"', + { path: input.stacItemPath }, + ); + } + + const itemJsonPath = path.join(storePath, input.stacItemPath); + const itemDir = path.dirname(itemJsonPath); + + // Decode + validate thumbnails up front — pre-commit, no writes yet. + let smallBuffer: Buffer | null = null; + let largeBuffer: Buffer | null = null; + if (input.thumbnails !== undefined) { + smallBuffer = Buffer.from(input.thumbnails.smallPngBase64, 'base64'); + largeBuffer = Buffer.from(input.thumbnails.largePngBase64, 'base64'); + if (smallBuffer.byteLength === 0) { + throw new StacWriterError( + 'empty-png', + 'commitPlotSave: smallPngBase64 decoded to zero bytes', + { path: input.stacItemPath }, + ); + } + if (largeBuffer.byteLength === 0) { + throw new StacWriterError( + 'empty-png', + 'commitPlotSave: largePngBase64 decoded to zero bytes', + { path: input.stacItemPath }, + ); + } + if (!fs.existsSync(itemJsonPath)) { + throw new StacWriterError( + 'stac-item-not-found', + `commitPlotSave: item.json not found at ${itemJsonPath} (required to commit thumbnails)`, + { path: input.stacItemPath }, + ); + } + } + + try { + fs.mkdirSync(itemDir, { recursive: true }); + } catch (cause) { + throw mapFsWriteError(cause, input.stacItemPath, 'commitPlotSave: mkdir'); + } + + // Assemble the artefacts that make up this save unit (final names + + // bytes). features.geojson is always present; thumbnails imply two PNGs + // plus an updated item.json (asset entries) committed in the same unit. + const artefacts: Array<{ finalName: string; data: Uint8Array | string }> = [ + { + finalName: 'features.geojson', + data: `${JSON.stringify(input.featureCollection, null, 2)}\n`, + }, + ]; + let thumbnailRel: string | null = null; + let overviewRel: string | null = null; + if (smallBuffer !== null && largeBuffer !== null) { + const updatedItem = buildItemWithThumbnails( + itemJsonPath, + input.stacItemPath, + smallBuffer, + largeBuffer, + ); + artefacts.push({ finalName: 'thumbnail.png', data: smallBuffer }); + artefacts.push({ finalName: 'overview.png', data: largeBuffer }); + // item.json LAST so it only ever references assets already in place. + artefacts.push({ + finalName: 'item.json', + data: `${JSON.stringify(updatedItem, null, 2)}\n`, + }); + thumbnailRel = path.relative(storePath, path.join(itemDir, 'thumbnail.png')); + overviewRel = path.relative(storePath, path.join(itemDir, 'overview.png')); + } + + // ── Phase 1: STAGE ── write every artefact to a temp; nothing in place. + const token = crypto.randomBytes(8).toString('hex'); + const renames: SaveJournalRename[] = []; + const stagedTemps: string[] = []; + try { + for (const { finalName, data } of artefacts) { + const tempName = `${finalName}.save-${token}.tmp`; + fs.writeFileSync(path.join(itemDir, tempName), data); + stagedTemps.push(path.join(itemDir, tempName)); + renames.push({ temp: tempName, final: finalName }); + } + } catch (cause) { + cleanupTemps(stagedTemps); + throw mapFsWriteError(cause, input.stacItemPath, 'commitPlotSave: stage'); + } + + // ── Phase 2: COMMIT POINT ── atomically write the intent journal. + const journalPath = path.join(itemDir, SAVE_JOURNAL_FILENAME); + const journal: SaveJournal = { + version: SAVE_JOURNAL_VERSION, + stacItemPath: input.stacItemPath, + createdAtMs: input.ctx.nowMs(), + renames, + }; + try { + atomicWriteSync(journalPath, `${JSON.stringify(journal, null, 2)}\n`); + } catch (cause) { + // Still pre-commit — discard the staged temps, originals untouched. + cleanupTemps(stagedTemps); + throw cause instanceof StacWriterError + ? cause + : mapFsWriteError(cause, input.stacItemPath, 'commitPlotSave: journal'); + } + + // ── Phase 3: APPLY ── rename each temp → final. Post-commit: a failure + // here leaves the journal (+ remaining temps) so the next open rolls + // forward; we do NOT roll back (the originals are already superseded). + try { + applyJournalRenames(itemDir, journal); + } catch (cause) { + throw mapFsWriteError( + cause, + input.stacItemPath, + 'commitPlotSave: apply (recoverable on next open)', + ); + } + + // ── Phase 4: CLEAR ── drop the journal. Save complete. + try { + fs.unlinkSync(journalPath); + } catch { + // A stray journal over a fully-applied save is harmless — the next + // reconcile re-applies (idempotent) and clears it. + } + + return { + featuresPath: path.relative(storePath, path.join(itemDir, 'features.geojson')), + thumbnailPath: thumbnailRel, + overviewPath: overviewRel, + }; + }, + + // eslint-disable-next-line @typescript-eslint/require-await -- StacWriter interface mandates Promise return; this adaptor wraps synchronous Node fs. + async reconcilePlotSave( + input: ReconcilePlotSaveInput, + ): Promise { + pathGuard('reconcilePlotSave.stacItemPath', input.stacItemPath); + const itemDir = path.dirname(path.join(storePath, input.stacItemPath)); + if (!fs.existsSync(itemDir)) { + return { recovered: false, outcome: 'clean' }; + } + const journalPath = path.join(itemDir, SAVE_JOURNAL_FILENAME); + + // ── Journal present → interrupted AFTER the commit point → roll FORWARD. + if (fs.existsSync(journalPath)) { + let journal: SaveJournal | null = null; + try { + journal = parseSaveJournal(JSON.parse(fs.readFileSync(journalPath, 'utf8'))); + } catch { + journal = null; + } + if (journal !== null) { + // Re-apply pending renames (idempotent — a missing temp is already + // applied), then drop the journal: the new coherent version (FR-007). + applyJournalRenames(itemDir, journal); + fs.rmSync(journalPath, { force: true }); + cleanupTemps(listSaveTemps(itemDir)); + return { recovered: true, outcome: 'rolled-forward' }; + } + // Malformed/unknown-version journal: we cannot trust the renames, so + // discard it and any staged temps, keeping the last-good originals. + fs.rmSync(journalPath, { force: true }); + cleanupTemps(listSaveTemps(itemDir)); + return { recovered: true, outcome: 'rolled-back' }; + } + + // ── No journal → interrupted BEFORE the commit point (or clean). + const strays = listSaveTemps(itemDir); + if (strays.length === 0) { + return { recovered: false, outcome: 'clean' }; + } + // Discard the staged temps, keep the originals = last-good (FR-008). + cleanupTemps(strays); + return { recovered: true, outcome: 'rolled-back' }; + }, + async patchItem(input: PatchItemInput): Promise { pathGuard('patchItem.itemPath', input.itemPath); // `input.provenance.tool` is statically the PROPERTIES_PANEL_TOOL_SENTINEL @@ -554,6 +751,140 @@ function atomicWriteSync(target: string, data: Uint8Array | string): void { } } +/** + * Build the updated `item.json` object for a thumbnail-bearing commit: reads + * the existing item, sets the `thumbnail` / `overview` assets (size + multihash + * checksum), refreshes timestamps. Identical asset shape to + * `writePlotThumbnailPair` so the two paths produce byte-compatible items. + * Pure — performs no writes (the caller stages the returned JSON). + */ +function buildItemWithThumbnails( + itemJsonPath: string, + stacItemPath: string, + smallBuffer: Buffer, + largeBuffer: Buffer, +): Record { + let item: Record; + try { + const raw = fs.readFileSync(itemJsonPath, 'utf8'); + item = parseJsonObject(raw, 'commitPlotSave', stacItemPath); + } catch (cause) { + if (cause instanceof StacWriterError) { + throw cause; + } + throw new StacWriterError( + 'item-json-malformed', + `commitPlotSave: item.json unreadable at ${itemJsonPath}`, + { path: stacItemPath, cause }, + ); + } + + const existingAssets = asPlainObject(item.assets) ?? {}; + const assets: Record = { ...existingAssets }; + delete assets['thumbnail-sm']; // drop legacy spec-241 key (idempotent) + assets['thumbnail'] = { + href: './thumbnail.png', + type: 'image/png', + title: 'Plot thumbnail (200x150)', + roles: ['thumbnail'], + 'proj:shape': [150, 200], + 'file:size': smallBuffer.byteLength, + 'file:checksum': multihashSha256(smallBuffer), + }; + assets['overview'] = { + href: './overview.png', + type: 'image/png', + title: 'Plot overview (800x600)', + roles: ['overview'], + 'proj:shape': [600, 800], + 'file:size': largeBuffer.byteLength, + 'file:checksum': multihashSha256(largeBuffer), + }; + item.assets = assets; + + const properties = asPlainObject(item.properties) ?? {}; + if (typeof properties['created'] !== 'string') { + properties['created'] = isoNowUtc(); + } + properties['updated'] = isoNowUtc(); + item.properties = properties; + + return item; +} + +/** + * Apply a save journal's pending `temp → final` renames, in order. Idempotent: + * a temp that no longer exists was already applied (a prior rename or a partial + * roll-forward), so it is skipped. Shared by `commitPlotSave` (apply phase) and + * `reconcilePlotSave` (roll forward). + */ +function applyJournalRenames(itemDir: string, journal: SaveJournal): void { + for (const rename of journal.renames) { + const tempAbs = path.join(itemDir, rename.temp); + if (!fs.existsSync(tempAbs)) { + continue; + } + fs.renameSync(tempAbs, path.join(itemDir, rename.final)); + } +} + +/** + * List absolute paths of this feature's save leftovers in an item directory: + * staged artefact temps (`.save-.tmp`) and the journal's own + * atomic-write temp (`.save-journal.json...tmp`). Used by reconcile + * to discard pre-commit debris. Other `.tmp` files are left untouched. + */ +function listSaveTemps(itemDir: string): string[] { + let names: string[]; + try { + names = fs.readdirSync(itemDir); + } catch { + return []; + } + return names + .filter( + (name) => + /\.save-[0-9a-f]+\.tmp$/.test(name) || + /^\.save-journal\.json\..*\.tmp$/.test(name), + ) + .map((name) => path.join(itemDir, name)); +} + +/** Best-effort removal of staged temp files (pre-commit rollback). */ +function cleanupTemps(tempPaths: ReadonlyArray): void { + for (const temp of tempPaths) { + try { + if (fs.existsSync(temp)) { + fs.unlinkSync(temp); + } + } catch { + // best-effort + } + } +} + +/** Map a raw fs error to the structured StacWriter taxonomy (Article I.3). */ +function mapFsWriteError( + cause: unknown, + itemPath: string, + ctx: string, +): StacWriterError { + if (cause instanceof StacWriterError) { + return cause; + } + if (isReadOnlyFsError(cause)) { + return new StacWriterError('read-only-fs', `${ctx}: filesystem is read-only`, { + path: itemPath, + cause, + }); + } + return new StacWriterError( + 'write-failed', + `${ctx}: ${cause instanceof Error ? cause.message : String(cause)}`, + { path: itemPath, cause }, + ); +} + /** * Multihash-encoded SHA-256 of `buffer`. Returns the hex string * `<32-byte digest>`, matching the diff --git a/apps/vscode/tests/unit/helpers/saveFaultInjection.ts b/apps/vscode/tests/unit/helpers/saveFaultInjection.ts new file mode 100644 index 000000000..d489d021f --- /dev/null +++ b/apps/vscode/tests/unit/helpers/saveFaultInjection.ts @@ -0,0 +1,85 @@ +/** + * Shared fault-injection helper for the atomic-save (#268) integration tests. + * + * Wraps any {@link StacWriter} and makes a chosen write method reject with a + * {@link StacWriterError} on its Nth invocation — the "process killed / disk + * full mid-save" condition the spec's fault-injection matrix exercises + * (SC-001/002/003). The remaining calls delegate to the wrapped writer + * unchanged. + * + * Implemented as a Proxy so it covers every current and future write method + * (including `commitPlotSave`, added in this feature) without re-listing the + * interface surface — the wrapper cannot silently miss a new method. + */ + +import { StacWriterError, type StacWriter } from '@debrief/stac-writer'; +import type { StacWriterErrorKind } from '@debrief/stac-writer'; + +/** Methods that mutate the store — the ones a "write" counter advances on. */ +const WRITE_METHODS: ReadonlySet = new Set([ + 'writeItem', + 'patchItem', + 'writeAsset', + 'writeSceneThumbnailPair', + 'writePlotThumbnailPair', + 'deleteItem', + 'deleteAsset', + 'commitPlotSave', +]); + +export interface SaveFaultInjectionOptions { + /** 1-based index of the counted write call that should reject. */ + readonly failOnCall: number; + /** + * Restrict counting + failure to a single method (e.g. `'commitPlotSave'`). + * When omitted, every method in {@link WRITE_METHODS} advances the counter. + */ + readonly method?: keyof StacWriter; + /** Error kind to throw. Defaults to `'write-failed'`. */ + readonly kind?: StacWriterErrorKind; + /** Error message. Defaults to a synthetic, descriptive message. */ + readonly message?: string; +} + +/** + * Return a {@link StacWriter} that delegates to `base` but rejects the + * `failOnCall`-th counted write with a {@link StacWriterError}. + */ +export function createFaultInjectingWriter( + base: StacWriter, + opts: SaveFaultInjectionOptions, +): StacWriter { + const failKind: StacWriterErrorKind = opts.kind ?? 'write-failed'; + let writeCount = 0; + + const handler: ProxyHandler = { + get(target, prop, receiver): unknown { + const value: unknown = Reflect.get(target, prop, receiver); + if (typeof prop !== 'string' || typeof value !== 'function') { + return value; + } + const counted = + opts.method === undefined ? WRITE_METHODS.has(prop) : prop === opts.method; + const fn = value as (...args: unknown[]) => unknown; + if (!counted) { + return fn.bind(target); + } + return (...args: unknown[]): unknown => { + writeCount += 1; + if (writeCount === opts.failOnCall) { + return Promise.reject( + new StacWriterError( + failKind, + opts.message ?? + `saveFaultInjection: simulated ${failKind} on ${prop} (call #${writeCount})`, + { path: 'fault-injection' }, + ), + ); + } + return fn.apply(target, args); + }; + }, + }; + + return new Proxy(base, handler); +} diff --git a/apps/vscode/tests/unit/openPlot.reconcile.test.ts b/apps/vscode/tests/unit/openPlot.reconcile.test.ts new file mode 100644 index 000000000..d9bf23753 --- /dev/null +++ b/apps/vscode/tests/unit/openPlot.reconcile.test.ts @@ -0,0 +1,105 @@ +/** + * @vitest-environment node + * + * Open-path reconcile integration (#268 US3). Seeds an "interrupted save" + * fixture (staged temps + journal) in a real store, runs the open-path + * reconcile hook with a REAL `stacWriterFs`, and asserts the plot is healed to + * a single coherent state before the read and that the recovery notice fires + * exactly once. Also asserts a clean plot opens silently. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { reconcileBeforeOpen } from '../../src/commands/reconcileOnOpen'; +import { createStacWriterFs } from '../../src/services/stacWriterFs'; +import { StacService } from '../../src/services/stacService'; +import { + SAVE_JOURNAL_FILENAME, + SAVE_JOURNAL_VERSION, + type SaveJournal, +} from '../../src/services/saveJournal'; + +const ITEM_REL = 'core--boat1/item.json'; +const tmpTag = '0011223344556677'; + +describe('reconcileBeforeOpen — open-path integration (#268 US3)', () => { + let storePath: string; + let itemDir: string; + let itemJson: string; + let featuresPath: string; + + beforeEach(() => { + storePath = fs.mkdtempSync(path.join(os.tmpdir(), 'debrief-openrec-')); + itemDir = path.join(storePath, 'core--boat1'); + fs.mkdirSync(itemDir, { recursive: true }); + itemJson = path.join(itemDir, 'item.json'); + featuresPath = path.join(itemDir, 'features.geojson'); + fs.writeFileSync(itemJson, 'ITEM_V1\n'); + fs.writeFileSync(featuresPath, 'FC_V1\n'); + }); + + afterEach(() => { + fs.rmSync(storePath, { recursive: true, force: true }); + }); + + const getStacWriter = (sp: string) => createStacWriterFs({ storePath: sp, stacService: new StacService() }); + + it('rolls a committed-but-unapplied save forward and notifies once', async () => { + // Interrupted AFTER the commit point: staged temps + journal present. + fs.writeFileSync(path.join(itemDir, `features.geojson.save-${tmpTag}.tmp`), 'FC_V2\n'); + fs.writeFileSync(path.join(itemDir, `item.json.save-${tmpTag}.tmp`), 'ITEM_V2\n'); + const journal: SaveJournal = { + version: SAVE_JOURNAL_VERSION, + stacItemPath: ITEM_REL, + createdAtMs: 1_700_000_000_000, + renames: [ + { temp: `features.geojson.save-${tmpTag}.tmp`, final: 'features.geojson' }, + { temp: `item.json.save-${tmpTag}.tmp`, final: 'item.json' }, + ], + }; + fs.writeFileSync(path.join(itemDir, SAVE_JOURNAL_FILENAME), `${JSON.stringify(journal)}\n`); + + const showWarning = vi.fn(); + const result = await reconcileBeforeOpen(getStacWriter, storePath, ITEM_REL, showWarning); + + expect(result?.outcome).toBe('rolled-forward'); + // The read that follows in openPlot will now see the coherent new version. + expect(fs.readFileSync(featuresPath, 'utf8')).toBe('FC_V2\n'); + expect(fs.readFileSync(itemJson, 'utf8')).toBe('ITEM_V2\n'); + expect(fs.readdirSync(itemDir).filter((f) => f.endsWith('.tmp'))).toEqual([]); + expect(fs.existsSync(path.join(itemDir, SAVE_JOURNAL_FILENAME))).toBe(false); + expect(showWarning).toHaveBeenCalledTimes(1); + expect(showWarning.mock.calls[0]?.[0]).toMatch(/Recovered an interrupted save/); + }); + + it('restores the last-good version when interrupted before the commit point', async () => { + // Staged temps but NO journal → pre-commit → roll back to last-good. + fs.writeFileSync(path.join(itemDir, `features.geojson.save-${tmpTag}.tmp`), 'FC_V2\n'); + + const showWarning = vi.fn(); + const result = await reconcileBeforeOpen(getStacWriter, storePath, ITEM_REL, showWarning); + + expect(result?.outcome).toBe('rolled-back'); + expect(fs.readFileSync(featuresPath, 'utf8')).toBe('FC_V1\n'); // last-good + expect(fs.readdirSync(itemDir).filter((f) => f.endsWith('.tmp'))).toEqual([]); + expect(showWarning).toHaveBeenCalledTimes(1); + }); + + it('opens a clean plot silently (no notice, no mutation)', async () => { + const showWarning = vi.fn(); + const result = await reconcileBeforeOpen(getStacWriter, storePath, ITEM_REL, showWarning); + + expect(result).toEqual({ recovered: false, outcome: 'clean' }); + expect(showWarning).not.toHaveBeenCalled(); + expect(fs.readFileSync(featuresPath, 'utf8')).toBe('FC_V1\n'); + }); + + it('is a no-op when no writer factory is provided', async () => { + const showWarning = vi.fn(); + const result = await reconcileBeforeOpen(undefined, storePath, ITEM_REL, showWarning); + expect(result).toBeNull(); + expect(showWarning).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/vscode/tests/unit/saveSession.commit.test.ts b/apps/vscode/tests/unit/saveSession.commit.test.ts new file mode 100644 index 000000000..f3a939810 --- /dev/null +++ b/apps/vscode/tests/unit/saveSession.commit.test.ts @@ -0,0 +1,139 @@ +/** + * @vitest-environment node + * + * saveSession integration (#268 US1) — a writer failure during the commit + * leaves the previously-persisted plot intact and openable, with no partial + * state and no false success. Drives the REAL `createSaveSessionCommand` with a + * REAL `stacWriterFs` wrapped in the shared fault-injection helper. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import * as vscode from 'vscode'; +import { createSaveSessionCommand } from '../../src/commands/saveSession'; +import { createStacWriterFs } from '../../src/services/stacWriterFs'; +import { StacService } from '../../src/services/stacService'; +import { createFaultInjectingWriter } from './helpers/saveFaultInjection'; +import type { SessionManager } from '../../src/services/sessionManager'; +import type { MapPanel } from '../../src/webview/mapPanel'; + +const PLOT_URI = 'stac://my-store/core--boat1/item.json'; + +function viewStateStub(): Record { + return { + currentTime: null, + timeRange: null, + timeFilter: null, + stepSize: { value: 1, unit: 'minute' }, + playbackRate: 1, + displayMode: 'full', + viewport: null, + rotation: 0, + featureCollectionUri: null, + selection: { featureIds: [], primary: null, timestamp: { epoch: 0, iso: '1970-01-01T00:00:00.000Z' } }, + hiddenFeatureIds: [], + markClean: vi.fn(), + }; +} + +const V1 = { + type: 'FeatureCollection', + features: [{ type: 'Feature', id: 't1', geometry: { type: 'Point', coordinates: [0, 0] }, properties: { kind: 'TRACK', name: 'v1' } }], +}; +const V2_FEATURES = [ + { type: 'Feature', id: 't2', geometry: { type: 'Point', coordinates: [3, 4] }, properties: { kind: 'TRACK', name: 'v2' } }, +]; + +describe('saveSession integration — writer failure preserves the previous plot (#268 US1)', () => { + let storePath: string; + let itemJson: string; + let featuresPath: string; + + beforeEach(() => { + vi.clearAllMocks(); + storePath = fs.mkdtempSync(path.join(os.tmpdir(), 'debrief-savesess-commit-')); + fs.mkdirSync(path.join(storePath, 'core--boat1'), { recursive: true }); + itemJson = path.join(storePath, 'core--boat1', 'item.json'); + featuresPath = path.join(storePath, 'core--boat1', 'features.geojson'); + fs.writeFileSync( + itemJson, + `${JSON.stringify({ type: 'Feature', stac_version: '1.1.0', id: 'boat1', geometry: { type: 'Point', coordinates: [0, 0] }, bbox: [0, 0, 0, 0], properties: { datetime: '2024-01-01T00:00:00Z', title: 'Boat 1' }, links: [], assets: {} }, null, 2)}\n`, + ); + fs.writeFileSync(featuresPath, `${JSON.stringify(V1, null, 2)}\n`); + }); + + afterEach(() => { + fs.rmSync(storePath, { recursive: true, force: true }); + }); + + it('a rejected commit surfaces a failure, keeps the plot dirty, and leaves both files byte-identical', async () => { + const itemBefore = fs.readFileSync(itemJson); + const fcBefore = fs.readFileSync(featuresPath); + + const state = viewStateStub(); + const sessionManager = { + getActiveSession: () => ({ getState: () => state }), + getActiveDocumentUri: () => PLOT_URI, + } as unknown as SessionManager; + const mapPanel = { + requestThumbnailCapture: vi.fn().mockResolvedValue({ largePngBase64: '', smallPngBase64: '' }), + getCurrentFeatures: vi.fn().mockReturnValue(V2_FEATURES), + } as unknown as MapPanel; + + // Real fs adaptor, wrapped so commitPlotSave rejects as if the store were + // read-only — the previously-persisted plot must be left untouched. + const realWriter = createStacWriterFs({ storePath, stacService: new StacService() }); + const faultyWriter = createFaultInjectingWriter(realWriter, { + method: 'commitPlotSave', + failOnCall: 1, + kind: 'read-only-fs', + message: 'simulated read-only filesystem', + }); + + const command = createSaveSessionCommand( + sessionManager, + () => storePath, + () => mapPanel, + () => faultyWriter, + ); + await command(); + + // Failure surfaced, no success, dirty retained. + const showErr = vscode.window.showErrorMessage as unknown as ReturnType; + expect(showErr).toHaveBeenCalledTimes(1); + expect(showErr.mock.calls[0]?.[0]).toMatch(/Failed to save plot/); + expect(state.markClean as ReturnType).not.toHaveBeenCalled(); + + // Previous version intact + openable (byte-identical) — no partial. + expect(fs.readFileSync(itemJson).equals(itemBefore)).toBe(true); + expect(fs.readFileSync(featuresPath).equals(fcBefore)).toBe(true); + expect(JSON.parse(fs.readFileSync(featuresPath, 'utf8'))).toMatchObject({ + features: [{ properties: { name: 'v1' } }], + }); + // No stray temps / journal from the aborted save. + expect(fs.readdirSync(path.join(storePath, 'core--boat1')).filter((f) => f.endsWith('.tmp') || f.endsWith('.save-journal.json'))).toEqual([]); + }); + + it('a clean save through the same path commits the new version', async () => { + const state = viewStateStub(); + const sessionManager = { + getActiveSession: () => ({ getState: () => state }), + getActiveDocumentUri: () => PLOT_URI, + } as unknown as SessionManager; + const mapPanel = { + requestThumbnailCapture: vi.fn().mockResolvedValue({ largePngBase64: '', smallPngBase64: '' }), + getCurrentFeatures: vi.fn().mockReturnValue(V2_FEATURES), + } as unknown as MapPanel; + + const realWriter = createStacWriterFs({ storePath, stacService: new StacService() }); + const command = createSaveSessionCommand(sessionManager, () => storePath, () => mapPanel, () => realWriter); + await command(); + + expect(state.markClean as ReturnType).toHaveBeenCalledTimes(1); + expect(JSON.parse(fs.readFileSync(featuresPath, 'utf8'))).toMatchObject({ + features: [{ properties: { name: 'v2' } }], + }); + }); +}); diff --git a/apps/vscode/tests/unit/saveSession.createSaveSessionCommand.test.ts b/apps/vscode/tests/unit/saveSession.createSaveSessionCommand.test.ts index 460edc85c..cec95a096 100644 --- a/apps/vscode/tests/unit/saveSession.createSaveSessionCommand.test.ts +++ b/apps/vscode/tests/unit/saveSession.createSaveSessionCommand.test.ts @@ -1,11 +1,13 @@ /** * @vitest-environment node * - * Unit tests for `createSaveSessionCommand` (Feature 242). + * Unit tests for `createSaveSessionCommand` — #268 atomic-save routing. * - * Targets the new `getStacWriter` injection point and the error-surface - * behaviour around `StacWriterError` (Article I.3 — no silent partial - * catalog writes for thumbnail persistence failures). + * The save now commits the whole unit (feature collection + optional + * thumbnails) through `StacWriter.commitPlotSave` instead of a raw + * features.geojson write followed by a separate thumbnail write. These tests + * assert the routing and the best-effort thumbnail-capture behaviour; the + * success/failure reporting ORDER is covered in saveSession.reporting.test.ts. */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; @@ -13,7 +15,11 @@ import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; import * as vscode from 'vscode'; -import { StacWriterError, type StacWriter } from '@debrief/stac-writer'; +import { + StacWriterError, + type CommitPlotSaveInput, + type StacWriter, +} from '@debrief/stac-writer'; import { createSaveSessionCommand } from '../../src/commands/saveSession'; import type { SessionManager } from '../../src/services/sessionManager'; import type { MapPanel } from '../../src/webview/mapPanel'; @@ -26,10 +32,6 @@ function makeSessionState(savePath: string): Record { return { dirty: true, savePath, - // Minimal store surface read by the feature-261 systemStateBridge during - // save (applyStateToFeatures reads the view-state slices). With timeRange / - // viewport null and an empty selection, no state.* features are written — - // the save still proceeds (FR-020) and writes features.geojson. currentTime: null, timeRange: null, timeFilter: null, @@ -46,7 +48,7 @@ function makeSessionState(savePath: string): Record { }; } -function makeSession(savePath: string): { getState: () => unknown } { +function makeSession(savePath: string): { getState: () => Record } { const state = makeSessionState(savePath); return { getState: () => state }; } @@ -64,23 +66,24 @@ function makeSessionManager(opts: { function makeMapPanel(opts: { thumbnails?: { largePngBase64: string; smallPngBase64: string }; features?: unknown[]; + captureRejects?: boolean; }): MapPanel { return { - requestThumbnailCapture: vi.fn().mockResolvedValue( - opts.thumbnails ?? { largePngBase64: '', smallPngBase64: '' }, - ), + requestThumbnailCapture: opts.captureRejects + ? vi.fn().mockRejectedValue(new Error('webview busy')) + : vi.fn().mockResolvedValue(opts.thumbnails ?? { largePngBase64: '', smallPngBase64: '' }), getCurrentFeatures: vi.fn().mockReturnValue(opts.features ?? []), } as unknown as MapPanel; } -describe('createSaveSessionCommand — getStacWriter wiring (#242)', () => { +describe('createSaveSessionCommand — #268 commitPlotSave routing', () => { let storePath: string; let savePath: string; beforeEach(() => { storePath = fs.mkdtempSync(path.join(os.tmpdir(), 'debrief-savesess-')); fs.mkdirSync(path.join(storePath, 'core--boat1'), { recursive: true }); - savePath = path.join(storePath, 'core--boat1', 'item.debrief-session'); + savePath = path.join(storePath, 'core--boat1', 'item.json'); vi.clearAllMocks(); }); @@ -88,129 +91,99 @@ describe('createSaveSessionCommand — getStacWriter wiring (#242)', () => { fs.rmSync(storePath, { recursive: true, force: true }); }); - it('routes thumbnail writes through the injected StacWriter', async () => { - const writePlotThumbnailPair = vi.fn().mockResolvedValue({ + it('commits the feature collection + thumbnails through commitPlotSave', async () => { + const commitPlotSave = vi.fn().mockResolvedValue({ + featuresPath: 'core--boat1/features.geojson', thumbnailPath: 'core--boat1/thumbnail.png', overviewPath: 'core--boat1/overview.png', }); - const writer: Partial = { writePlotThumbnailPair }; - const getStacWriter = vi.fn().mockReturnValue(writer as StacWriter); - - const sessionManager = makeSessionManager({ - session: makeSession(savePath), - plotUri: PLOT_URI, - }); - const mapPanel = makeMapPanel({ - thumbnails: { largePngBase64: LARGE_B64, smallPngBase64: SMALL_B64 }, - }); + const getStacWriter = vi.fn().mockReturnValue({ commitPlotSave } as Partial as StacWriter); + const session = makeSession(savePath); const command = createSaveSessionCommand( - sessionManager, + makeSessionManager({ session, plotUri: PLOT_URI }), () => storePath, - () => mapPanel, + () => makeMapPanel({ thumbnails: { largePngBase64: LARGE_B64, smallPngBase64: SMALL_B64 } }), getStacWriter, ); - await command(); expect(getStacWriter).toHaveBeenCalledWith(storePath); - expect(writePlotThumbnailPair).toHaveBeenCalledTimes(1); - const callArg = writePlotThumbnailPair.mock.calls[0]?.[0] as { - stacItemPath: string; - smallPngBase64: string; - largePngBase64: string; - }; - expect(callArg.stacItemPath).toBe('core--boat1/item.json'); - expect(callArg.smallPngBase64).toBe(SMALL_B64); - expect(callArg.largePngBase64).toBe(LARGE_B64); + expect(commitPlotSave).toHaveBeenCalledTimes(1); + const arg = commitPlotSave.mock.calls[0]?.[0] as CommitPlotSaveInput; + expect(arg.stacItemPath).toBe('core--boat1/item.json'); + expect(arg.featureCollection.type).toBe('FeatureCollection'); + expect(arg.thumbnails).toEqual({ largePngBase64: LARGE_B64, smallPngBase64: SMALL_B64 }); + expect((session.getState().markClean as ReturnType)).toHaveBeenCalledTimes(1); }); - it('skips the writer call when no thumbnails were captured', async () => { - const writePlotThumbnailPair = vi.fn(); - const writer: Partial = { writePlotThumbnailPair }; - const getStacWriter = vi.fn().mockReturnValue(writer as StacWriter); - - const sessionManager = makeSessionManager({ - session: makeSession(savePath), - plotUri: PLOT_URI, + it('still commits the feature collection when no thumbnails were captured', async () => { + const commitPlotSave = vi.fn().mockResolvedValue({ + featuresPath: 'core--boat1/features.geojson', + thumbnailPath: null, + overviewPath: null, }); - // Empty base64 → falsy → writer must not be invoked. - const mapPanel = makeMapPanel({}); + const getStacWriter = vi.fn().mockReturnValue({ commitPlotSave } as Partial as StacWriter); const command = createSaveSessionCommand( - sessionManager, + makeSessionManager({ session: makeSession(savePath), plotUri: PLOT_URI }), () => storePath, - () => mapPanel, + () => makeMapPanel({}), // empty base64 → no thumbnails getStacWriter, ); - await command(); - expect(writePlotThumbnailPair).not.toHaveBeenCalled(); + expect(commitPlotSave).toHaveBeenCalledTimes(1); + expect((commitPlotSave.mock.calls[0]?.[0] as CommitPlotSaveInput).thumbnails).toBeUndefined(); }); - it('surfaces StacWriterError via showErrorMessage (Article I.3)', async () => { - const writePlotThumbnailPair = vi.fn().mockRejectedValue( - new StacWriterError( - 'write-failed', - 'simulated service-write failure', - { path: 'core--boat1/item.json' }, - ), + it('surfaces a commit failure via showErrorMessage and does NOT clear the dirty flag', async () => { + const commitPlotSave = vi.fn().mockRejectedValue( + new StacWriterError('read-only-fs', 'simulated read-only filesystem', { + path: 'core--boat1/item.json', + }), ); - const writer: Partial = { writePlotThumbnailPair }; - const getStacWriter = vi.fn().mockReturnValue(writer as StacWriter); - - const sessionManager = makeSessionManager({ - session: makeSession(savePath), - plotUri: PLOT_URI, - }); - const mapPanel = makeMapPanel({ - thumbnails: { largePngBase64: LARGE_B64, smallPngBase64: SMALL_B64 }, - }); + const getStacWriter = vi.fn().mockReturnValue({ commitPlotSave } as Partial as StacWriter); + const session = makeSession(savePath); const command = createSaveSessionCommand( - sessionManager, + makeSessionManager({ session, plotUri: PLOT_URI }), () => storePath, - () => mapPanel, + () => makeMapPanel({ thumbnails: { largePngBase64: LARGE_B64, smallPngBase64: SMALL_B64 } }), getStacWriter, ); - await command(); const showErr = vscode.window.showErrorMessage as unknown as ReturnType; expect(showErr).toHaveBeenCalledTimes(1); - expect(showErr.mock.calls[0]?.[0]).toMatch(/Thumbnail save failed: simulated/); + expect(showErr.mock.calls[0]?.[0]).toMatch(/Failed to save plot: simulated read-only/); + expect((session.getState().markClean as ReturnType)).not.toHaveBeenCalled(); }); - it('treats non-StacWriterError exceptions as non-blocking (warn only)', async () => { + it('treats a thumbnail-capture failure as non-blocking and still commits FC-only', async () => { const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); - const writePlotThumbnailPair = vi.fn(); - const writer: Partial = { writePlotThumbnailPair }; - const getStacWriter = vi.fn().mockReturnValue(writer as StacWriter); - - const sessionManager = makeSessionManager({ - session: makeSession(savePath), - plotUri: PLOT_URI, + const commitPlotSave = vi.fn().mockResolvedValue({ + featuresPath: 'core--boat1/features.geojson', + thumbnailPath: null, + overviewPath: null, }); - const mapPanel = { - requestThumbnailCapture: vi - .fn() - .mockRejectedValue(new Error('webview busy')), - getCurrentFeatures: vi.fn().mockReturnValue([]), - } as unknown as MapPanel; + const getStacWriter = vi.fn().mockReturnValue({ commitPlotSave } as Partial as StacWriter); + const session = makeSession(savePath); const command = createSaveSessionCommand( - sessionManager, + makeSessionManager({ session, plotUri: PLOT_URI }), () => storePath, - () => mapPanel, + () => makeMapPanel({ captureRejects: true }), getStacWriter, ); - await command(); const showErr = vscode.window.showErrorMessage as unknown as ReturnType; expect(showErr).not.toHaveBeenCalled(); expect(warnSpy).toHaveBeenCalled(); + expect(commitPlotSave).toHaveBeenCalledTimes(1); + expect((commitPlotSave.mock.calls[0]?.[0] as CommitPlotSaveInput).thumbnails).toBeUndefined(); + expect((session.getState().markClean as ReturnType)).toHaveBeenCalledTimes(1); warnSpy.mockRestore(); }); }); diff --git a/apps/vscode/tests/unit/saveSession.reporting.test.ts b/apps/vscode/tests/unit/saveSession.reporting.test.ts new file mode 100644 index 000000000..197e08fb0 --- /dev/null +++ b/apps/vscode/tests/unit/saveSession.reporting.test.ts @@ -0,0 +1,135 @@ +/** + * @vitest-environment node + * + * saveSession honest-reporting / ordering tests (#268 US2 — contract C3, + * SC-003). Success (cleared dirty flag + "Plot saved") MUST appear only AFTER + * the whole save unit has committed; a rejected commit MUST surface a failure, + * keep the plot dirty, and show no success message. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import * as vscode from 'vscode'; +import { StacWriterError, type StacWriter } from '@debrief/stac-writer'; +import { createSaveSessionCommand } from '../../src/commands/saveSession'; +import type { SessionManager } from '../../src/services/sessionManager'; +import type { MapPanel } from '../../src/webview/mapPanel'; + +const PLOT_URI = 'stac://my-store/core--boat1/item.json'; + +function makeState(): Record { + return { + currentTime: null, + timeRange: null, + timeFilter: null, + stepSize: { value: 1, unit: 'minute' }, + playbackRate: 1, + displayMode: 'full', + viewport: null, + rotation: 0, + featureCollectionUri: null, + selection: { featureIds: [], primary: null, timestamp: { epoch: 0, iso: '1970-01-01T00:00:00.000Z' } }, + hiddenFeatureIds: [], + markClean: vi.fn(), + }; +} + +function build(opts: { + storePath: string; + commitPlotSave: ReturnType; + state: Record; +}) { + const sessionManager = { + getActiveSession: () => ({ getState: () => opts.state }), + getActiveDocumentUri: () => PLOT_URI, + } as unknown as SessionManager; + const mapPanel = { + requestThumbnailCapture: vi.fn().mockResolvedValue({ largePngBase64: '', smallPngBase64: '' }), + getCurrentFeatures: vi.fn().mockReturnValue([]), + } as unknown as MapPanel; + const getStacWriter = vi + .fn() + .mockReturnValue({ commitPlotSave: opts.commitPlotSave } as Partial as StacWriter); + return createSaveSessionCommand(sessionManager, () => opts.storePath, () => mapPanel, getStacWriter); +} + +describe('saveSession reporting order (#268 US2)', () => { + let storePath: string; + + beforeEach(() => { + vi.clearAllMocks(); + storePath = fs.mkdtempSync(path.join(os.tmpdir(), 'debrief-report-')); + fs.mkdirSync(path.join(storePath, 'core--boat1'), { recursive: true }); + }); + + afterEach(() => { + fs.rmSync(storePath, { recursive: true, force: true }); + }); + + it('markClean + "Plot saved" fire strictly AFTER commitPlotSave resolves', async () => { + const commitPlotSave = vi.fn().mockResolvedValue({ + featuresPath: 'core--boat1/features.geojson', + thumbnailPath: null, + overviewPath: null, + }); + const state = makeState(); + await build({ storePath, commitPlotSave, state })(); + + const markClean = state.markClean as ReturnType; + const showInfo = vscode.window.showInformationMessage as unknown as ReturnType; + expect(commitPlotSave).toHaveBeenCalledTimes(1); + expect(markClean).toHaveBeenCalledTimes(1); + expect(showInfo).toHaveBeenCalledWith('Plot saved'); + + // Strict ordering via global mock invocation order. + const commitOrder = commitPlotSave.mock.invocationCallOrder[0]!; + const markCleanOrder = markClean.mock.invocationCallOrder[0]!; + const successOrder = showInfo.mock.invocationCallOrder[0]!; + expect(commitOrder).toBeLessThan(markCleanOrder); + expect(markCleanOrder).toBeLessThan(successOrder); + }); + + it('a rejected commit shows a failure, keeps dirty, and shows NO success (SC-003)', async () => { + const commitPlotSave = vi + .fn() + .mockRejectedValue(new StacWriterError('write-failed', 'disk full', { path: 'core--boat1/item.json' })); + const state = makeState(); + await build({ storePath, commitPlotSave, state })(); + + const markClean = state.markClean as ReturnType; + const showInfo = vscode.window.showInformationMessage as unknown as ReturnType; + const showErr = vscode.window.showErrorMessage as unknown as ReturnType; + + expect(markClean).not.toHaveBeenCalled(); + // No success message for a save that did not fully commit (SC-003). + expect(showInfo.mock.calls.find((c) => c[0] === 'Plot saved')).toBeUndefined(); + expect(showErr).toHaveBeenCalledTimes(1); + expect(showErr.mock.calls[0]?.[0]).toMatch(/Failed to save plot: disk full/); + }); + + it('does not show success until the (slow) commit settles', async () => { + let resolveCommit: (() => void) | undefined; + const gate = new Promise((r) => { + resolveCommit = r; + }); + const commitPlotSave = vi.fn().mockImplementation(async () => { + await gate; + return { featuresPath: 'core--boat1/features.geojson', thumbnailPath: null, overviewPath: null }; + }); + const state = makeState(); + const pending = build({ storePath, commitPlotSave, state })(); + + // Let microtasks drain — the commit is still gated (unresolved). + await Promise.resolve(); + const showInfo = vscode.window.showInformationMessage as unknown as ReturnType; + expect(state.markClean as ReturnType).not.toHaveBeenCalled(); + expect(showInfo.mock.calls.find((c) => c[0] === 'Plot saved')).toBeUndefined(); + + resolveCommit?.(); + await pending; + expect(state.markClean as ReturnType).toHaveBeenCalledTimes(1); + expect(showInfo).toHaveBeenCalledWith('Plot saved'); + }); +}); diff --git a/apps/vscode/tests/unit/stacWriterFs.commitPlotSave.test.ts b/apps/vscode/tests/unit/stacWriterFs.commitPlotSave.test.ts new file mode 100644 index 000000000..766a4d43b --- /dev/null +++ b/apps/vscode/tests/unit/stacWriterFs.commitPlotSave.test.ts @@ -0,0 +1,244 @@ +/** + * @vitest-environment node + * + * FS `commitPlotSave` atomicity tests (#268, US1 — contract C1/C2). + * + * Drives the real fs adaptor against a temp dir and injects a failure at each + * distinct phase of the four-phase commit (stage → journal → apply). Asserts: + * - pre-commit failure (stage / journal) → originals byte-identical, no stray + * `.tmp`, no journal (C1 / FR-001 / FR-010); + * - success → features.geojson + item.json (thumbnail asset entries) + both + * PNGs all reflect the new state (C2 / FR-002); + * - apply-phase failure (post-commit) → the journal REMAINS so the next open + * can roll forward (the reconcile half is proven in stacWriterFs.reconcile). + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; + +// Make fs exports assignable so we can swap writeFileSync / renameSync to throw. +vi.mock('fs', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, default: actual }; +}); + +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { StacWriterError } from '@debrief/stac-writer'; +import type { + RawGeoJSONFeatureCollection, + StoreContext, +} from '@debrief/stac-writer'; +import { createStacWriterFs } from '../../src/services/stacWriterFs'; +import { StacService } from '../../src/services/stacService'; +import { SAVE_JOURNAL_FILENAME } from '../../src/services/saveJournal'; + +const ITEM_REL = 'core--boat1/item.json'; +// 1x1 transparent PNGs — distinct bytes for large vs small so we can assert. +const SMALL_PNG = Buffer.from( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII=', + 'base64', +); +const LARGE_PNG = Buffer.concat([SMALL_PNG, Buffer.from('LARGE')]); + +const ctx: StoreContext = { + kind: 'fs', + nowMs: () => 1_700_000_000_000, + randomId: () => 'test-id', +}; + +function newFc(label: string): RawGeoJSONFeatureCollection { + return { + type: 'FeatureCollection', + features: [ + { + type: 'Feature', + id: `t-${label}`, + geometry: { type: 'Point', coordinates: [1, 2] }, + properties: { kind: 'TRACK', name: label }, + }, + ], + }; +} + +function originalItem(): Record { + return { + type: 'Feature', + stac_version: '1.1.0', + id: 'boat1', + geometry: { type: 'Point', coordinates: [0, 0] }, + bbox: [0, 0, 0, 0], + properties: { datetime: '2024-01-01T00:00:00Z', title: 'Boat 1' }, + links: [{ rel: 'self', href: './item.json' }], + assets: { data: { href: './features.geojson', type: 'application/geo+json', roles: ['data'] } }, + }; +} + +describe('stacWriterFs.commitPlotSave (#268 US1)', () => { + let storePath: string; + let itemDir: string; + let itemJson: string; + let featuresPath: string; + let realWriteFileSync: typeof fs.writeFileSync; + let realRenameSync: typeof fs.renameSync; + + beforeEach(() => { + storePath = fs.mkdtempSync(path.join(os.tmpdir(), 'debrief-commit-')); + itemDir = path.join(storePath, 'core--boat1'); + fs.mkdirSync(itemDir, { recursive: true }); + itemJson = path.join(itemDir, 'item.json'); + featuresPath = path.join(itemDir, 'features.geojson'); + fs.writeFileSync(itemJson, `${JSON.stringify(originalItem(), null, 2)}\n`); + fs.writeFileSync(featuresPath, `${JSON.stringify(newFc('original'), null, 2)}\n`); + realWriteFileSync = fs.writeFileSync; + realRenameSync = fs.renameSync; + }); + + afterEach(() => { + (fs as { writeFileSync: typeof fs.writeFileSync }).writeFileSync = realWriteFileSync; + (fs as { renameSync: typeof fs.renameSync }).renameSync = realRenameSync; + vi.restoreAllMocks(); + fs.rmSync(storePath, { recursive: true, force: true }); + }); + + function writer() { + return createStacWriterFs({ storePath, stacService: new StacService() }); + } + + function leftoverTemps(): string[] { + return fs.readdirSync(itemDir).filter((f) => f.endsWith('.tmp')); + } + + function hasJournal(): boolean { + return fs.existsSync(path.join(itemDir, SAVE_JOURNAL_FILENAME)); + } + + it('C2 — success without thumbnails writes only features.geojson, no leftovers', async () => { + const result = await writer().commitPlotSave({ + ctx, + stacItemPath: ITEM_REL, + featureCollection: newFc('v2'), + }); + + const written = JSON.parse(fs.readFileSync(featuresPath, 'utf8')) as RawGeoJSONFeatureCollection; + expect(written.features[0]?.properties).toMatchObject({ name: 'v2' }); + expect(result.featuresPath).toBe('core--boat1/features.geojson'); + expect(result.thumbnailPath).toBeNull(); + expect(result.overviewPath).toBeNull(); + // item.json untouched when no thumbnails. + expect(JSON.parse(fs.readFileSync(itemJson, 'utf8'))).toMatchObject({ id: 'boat1' }); + expect(leftoverTemps()).toEqual([]); + expect(hasJournal()).toBe(false); + }); + + it('C2 — success with thumbnails commits FC + PNGs + item.json asset entries', async () => { + const result = await writer().commitPlotSave({ + ctx, + stacItemPath: ITEM_REL, + featureCollection: newFc('v2'), + thumbnails: { + smallPngBase64: SMALL_PNG.toString('base64'), + largePngBase64: LARGE_PNG.toString('base64'), + }, + }); + + expect((JSON.parse(fs.readFileSync(featuresPath, 'utf8')) as RawGeoJSONFeatureCollection).features[0]?.properties).toMatchObject({ name: 'v2' }); + expect(fs.readFileSync(path.join(itemDir, 'thumbnail.png')).equals(SMALL_PNG)).toBe(true); + expect(fs.readFileSync(path.join(itemDir, 'overview.png')).equals(LARGE_PNG)).toBe(true); + + const item = JSON.parse(fs.readFileSync(itemJson, 'utf8')) as { assets: Record }; + expect(item.assets.thumbnail?.href).toBe('./thumbnail.png'); + expect(item.assets.overview?.href).toBe('./overview.png'); + expect(item.assets.thumbnail?.['file:size']).toBe(SMALL_PNG.byteLength); + + expect(result.thumbnailPath).toBe('core--boat1/thumbnail.png'); + expect(result.overviewPath).toBe('core--boat1/overview.png'); + expect(leftoverTemps()).toEqual([]); + expect(hasJournal()).toBe(false); + }); + + it('C1 — stage failure rolls back: originals byte-identical, no temps, no journal', async () => { + const itemBefore = fs.readFileSync(itemJson); + const fcBefore = fs.readFileSync(featuresPath); + + // Throw while staging the features.geojson temp (pre-commit). + (fs as { writeFileSync: typeof fs.writeFileSync }).writeFileSync = (( + target: fs.PathOrFileDescriptor, + data: string | NodeJS.ArrayBufferView, + opts?: fs.WriteFileOptions, + ) => { + if (String(target).includes('features.geojson.') && String(target).endsWith('.tmp')) { + throw Object.assign(new Error('synthetic stage failure'), { code: 'ENOSPC' }); + } + return realWriteFileSync(target, data, opts); + }) as typeof fs.writeFileSync; + + await expect( + writer().commitPlotSave({ ctx, stacItemPath: ITEM_REL, featureCollection: newFc('v2') }), + ).rejects.toBeInstanceOf(StacWriterError); + + expect(fs.readFileSync(itemJson).equals(itemBefore)).toBe(true); + expect(fs.readFileSync(featuresPath).equals(fcBefore)).toBe(true); + expect(leftoverTemps()).toEqual([]); + expect(hasJournal()).toBe(false); + }); + + it('C1 — journal-write failure rolls back: originals intact, no temps, no journal', async () => { + const itemBefore = fs.readFileSync(itemJson); + const fcBefore = fs.readFileSync(featuresPath); + + // Let staging succeed; throw when renaming the journal temp into place + // (the commit point). Originals are still untouched at this instant. + (fs as { renameSync: typeof fs.renameSync }).renameSync = (( + from: fs.PathLike, + to: fs.PathLike, + ) => { + if (String(to).endsWith(SAVE_JOURNAL_FILENAME)) { + throw Object.assign(new Error('synthetic journal failure'), { code: 'EROFS' }); + } + return realRenameSync(from, to); + }) as typeof fs.renameSync; + + await expect( + writer().commitPlotSave({ + ctx, + stacItemPath: ITEM_REL, + featureCollection: newFc('v2'), + thumbnails: { smallPngBase64: SMALL_PNG.toString('base64'), largePngBase64: LARGE_PNG.toString('base64') }, + }), + ).rejects.toBeInstanceOf(StacWriterError); + + expect(fs.readFileSync(itemJson).equals(itemBefore)).toBe(true); + expect(fs.readFileSync(featuresPath).equals(fcBefore)).toBe(true); + expect(fs.existsSync(path.join(itemDir, 'thumbnail.png'))).toBe(false); + expect(leftoverTemps()).toEqual([]); + expect(hasJournal()).toBe(false); + }); + + it('apply-phase failure leaves the journal in place for roll-forward on open', async () => { + // Let staging + journal succeed; throw on the item.json apply rename + // (post-commit). The journal must remain so reconcile can complete it. + (fs as { renameSync: typeof fs.renameSync }).renameSync = (( + from: fs.PathLike, + to: fs.PathLike, + ) => { + if (String(to).endsWith('item.json')) { + throw Object.assign(new Error('synthetic apply failure'), { code: 'EIO' }); + } + return realRenameSync(from, to); + }) as typeof fs.renameSync; + + await expect( + writer().commitPlotSave({ + ctx, + stacItemPath: ITEM_REL, + featureCollection: newFc('v2'), + thumbnails: { smallPngBase64: SMALL_PNG.toString('base64'), largePngBase64: LARGE_PNG.toString('base64') }, + }), + ).rejects.toBeInstanceOf(StacWriterError); + + // The commit point was reached: a journal remains, listing the pending + // renames, so the next open rolls forward rather than reading a partial. + expect(hasJournal()).toBe(true); + }); +}); diff --git a/apps/vscode/tests/unit/stacWriterFs.reconcile.test.ts b/apps/vscode/tests/unit/stacWriterFs.reconcile.test.ts new file mode 100644 index 000000000..bfb0042ab --- /dev/null +++ b/apps/vscode/tests/unit/stacWriterFs.reconcile.test.ts @@ -0,0 +1,157 @@ +/** + * @vitest-environment node + * + * FS `reconcilePlotSave` tests (#268, US3 — contracts C3/C5, SC-002). + * + * Seeds each mid-save leftover condition in a temp dir and asserts reconcile + * resolves it to a single coherent state, leaving no `.tmp` / journal behind: + * - temps but NO journal → rolled-back (pre-commit; originals kept) + * - journal + pending renames → rolled-forward (post-commit; new version) + * - clean → clean (no-op; idempotent) + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import type { StoreContext } from '@debrief/stac-writer'; +import { createStacWriterFs } from '../../src/services/stacWriterFs'; +import { StacService } from '../../src/services/stacService'; +import { + SAVE_JOURNAL_FILENAME, + SAVE_JOURNAL_VERSION, + type SaveJournal, +} from '../../src/services/saveJournal'; + +const ITEM_REL = 'core--boat1/item.json'; +const tmpTag = 'abcdef0123456789'; + +const ctx: StoreContext = { + kind: 'fs', + nowMs: () => 1_700_000_000_000, + randomId: () => 'test-id', +}; + +describe('stacWriterFs.reconcilePlotSave (#268 US3)', () => { + let storePath: string; + let itemDir: string; + let itemJson: string; + let featuresPath: string; + + beforeEach(() => { + storePath = fs.mkdtempSync(path.join(os.tmpdir(), 'debrief-reconcile-')); + itemDir = path.join(storePath, 'core--boat1'); + fs.mkdirSync(itemDir, { recursive: true }); + itemJson = path.join(itemDir, 'item.json'); + featuresPath = path.join(itemDir, 'features.geojson'); + fs.writeFileSync(itemJson, 'ITEM_V1\n'); + fs.writeFileSync(featuresPath, 'FC_V1\n'); + }); + + afterEach(() => { + fs.rmSync(storePath, { recursive: true, force: true }); + }); + + function writer() { + return createStacWriterFs({ storePath, stacService: new StacService() }); + } + + function tempsLeft(): string[] { + return fs.readdirSync(itemDir).filter((f) => f.endsWith('.tmp')); + } + function hasJournal(): boolean { + return fs.existsSync(path.join(itemDir, SAVE_JOURNAL_FILENAME)); + } + function writeJournal(renames: ReadonlyArray<{ temp: string; final: string }>): void { + const journal: SaveJournal = { + version: SAVE_JOURNAL_VERSION, + stacItemPath: ITEM_REL, + createdAtMs: ctx.nowMs(), + renames, + }; + fs.writeFileSync(path.join(itemDir, SAVE_JOURNAL_FILENAME), `${JSON.stringify(journal, null, 2)}\n`); + } + + it('clean store → { recovered: false, outcome: "clean" } and mutates nothing', async () => { + const result = await writer().reconcilePlotSave({ ctx, stacItemPath: ITEM_REL }); + expect(result).toEqual({ recovered: false, outcome: 'clean' }); + expect(fs.readFileSync(featuresPath, 'utf8')).toBe('FC_V1\n'); + expect(fs.readFileSync(itemJson, 'utf8')).toBe('ITEM_V1\n'); + expect(tempsLeft()).toEqual([]); + }); + + it('stray temps but NO journal → rolled-back: originals kept, temps removed', async () => { + // Interrupted BEFORE the commit point: staged temps exist, no journal. + fs.writeFileSync(path.join(itemDir, `features.geojson.save-${tmpTag}.tmp`), 'FC_V2\n'); + fs.writeFileSync(path.join(itemDir, `item.json.save-${tmpTag}.tmp`), 'ITEM_V2\n'); + + const result = await writer().reconcilePlotSave({ ctx, stacItemPath: ITEM_REL }); + + expect(result).toEqual({ recovered: true, outcome: 'rolled-back' }); + expect(fs.readFileSync(featuresPath, 'utf8')).toBe('FC_V1\n'); // last-good kept + expect(fs.readFileSync(itemJson, 'utf8')).toBe('ITEM_V1\n'); + expect(tempsLeft()).toEqual([]); + expect(hasJournal()).toBe(false); + }); + + it('journal + pending renames → rolled-forward: new version applied, journal gone', async () => { + // Interrupted AFTER the commit point: temps staged + journal present. + fs.writeFileSync(path.join(itemDir, `features.geojson.save-${tmpTag}.tmp`), 'FC_V2\n'); + fs.writeFileSync(path.join(itemDir, `item.json.save-${tmpTag}.tmp`), 'ITEM_V2\n'); + writeJournal([ + { temp: `features.geojson.save-${tmpTag}.tmp`, final: 'features.geojson' }, + { temp: `item.json.save-${tmpTag}.tmp`, final: 'item.json' }, + ]); + + const result = await writer().reconcilePlotSave({ ctx, stacItemPath: ITEM_REL }); + + expect(result).toEqual({ recovered: true, outcome: 'rolled-forward' }); + expect(fs.readFileSync(featuresPath, 'utf8')).toBe('FC_V2\n'); // new version + expect(fs.readFileSync(itemJson, 'utf8')).toBe('ITEM_V2\n'); + expect(tempsLeft()).toEqual([]); + expect(hasJournal()).toBe(false); + }); + + it('roll-forward is idempotent when some renames already applied', async () => { + // features temp already consumed (rename happened), item temp still pending. + fs.writeFileSync(featuresPath, 'FC_V2\n'); // features already applied + fs.writeFileSync(path.join(itemDir, `item.json.save-${tmpTag}.tmp`), 'ITEM_V2\n'); + writeJournal([ + { temp: `features.geojson.save-${tmpTag}.tmp`, final: 'features.geojson' }, // temp missing + { temp: `item.json.save-${tmpTag}.tmp`, final: 'item.json' }, + ]); + + const result = await writer().reconcilePlotSave({ ctx, stacItemPath: ITEM_REL }); + + expect(result).toEqual({ recovered: true, outcome: 'rolled-forward' }); + expect(fs.readFileSync(featuresPath, 'utf8')).toBe('FC_V2\n'); + expect(fs.readFileSync(itemJson, 'utf8')).toBe('ITEM_V2\n'); + expect(tempsLeft()).toEqual([]); + expect(hasJournal()).toBe(false); + }); + + it('a second reconcile after a roll-forward is a clean no-op (idempotent)', async () => { + fs.writeFileSync(path.join(itemDir, `features.geojson.save-${tmpTag}.tmp`), 'FC_V2\n'); + writeJournal([{ temp: `features.geojson.save-${tmpTag}.tmp`, final: 'features.geojson' }]); + + const first = await writer().reconcilePlotSave({ ctx, stacItemPath: ITEM_REL }); + expect(first.outcome).toBe('rolled-forward'); + + const second = await writer().reconcilePlotSave({ ctx, stacItemPath: ITEM_REL }); + expect(second).toEqual({ recovered: false, outcome: 'clean' }); + expect(fs.readFileSync(featuresPath, 'utf8')).toBe('FC_V2\n'); + }); + + it('a malformed journal is treated as no usable journal → rolled-back, leftovers cleared', async () => { + fs.writeFileSync(path.join(itemDir, `features.geojson.save-${tmpTag}.tmp`), 'FC_V2\n'); + fs.writeFileSync(path.join(itemDir, SAVE_JOURNAL_FILENAME), '{ not valid json'); + + const result = await writer().reconcilePlotSave({ ctx, stacItemPath: ITEM_REL }); + + expect(result.recovered).toBe(true); + expect(result.outcome).toBe('rolled-back'); + expect(fs.readFileSync(featuresPath, 'utf8')).toBe('FC_V1\n'); // originals kept + expect(tempsLeft()).toEqual([]); + expect(hasJournal()).toBe(false); + }); +}); diff --git a/apps/web-shell/playwright/tests/save-atomicity.spec.ts b/apps/web-shell/playwright/tests/save-atomicity.spec.ts new file mode 100644 index 000000000..d2543812b --- /dev/null +++ b/apps/web-shell/playwright/tests/save-atomicity.spec.ts @@ -0,0 +1,33 @@ +import { test, expect } from '@playwright/test'; + +/** + * #268 atomic save — web-shell happy-path smoke (FR-011 regression guard). + * + * The fault-injection guarantees (SC-001/002/003/005) are covered by + * unit/integration tests — injecting a mid-write crash is reliable only at the + * adaptor seam. This E2E proves the *normal* path is unchanged: a plot loads + * coherently, and reopening it yields the same coherent state. Opening drives + * `getPlotData`, which now runs `reconcilePlotSave` (a no-op on the IndexedDB + * host) before the read — so this also guards that the reconcile-before-read + * wiring did not regress normal loading. + */ +test.describe('#268 save atomicity — load/reopen coherence', () => { + test('a plot loads coherently and reopening yields the same coherent state', async ({ page }) => { + await page.goto('/'); + await expect(page.locator('[data-testid="stac-browser"]')).toBeVisible(); + + // Open a plot — map + GeoJSON features must render (coherent read). + await page.locator('[data-testid="exercise-list-item-row"]').first().dblclick(); + await expect(page.locator('.leaflet-container')).toBeVisible(); + await expect(page.locator('.leaflet-interactive').first()).toBeVisible({ timeout: 5000 }); + + // Back to catalog, then reopen the same plot — reconcile-before-read must + // still produce the coherent plot (no torn state, features intact). + await page.locator('.web-shell__back-button[aria-label="Back to catalog"]').click(); + await expect(page.locator('[data-testid="stac-browser"]')).toBeVisible(); + + await page.locator('[data-testid="exercise-list-item-row"]').first().dblclick(); + await expect(page.locator('.leaflet-container')).toBeVisible(); + await expect(page.locator('.leaflet-interactive').first()).toBeVisible({ timeout: 5000 }); + }); +}); diff --git a/apps/web-shell/src/mocks/stacService.ts b/apps/web-shell/src/mocks/stacService.ts index 794388ac1..589d7adf3 100644 --- a/apps/web-shell/src/mocks/stacService.ts +++ b/apps/web-shell/src/mocks/stacService.ts @@ -10,7 +10,12 @@ */ import type { CatalogOverviewItem } from '@debrief/components'; -import type { PlatformRecord, StacCatalog, StacItem } from '@debrief/schemas'; +import type { + PlatformRecord, + RawGeoJSONFeatureCollection, + StacCatalog, + StacItem, +} from '@debrief/schemas'; import type { FeatureCollection } from 'geojson'; import { getActiveStacWriter } from '../services/stacWriterRegistry'; @@ -306,6 +311,26 @@ export function createMockStacService(): MockStacService { }, async getPlotData(itemPath: string): Promise { + // #268 — reconcile any interrupted save BEFORE the read. On the browser + // host IndexedDB transactions are atomic (a tab kill discards an + // uncommitted transaction), so this is a clean no-op; it honours the + // boundary contract symmetrically with the desktop host and surfaces a + // change notification if a future orphan-prune ever recovers state. + const reconcileWriter = getActiveStacWriter(); + if (reconcileWriter) { + try { + const reconciliation = await reconcileWriter.reconcilePlotSave({ + ctx: { kind: 'idb', nowMs: () => Date.now(), randomId: () => '' }, + stacItemPath: itemPath, + }); + if (reconciliation.recovered) { + for (const listener of listeners) listener(itemPath); + } + } catch { + // Reconcile must never block opening a plot. + } + } + // Check cache first (includes bundled fallback data) const cached = geojsonCache.get(itemPath); if (cached) return cached; @@ -443,17 +468,18 @@ export function createMockStacService(): MockStacService { randomId: () => id, }; try { - // Both the local StacItem and @debrief/stac-writer.StacItem now - // reference @debrief/schemas.StacItem (spec #223 Decision 1B); - // no projection cast required. - await writer.writeItem({ ctx, itemPath, item, mode: 'create' }); - await writer.writeAsset({ + // #268 — commit the item record + geojson payload in ONE IndexedDB + // transaction (was a writeItem + writeAsset pair across two separate + // transactions, the web-shell save-atomicity gap). `input.geojson` is + // the `geojson`-package FeatureCollection (nullable geometry); + // commitPlotSave only serialises it, so we cross the parse boundary. + // (The rich `item` below still drives the in-memory session catalog; + // the store record commitPlotSave synthesises is a minimal standalone — + // this create path has no UI callers today.) + await writer.commitPlotSave({ ctx, - itemPath, - assetHref: './data.geojson', - body: JSON.stringify(input.geojson), - mediaType: 'application/geo+json', - assetEntry: { key: 'data', roles: ['data'], title: 'GeoJSON payload' }, + stacItemPath: itemPath, + featureCollection: input.geojson as RawGeoJSONFeatureCollection, }); // Register in the in-memory catalog so the next render pass shows it. itemMap.set(itemPath, item); diff --git a/apps/web-shell/src/services/__tests__/stacWriterIdb.commitPlotSave.test.ts b/apps/web-shell/src/services/__tests__/stacWriterIdb.commitPlotSave.test.ts new file mode 100644 index 000000000..b2dd6a502 --- /dev/null +++ b/apps/web-shell/src/services/__tests__/stacWriterIdb.commitPlotSave.test.ts @@ -0,0 +1,177 @@ +/** + * IDB `commitPlotSave` atomicity tests (#268, US1 — contract C4). + * + * Uses `fake-indexeddb`. Asserts: + * - a single commitPlotSave performs exactly ONE readwrite transaction over + * the item + payload stores (C4); + * - aborting that transaction leaves the store byte-identical to before + * (atomic — the item record and payload land together or not at all); + * - a successful commit writes the item record + geojson payload together. + */ + +import 'fake-indexeddb/auto'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { StacWriterError } from '@debrief/stac-writer'; +import type { + RawGeoJSONFeatureCollection, + StoreContext, +} from '@debrief/stac-writer'; +import { createStacWriterIdb, type StacWriterIdb } from '../stacWriterIdb'; +import { WRITER_DB_NAME } from '../stacWriterCapability'; + +const ITEM_PATH = 'user/plot-1/item.json'; + +const ctx: StoreContext = { + kind: 'idb', + nowMs: () => 1_700_000_000_000, + randomId: () => 'id-1', +}; + +function fc(label: string): RawGeoJSONFeatureCollection { + return { + type: 'FeatureCollection', + features: [ + { + type: 'Feature', + id: `t-${label}`, + geometry: { type: 'Point', coordinates: [1, 2] }, + properties: { kind: 'TRACK', name: label }, + }, + ], + }; +} + +let openWriters: StacWriterIdb[] = []; + +async function freshWriter(): Promise { + for (const w of openWriters) { + try { + await w.close(); + } catch { + // ignore + } + } + openWriters = []; + await new Promise((resolve) => { + const req = globalThis.indexedDB.deleteDatabase(WRITER_DB_NAME); + req.onsuccess = () => resolve(); + req.onerror = () => resolve(); + req.onblocked = () => resolve(); + }); + const writer = await createStacWriterIdb({ + broadcastChannelCtor: null, + fetchBundledItem: async () => null, + }); + openWriters.push(writer); + return writer; +} + +afterEach(async () => { + vi.restoreAllMocks(); + for (const w of openWriters) { + try { + await w.close(); + } catch { + // ignore + } + } + openWriters = []; +}); + +describe('stacWriterIdb.commitPlotSave (#268 US1)', () => { + it('commits the item record + geojson payload together (create case)', async () => { + const w = await freshWriter(); + const result = await w.commitPlotSave({ + ctx, + stacItemPath: ITEM_PATH, + featureCollection: fc('v1'), + }); + + expect(result.featuresPath).toBe(`idb:${ITEM_PATH}::data`); + expect(result.thumbnailPath).toBeNull(); + expect(result.overviewPath).toBeNull(); + + const stored = await w.readStoredItem(ITEM_PATH); + expect(stored?.kind).toBe('standalone'); + expect(stored?.record.assets?.['data']?.href).toBe(`idb:${ITEM_PATH}::data`); + const payload = await w.readPayload(ITEM_PATH); + expect(JSON.parse(payload ?? 'null')).toMatchObject({ type: 'FeatureCollection' }); + expect((JSON.parse(payload ?? 'null') as RawGeoJSONFeatureCollection).features[0]?.properties) + .toMatchObject({ name: 'v1' }); + }); + + it('C4 — a save uses exactly ONE readwrite transaction touching items', async () => { + const w = await freshWriter(); + // Prime first-write bookkeeping so onFirstWrite's meta puts don't count. + await w.commitPlotSave({ ctx, stacItemPath: ITEM_PATH, featureCollection: fc('v1') }); + + const seen: Array<{ stores: string[]; mode: IDBTransactionMode }> = []; + const realTransaction = IDBDatabase.prototype.transaction; + vi.spyOn(IDBDatabase.prototype, 'transaction').mockImplementation(function ( + this: IDBDatabase, + stores: string | Iterable, + mode?: IDBTransactionMode, + options?: IDBTransactionOptions, + ) { + seen.push({ + stores: typeof stores === 'string' ? [stores] : Array.from(stores), + mode: mode ?? 'readonly', + }); + return realTransaction.call(this, stores, mode, options); + }); + + await w.commitPlotSave({ ctx, stacItemPath: ITEM_PATH, featureCollection: fc('v2') }); + + const writeTxTouchingItems = seen.filter( + (t) => t.mode === 'readwrite' && t.stores.includes('items'), + ); + expect(writeTxTouchingItems).toHaveLength(1); + // And that one transaction spans the payload store too (atomic pairing). + expect(writeTxTouchingItems[0]?.stores).toContain('payloads'); + }); + + it('C4 — aborting the commit transaction leaves the store byte-identical', async () => { + const w = await freshWriter(); + await w.commitPlotSave({ ctx, stacItemPath: ITEM_PATH, featureCollection: fc('v1') }); + + const itemBefore = JSON.stringify((await w.readStoredItem(ITEM_PATH))?.record); + const payloadBefore = await w.readPayload(ITEM_PATH); + + // Abort the transaction the moment the item put is attempted — the payload + // put queued just before is rolled back with it. + const realPut = IDBObjectStore.prototype.put; + vi.spyOn(IDBObjectStore.prototype, 'put').mockImplementation(function ( + this: IDBObjectStore, + value: unknown, + key?: IDBValidKey, + ) { + if (this.name === 'items') { + this.transaction.abort(); + throw new Error('synthetic abort: items put failed mid-commit'); + } + return realPut.call(this, value, key); + }); + + await expect( + w.commitPlotSave({ ctx, stacItemPath: ITEM_PATH, featureCollection: fc('v2') }), + ).rejects.toBeTruthy(); + + vi.restoreAllMocks(); + expect(JSON.stringify((await w.readStoredItem(ITEM_PATH))?.record)).toBe(itemBefore); + expect(await w.readPayload(ITEM_PATH)).toBe(payloadBefore); + }); + + it('rejects a non-FeatureCollection payload before any write', async () => { + const w = await freshWriter(); + await expect( + w.commitPlotSave({ + ctx, + stacItemPath: ITEM_PATH, + // @ts-expect-error — deliberately wrong type at the runtime boundary. + featureCollection: { type: 'Feature' }, + }), + ).rejects.toBeInstanceOf(StacWriterError); + expect(await w.readStoredItem(ITEM_PATH)).toBeNull(); + }); +}); diff --git a/apps/web-shell/src/services/__tests__/stacWriterIdb.reconcile.test.ts b/apps/web-shell/src/services/__tests__/stacWriterIdb.reconcile.test.ts new file mode 100644 index 000000000..091d710dc --- /dev/null +++ b/apps/web-shell/src/services/__tests__/stacWriterIdb.reconcile.test.ts @@ -0,0 +1,93 @@ +/** + * IDB `reconcilePlotSave` tests (#268, US3 — contract C5). + * + * IndexedDB transactions are atomic: a tab/browser kill discards an + * uncommitted transaction, so the browser host never has a partial save to + * reconcile. `reconcilePlotSave` is therefore a clean no-op that mutates + * nothing — proven here for both an empty store and one carrying a committed + * plot. + */ + +import 'fake-indexeddb/auto'; +import { afterEach, describe, expect, it } from 'vitest'; + +import type { + RawGeoJSONFeatureCollection, + StoreContext, +} from '@debrief/stac-writer'; +import { createStacWriterIdb, type StacWriterIdb } from '../stacWriterIdb'; +import { WRITER_DB_NAME } from '../stacWriterCapability'; + +const ITEM_PATH = 'user/plot-1/item.json'; +const ctx: StoreContext = { + kind: 'idb', + nowMs: () => 1_700_000_000_000, + randomId: () => 'id-1', +}; + +const FC: RawGeoJSONFeatureCollection = { + type: 'FeatureCollection', + features: [ + { type: 'Feature', id: 't1', geometry: { type: 'Point', coordinates: [1, 2] }, properties: { kind: 'TRACK' } }, + ], +}; + +let openWriters: StacWriterIdb[] = []; +async function freshWriter(): Promise { + for (const w of openWriters) { + try { + await w.close(); + } catch { + // ignore + } + } + openWriters = []; + await new Promise((resolve) => { + const req = globalThis.indexedDB.deleteDatabase(WRITER_DB_NAME); + req.onsuccess = () => resolve(); + req.onerror = () => resolve(); + req.onblocked = () => resolve(); + }); + const w = await createStacWriterIdb({ broadcastChannelCtor: null, fetchBundledItem: async () => null }); + openWriters.push(w); + return w; +} + +afterEach(async () => { + for (const w of openWriters) { + try { + await w.close(); + } catch { + // ignore + } + } + openWriters = []; +}); + +describe('stacWriterIdb.reconcilePlotSave (#268 US3)', () => { + it('empty store → { recovered: false, outcome: "clean" }', async () => { + const w = await freshWriter(); + const result = await w.reconcilePlotSave({ ctx, stacItemPath: ITEM_PATH }); + expect(result).toEqual({ recovered: false, outcome: 'clean' }); + }); + + it('is a no-op against a committed plot (mutates nothing)', async () => { + const w = await freshWriter(); + await w.commitPlotSave({ ctx, stacItemPath: ITEM_PATH, featureCollection: FC }); + + const itemBefore = JSON.stringify((await w.readStoredItem(ITEM_PATH))?.record); + const payloadBefore = await w.readPayload(ITEM_PATH); + + const result = await w.reconcilePlotSave({ ctx, stacItemPath: ITEM_PATH }); + expect(result).toEqual({ recovered: false, outcome: 'clean' }); + + expect(JSON.stringify((await w.readStoredItem(ITEM_PATH))?.record)).toBe(itemBefore); + expect(await w.readPayload(ITEM_PATH)).toBe(payloadBefore); + }); + + it('is idempotent (repeat calls stay clean)', async () => { + const w = await freshWriter(); + expect((await w.reconcilePlotSave({ ctx, stacItemPath: ITEM_PATH })).outcome).toBe('clean'); + expect((await w.reconcilePlotSave({ ctx, stacItemPath: ITEM_PATH })).outcome).toBe('clean'); + }); +}); diff --git a/apps/web-shell/src/services/stacWriterIdb.ts b/apps/web-shell/src/services/stacWriterIdb.ts index f4fa4e841..cd93dd9d0 100644 --- a/apps/web-shell/src/services/stacWriterIdb.ts +++ b/apps/web-shell/src/services/stacWriterIdb.ts @@ -39,6 +39,8 @@ import { openDB } from 'idb'; import type { CapabilityReport, + CommitPlotSaveInput, + CommitPlotSaveResult, DeleteAssetInput, DeleteAssetResult, DeleteItemInput, @@ -46,6 +48,8 @@ import type { PatchItemInput, PatchItemResult, PropertiesProvenanceEntry, + ReconcilePlotSaveInput, + ReconcilePlotSaveResult, StacItem, StacWriter, StoredItem, @@ -643,6 +647,112 @@ export async function createStacWriterIdb( ); }, + async commitPlotSave( + input: CommitPlotSaveInput, + ): Promise { + pathGuard('commitPlotSave.stacItemPath', input.stacItemPath); + if (input.featureCollection.type !== 'FeatureCollection') { + throw new StacWriterError( + 'validation-failed', + 'commitPlotSave: featureCollection.type must be "FeatureCollection"', + { path: input.stacItemPath }, + ); + } + // The browser host does not write plot thumbnails (writePlotThumbnailPair + // is unsupported), so `thumbnails`, if supplied, is intentionally not + // persisted — the guarantee covers only the writes this host performs + // (FR-009 / spec Assumptions). + const itemPath = input.stacItemPath; + const payloadText = JSON.stringify(input.featureCollection); + const byteLength = new TextEncoder().encode(payloadText).length; + const dataHref = `idb:${itemPath}::data`; + + // Resolve the base item: existing record → use it; bundled → seed an + // overlay; neither → synthesise a minimal standalone from the FC. + const existing = (await db.get('items', itemPath)) as StoredItem | undefined; + let baseItem: StacItem; + let kind: 'overlay' | 'standalone'; + if (existing !== undefined) { + baseItem = existing.record; + kind = existing.kind; + } else { + const bundled = await fetchBundledItem(itemPath); + if (bundled !== null) { + baseItem = bundled; + kind = 'overlay'; + } else { + baseItem = synthStandaloneItem(itemPath, nowMs()); + kind = 'standalone'; + } + } + + const nextAssets: Record = { + ...((baseItem.assets as Record | undefined) ?? {}), + data: { + href: dataHref, + type: 'application/geo+json', + roles: ['data'], + title: 'GeoJSON payload', + }, + }; + const updated: StacItem = { + ...baseItem, + assets: nextAssets as StacItem['assets'], + }; + const stored: StoredItem = { kind, record: updated, mtimeMs: nowMs() }; + + // ── The commit point: ONE transaction over items + payloads (+ meta). + // IndexedDB aborts the whole transaction on any error / tab kill, so the + // item record and the geojson payload land together or not at all + // (research Decision 3 — atomicity for free, no journal). + const tx = db.transaction(['items', 'payloads', 'meta'], 'readwrite'); + try { + await tx.objectStore('payloads').put( + { + payload: payloadText, + mediaType: 'application/geo+json', + byteLength, + mtimeMs: nowMs(), + } satisfies PayloadRecord, + itemPath, + ); + await tx.objectStore('items').put(stored, itemPath); + await tx.done; + } catch (cause) { + // The transaction aborted (or a put failed). Observe its `done` + // rejection so it never surfaces as an unhandled rejection, then map. + await tx.done.catch(() => undefined); + throw cause instanceof StacWriterError + ? cause + : new StacWriterError( + 'write-failed', + `commitPlotSave: transaction aborted — ${cause instanceof Error ? cause.message : String(cause)}`, + { path: itemPath, cause }, + ); + } + await onFirstWrite(); + broadcast({ kind: 'item-changed', itemPath, mtimeMs: stored.mtimeMs }); + + return { + featuresPath: dataHref, + thumbnailPath: null, + overviewPath: null, + }; + }, + + // eslint-disable-next-line @typescript-eslint/require-await -- StacWriter interface mandates Promise return; IndexedDB never exposes partial transaction state. + async reconcilePlotSave( + input: ReconcilePlotSaveInput, + ): Promise { + // IndexedDB transactions are atomic and durable: a tab/browser kill + // discards any uncommitted transaction, leaving the store at the last + // committed state. There is therefore never a partial save to reconcile + // on the browser host — this is a no-op (FR-007/FR-008). See #268 ADR-039 + // / research Decision 3. + void input; + return { recovered: false, outcome: 'clean' }; + }, + async deleteItem(input: DeleteItemInput): Promise { pathGuard('deleteItem.itemPath', input.itemPath); const existing = (await db.get('items', input.itemPath)) as @@ -777,6 +887,35 @@ async function readAssetBlobInTx( } } +/** + * Synthesise a minimal standalone {@link StacItem} for a `commitPlotSave` whose + * item does not yet exist in the store and has no bundled counterpart (the + * first-ever-save / create case). Mirrors the shape `createStandaloneItem` + * produces. Callers carrying richer metadata (title/platforms/tags/bbox) should + * pre-seed the store record; the create path here has no UI callers today. + */ +function synthStandaloneItem(itemPath: string, mtimeMs: number): StacItem { + const segments = itemPath.split('/').filter((s) => s.length > 0); + const id = segments.length >= 2 ? segments[segments.length - 2]! : itemPath; + const nowIso = new Date(mtimeMs).toISOString(); + return { + type: 'Feature', + stac_version: '1.1.0', + id, + geometry: { type: 'Point', coordinates: [0, 0] }, + bbox: [0, 0, 0, 0], + properties: { + title: 'Untitled plot', + datetime: nowIso, + 'debrief:platforms': [], + 'debrief:tags': [], + 'debrief:feature_tags': [], + }, + links: [{ rel: 'self', href: `./${id}/item.json` }], + assets: {}, + }; +} + function decodeBase64(b64: string): Uint8Array { if (typeof b64 !== 'string' || b64.length === 0) { throw new StacWriterError('empty-png', 'decodeBase64: input is empty'); diff --git a/docs/project_notes/decisions.md b/docs/project_notes/decisions.md index bee713202..a21b699e2 100644 --- a/docs/project_notes/decisions.md +++ b/docs/project_notes/decisions.md @@ -1826,3 +1826,112 @@ backlog item. `apps/web-shell/playwright/tests/storyboard-preview.spec.ts` (asserts every captured scene references both tracks and the renderer draws them). Related: ADR-011 (cast governance), ADR-033 (Article IV.5 — derived boundary types). + +### ADR-039: Atomic plot save — a write-ahead intent journal is the filesystem commit point; one IndexedDB transaction on the browser (#268, 2026-06-01) + +**Status:** Accepted. + +**Context.** A "Save" of a plot is not one write — it is several: the feature +collection (`features.geojson`), the STAC `item.json` metadata, and the +thumbnail/overview PNGs. Before this change those writes were independently +committable and reported success too early. The VS Code save did a raw +`fs.writeFileSync` of `features.geojson` (non-atomic, *bypassing* the +`StacWriter` boundary), then — *after* clearing the dirty flag and showing +"Plot saved" — wrote the thumbnails and patched `item.json` through the writer +(`saveSession.ts:124,133-134,142`). A failure or interruption between those +phases (disk full, permission denied, browser quota, process kill, power loss) +could leave any subset applied: new features with stale thumbnails, a torn +`features.geojson`, or a plot the analyst believes is saved when it is not. A +single-file atomic-write primitive (`atomicWriteSync`, temp→rename) existed, but +there was no way to commit *multiple* writes as one unit, and the FC write did +not even use the single-file primitive. The web-shell had a narrower version of +the same gap: its standalone-create path ran `writeItem()` then `writeAsset()` +in **two** separate IndexedDB transactions (`mocks/stacService.ts:449-457`). + +**Decision.** + +1. **One host-agnostic boundary operation, `commitPlotSave`.** The whole save + unit (feature collection + optional thumbnail pair, with the `item.json` + metadata they imply) is handed to a single `StacWriter.commitPlotSave(input)` + that commits it atomically. A companion `reconcilePlotSave(input)` is called + on open, *before* the read, to heal an interrupted save. Each host implements + both once against its native backend (Article IV.4). Atomicity must be + enforced at the boundary (FR-009) — a frontend orchestrating several writer + calls cannot make multiple FS renames or IDB transactions atomic. This also + moves the feature-collection write onto the boundary (FR-004 / Article IV.2). + +2. **Filesystem commit point = a write-ahead intent journal.** `commitPlotSave` + on the fs adaptor runs four phases: **stage** every artefact to a + `..tmp` temp (reusing the existing `atomicWriteSync` temp step); + **commit point** — atomically write a single `.save-journal.json` listing the + pending `temp → final` renames; **apply** the renames (POSIX `rename` is + atomic per file); **clear** the journal. The atomic creation of the journal + is the commit boundary: `reconcilePlotSave` rolls **back** before it (stray + temps, no journal → delete temps, keep originals = last-good) and **forward** + after it (journal present → re-apply pending renames idempotently, then delete + the journal = the new version). Every interruption point therefore resolves + to a single coherent plot (FR-001/FR-007/FR-008). The journal is validated on + read through a typed `parseSaveJournal()` (no `any`, Article XV.5). + +3. **Browser commit point = one multi-store IndexedDB transaction.** + `commitPlotSave` on the idb adaptor opens **one** `readwrite` transaction over + `items` + `payloads` + `assets` + `meta`, enqueues all puts, and `await + tx.done`. IndexedDB transactions are already atomic — an error aborts the + whole transaction and a tab/process kill discards an uncommitted one — so this + gives all-or-nothing for free and needs no journal. `reconcilePlotSave` + returns `clean` (IndexedDB never exposes partial transaction state). + +4. **Report success only after commit.** `markClean()` and the "Plot saved" + message move to *after* `commitPlotSave` resolves; on rejection the host + surfaces a clear failure, leaves the dirty flag set, and the previous version + is intact (FR-005/FR-006). Thumbnail *capture* stays best-effort: a capture + failure simply omits `thumbnails` from the commit; a capture *write* that + begins is part of the atomic unit. + +5. **Durability is explicitly not a goal.** Per the spec clarifications, the + guarantee is atomicity/coherence, **not** power-loss durability of the newest + save. We add no `fsync` machinery — the existing best-effort temp→rename and + the IndexedDB transaction are sufficient. On power loss a coherent *earlier* + version may open; whatever opens is never torn. + +**Boundary types are derived (Article IV.5).** `CommitPlotSaveInput.thumbnails` +is `Pick`; +`featureCollection` reuses the generated `@debrief/schemas` `FeatureCollection`. +A compile-time `Pick` guard test (`shared/stac-writer/src/__tests__/commitPlotSave.types.test.ts`) +keeps the save unit from silently dropping fields as the thumbnail input grows. + +**Alternatives rejected.** +- *Caller-side "transaction" wrapping the existing `writeFeatureCollection` + + `writePlotThumbnailPair`.* The frontend cannot make multiple FS renames or IDB + transactions atomic — this moves the seam to the wrong layer (violates FR-009). +- *Sequential temp→rename with no journal.* A crash mid-rename leaves new + `features.geojson` + old `item.json` = incoherent, and `rename` has already + destroyed the original so it cannot be rolled back. This **is** the core bug. +- *Backup-originals-then-overwrite, roll back on open.* Doubles I/O on every + save and still needs a marker to know a save was in flight; the + journal + roll-forward is simpler and cheaper. +- *Whole-item shadow directory + atomic dir swap.* Item dirs have stable paths + referenced elsewhere (catalog links, assets); swapping directories is + disruptive and heavier than a per-file journal. +- *Replicate the FS journal in the browser.* Redundant — IndexedDB already + provides transactional atomicity. + +**Scene-capture's eager FC write (`captureScene.ts`) is deferred, deliberately.** +`captureScene`'s `defaultWriteFeatureCollection` writes only `features.geojson` +(no `item.json`, no thumbnails) as an *eager, best-effort* convenience so a +captured scene survives a reload without an explicit Save; its failure path +already tells the analyst to "Run Save Session to retry", and Save Session now +routes through `commitPlotSave`. Because it writes a single file with no +cross-artefact unit to keep coherent, and threading the `storePath` / +catalog-relative split + a `StacWriter` handle into the capture command is a +larger refactor than its best-effort nature warrants, it is left on its existing +write for this delivery and tracked as follow-up tech-debt. The authoritative, +atomic write is Save Session. + +**Provenance.** Spec `specs/268-save-atomicity/`. Type contract: +`specs/268-save-atomicity/contracts/stac-writer-commit.ts`. Tests: +`apps/vscode/tests/unit/stacWriterFs.commitPlotSave.test.ts`, +`apps/vscode/tests/unit/stacWriterFs.reconcile.test.ts`, +`apps/web-shell/src/services/__tests__/stacWriterIdb.commitPlotSave.test.ts`. +Related: ADR-033 (Article IV.5 — derived boundary types), ADR-034 (sidecar +retirement — the prior save-shape change this builds on). diff --git a/shared/stac-writer/src/__tests__/commitPlotSave.types.test.ts b/shared/stac-writer/src/__tests__/commitPlotSave.types.test.ts new file mode 100644 index 000000000..e11731646 --- /dev/null +++ b/shared/stac-writer/src/__tests__/commitPlotSave.types.test.ts @@ -0,0 +1,49 @@ +/** + * Compile-time contract guard (#268, ADR-033 / Article IV.5). + * + * `CommitPlotSaveInput.thumbnails` MUST stay structurally DERIVED from + * `WritePlotThumbnailPairInput` via `Pick<>`, and `featureCollection` MUST + * reuse the generated `RawGeoJSONFeatureCollection` — never a hand-re-listed + * shape. Re-listing fields by name is the known root cause of silently-dropped + * data when the source type grows (ADR-033 / PR #623). If either boundary type + * drifts, one of the `true` assignments below resolves to `never` and this file + * fails `tsc --noEmit` (the CI typecheck gate — `tests/` is excluded from the + * package tsconfig, so the guard lives under `src/` deliberately). + * + * No runtime: this is a type-level test (vitest's glob is `tests/**`, so it is + * intentionally not executed — its failure mode is a compile error). + */ + +import type { RawGeoJSONFeatureCollection } from '@debrief/schemas'; +import type { + CommitPlotSaveInput, + WritePlotThumbnailPairInput, +} from '../interface.js'; + +/** Invariant (bidirectional) type equality. */ +type Equals = + (() => T extends A ? 1 : 2) extends () => T extends B ? 1 : 2 + ? true + : false; + +// 1. thumbnails === Pick +type _ThumbnailsStayDerived = + Equals< + NonNullable, + Pick + > extends true + ? true + : never; +const _thumbnailsStayDerived: _ThumbnailsStayDerived = true; +void _thumbnailsStayDerived; + +// 2. featureCollection reuses the generated parse-boundary FC (not re-listed). +type _FeatureCollectionReused = + Equals< + CommitPlotSaveInput['featureCollection'], + RawGeoJSONFeatureCollection + > extends true + ? true + : never; +const _featureCollectionReused: _FeatureCollectionReused = true; +void _featureCollectionReused; diff --git a/shared/stac-writer/src/index.ts b/shared/stac-writer/src/index.ts index ffb741b61..ed190b249 100644 --- a/shared/stac-writer/src/index.ts +++ b/shared/stac-writer/src/index.ts @@ -6,6 +6,8 @@ export type { CapabilityReport, + CommitPlotSaveInput, + CommitPlotSaveResult, DeleteAssetInput, DeleteAssetResult, DeleteItemInput, @@ -13,6 +15,9 @@ export type { PatchItemInput, PatchItemResult, PropertiesProvenanceEntry, + RawGeoJSONFeatureCollection, + ReconcilePlotSaveInput, + ReconcilePlotSaveResult, StacAsset, StacItem, StacWriter, diff --git a/shared/stac-writer/src/interface.ts b/shared/stac-writer/src/interface.ts index f74cd11b6..5f474ee9a 100644 --- a/shared/stac-writer/src/interface.ts +++ b/shared/stac-writer/src/interface.ts @@ -15,6 +15,10 @@ import type { PropertiesProvenanceEntry } from '@debrief/components/PropertiesPa // the same generated class, closing spec 223 Decision 1B / A-009 (no // projection cast at call sites). import type { StacAsset, StacItem } from '@debrief/schemas'; +// RawGeoJSONFeatureCollection is the generated parse-boundary FeatureCollection +// (RFC 7946 §3.3) the schema uses for STAC item payloads. Reused — not +// re-listed — by CommitPlotSaveInput.featureCollection (Article IV.5). +import type { RawGeoJSONFeatureCollection } from '@debrief/schemas'; // ─── Core context ────────────────────────────────────────────────────────── @@ -36,6 +40,9 @@ export interface StoreContext { // have to update their import paths. export type { StacAsset, StacItem }; +// Re-export the generated parse-boundary FeatureCollection so consumers can +// import it alongside CommitPlotSaveInput without a second import path. +export type { RawGeoJSONFeatureCollection }; // Re-export the LinkML-derived PropertiesProvenanceEntry so downstream // consumers see no change — same name, same import path, plus the schema @@ -129,6 +136,55 @@ export interface WritePlotThumbnailPairResult { readonly overviewPath: string; } +// ─── Atomic plot save (feature 268) ────────────────────────────────────────── +// +// `commitPlotSave` persists the whole save unit (feature collection + optional +// thumbnail pair + the item.json metadata they imply) as ONE atomic unit, and +// `reconcilePlotSave` heals an interrupted save on open. See +// specs/268-save-atomicity/contracts/stac-writer-commit.ts for the normative +// contract and behavioural assertions (C1–C5). + +export interface CommitPlotSaveInput { + readonly ctx: StoreContext; + /** Catalog-relative item path, e.g. `core--boat1/item.json`. */ + readonly stacItemPath: string; + /** Full feature collection to persist (features.geojson / geojson payload). */ + readonly featureCollection: RawGeoJSONFeatureCollection; + /** + * Thumbnail pair to commit alongside the feature collection. Omitted when + * capture was skipped (best-effort) or unsupported (web-shell host). + * Derived from the existing thumbnail-write input — not re-listed + * (Article IV.5). + */ + readonly thumbnails?: Pick< + WritePlotThumbnailPairInput, + 'largePngBase64' | 'smallPngBase64' + >; +} +export interface CommitPlotSaveResult { + /** Catalog-relative path written for the feature collection. */ + readonly featuresPath: string; + /** Catalog-relative thumbnail path, or null when none committed. */ + readonly thumbnailPath: string | null; + /** Catalog-relative overview path, or null when none committed. */ + readonly overviewPath: string | null; +} + +export interface ReconcilePlotSaveInput { + readonly ctx: StoreContext; + readonly stacItemPath: string; +} +export interface ReconcilePlotSaveResult { + /** True iff leftover state was acted on (drives the non-blocking notice). */ + readonly recovered: boolean; + /** + * `clean` — nothing to reconcile. + * `rolled-back` — pre-commit temps discarded; last-good kept (FR-008). + * `rolled-forward` — post-commit renames completed; new version (FR-007). + */ + readonly outcome: 'clean' | 'rolled-back' | 'rolled-forward'; +} + export interface DeleteItemInput { readonly ctx: StoreContext; readonly itemPath: string; @@ -159,6 +215,25 @@ export interface StacWriter { writePlotThumbnailPair( input: WritePlotThumbnailPairInput, ): Promise; + /** + * Persist the whole save unit (feature collection + optional thumbnails + + * the STAC item metadata they imply) atomically. MUST be all-or-nothing: on + * rejection no part of the new state is observable to a later reader + * (FR-001/FR-002) and the previously-persisted plot is intact + * (FR-006/FR-010). MUST route the feature-collection write through this + * boundary (FR-004). + */ + commitPlotSave(input: CommitPlotSaveInput): Promise; + /** + * Inspect the store for leftovers from an interrupted `commitPlotSave` and + * resolve them to a single coherent state WITHOUT prompting the user + * (FR-007/FR-008). MUST be called before the plot is read on open, MUST be + * idempotent, and returns whether a recovery happened so the host can show a + * non-blocking notice. + */ + reconcilePlotSave( + input: ReconcilePlotSaveInput, + ): Promise; deleteItem(input: DeleteItemInput): Promise; deleteAsset(input: DeleteAssetInput): Promise; } diff --git a/specs/268-save-atomicity/checklists/requirements.md b/specs/268-save-atomicity/checklists/requirements.md new file mode 100644 index 000000000..eb0989b23 --- /dev/null +++ b/specs/268-save-atomicity/checklists/requirements.md @@ -0,0 +1,44 @@ +# Specification Quality Checklist: Atomic (Transactional) Plot Save + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-06-01 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## UI Feature Validation *(only if User Interface Flow section present)* + +- [ ] N/A — this is a persistence/service feature with no User Interface Flow section. The save flow surfaces only success/failure notifications (already covered by FR-005/FR-006/FR-008), not a new screen, dialog, or panel. UI Feature Validation items are intentionally skipped per the checklist's own guidance. + +## Notes + +- Items marked incomplete require spec updates before `/speckit.clarify` or `/speckit.plan`. +- **UI Feature Validation items only apply if the spec contains a "User Interface Flow" section** — this spec does not, so those items are skipped (marked N/A), not failed. +- Clarifications resolved (Session 2026-06-01 — see spec `## Clarifications`): + 1. **Scope confirmed: both hosts** (desktop filesystem + browser storage), enforced at the shared persistence boundary (FR-009). + 2. **Recovery UX confirmed: auto-restore last-good + non-blocking notice** — no analyst prompt, so the feature stays non-UI (FR-008). + 3. **Reliability target confirmed: atomicity with a coherent fallback** — no guaranteed power-loss durability of the newest save (FR-005/FR-007). +- **Commit mechanism** (staging+atomic-move vs. last-good recovery file) remains deliberately deferred to `/speckit.plan`. diff --git a/specs/268-save-atomicity/contracts/save-flow.md b/specs/268-save-atomicity/contracts/save-flow.md new file mode 100644 index 000000000..f0aa5d27f --- /dev/null +++ b/specs/268-save-atomicity/contracts/save-flow.md @@ -0,0 +1,68 @@ +# Contract: Save & Reconcile-on-Open Flow + +**Feature**: 268-save-atomicity · **Phase 1** + +Behavioural contract for how the host commands drive `commitPlotSave` / +`reconcilePlotSave`. Companion to `stac-writer-commit.ts` (the type contract). + +## Save (both hosts) + +``` +analyst triggers Save + │ + ├─ host gathers FeatureCollection (view-state folded in) + best-effort thumbnail capture + │ + ├─ await writer.commitPlotSave({ ctx, stacItemPath, featureCollection, thumbnails? }) + │ │ + │ ├─ REJECTS ─▶ host shows error; dirty flag STAYS set; previous plot intact [FR-005/006/010] + │ │ + │ └─ RESOLVES ─▶ host markClean(); host shows "Plot saved" [FR-005] +``` + +Key change vs. today: `markClean()` and the success message move to **after** +`commitPlotSave` resolves (currently they fire before the thumbnail write — +`saveSession.ts:133-134`). Thumbnail *capture* failure is non-fatal: omit +`thumbnails` and still commit the feature collection. + +### VS Code — `apps/vscode/src/commands/saveSession.ts` +- Replace `storeFeatureCollection` (raw `fs.writeFileSync`) **and** `storeThumbnails` + with a single `await writer.commitPlotSave(...)`. +- The feature-collection write is now on the boundary (FR-004 / Article IV.2). + +### Web-shell — `apps/web-shell/src/mocks/stacService.ts` (save path) +- Replace the sequential `writeItem()` + `writeAsset()` (lines 449-457, two + transactions) with one `commitPlotSave(...)` (one IDB transaction). +- Bundled-plot edits via `patchItem` are already single-transaction; tasks + confirm whether they also route through `commitPlotSave` or stay as-is. + +## Reconcile-on-open (both hosts) + +``` +analyst opens plot + │ + ├─ await writer.reconcilePlotSave({ ctx, stacItemPath }) ◀── BEFORE the read + │ │ + │ ├─ { recovered:false, outcome:'clean' } ─▶ (silent) + │ └─ { recovered:true, outcome:'rolled-back'|'rolled-forward' } ─▶ non-blocking notice [FR-008] + │ + ├─ read plot data (loadPlotData / catalogReadView) ◀── now sees coherent state + └─ hydrate store +``` + +`reconcilePlotSave` MUST run **before** the read because it can change what is +on disk (roll forward applies pending renames; roll back deletes temps). + +### VS Code — `apps/vscode/src/commands/openPlot.ts` +- Insert the reconcile call **before** `loadPlotData` (`:155`). +- On `recovered`, `vscode.window.showWarningMessage('Recovered an interrupted save — opened the last good version of this plot.')` (wording finalised in tasks; must be non-modal). + +### Web-shell — `apps/web-shell/src/services/catalogReadView.ts` / `App.tsx` +- Insert reconcile before the IDB read; `outcome` is normally `clean` + (IDB transactions never expose partial state). On the rare orphan-prune, + surface the existing toast. + +## Notes +- `reconcilePlotSave` is idempotent and cheap on the common (clean) path — + it is acceptable to call on every open (FR-011: no perceptible regression). +- No new user-facing surface beyond the existing notification APIs → the + feature stays non-UI (Clarifications Q2). diff --git a/specs/268-save-atomicity/contracts/stac-writer-commit.ts b/specs/268-save-atomicity/contracts/stac-writer-commit.ts new file mode 100644 index 000000000..369a94a58 --- /dev/null +++ b/specs/268-save-atomicity/contracts/stac-writer-commit.ts @@ -0,0 +1,109 @@ +/** + * Contract: StacWriter atomic-save extension (feature 268-save-atomicity). + * + * Normative TypeScript contract for the two operations added to the + * host-agnostic `StacWriter` interface so that a plot save commits as one + * unit and an interrupted save reconciles to a coherent state on open. + * + * Browser-safe — no Node imports. Both adaptors (apps/vscode stacWriterFs, + * apps/web-shell stacWriterIdb) implement these once against their native + * backend (Constitution Article IV.4). + * + * Types are DERIVED from existing sources (Article IV.5 / XV): `thumbnails` + * is a `Pick<>` of the existing thumbnail input; `featureCollection` reuses + * the generated schema type. No fields are re-listed by name. + */ + +import type { FeatureCollection } from '@debrief/schemas'; +import type { + StoreContext, + WritePlotThumbnailPairInput, +} from '../../../shared/stac-writer/src/interface'; + +// ─── commitPlotSave ────────────────────────────────────────────────────────── + +export interface CommitPlotSaveInput { + readonly ctx: StoreContext; + /** Catalog-relative item path, e.g. `core--boat1/item.json`. */ + readonly stacItemPath: string; + /** Full feature collection to persist (features.geojson / geojson payload). */ + readonly featureCollection: FeatureCollection; + /** + * Thumbnail pair to commit alongside the feature collection. Omitted when + * capture was skipped (best-effort) or unsupported (web-shell host). + * Derived from the existing thumbnail-write input — not re-listed. + */ + readonly thumbnails?: Pick< + WritePlotThumbnailPairInput, + 'largePngBase64' | 'smallPngBase64' + >; +} + +export interface CommitPlotSaveResult { + /** Catalog-relative path written for the feature collection. */ + readonly featuresPath: string; + /** Catalog-relative thumbnail path, or null when none committed. */ + readonly thumbnailPath: string | null; + /** Catalog-relative overview path, or null when none committed. */ + readonly overviewPath: string | null; +} + +// ─── reconcilePlotSave ─────────────────────────────────────────────────────── + +export interface ReconcilePlotSaveInput { + readonly ctx: StoreContext; + readonly stacItemPath: string; +} + +export interface ReconcilePlotSaveResult { + /** True iff leftover state was acted on (drives the non-blocking notice). */ + readonly recovered: boolean; + /** + * `clean` — nothing to reconcile. + * `rolled-back` — pre-commit temps discarded; last-good kept (FR-008). + * `rolled-forward`— post-commit renames completed; new version (FR-007). + */ + readonly outcome: 'clean' | 'rolled-back' | 'rolled-forward'; +} + +// ─── interface extension ───────────────────────────────────────────────────── + +/** + * Additions to `StacWriter`. The real interface gains these two members; + * shown here standalone for the contract. + */ +export interface StacWriterAtomicSaveExt { + /** + * Persist the whole save unit (feature collection + optional thumbnails + + * the STAC item metadata they imply) atomically. MUST be all-or-nothing: + * on rejection, no part of the new state is observable to a later reader + * (FR-001/FR-002), and the previously-persisted plot is intact (FR-006/FR-010). + * MUST route the feature-collection write through this boundary (FR-004). + */ + commitPlotSave(input: CommitPlotSaveInput): Promise; + + /** + * Inspect the store for leftovers from an interrupted `commitPlotSave` and + * resolve them to a single coherent state, WITHOUT prompting the user + * (FR-007/FR-008). MUST be called before the plot is read on open. Returns + * whether a recovery happened so the host can show a non-blocking notice. + * MUST be idempotent (safe to call when nothing needs reconciling). + */ + reconcilePlotSave( + input: ReconcilePlotSaveInput, + ): Promise; +} + +// ─── Behavioural contract (asserted by acceptance tests) ───────────────────── +// +// C1 After commitPlotSave REJECTS at any internal step, a fresh read of the +// item returns exactly the pre-save state (no partial). [SC-001, FR-001] +// C2 After commitPlotSave RESOLVES, a fresh read returns the new state for +// EVERY artefact (features + item + thumbnails agree). [FR-002, US1-3] +// C3 Simulating an interruption at each phase, then calling reconcilePlotSave, +// yields a coherent read every time; pre-commit → previous, post-commit → +// new. No `.tmp` / journal remain afterwards. [SC-002, FR-007/008] +// C4 Web-shell: a single commitPlotSave performs exactly ONE IndexedDB +// transaction; aborting it leaves the store byte-identical to before. [SC-005] +// C5 reconcilePlotSave is idempotent: calling it on a clean store returns +// { recovered: false, outcome: 'clean' } and mutates nothing. diff --git a/specs/268-save-atomicity/data-model.md b/specs/268-save-atomicity/data-model.md new file mode 100644 index 000000000..9f845ad61 --- /dev/null +++ b/specs/268-save-atomicity/data-model.md @@ -0,0 +1,82 @@ +# Data Model: Atomic (Transactional) Plot Save + +**Feature**: 268-save-atomicity · **Phase 1** · **Date**: 2026-06-01 + +This feature introduces **no LinkML/schema change**. The "save unit" is composed of artefacts that already have canonical types (`FeatureCollection`, `StacItem`, thumbnail PNG assets). The new types below are boundary DTOs and one internal FS record. All derive from existing types where a source exists (Article IV.5 / XV). + +## Entities + +### 1. Save unit (conceptual) + +The complete set of artefacts written by one save of one plot — the granularity at which atomicity is guaranteed (FR-002). + +| Artefact | Backing type | VS Code (fs) | Web-shell (idb) | +|----------|--------------|--------------|-----------------| +| Feature collection | `FeatureCollection` (`@debrief/schemas`) | `features.geojson` | `payloads` store, keyed by item path | +| STAC item metadata | `StacItem` (`@debrief/schemas`) | `item.json` | `items` store | +| Thumbnail (large/overview) | PNG asset | `*.png` files | *not written* (unsupported) | +| Thumbnail (small) | PNG asset | `*.png` file | *not written* | + +The unit is **not** a stored object; it is the commit grouping. No new persisted shape for the unit itself. + +### 2. `CommitPlotSaveInput` (boundary DTO — new) + +Argument to the new `StacWriter.commitPlotSave` operation. + +| Field | Type | Required | Notes | +|-------|------|----------|-------| +| `ctx` | `StoreContext` | yes | Existing writer context (`kind`, `nowMs`, `randomId`). | +| `stacItemPath` | `string` | yes | Catalog-relative item path (e.g. `core--boat1/item.json`). Same convention as `WritePlotThumbnailPairInput.stacItemPath`. | +| `featureCollection` | `FeatureCollection` | yes | The full FC to persist as `features.geojson` / the geojson payload. Reuses the generated type — not re-listed. | +| `thumbnails` | `Pick` | no | Omitted when thumbnail capture was skipped or unsupported (web-shell). **Derived** from the existing input type. | + +**Validation**: `stacItemPath` passes the existing `pathGuard`; `featureCollection.type === 'FeatureCollection'`; when `thumbnails` present, both PNGs decode to > 0 bytes (mirrors `stacWriterFs.ts:158-165`). + +### 3. `CommitPlotSaveResult` (boundary DTO — new) + +| Field | Type | Notes | +|-------|------|-------| +| `featuresPath` | `string` | Catalog-relative path written for the feature collection. | +| `thumbnailPath` | `string \| null` | Null when no thumbnails were committed. | +| `overviewPath` | `string \| null` | Null when no thumbnails were committed. | + +### 4. `SaveJournal` (internal, FS-only — new) + +The write-ahead intent record that marks the FS commit point (Decision 2). Lives at `/.save-journal.json`, written atomically, removed on success. Never crosses the public interface; browser host does not use it. + +| Field | Type | Notes | +|-------|------|-------| +| `version` | `1` | Journal format version (forward-compat guard). | +| `stacItemPath` | `string` | Item this save belongs to. | +| `createdAtMs` | `number` | `ctx.nowMs()` at commit-point write. | +| `renames` | `Array<{ temp: string; final: string }>` | Pending `temp → final` renames, in apply order. Paths are item-dir-relative. | + +**Lifecycle / state transitions** (FS save unit): + +``` + stage temps write journal (commit point) apply renames delete journal +[clean] ───────────────▶ [staged] ──────────────────────────▶ [committed] ───────────▶ [applied] ─────────▶ [clean] + │ │ │ │ + │ interrupt │ interrupt (no journal) │ interrupt (journal) │ interrupt (journal, temps gone) + ▼ ▼ ▼ ▼ + reconcile: reconcile: delete temps, reconcile: roll FORWARD reconcile: delete + nothing to do keep originals = LAST-GOOD (apply pending) = NEW stale journal = NEW +``` + +Invariant: **before** the journal exists → reconcile yields last-good; **after** → reconcile yields the new version. Either way the result is coherent (FR-001/FR-007/FR-008). + +### 5. `ReconcileResult` (boundary DTO — new) + +Returned by `StacWriter.reconcilePlotSave`, consumed by the open path to decide whether to notify. + +| Field | Type | Notes | +|-------|------|-------| +| `recovered` | `boolean` | True iff any leftover state was acted on. Drives the non-blocking notice. | +| `outcome` | `'clean' \| 'rolled-back' \| 'rolled-forward'` | `clean` = nothing found; `rolled-back` = pre-commit temps discarded (last-good kept); `rolled-forward` = post-commit renames completed (new version). Web-shell returns `clean` (or `rolled-back` if it prunes an orphan). | + +## Type-derivation compliance (Article IV.5 / XV) + +- `thumbnails` is `Pick` — not re-listed. +- `featureCollection` reuses `@debrief/schemas` `FeatureCollection`. +- `CommitPlotSaveResult` mirrors the path-returning shape of `WritePlotThumbnailPairResult`; if it grows to mirror that type's fields, derive via `Pick`/`&` rather than re-listing. +- `SaveJournal` has no upstream source (it is a new internal record); it is validated through a typed parse on read (Article XV.5) — no `any`. diff --git a/specs/268-save-atomicity/evidence/fault-injection-matrix.md b/specs/268-save-atomicity/evidence/fault-injection-matrix.md new file mode 100644 index 000000000..7e7ac5a57 --- /dev/null +++ b/specs/268-save-atomicity/evidence/fault-injection-matrix.md @@ -0,0 +1,54 @@ +# Fault-Injection Matrix — Atomic Plot Save (#268) + +Every distinct write step of a save is driven to failure (or interruption) on +both hosts, and the plot is asserted to resolve to exactly one **coherent** +version — never a torn or mismatched mixture. This table is the credibility +artifact for SC-001 / SC-002 / SC-003 / SC-005. + +Legend: **roll-back** = previous version preserved (last-good); **roll-forward** += new version completed; **clean** = nothing to do. + +## VS Code — filesystem host (`stacWriterFs`) + +The commit runs **stage → journal (commit point) → apply → clear**. The atomic +creation of the journal is the boundary: a failure *before* it rolls back; a +failure *after* it rolls forward on the next open. + +| # | Injected failure point | Phase (vs. commit point) | `commitPlotSave` result | On-disk after failure | Reconcile-on-open outcome | Coherent? | Proven by | +|---|------------------------|--------------------------|-------------------------|-----------------------|---------------------------|-----------|-----------| +| 1 | `writeFileSync` of a staged temp | stage — pre-commit | rejects | originals byte-identical, no temps, no journal | n/a (clean) | ✅ previous | `stacWriterFs.commitPlotSave.test.ts` | +| 2 | `renameSync` of the journal into place | journal write — pre-commit | rejects | originals byte-identical, no temps, no journal | n/a (clean) | ✅ previous | `stacWriterFs.commitPlotSave.test.ts` | +| 3 | `renameSync` of `item.json` (apply) | apply — **post-commit** | rejects | journal remains, listing pending renames | **roll-forward** → new version | ✅ new | `stacWriterFs.commitPlotSave.test.ts` + `stacWriterFs.reconcile.test.ts` | +| 4 | interruption: temps staged, **no** journal | pre-commit | (process killed) | stray temps + originals | **roll-back** → temps discarded, originals kept | ✅ previous | `stacWriterFs.reconcile.test.ts` | +| 5 | interruption: journal + pending renames | post-commit | (process killed) | temps + journal | **roll-forward** → renames applied, journal dropped | ✅ new | `stacWriterFs.reconcile.test.ts` | +| 6 | interruption: journal + some renames applied | post-commit (partial) | (process killed) | one temp consumed, one pending | **roll-forward** (idempotent — skips missing temp) | ✅ new | `stacWriterFs.reconcile.test.ts` | +| 7 | malformed / unknown-version journal | corrupt commit marker | (n/a) | bad journal + temps | **roll-back** → discard bad journal + temps, keep originals | ✅ previous | `stacWriterFs.reconcile.test.ts` | +| 8 | clean store (no leftovers) | — | — | unchanged | **clean** no-op (idempotent) | ✅ | `stacWriterFs.reconcile.test.ts` | + +## Web-shell — IndexedDB host (`stacWriterIdb`) + +The commit is **one multi-store transaction** over `items` + `payloads` +(+ `meta`). IndexedDB transactions are atomic and discard uncommitted work on +abort / tab-kill, so there is never a partial save to reconcile. + +| # | Injected failure point | Mechanism | `commitPlotSave` result | Store after failure | Reconcile-on-open outcome | Coherent? | Proven by | +|---|------------------------|-----------|-------------------------|---------------------|---------------------------|-----------|-----------| +| 1 | abort mid-commit (items put) | `tx.abort()` on the `items` put | rejects | **byte-identical** to before (payload put rolled back too) | n/a | ✅ previous | `stacWriterIdb.commitPlotSave.test.ts` | +| 2 | invalid payload (not a FeatureCollection) | runtime type check | rejects before any write | nothing written | n/a | ✅ previous | `stacWriterIdb.commitPlotSave.test.ts` | +| 3 | tab/browser kill during commit | IndexedDB transaction discard | (killed) | uncommitted transaction discarded → last committed state | **clean** (no partial exists) | ✅ | by IndexedDB semantics; reconcile no-op proven in `stacWriterIdb.reconcile.test.ts` | +| 4 | clean / committed store | — | — | unchanged | **clean** no-op (idempotent) | ✅ | `stacWriterIdb.reconcile.test.ts` | + +## Single-transaction proof (SC-005) + +`stacWriterIdb.commitPlotSave.test.ts` spies on `IDBDatabase.prototype.transaction` +and asserts a save opens **exactly one** `readwrite` transaction, and that the +one transaction spans both `items` and `payloads` — the item record and the +geojson payload land together or not at all. + +## Honest-reporting proof (SC-003) + +`saveSession.reporting.test.ts` asserts (via mock `invocationCallOrder`) that +`markClean()` and the "Plot saved" message fire **strictly after** +`commitPlotSave` resolves, and that a rejected commit shows a failure, keeps the +plot dirty, and shows **no** success — so the success indication appears for 0% +of saves that did not fully commit. diff --git a/specs/268-save-atomicity/evidence/opening-context.md b/specs/268-save-atomicity/evidence/opening-context.md new file mode 100644 index 000000000..1ce97f187 --- /dev/null +++ b/specs/268-save-atomicity/evidence/opening-context.md @@ -0,0 +1,43 @@ +## Hook + +```mermaid +stateDiagram-v2 + direction LR + [*] --> Staged: write new files as temps + Staged --> Committed: write journal (atomic commit point) + Committed --> Applied: rename temps into place + Applied --> [*]: delete journal + + Staged --> RollBack: interrupted (no journal yet) + Committed --> RollForward: interrupted (journal present) + RollBack --> LastGood: discard temps, keep originals + RollForward --> NewVersion: finish the renames + + note right of Committed + One atomic write flips + the whole save from + "old" to "new" + end note +``` + +## What We're Building + +Saving a plot looks like a single action, but under the hood it is several separate writes — the feature collection that holds your tracks, the STAC metadata that describes them, and the thumbnail images that preview the plot. Today those land one at a time, and the app says "Plot saved" before the last of them is on disk. So if anything goes wrong partway through — the disk fills up, a permission is denied, the browser's storage quota is hit, or the machine simply dies mid-write — you can be left with a plot that is quietly broken: new tracks paired with last week's thumbnail, a feature file torn off mid-sentence, or a plot you believe is safe that never fully wrote. You wouldn't find out until you reopened it, and by then the good version might be gone. + +We are making a save all-or-nothing. After any save attempt, the plot on disk is observable as *either* the complete new version *or* the complete previous version — never a half-and-half. "Plot saved" appears only once the whole thing has committed; if it can't, you're told plainly, your in-editor changes are kept so you can retry, and the version already on disk is left untouched. And if a save is ever cut short by something you can't catch — a crash, an out-of-memory kill, a power loss — the next time you open the plot it quietly restores itself to the last good version and tells you, in a notice you can ignore, that it cleaned up an interrupted save. No dialog, no decision to make. The plot of record is simply never left corrupt. + +## How It Fits + +Every write in the platform already flows through one place: the host-agnostic persistence boundary that lets thick Python-style services and thin frontends agree on how data reaches storage. Both the VS Code desktop app and the in-browser web-shell route their saves through it. This work hardens that boundary so a guarantee we make once holds everywhere — instead of each frontend stitching several writes together and hoping they all land, the boundary itself accepts the whole save as one unit and commits it atomically. It also pulls the feature-collection write back *onto* the boundary; it had been slipping past it with a raw, non-atomic file write. The result is a single rule, enforced in one spot, that neither host can quietly regress — and a concrete answer to a principle the project holds as non-negotiable: no silent failures, an operation succeeds fully or fails explicitly, and you always know the true state of your data. + +## Key Decisions + +- **Make the boundary the only place atomicity can live.** We added one operation, `commitPlotSave`, that takes the entire save unit — feature collection plus thumbnails — and a companion `reconcilePlotSave` that runs when a plot is opened. A caller orchestrating several separate writes could never promise all-or-nothing across them; only the boundary, sitting on the native storage, can. Each host implements it once. + +- **On the desktop, a tiny write-ahead journal is the moment of commit.** The filesystem has no single syscall that swaps several files at once, so we stage every new file as a temp, then atomically write a small journal listing the renames still to apply — and *that one atomic write is the commit*. Then we apply the renames and delete the journal. On open, reconcile reads the situation: no journal means the save never committed, so the stray temps are discarded and the originals kept (the last good version); a journal present means it did commit, so the renames are finished (the new version). Every point you could be interrupted at resolves to a coherent plot — rolled back before the journal exists, rolled forward after. + +- **In the browser, the storage engine already does the hard part.** IndexedDB transactions are atomic: a killed tab simply throws away an uncommitted one. So we group the save's writes into a single multi-store transaction and get all-or-nothing for free — no journal needed. The only gap was that two of those writes had been running in separate transactions; now they share one. + +- **Aim for coherence, not power-loss durability of the newest save — on purpose.** If the power cuts at the wrong instant, you may get the last good version back rather than the in-flight one. What you will *never* get is a torn or mismatched plot. That is a deliberately simpler, sharper target than guaranteeing the very latest keystroke survives a crash, and it is the one that actually protects the plot of record. + +- **No new dependencies.** It reuses the temp-then-rename primitive the desktop adaptor already had and the IndexedDB transactions the browser already used — the safety comes from how they're sequenced, not from new machinery. diff --git a/specs/268-save-atomicity/evidence/test-summary.md b/specs/268-save-atomicity/evidence/test-summary.md new file mode 100644 index 000000000..c7ab5e42a --- /dev/null +++ b/specs/268-save-atomicity/evidence/test-summary.md @@ -0,0 +1,139 @@ +--- +feature: "268-save-atomicity" +captured_at: "2026-06-01T20:48:00Z" +git_sha: "040e9c5" +tests_passed: 32 +tests_failed: 0 +tests_skipped: 0 +coverage_pct: null +--- + +# Test Summary: Atomic (Transactional) Plot Save + +The credibility artifact for this reliability feature is the **fault-injection +matrix** (`fault-injection-matrix.md`): every distinct interruption point of a +save, on both hosts, is driven to failure and the plot is asserted to resolve to +exactly one coherent version. The compile-time `Pick`-derivation guard +(mutation-verified) keeps the boundary save-unit type from silently dropping +fields. + +## Results (feature-specific tests) + +| Metric | Value | +|--------|-------| +| Total new tests (#268) | 32 | +| Passed | 32 | +| Failed | 0 | +| Skipped | 0 | +| Coverage | not separately measured | + +Of the 32: 31 Vitest unit/integration + 1 web-shell Playwright smoke. One +additional **compile-time** guard (`commitPlotSave.types.test.ts`) is enforced +by `tsc --noEmit` (the CI typecheck gate), not by a runtime runner. + +## Test Breakdown + +### Boundary contract (shared/stac-writer) + +| Test | Status | +|------|--------| +| `commitPlotSave.types.test.ts` — `thumbnails` stays `Pick`; `featureCollection` reuses `RawGeoJSONFeatureCollection` (compile-time, mutation-verified) | Pass | + +### FS adaptor — commit (apps/vscode, contract C1/C2) + +| Test | Status | +|------|--------| +| success without thumbnails writes only `features.geojson`, no leftovers | Pass | +| success with thumbnails commits FC + both PNGs + `item.json` asset entries | Pass | +| stage failure → rolled back: originals byte-identical, no temps, no journal | Pass | +| journal-write failure → rolled back: originals intact, no temps, no journal | Pass | +| apply-phase failure leaves the journal for roll-forward on open | Pass | + +### FS adaptor — reconcile (apps/vscode, contract C3/C5) + +| Test | Status | +|------|--------| +| clean store → `{ recovered:false, outcome:'clean' }`, mutates nothing | Pass | +| stray temps, no journal → rolled-back (last-good kept, temps removed) | Pass | +| journal + pending renames → rolled-forward (new version, journal gone) | Pass | +| roll-forward idempotent when some renames already applied | Pass | +| second reconcile after roll-forward is a clean no-op | Pass | +| malformed journal → safe roll-back, leftovers cleared | Pass | + +### IDB adaptor — commit + reconcile (apps/web-shell, contract C4/C5) + +| Test | Status | +|------|--------| +| commits item record + geojson payload together (create case) | Pass | +| a save uses exactly ONE readwrite transaction touching `items` + `payloads` | Pass | +| aborting the commit transaction leaves the store byte-identical | Pass | +| rejects a non-FeatureCollection payload before any write | Pass | +| reconcile: empty store → clean | Pass | +| reconcile: no-op against a committed plot (mutates nothing) | Pass | +| reconcile: idempotent (repeat calls stay clean) | Pass | + +### Host call sites — saveSession + openPlot (apps/vscode) + +| Test | Status | +|------|--------| +| save routes FC + thumbnails through `commitPlotSave` | Pass | +| save still commits FC when no thumbnails captured | Pass | +| commit failure → `showErrorMessage`, dirty NOT cleared | Pass | +| thumbnail-capture failure is non-blocking, still commits FC-only | Pass | +| reporting order: `markClean` + "Plot saved" fire strictly AFTER commit resolves | Pass | +| reporting: rejected commit shows failure, keeps dirty, NO success (SC-003) | Pass | +| reporting: no success shown until a slow commit settles | Pass | +| integration: a rejected commit leaves both files byte-identical, dirty kept | Pass | +| integration: a clean save through the same path commits the new version | Pass | +| open-path: rolls a committed-but-unapplied save forward + notifies once | Pass | +| open-path: restores last-good when interrupted before the commit point | Pass | +| open-path: clean plot opens silently (no notice, no mutation) | Pass | +| open-path: no-op when no writer factory is provided | Pass | + +### E2E smoke (apps/web-shell, Playwright) + +| Test | Status | +|------|--------| +| a plot loads coherently and reopening yields the same coherent state (FR-011) | Pass | + +## Key Scenarios Verified + +- **No partial after a catchable failure (SC-001).** Injecting a failure at the + stage and journal-write phases (the pre-commit points where rollback is + guaranteed) leaves `features.geojson` + `item.json` byte-identical and no + stray `.tmp`/journal — the previous version is openable and unchanged. +- **Coherent after an uncatchable interruption (SC-002).** Seeding each + mid-save leftover condition and reconciling yields exactly one coherent + version: pre-commit → last-good, post-commit → new version, with no `.tmp`/ + journal remaining; idempotent on repeat. +- **Honest success reporting (SC-003).** `markClean` + "Plot saved" fire only + after `commitPlotSave` resolves; a rejected commit shows a failure, keeps the + plot dirty, and shows no success message. +- **Same guarantee on both backends (SC-005).** The IndexedDB host commits + item + payload in exactly one transaction; aborting it rolls both back to a + byte-identical store. + +## Full-suite regression context (same SHA) + +| Suite | Result | +|-------|--------| +| `@debrief/stac-writer` vitest | 22 passed | +| `debrief-vscode` vitest | 841 passed, 1 skipped (pre-existing, unrelated) | +| `@debrief/web-shell` vitest | 135 passed | +| `pnpm -r typecheck` + `apps/vscode tsc --noEmit` | clean | +| ESLint (all packages) | 0 errors | +| ruff / pyright | clean (no Python changes) | +| pytest | 2161 passed, 2 skipped, 1 xfailed (1 timing-flaky perf budget passes on re-run) | + +## Known Issues + +- The pre-existing vscode skipped test is unrelated to #268. +- `shared/schemas` `test_10k_feature_collection_validates_within_budget` is a + timing-sensitive performance budget (0.5s) that occasionally exceeds on a busy + cloud VM and passes on re-run — not a #268 regression (no Python changed). + +## Environment + +- Runner: vitest (unit/integration) + Playwright (E2E, `@sparticuz/chromium`) +- Branch: `claude/eloquent-fermi-bzLml` (spec `268-save-atomicity`) +- Date: 2026-06-01 diff --git a/specs/268-save-atomicity/evidence/usage-example.md b/specs/268-save-atomicity/evidence/usage-example.md new file mode 100644 index 000000000..7fa005cf2 --- /dev/null +++ b/specs/268-save-atomicity/evidence/usage-example.md @@ -0,0 +1,99 @@ +# Usage Example — Atomic Plot Save (#268) + +Two walkthroughs of the headline behaviour: a save that fails mid-write leaves +the plot intact, and an interrupted save auto-recovers on open. Both are +exercised by the test suite; the excerpts below show the flow and the asserted +outcome. + +## 1. A save that fails mid-write leaves the plot intact + +When `commitPlotSave` cannot complete, the analyst sees a failure, the plot +stays dirty (so they can retry), and the previously-persisted version is left +byte-identical — there is no partial on disk and no false "Plot saved". + +```ts +// A real fs adaptor, wrapped so commitPlotSave rejects as if the store were +// read-only (apps/vscode/tests/unit/saveSession.commit.test.ts). +const realWriter = createStacWriterFs({ storePath, stacService: new StacService() }); +const faultyWriter = createFaultInjectingWriter(realWriter, { + method: 'commitPlotSave', + failOnCall: 1, + kind: 'read-only-fs', + message: 'simulated read-only filesystem', +}); + +const command = createSaveSessionCommand( + sessionManager, () => storePath, () => mapPanel, () => faultyWriter, +); +await command(); +``` + +Expected behaviour (asserted): + +``` +showErrorMessage → "Failed to save plot: simulated read-only filesystem" +markClean() → NOT called (plot stays dirty; analyst can retry) +features.geojson → byte-identical to the previous version (v1, not v2) +item.json → byte-identical to the previous version +item directory → no *.tmp, no .save-journal.json (no partial) +``` + +Under the hood, the failure happened during the **stage** phase (or before the +journal was written), so the originals were never touched — the four-phase +commit only ever destroys the previous files *after* the atomic journal write +marks the save as committed. + +## 2. An interrupted save auto-recovers on open + +If the process is killed mid-save, the next open heals the plot **before** the +read — automatically, with a non-blocking notice, no dialog. Whether it rolls +back (to the last good version) or forward (to the new version) depends only on +whether the atomic commit point — the journal — had been written. + +```ts +// Seed an "interrupted AFTER the commit point" fixture: staged temps + journal +// (apps/vscode/tests/unit/openPlot.reconcile.test.ts). +fs.writeFileSync(`${itemDir}/features.geojson.save-${TOKEN}.tmp`, 'FC_V2\n'); +fs.writeFileSync(`${itemDir}/item.json.save-${TOKEN}.tmp`, 'ITEM_V2\n'); +fs.writeFileSync(`${itemDir}/.save-journal.json`, JSON.stringify({ + version: 1, stacItemPath, createdAtMs, + renames: [ + { temp: `features.geojson.save-${TOKEN}.tmp`, final: 'features.geojson' }, + { temp: `item.json.save-${TOKEN}.tmp`, final: 'item.json' }, + ], +})); + +// openPlot runs this BEFORE loadPlot / loadPlotData: +const result = await reconcileBeforeOpen(getStacWriter, storePath, itemPath, showWarning); +``` + +Expected behaviour (asserted): + +``` +result.outcome → 'rolled-forward' +features.geojson → 'FC_V2' (new version completed) +item.json → 'ITEM_V2' +item directory → no *.tmp, no .save-journal.json +showWarning → called once: "Recovered an interrupted save — opened the + last good version of this plot." +``` + +The mirror case — staged temps but **no** journal (interrupted *before* the +commit point) — rolls **back**: the temps are discarded, `features.geojson` +stays at the last-good `FC_V1`, and the same non-blocking notice fires. Either +way, the read that follows sees exactly one coherent plot. + +## 3. The browser host: one transaction, atomic for free + +On the web-shell, `commitPlotSave` writes the item record and the geojson +payload in a single IndexedDB transaction: + +```ts +const result = await writer.commitPlotSave({ ctx, stacItemPath, featureCollection }); +// stored.record.assets.data.href === `idb:${stacItemPath}::data` +// payload(stacItemPath) === JSON.stringify(featureCollection) +``` + +If that transaction aborts (quota, tab kill), IndexedDB rolls back **both** +puts — the store is left byte-identical to before — so `reconcilePlotSave` is a +clean no-op: the browser never exposes a partial save to reconcile. diff --git a/specs/268-save-atomicity/media/shipped-post.md b/specs/268-save-atomicity/media/shipped-post.md new file mode 100644 index 000000000..e6eede6d8 --- /dev/null +++ b/specs/268-save-atomicity/media/shipped-post.md @@ -0,0 +1,89 @@ +--- +layout: future-post +title: "Building an all-or-nothing plot save" +date: 2026-06-01 +spec: 268-save-atomicity +track: [credibility] +author: Ian +reading_time: 8 +tags: [tracer-bullet, stac-writer, persistence, reliability, atomicity] +excerpt: "A plot save is now all-or-nothing. After any save, the plot on disk is the complete new version or the complete old one — never a torn mixture." +--- + +```mermaid +stateDiagram-v2 + direction LR + [*] --> Staged: write new files as temps + Staged --> Committed: write journal (atomic commit point) + Committed --> Applied: rename temps into place + Applied --> [*]: delete journal + + Staged --> RollBack: interrupted (no journal yet) + Committed --> RollForward: interrupted (journal present) + RollBack --> LastGood: discard temps, keep originals + RollForward --> NewVersion: finish the renames + + note right of Committed + One atomic write flips + the whole save from + "old" to "new" + end note +``` + +## What We're Building + +Saving a plot looks like a single action, but under the hood it is several separate writes — the feature collection that holds your tracks, the STAC metadata that describes them, and the thumbnail images that preview the plot. Today those land one at a time, and the app says "Plot saved" before the last of them is on disk. So if anything goes wrong partway through — the disk fills up, a permission is denied, the browser's storage quota is hit, or the machine simply dies mid-write — you can be left with a plot that is quietly broken: new tracks paired with last week's thumbnail, a feature file torn off mid-sentence, or a plot you believe is safe that never fully wrote. You wouldn't find out until you reopened it, and by then the good version might be gone. + +We are making a save all-or-nothing. After any save attempt, the plot on disk is observable as *either* the complete new version *or* the complete previous version — never a half-and-half. "Plot saved" appears only once the whole thing has committed; if it can't, you're told plainly, your in-editor changes are kept so you can retry, and the version already on disk is left untouched. And if a save is ever cut short by something you can't catch — a crash, an out-of-memory kill, a power loss — the next time you open the plot it quietly restores itself to the last good version and tells you, in a notice you can ignore, that it cleaned up an interrupted save. No dialog, no decision to make. The plot of record is simply never left corrupt. + +## How It Fits + +Every write in the platform already flows through one place: the host-agnostic persistence boundary that lets thick Python-style services and thin frontends agree on how data reaches storage. Both the VS Code desktop app and the in-browser web-shell route their saves through it. This work hardens that boundary so a guarantee we make once holds everywhere — instead of each frontend stitching several writes together and hoping they all land, the boundary itself accepts the whole save as one unit and commits it atomically. It also pulls the feature-collection write back *onto* the boundary; it had been slipping past it with a raw, non-atomic file write. The result is a single rule, enforced in one spot, that neither host can quietly regress — and a concrete answer to a principle the project holds as non-negotiable: no silent failures, an operation succeeds fully or fails explicitly, and you always know the true state of your data. + +## Key Decisions + +- **Make the boundary the only place atomicity can live.** We added one operation, `commitPlotSave`, that takes the entire save unit — feature collection plus thumbnails — and a companion `reconcilePlotSave` that runs when a plot is opened. A caller orchestrating several separate writes could never promise all-or-nothing across them; only the boundary, sitting on the native storage, can. Each host implements it once. + +- **On the desktop, a tiny write-ahead journal is the moment of commit.** The filesystem has no single syscall that swaps several files at once, so we stage every new file as a temp, then atomically write a small journal listing the renames still to apply — and *that one atomic write is the commit*. Then we apply the renames and delete the journal. On open, reconcile reads the situation: no journal means the save never committed, so the stray temps are discarded and the originals kept (the last good version); a journal present means it did commit, so the renames are finished (the new version). Every point you could be interrupted at resolves to a coherent plot — rolled back before the journal exists, rolled forward after. + +- **In the browser, the storage engine already does the hard part.** IndexedDB transactions are atomic: a killed tab simply throws away an uncommitted one. So we group the save's writes into a single multi-store transaction and get all-or-nothing for free — no journal needed. The only gap was that two of those writes had been running in separate transactions; now they share one. + +- **Aim for coherence, not power-loss durability of the newest save — on purpose.** If the power cuts at the wrong instant, you may get the last good version back rather than the in-flight one. What you will *never* get is a torn or mismatched plot. That is a deliberately simpler, sharper target than guaranteeing the very latest keystroke survives a crash, and it is the one that actually protects the plot of record. + +- **No new dependencies.** It reuses the temp-then-rename primitive the desktop adaptor already had and the IndexedDB transactions the browser already used — the safety comes from how they're sequenced, not from new machinery. + +## By the Numbers + +A reliability feature is only as good as its proof, and for this one the proof is a **fault-injection matrix**: every distinct write step of a save, on both hosts, is driven to failure or interruption, and the plot is asserted to resolve to exactly one coherent version. The matrix drives **8** interruption points on the filesystem host and **4** on the IndexedDB host — staging failures, journal-write failures, mid-apply kills, partial-apply kills, malformed journals, aborted transactions — and each row lands on either the complete previous version or the complete new one. None lands in between. + +| | | +|---|---| +| New tests passing | 32 | +| ├ Vitest unit / integration | 31 | +| └ Web-shell Playwright smoke | 1 | +| Compile-time type guard (mutation-verified) | 1 | +| Filesystem interruption points covered `[credibility]` | 8 / 8 coherent | +| IndexedDB interruption points covered `[credibility]` | 4 / 4 coherent | +| IndexedDB transactions per save `[credibility]` | exactly 1 | +| "Plot saved" shown for a save that didn't fully commit `[credibility]` | 0% | + +Three of those figures are worth spot-checking, because they are the whole point of the feature. The browser save uses **exactly one** `readwrite` transaction spanning both the `items` and `payloads` stores — a test spies on `IDBDatabase.prototype.transaction` and counts, so the item record and its GeoJSON payload genuinely land together or not at all. The success path is held strictly honest: a separate test asserts via mock call-order that `markClean()` and the "Plot saved" message fire *after* `commitPlotSave` resolves, and that a rejected commit shows a failure, keeps the plot dirty, and shows no success — so the success indication appears for **0%** of saves that did not fully commit. And the boundary's save-unit type is structurally derived with `Pick<>`, guarded by a compile-time test that `tsc --noEmit` enforces and that mutation testing confirms actually fails when the derivation is broken. + +The change stayed inside its blast radius. The full suites at the same SHA came back green: `@debrief/stac-writer` (22 Vitest), `debrief-vscode` (841 Vitest, 1 pre-existing unrelated skip), `@debrief/web-shell` (135 Vitest), with `pnpm -r typecheck`, the VS Code `tsc --noEmit` gate, ESLint, ruff and pyright all clean. No Python changed. + +## Lessons Learned + +**The commit point has to be a single atomic act, or none of the reasoning holds.** The entire design rests on being able to say "before this instant, roll back; after it, roll forward." That sentence is only crisp if there is exactly one instant. On the filesystem we get it by anchoring "committed" to one atomic journal write — the moment that file exists, the save is, by definition, done; before it, the save never happened. Every one of the eight interruption points then resolves trivially, because each one is unambiguously on one side of that line. The hard part wasn't the staging or the renames; it was choosing the one write that *means* committed. + +**Atomicity is not durability, and conflating the two would have bloated the design.** It's tempting to also promise that your very latest save survives a power cut. We deliberately didn't. Reconcile never repairs a torn version — it only ever *chooses* which already-coherent version to present. That restraint is what keeps it small: there is no fsync dance, no partial-write repair, no torn-record detective work, because a torn record is, by construction, something that can never be observed. Picking the narrower guarantee made the implementation both simpler and more obviously correct. + +**The two hosts needed opposite amounts of machinery.** The filesystem has no transaction, so we built one — a hand-rolled write-ahead journal with stage, commit, apply, clear and a reconcile pass. IndexedDB already *is* transactional; a killed tab discards uncommitted work for free. There the "fix" was almost embarrassingly small: two writes had been running in separate transactions, and all we had to do was collapse them into one. Same guarantee, wildly different effort — a useful reminder that "make it atomic" is a property of the substrate, not a fixed amount of code. + +**Deriving the boundary type structurally paid for itself immediately.** The save unit mirrors a subset of existing typed sources, so it's built with `Pick<>` rather than re-listed fields, with a compile-time exhaustiveness guard. It would have been faster in the moment to just type out the fields. The guard exists precisely because re-listing is the known root cause of silently-dropped data when a source type later grows — and a save that silently drops a field is exactly the kind of quiet corruption this whole feature set out to abolish. + +## What's Next + +Two threads stay open. Scene-capture still performs an eager, best-effort write of `features.geojson` outside the new commit path — a deliberate, documented deferral, since that single-file write is superseded by Save Session and isn't part of the save unit. Folding it under the same atomic-commit discipline would close the last raw write that bypasses the boundary. And the web-shell's standalone create path could grow a richer-metadata atomic create once it regains UI callers; today the committed create case is covered, but the fuller metadata story waits on a consumer that needs it. + +→ [See the code](https://github.com/debrief/debrief-future/pull/658) +→ ADR-039 in `docs/project_notes/decisions.md` records the decision. diff --git a/specs/268-save-atomicity/plan.md b/specs/268-save-atomicity/plan.md new file mode 100644 index 000000000..375a3b873 --- /dev/null +++ b/specs/268-save-atomicity/plan.md @@ -0,0 +1,134 @@ +# Implementation Plan: Atomic (Transactional) Plot Save + +**Branch**: `268-save-atomicity` | **Date**: 2026-06-01 | **Spec**: [spec.md](./spec.md) +**Input**: Feature specification from `specs/268-save-atomicity/spec.md` + +## Summary + +Make a plot save all-or-nothing. Today a save is several independent writes — +`features.geojson` (a raw, non-atomic `fs.writeFileSync` that bypasses the +writer boundary), the STAC `item.json`, and thumbnail PNGs — and success is +reported before they all land, so a failure or interruption can leave a +half-updated, internally-inconsistent plot. + +The approach adds **one host-agnostic boundary operation** — `commitPlotSave` — +that commits the whole save unit atomically, plus `reconcilePlotSave`, called +before the read on open, to heal an interrupted save. The desktop (filesystem) +adaptor implements atomicity with a **write-ahead intent journal** as the +commit point (stage temps → write journal → apply renames → clear; reconcile +rolls back before the journal exists, forward after). The browser (IndexedDB) +adaptor implements it with **one multi-store transaction** (atomic for free). +Enforcing this at the shared boundary (Article IV.4) means neither host can +regress it and the feature-collection write moves onto the boundary (FR-004). + +## Technical Context + +**Language/Version**: TypeScript 5.x (strict; Article XV). No Python change. +**Primary Dependencies**: `@debrief/stac-writer` (interface + core), `node:fs`/`node:crypto` (fs adaptor — existing), `idb` (web-shell adaptor — existing), `@debrief/schemas` (`FeatureCollection`, `StacItem`), VS Code Extension API (`window.showWarningMessage`). **No new runtime dependencies.** +**Storage**: VS Code → local filesystem STAC catalog (`item.json`, `features.geojson`, thumbnail PNGs). Web-shell → IndexedDB stores `items` / `payloads` / `assets` / `meta` (per-origin). +**Testing**: Vitest unit/integration (fault injection via mock-throw-on-Nth-write + `fake-indexeddb`); one web-shell Playwright happy-path smoke. No new test framework. +**Target Platform**: VS Code extension host (Node) + web-shell (browser). +**Project Type**: web (TS monorepo — shared library + two frontends). +**Performance Goals**: A normal save within 10% of current duration (SC-004) — staging adds one temp-write for `features.geojson` (thumbnails already staged) + a small journal write + renames; negligible for typical plots. +**Constraints**: Offline by default; no observable partial state (FR-001); atomicity not power-loss durability (Clarifications Q3); reconcile must precede the read on open. +**Scale/Scope**: Single-plot save flow, both hosts. Out of scope: multi-plot/batch save, concurrency control, browser thumbnail writing. + +## Constitution Check + +*GATE: must pass before Phase 0. Re-checked after Phase 1 (below).* + +| Article | Gate | Status | +|---------|------|--------| +| **I. Defence-grade reliability** | No silent failures; offline; reproducible | ✅ **Directly serves I.3** — converts a silent partial-save into explicit success/failure (FR-005/006). Offline-only. | +| **II. Schema integrity** | Derived schemas, adherence tests | ✅ **No schema change** — reuses `FeatureCollection`/`StacItem`. LinkML untouched; adherence tests unaffected. | +| **III. Data sovereignty / provenance** | Provenance preserved, source retained | ✅ Commit path must not drop existing `item.json` provenance; source assets untouched. Verified in tests. | +| **IV. Architectural boundaries** | Frontends never persist; writer is the boundary | ✅ **Major alignment** — the new ops live on the shared `StacWriter`; the feature-collection write moves off raw `fs` onto the boundary (IV.2/IV.4); each host implements once. | +| **V. Extensibility** | Fail-safe loading | ✅ A failed save cannot corrupt the plot or crash core. | +| **VI/VII. Testing** | Tests gate merges; tests-first | ✅ Fault-injection acceptance tests written before/with implementation; contracts C1–C5 are the executable "done". | +| **VIII. Documentation** | Specs before code; ADRs for significant choices | ⚠️ **Add an ADR** for the journal/commit-marker decision (tracked as a task). Spec + plan present. | +| **IX. Dependencies** | Minimal, pinned | ✅ **No new dependencies.** | +| **XV. Strict types** | No `any`; strict; types derived | ✅ Strict TS; `CommitPlotSaveInput.thumbnails` is `Pick<>`; `SaveJournal` validated on read (XV.5). | + +**Result**: PASS. No violations → Complexity Tracking is empty. One follow-up (ADR) tracked as a task, not a gate failure. + +## Project Structure + +### Documentation (this feature) + +```text +specs/268-save-atomicity/ +├── plan.md # This file +├── research.md # Phase 0 — commit-mechanism decision + grounding +├── data-model.md # Phase 1 — boundary DTOs + SaveJournal record +├── quickstart.md # Phase 1 — how to verify +├── contracts/ +│ ├── stac-writer-commit.ts # Type contract for the interface additions +│ └── save-flow.md # Save + reconcile-on-open behavioural contract +├── checklists/ +│ └── requirements.md # Spec quality checklist (from /speckit.specify) +└── tasks.md # /speckit.tasks output (NOT created here) +``` + +### Source Code (repository root) + +```text +shared/stac-writer/src/ +├── interface.ts # +commitPlotSave, +reconcilePlotSave on StacWriter; +│ # +CommitPlotSaveInput/Result, +ReconcilePlotSave* +└── (errors.ts unchanged — reuse StacWriterError) + +apps/vscode/src/ +├── services/stacWriterFs.ts # Implement commitPlotSave (stage→journal→apply→clear) +│ # + reconcilePlotSave (roll back/forward); reuse atomicWriteSync +├── commands/saveSession.ts # Call commitPlotSave; move markClean/"Plot saved" AFTER commit +└── commands/openPlot.ts # Call reconcilePlotSave BEFORE loadPlotData; notify if recovered + +apps/web-shell/src/ +├── services/stacWriterIdb.ts # Implement commitPlotSave (one multi-store txn) + reconcilePlotSave (clean/prune) +├── mocks/stacService.ts # Save path: replace writeItem+writeAsset pair with commitPlotSave +└── (App.tsx / catalogReadView.ts) # Reconcile before read; surface non-blocking notice + +docs/project_notes/decisions.md # +ADR: FS save journal / commit-marker + +tests: +apps/vscode/tests/unit/ # stacWriterFs.commitPlotSave.test.ts, reconcile, saveSession (extend) +apps/web-shell/src/services/__tests__/ # stacWriterIdb.commitPlotSave.test.ts (fake-indexeddb) +shared/stac-writer/ # interface/contract type tests +apps/web-shell/playwright/tests/ # save-atomicity.spec.ts (happy-path smoke) +``` + +**Structure Decision**: Existing monorepo layout — the change is concentrated +at the `@debrief/stac-writer` boundary (interface) and its two adaptors, with +thin call-site edits in each host's save command and open path. No new packages. + +## Media Components + +None - backend/infrastructure feature. The only user-visible surface is a +non-blocking notification on save failure / interrupted-save recovery, which +reuses existing host notification APIs; there is no new or changed visual +component and no Storybook story to bundle. + +## Storybook E2E Testing + +None - no interactive UI components. + +## Web-Shell E2E Testing + +| Workflow | Panels/Components Involved | Key Selectors | Interactions | +|----------|---------------------------|---------------|--------------| +| Save plot then reopen → coherent | MapView, save affordance, plot load | `.leaflet-container`, save control, `[data-testid]` on the plot view | edit a plot, Save, reopen, assert it loads coherently with the new state | + +**Testing Strategy**: +- [x] Happy-path save → reopen runs end-to-end in the web-shell (regression guard for FR-011) +- [x] Fault-injection guarantees (SC-001/002/003/005) are covered by **unit/integration** tests, not E2E (injecting a mid-write crash is reliable only at the adaptor seam) +- [ ] Screenshot not required (no visual change); the smoke asserts behaviour, not pixels + +**Test File Location**: `apps/web-shell/playwright/tests/save-atomicity.spec.ts` + +**Run Commands**: +- Cloud: `cd apps/web-shell && node run-playwright.mjs save-atomicity` +- Local: `pnpm --filter @debrief/web-shell test save-atomicity` + +## Complexity Tracking + +No constitution violations — section intentionally empty. diff --git a/specs/268-save-atomicity/quickstart.md b/specs/268-save-atomicity/quickstart.md new file mode 100644 index 000000000..172532df1 --- /dev/null +++ b/specs/268-save-atomicity/quickstart.md @@ -0,0 +1,67 @@ +# Quickstart: Atomic (Transactional) Plot Save + +**Feature**: 268-save-atomicity · **Phase 1** · **Date**: 2026-06-01 + +How to exercise and verify the atomic-save guarantee once implemented. + +## What changes for a user + +Nothing, on the happy path: Save still produces the same `features.geojson`, +`item.json`, and thumbnails, and still shows "Plot saved" (FR-011). The +difference is only visible on failure: a save that errors or is interrupted +never leaves a half-updated plot, and "Plot saved" appears only when the whole +save committed. + +## Manual smoke (VS Code) + +1. Open a plot in the dev extension, make an edit, **Save**. Confirm + `features.geojson` + `item.json` + thumbnails all updated and "Plot saved". +2. Make the item directory read-only (`chmod -w`), edit, **Save**. Expect a + clear error, the dirty indicator **still set**, and the previous files + intact and openable. No `.tmp` or `.save-journal.json` left behind. +3. Simulate an interrupted save (leave a `.save-journal.json` + temps in the + item dir from the test fixtures), then **reopen** the plot. Expect a + non-blocking "recovered an interrupted save" notice and a coherent plot. + +## Manual smoke (web-shell) + +1. `cd apps/web-shell && pnpm dev`, open a plot, edit, **Save** → reopens + coherent. (IndexedDB transaction atomicity means an interrupted save in the + browser simply leaves the last committed version.) + +## Automated verification (the source of record) + +Fault-injection acceptance tests prove the guarantees (SC-001..005): + +```sh +# VS Code adaptor + save command (Node, temp dirs + mock-throw-on-Nth-write) +uv run pytest -q # (no Python change) ; then: +pnpm --filter @debrief/vscode test -- stacWriterFs commitPlotSave reconcile +pnpm --filter @debrief/vscode test -- saveSession + +# Web-shell adaptor (fake-indexeddb; one-transaction + abort-leaves-unchanged) +pnpm --filter @debrief/web-shell test -- stacWriterIdb commitPlotSave + +# Shared interface contract types +pnpm --filter @debrief/stac-writer test +``` + +What the tests assert (maps to `contracts/stac-writer-commit.ts` C1–C5): +- **C1/SC-001** — inject a failure at each write phase; a follow-up read returns + the *pre-save* state (no partial); originals intact for pre-commit failures. +- **C2** — on success every artefact reflects the new state. +- **C3/SC-002** — for each simulated interruption phase, `reconcilePlotSave` + yields a coherent read (pre-commit → previous, post-commit → new); no + `.tmp`/journal remain. +- **C3/SC-003** — `markClean`/"Plot saved" never fire for a non-committed save. +- **C4/SC-005** — web-shell uses exactly one IDB transaction; aborting leaves + the store byte-identical. +- **C5** — `reconcilePlotSave` on a clean store is a no-op. + +## Full gate before pushing + +```sh +task verify # lint + typecheck + test (Python + TS) +# plus the web-shell happy-path save→reopen Playwright smoke: +cd apps/web-shell && node run-playwright.mjs save-atomicity +``` diff --git a/specs/268-save-atomicity/research.md b/specs/268-save-atomicity/research.md new file mode 100644 index 000000000..51478dddf --- /dev/null +++ b/specs/268-save-atomicity/research.md @@ -0,0 +1,88 @@ +# Research: Atomic (Transactional) Plot Save + +**Feature**: 268-save-atomicity · **Phase 0** · **Date**: 2026-06-01 + +This document resolves the one decision the spec deliberately deferred (the commit mechanism) plus the supporting design choices, grounded in the current code paths. + +## Current state (grounding) + +| Concern | Where | Finding | +|---------|-------|---------| +| VS Code save — feature collection | `apps/vscode/src/commands/saveSession.ts:50` | Raw `fs.writeFileSync` of `features.geojson` — **non-atomic and bypasses the writer boundary**. | +| VS Code save — thumbnails | `saveSession.ts:72` → `writer.writePlotThumbnailPair` | Atomic *per file*, but a separate step from the FC write. | +| VS Code save — success reporting | `saveSession.ts:133-134` | `markClean()` + "Plot saved" fire **before** the thumbnail write (FR-005 violation). | +| VS Code open | `apps/vscode/src/commands/openPlot.ts:155` (`loadPlotData`) → `:182` (`hydrateStoreFromFeatures`) | Natural reconcile slot is **before** the read at :155. Notifications via `vscode.window.showWarningMessage`. | +| Web-shell save | `apps/web-shell/src/mocks/stacService.ts:449-457` | `writeItem()` then `writeAsset()` in **two separate IndexedDB transactions** (not atomic as a pair). Bundled-plot edits use `patchItem` (already one transaction). No thumbnail write (`stacWriterIdb.ts:642` throws "not supported"). | +| Web-shell open | `apps/web-shell/src/services/catalogReadView.ts:94,123` (`readStoredItem`/`readPayload`) | Reconcile slot before the read; toast surface via App/Zustand. | +| FS atomic primitive | `apps/vscode/src/services/stacWriterFs.ts:527-555` (`atomicWriteSync`) | `temp file → renameSync`, per file. **No multi-file grouping / journal exists.** | +| IDB atomicity | `apps/web-shell/src/services/stacWriterIdb.ts` | Each writer method opens its **own** `readwrite` transaction over the relevant stores (`items`/`payloads`/`assets`/`meta`) and `await tx.done`. IndexedDB transactions are atomic and discard uncommitted work on abort/kill. | +| Article IV.4 ESLint | `shared/eslint-rules/no-direct-persistence-in-frontend.cjs` | **web-shell-scoped only**; the VS Code raw `fs.writeFileSync` is *not* flagged (Node host). So FR-004 is an Article IV.2 *consistency* fix, not the closing of an existing suppression. | +| Fault-injection test patterns | `apps/vscode/tests/unit/saveSession.createSaveSessionCommand.test.ts`, `apps/web-shell/src/services/__tests__/stacWriterIdb.test.ts` (fake-indexeddb), `shared/components/src/PropertiesPanel/__tests__/saveSession-integration.test.ts` (`ReadOnlyFilesystemError` sim) | Mock-writer-throws-on-Nth-call and `fake-indexeddb` factories already exist and are reusable. | + +## Decision 1 — One boundary operation for the whole save unit + +**Decision**: Add a single host-agnostic operation to the `StacWriter` interface — `commitPlotSave(input)` — that takes *all* artefacts of a save (the feature collection, plus the optional thumbnail pair) and commits them atomically. Add a companion `reconcilePlotSave(input)` called on open. Each host implements both once against its native backend. + +**Rationale**: The spec's "save unit" is exactly `features.geojson` + STAC `item.json` + thumbnail PNGs (FR-002). Atomicity must be enforced *at the boundary* (FR-009) so neither host can regress it and the feature-collection write stops bypassing the boundary (FR-004, Article IV.2/IV.4). A single operation that receives the whole unit is the only place both hosts can guarantee all-or-nothing — a caller orchestrating several writer calls cannot. + +**Alternatives rejected**: +- *Keep separate `writeFeatureCollection` + `writePlotThumbnailPair` and wrap them in a caller-side "transaction"* — the caller (frontend) cannot make multiple FS renames or IDB transactions atomic; this just moves the seam to the wrong layer (violates FR-009). +- *Expose raw FS/IDB transaction handles to callers* — leaks backend specifics across the boundary, breaks Article IV.4's "implement once against native backend". + +## Decision 2 — FS commit mechanism: a write-ahead intent journal as the atomic commit point + +**Decision**: In `stacWriterFs`, `commitPlotSave` proceeds in four phases: +1. **Stage** — write every new artefact to a temp file (`..tmp`) using the existing `atomicWriteSync` temp step. Nothing in place yet. +2. **Commit point** — atomically write a single **save journal** (`.save-journal.json`, itself via temp→rename) listing the pending `temp → final` renames for this save. *The atomic creation of the journal is the commit point.* +3. **Apply** — rename each temp → final (POSIX `rename` is atomic per file). +4. **Clear** — delete the journal. Save complete. + +**Reconcile-on-open** (`reconcilePlotSave`): +- *No journal, no stray temps* → clean, no-op. +- *Stray temps but no journal* → interrupted **before** the commit point → delete the temps, keep the originals = **last-good** (FR-008, satisfies Q2 "auto-restore last good"). +- *Journal present* → interrupted **after** the commit point → **roll forward**: for each `temp → final` still pending, rename it (idempotent — a missing temp means that final already landed), then delete the journal = the **new coherent version** (FR-007 "the new version if it had committed"). + +**Rationale**: A multi-file FS save has no single atomic syscall, so we need an explicit commit marker. Anchoring "committed" to the atomic journal write gives a crisp boundary: reconcile rolls **back** before it and **forward** after it, and *every* interruption point yields a coherent plot (FR-001/FR-007). It reuses the existing `atomicWriteSync` primitive (no new low-level primitive — matches the spec assumption) and needs no fsync/durability guarantee (Decision 7). + +**Alternatives rejected**: +- *Sequential temp→rename with no journal* — a crash mid-rename leaves new `features.geojson` + old `item.json` = incoherent, and rename has already destroyed the original so it can't be rolled back. **Unsafe** — this is the core bug. +- *Backup-originals-then-overwrite, roll back on open* — doubles I/O for every save and still needs a marker to know a save was in flight; the journal+roll-forward is simpler and cheaper. +- *Whole-item shadow directory + atomic dir swap* — item dirs have stable paths referenced elsewhere (catalog, assets); swapping directories is disruptive and heavier than a per-file journal. + +## Decision 3 — Web-shell commit mechanism: a single multi-store IndexedDB transaction + +**Decision**: In `stacWriterIdb`, `commitPlotSave` opens **one** `readwrite` transaction spanning `items` + `payloads` + `assets` + `meta`, enqueues all puts (item record, geojson payload, any binary assets), then `await tx.done`. `reconcilePlotSave` is effectively a no-op (optionally prunes orphaned overlay-only records). + +**Rationale**: IndexedDB transactions are already atomic and durable — an error aborts the whole transaction, and a tab/browser kill discards an uncommitted transaction, leaving the store at the last committed state. Grouping the save's writes into one transaction gives true all-or-nothing for free and satisfies the "coherent fallback" (Q3) with no journal. The current gap is purely that `writeItem` and `writeAsset` run in *separate* transactions (`mocks/stacService.ts:449-457`); `commitPlotSave` closes that. + +**Alternatives rejected**: +- *Replicate the FS journal in the browser* — unnecessary; IDB already provides transactional atomicity. Adding a journal would be redundant complexity. + +## Decision 4 — Reconcile runs *before* the read on open + +**Decision**: `reconcilePlotSave` is invoked **before** the plot data is read (VS Code: before `loadPlotData` at `openPlot.ts:155`; web-shell: before `catalogReadView` read). If it reports `recovered: true`, the host shows a **non-blocking** notice (`showWarningMessage` / web-shell toast). + +**Rationale**: Reconcile can change what is on disk (roll forward applies pending renames; roll back deletes temps). It must run first so the subsequent read sees the reconciled, coherent state. The non-blocking notice (no prompt) implements Q2/FR-008 and keeps this a non-UI feature. + +## Decision 5 — Report success only after commit + +**Decision**: In `saveSession.ts`, replace `storeFeatureCollection` + `storeThumbnails` with a single `await writer.commitPlotSave({ featureCollection, thumbnails })`. Move `markClean()` and the "Plot saved" message to **after** it resolves; on rejection, surface the error and leave the dirty flag set (FR-005/FR-006). Thumbnail *capture* stays best-effort: a capture failure simply omits `thumbnails` from the commit; a capture *write* that begins is part of the atomic unit. + +## Decision 6 — Boundary types are derived (Article IV.5) + +**Decision**: `CommitPlotSaveInput.thumbnails` is `Pick`; the feature-collection field reuses the generated `FeatureCollection` type from `@debrief/schemas`. No fields are re-listed by name. The internal `SaveJournal` record (FS-only) is a new typed shape (no existing source to derive from) and is validated on read. + +## Decision 7 — No new durability guarantee, no new dependencies + +Per Q3, the guarantee is atomicity/coherence, not power-loss durability of the newest save, so we do **not** add fsync requirements (the existing best-effort `temp→rename` is sufficient). No new runtime dependencies: FS uses `node:fs`/`node:crypto` (already used by `stacWriterFs`), web-shell uses the existing `idb` transactions. + +## Decision 8 — Validation strategy (fault injection) + +- **Unit/integration (primary, covers SC-001/002/003/005)**: reuse the mock-writer-throws-on-Nth-call pattern and `fake-indexeddb` factory. For FS, drive `commitPlotSave` against a real temp dir (`fs.mkdtempSync`), inject failures at each phase (stage / journal / apply), and assert: (a) no observable partial via a follow-up read, (b) for pre-commit failures the originals are intact, (c) `reconcilePlotSave` heals each leftover condition, (d) no stray `.tmp`/journal remain after reconcile. For web-shell, spy on `db.transaction` to assert a save uses exactly one transaction and that an aborted transaction leaves the store unchanged. +- **E2E (smoke, regression)**: one web-shell Playwright happy-path save → reopen coherence check (no fault injection — that lives in unit/integration). + +## Open items handed to `/speckit.tasks` + +1. Confirm the exact web-shell *live* save entrypoint that should call `commitPlotSave` (the `mocks/stacService.ts` create path vs. the `patchItem` overlay path for bundled-plot edits — the overlay path is already single-transaction; the standalone create path is the two-transaction gap). +2. Decide whether `writeFeatureCollection` should remain a thin public method delegating to `commitPlotSave` for non-thumbnail callers (e.g. `captureScene.ts:474` writes a FeatureCollection without thumbnails). +3. Add an ADR recording the journal/commit-marker decision (Article VIII.3). diff --git a/specs/268-save-atomicity/spec.md b/specs/268-save-atomicity/spec.md new file mode 100644 index 000000000..52a27312a --- /dev/null +++ b/specs/268-save-atomicity/spec.md @@ -0,0 +1,141 @@ +# Feature Specification: Atomic (Transactional) Plot Save + +**Feature Branch**: `268-save-atomicity` +**Created**: 2026-06-01 +**Status**: Draft +**Input**: Backlog item #268 — "VS Code save atomicity (broader)". Make a plot save all‑or‑nothing: the writes that make up one save (the feature collection, the STAC item metadata, and the thumbnail assets) must commit as a single unit, so a failure or interruption partway through can never leave the analyst with a half‑updated, corrupt, or internally‑inconsistent plot. + +## Context *(why this exists)* + +A "Save" of a plot is not one write — it is several. The current save flow performs them independently and reports success too early: + +- The feature collection (`features.geojson`) is written with a direct, non‑atomic file write that **bypasses the shared persistence boundary** — a reader can observe a half‑written file, and the write benefits from none of the boundary's guarantees. +- The thumbnail/overview images and the STAC item metadata are written separately, through the persistence boundary, **after** the analyst has already been shown "Plot saved" and the unsaved‑changes indicator has already been cleared. + +Because these writes are independently committable, a real‑world failure between them (disk full, permission denied, browser storage quota, or an outright process kill / power loss) can leave any subset updated: a new feature collection paired with stale thumbnails, a torn `features.geojson`, or a plot the analyst believes is saved when it is not. A single‑file atomic‑write primitive already exists, but there is no way to commit *multiple* writes as one unit, and the feature‑collection write does not even use the single‑file primitive. + +This was a deliberately parked tech‑debt item; this specification is authored on explicit request to address it. + +## Clarifications + +### Session 2026-06-01 + +- Q: What host scope should the atomic-save guarantee cover for this delivery? → A: Both hosts — enforced at the shared persistence boundary so the desktop (filesystem) and browser (storage) hosts both get all-or-nothing saves; the browser host covers only the writes it actually performs. +- Q: When a plot is opened and leftovers from an interrupted/partial save are detected, how should they be resolved? → A: Automatically restore the last fully-committed version and show a non-blocking notice — the analyst is not prompted, so this remains a non-UI feature. +- Q: For a save reported as successful, what reliability guarantee is required? → A: Atomicity with a coherent fallback — no torn/partial/inconsistent plot is ever observable; on a sudden power loss the plot may open as the last fully-committed version (the newest in-flight save can be lost). Guaranteed power-loss durability of the newest save is not required. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - A failed save never corrupts my plot (Priority: P1) + +As an analyst, when I save a plot and something goes wrong partway through the save, I am left with a *coherent* plot — either the complete new version or the complete previous version — never a mixture of the two. + +**Why this priority**: This is the core of the ticket. A partially‑applied save is silent data corruption: the analyst's plot of record becomes internally inconsistent (e.g. new tracks but old thumbnail, or an unreadable feature file). Preventing that is the whole point. + +**Independent Test**: Drive a save while injecting a failure at each distinct write step (feature collection, STAC item, each thumbnail asset). After every injected failure, open the plot and assert it loads cleanly as exactly one coherent version, with features and metadata/thumbnails agreeing. + +**Acceptance Scenarios**: + +1. **Given** a previously‑saved plot, **When** the analyst saves changes and the thumbnail write fails, **Then** opening the plot shows either the complete new state (with new thumbnails) or the complete previous state — not new features with stale thumbnails. +2. **Given** a previously‑saved plot, **When** the analyst saves changes and the feature‑collection write fails, **Then** the previously‑persisted plot is still openable and unchanged. +3. **Given** any save attempt, **When** the save completes successfully, **Then** every artefact of the save (features, metadata, thumbnails) reflects the same new state. + +--- + +### User Story 2 - Honest reporting and a safe retry (Priority: P2) + +As an analyst, if a save cannot fully complete, I am clearly told the save failed, my in‑editor changes are preserved so I can retry, and my previously‑saved version is left intact. + +**Why this priority**: Today the success message and the cleared unsaved‑changes indicator can appear *before* all writes have committed, so the analyst can be told "saved" when the on‑disk plot is partial — and may then close the editor and lose work. Trustworthy success/failure reporting is required for the atomicity guarantee to be useful. + +**Independent Test**: Inject a failure during a save and assert that (a) a failure is surfaced to the analyst, (b) the unsaved‑changes indicator remains set, and (c) the previously‑persisted plot still opens. Then assert that no success message is shown for the failed attempt. + +**Acceptance Scenarios**: + +1. **Given** a save that fails partway, **When** the failure occurs, **Then** the analyst sees a clear failure message (not a success message). +2. **Given** a save that fails partway, **When** the failure occurs, **Then** the plot remains marked as having unsaved changes and the analyst can immediately retry. +3. **Given** a save that fully succeeds, **When** all of the save's writes have committed, **Then** — and only then — the success indication is shown and the unsaved‑changes indicator is cleared. + +--- + +### User Story 3 - Coherent plot after an interrupted save (Priority: P3) + +As an analyst, if a save is interrupted by something I cannot catch — a crash, an out‑of‑memory kill, or a power loss — the next time I open the plot it is coherent (the last good state, or the new state if it had committed), and if a partial save had to be cleaned up, I am told. + +**Why this priority**: Catchable errors (US1/US2) cover most failures, but an uncatchable interruption mid‑write is exactly when a non‑transactional save does the most damage. Resilience here is valuable but rarer than the catchable‑error path, hence P3. + +**Independent Test**: Simulate an interruption by leaving the on‑disk state in each possible mid‑save condition (staging present but not committed; some writes applied, others not), then open the plot and assert it resolves to a single coherent state and that a recovery, if performed, is reported non‑blockingly. + +**Acceptance Scenarios**: + +1. **Given** a save interrupted before it committed, **When** the analyst reopens the plot, **Then** the last fully‑committed version opens without error. +2. **Given** an interruption that left partial leftovers, **When** the analyst reopens the plot, **Then** the leftovers are reconciled to one coherent state and a non‑blocking notice informs the analyst that a partial save was recovered. +3. **Given** a save that had fully committed just before interruption, **When** the analyst reopens the plot, **Then** the new state opens (no spurious rollback). + +--- + +### Edge Cases + +- **Failure on the first write vs a later write**: a failure before anything commits must leave the previous version untouched; a failure after some writes commit must still resolve to a single coherent version (no observable partial). +- **Storage exhaustion mid‑save** (disk full / browser quota exceeded): the save aborts, the previous version is intact, and the analyst is told the save did not complete. +- **Permission / availability denied** (read‑only location, private‑mode browser storage): the save reports failure cleanly without damaging the existing plot. +- **Uncatchable interruption** (process kill, power loss) at any point during the write sequence — covered by US3. +- **Empty or first‑ever save** (no previous version to preserve): a failed first save must not leave a half‑created plot that later reads as corrupt. +- **Concurrent saves of the same plot** from two editor windows: out of scope (see Out of Scope); a save is treated as the sole writer. +- **Thumbnail capture itself fails** (as opposed to the thumbnail *write*): capture is best‑effort and may legitimately be skipped; skipping it must not be treated as a save failure, but a *write* that begins and then fails must obey the atomicity guarantee. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: A plot save MUST be atomic from the analyst's perspective — after any save attempt, the persisted plot MUST be observable as **either** the complete new state **or** the complete previous state, never a partial mixture. +- **FR-002**: All writes that constitute a single save (feature collection, STAC item metadata, and thumbnail/overview assets) MUST be committed as one unit; a failure in any one of them MUST prevent the others from becoming observable to a subsequent reader. +- **FR-003**: The feature collection MUST be persisted such that a reader can never observe a partially‑written or torn file, in both the desktop and browser hosts. +- **FR-004**: The feature‑collection write MUST go through the shared persistence boundary rather than a direct storage call, so it is subject to the same atomicity (and provenance/guard) guarantees as the other writes in the save. +- **FR-005**: The system MUST NOT indicate a save as successful — i.e. MUST NOT clear the unsaved‑changes indicator or show a success message — unless every write that makes up the save has committed (all of the save's writes atomically in place and observable as the new state). Guaranteed power-loss durability of the newest save is not required — see FR-007. +- **FR-006**: When a save cannot complete, the system MUST surface a clear failure to the analyst, MUST retain the in‑editor changes so the analyst can retry, and MUST leave the previously‑persisted version intact and openable. +- **FR-007**: If a save is interrupted by an uncatchable process termination, the next time the plot is opened the system MUST present a single coherent plot (the last committed version, or the new version if it had committed) and MUST NOT present a torn feature file or a plot whose features and metadata/thumbnails disagree. The system is NOT required to guarantee that the newest in-flight save survives a power loss — only that whatever opens is coherent. +- **FR-008**: If an interruption left recoverable partial state, opening the plot MUST **automatically** restore the last fully-committed state — without prompting the analyst — and MUST inform the analyst, non‑blockingly, that a partial save was recovered. +- **FR-009**: The atomicity guarantee MUST be enforced at the shared persistence boundary so that it holds for whichever writes each host performs (desktop filesystem and browser storage) and so neither host can silently regress it. +- **FR-010**: A rejected or invalid new state (failing validation, quota, or permission *before* any commit) MUST NOT modify, truncate, or delete the existing persisted plot. +- **FR-011**: A normal, non‑failing save MUST remain functionally unchanged for the analyst — same artefacts produced, no new mandatory steps, and no perceptible regression in save responsiveness for typical plots. + +### Key Entities + +- **Save unit**: the complete set of artefacts produced by a single save of one plot — the feature collection, the STAC item metadata, and the plot thumbnail/overview assets. The unit is the granularity at which atomicity is guaranteed. +- **Persisted plot**: the on‑store representation a reader loads. It must always be readable as exactly one coherent save unit. +- **Last‑good reference**: an abstract notion of the most recent fully‑committed save unit, used to preserve or restore the plot when a save does not complete. (Whether this is a staging area, a recovery marker, or implicit in the commit mechanism is a design decision, not a requirement.) + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: Under fault injection at every distinct write step, 100% of failed save attempts leave the plot openable as exactly one coherent version (complete previous or complete new) — zero partial/corrupt outcomes across all injected failure points. +- **SC-002**: After a simulated uncatchable interruption at any point in the save sequence, 100% of reopened plots load without error and show features and metadata/thumbnails that agree (no "features updated, thumbnail stale" mismatch). +- **SC-003**: Across the fault‑injection suite, the success indication (cleared unsaved‑changes + success message) appears for 0% of saves that did not fully commit. +- **SC-004**: A normal save of a typical plot completes within 10% of the current save duration — the transactional model adds negligible overhead. +- **SC-005**: The same fault‑injection acceptance suite passes for both persistence backends (desktop filesystem and browser storage), demonstrating the guarantee is enforced at the shared boundary rather than per‑host. + +## Assumptions + +- Scope is the existing single‑plot "Save" flow. The analyst‑facing behaviour of a successful save is unchanged; only its failure/interruption behaviour and its commit boundary change. +- "Atomic from the analyst's perspective" means no externally‑observable partial state. It does **not** require an OS‑level transactional filesystem — a stage‑then‑commit sequence or a recovery‑on‑open mechanism are both acceptable ways to meet the requirements. +- Scope covers **both hosts** (desktop filesystem and browser storage), enforced at the shared persistence boundary (confirmed in Clarifications). The guarantee applies to whichever writes each host actually performs; the browser host currently performs a subset (it does not write plot thumbnails), and the guarantee still applies to the writes it does perform. +- The required guarantee is atomicity and coherence, **not** guaranteed durability of the newest save against a sudden power loss (confirmed in Clarifications). On power loss a coherent earlier version may open; a best-effort flush is sufficient. +- Single‑file atomicity and per‑transaction storage atomicity primitives already exist at the persistence boundary and can be built upon; this feature adds *multi‑write* (cross‑artefact) atomicity, not a new low‑level write primitive. +- A plot is saved by a single writer at a time; simultaneous saves of the same plot from two editors are not a supported scenario. + +## Dependencies + +- The shared persistence boundary (the host‑agnostic STAC writer) and its desktop‑filesystem and browser‑storage adaptors. +- The existing single‑file atomic‑write primitive (temp‑then‑rename) and the browser store's per‑transaction atomicity. +- The plot open/load path, which must perform any reconciliation or recovery described in FR‑007/FR‑008. +- Constitution Article IV (services/persistence boundary; the feature‑collection write moving onto the boundary is an Article IV.1/IV.4 alignment) — this feature may extend the boundary contract with a multi‑write commit concept. + +## Out of Scope + +- Choosing the specific commit mechanism (e.g. staging area + atomic move vs. last‑good recovery file) — that is a planning/design decision evaluated in `/speckit.plan`. +- Version history, save snapshots, or undo beyond restoring the single last‑good state. +- Multi‑plot or batch transactional save (committing several plots as one unit). +- Adding browser‑host plot‑thumbnail writing where it is currently unsupported. +- Concurrency control for simultaneous saves of the same plot from multiple editors. diff --git a/specs/268-save-atomicity/tasks.md b/specs/268-save-atomicity/tasks.md new file mode 100644 index 000000000..9c0355a86 --- /dev/null +++ b/specs/268-save-atomicity/tasks.md @@ -0,0 +1,199 @@ +# Tasks: Atomic (Transactional) Plot Save + +**Feature**: 268-save-atomicity · **Branch**: `claude/eloquent-fermi-bzLml` (spec dir `268-save-atomicity`) +**Input**: [spec.md](./spec.md) · [plan.md](./plan.md) · [research.md](./research.md) · [data-model.md](./data-model.md) · [contracts/](./contracts/) + +TypeScript-only feature (no Python change). Work concentrates at the `@debrief/stac-writer` boundary and its two adaptors, with thin call-site edits in each host's save command and open path. + +## Evidence Requirements + +**Evidence Directory**: `specs/268-save-atomicity/evidence/` +**Media Directory**: `specs/268-save-atomicity/media/` + +This is an **infrastructure / reliability** feature with **no new UI** (the only user-visible surface is a non-blocking recovery notice that reuses existing host notification APIs). Evidence is therefore test- and document-driven; the credibility artifact is the fault-injection matrix proving every interruption point resolves to a coherent plot. + +### Planned Artifacts + +| Artifact | Description | Captured When | +|----------|-------------|---------------| +| `evidence/test-summary.md` | Vitest results across the new adaptor + host tests (uses test-summary template, YAML front matter) | After all tests pass | +| `evidence/usage-example.md` | Walkthrough: a save that fails mid-write leaves the plot intact; an interrupted save auto-recovers on open | After US1+US3 complete | +| `evidence/fault-injection-matrix.md` | Table mapping each injected failure point (stage / journal / apply, per host) → observed coherent outcome (SC-001/002/003/005) | After US1+US3 tests | +| `evidence/opening-context.md` | Cached blog opener (mermaid Hook) — **already created during /speckit.plan** | Done | +| `media/shipped-post.md` | Feature post: cached opener (verbatim first 3 sections) + ship-time evidence | Polish phase | + +### Media Content + +| Artifact | Description | Created When | +|----------|-------------|--------------| +| `evidence/opening-context.md` | Cached opener (Hook + What We're Building + How It Fits + Key Decisions) | During /speckit.plan (done) | +| `media/shipped-post.md` | Feature post combining cached opener + ship-time evidence | Polish phase | + +### PR Creation + +| Action | Description | Created When | +|--------|-------------|--------------| +| Feature PR | PR #658 in debrief-future (already open on this branch) — updated with evidence | Final task | +| Blog PR | PR in debrief.github.io with shipped-post.md | Triggered by /speckit.pr | + +## Phase 1: Setup + +Documents the design decision and provides the shared fault-injection scaffolding the story phases reuse. No new package or build config. + +- [x] T001 Record an ADR for the filesystem **save-journal / commit-marker** decision (stage → journal → apply → clear; reconcile rolls back before the journal, forward after) with the rejected alternatives from research.md `docs/project_notes/decisions.md` +- [x] T002 [P] Add a shared host-level fault-injection helper — a `StacWriter` wrapper that throws a `StacWriterError` on the Nth underlying write — for the saveSession/open integration tests `apps/vscode/tests/unit/helpers/saveFaultInjection.ts` + +**Checkpoint**: design recorded; test scaffolding ready. (Adaptor-internal tests inject failures at the `node:fs` / IndexedDB seam inline — see their tasks.) + + +## Phase 2: Foundation (blocks all stories) + +Extend the shared boundary contract. After this phase the monorepo type-checks with the new interface members stubbed everywhere; the two real adaptors get real implementations in the story phases. + +**⚠️ No story can be implemented until this phase is complete** — both adaptors and both call sites depend on the interface and the `SaveJournal` shape. + +- [x] T003 Add `commitPlotSave` + `reconcilePlotSave` method signatures and the `CommitPlotSaveInput`/`CommitPlotSaveResult`/`ReconcilePlotSaveInput`/`ReconcilePlotSaveResult` types to the `StacWriter` interface, matching `contracts/stac-writer-commit.ts` — `thumbnails` MUST be `Pick` (Article IV.5, no re-listing) `shared/stac-writer/src/interface.ts` +- [x] T004 [P] Add the internal FS-only `SaveJournal` type (`version`, `stacItemPath`, `createdAtMs`, `renames[]`) and a typed `parseSaveJournal()` validator that narrows untyped JSON with no `any` (Article XV.5) `apps/vscode/src/services/saveJournal.ts` +- [x] T005 Add stub implementations of `commitPlotSave`/`reconcilePlotSave` (throwing "not yet implemented") to every non-adaptor `StacWriter` test double / mock so the workspace type-checks; find them with a search for `satisfies StacWriter` / `: StacWriter` — NONE found (all test doubles use `Partial` or the `saveFaultInjection` Proxy, which auto-covers new methods) +- [x] T006 [P][test] Type-contract test asserting `CommitPlotSaveInput.thumbnails` stays structurally derived from `WritePlotThumbnailPairInput` (compile-time `Pick` guard) so the boundary type cannot silently drift `shared/stac-writer/src/__tests__/commitPlotSave.types.test.ts` + +**Checkpoint**: interface extended, `SaveJournal` typed, workspace green (`pnpm -r typecheck`). + + +## Phase 3: User Story 1 — A failed save never corrupts my plot (P1) + +**Goal**: `commitPlotSave` commits the whole save unit atomically on both hosts, so a failure at any write step leaves the plot observable as exactly one coherent version (FR-001/002/003/004/010). + +**Independent test**: Drive a save while injecting a failure at each distinct write step (feature collection, item metadata, each thumbnail). After every injected failure, read the item back and assert it loads as exactly one coherent version, with features and metadata/thumbnails agreeing; a pre-commit failure leaves the previous version byte-identical. + +### Tests (write first — Article VII) + +- [x] T007 [P][test] FS `commitPlotSave` atomicity tests — against a real temp dir (`fs.mkdtempSync`), inject a failure at each phase (stage / journal-write / apply-rename); assert: pre-commit failure → originals byte-identical + no stray `.tmp`; success → features.geojson + item.json (with thumbnail asset entries) + both PNGs all reflect the new state; covers contract C1/C2 `apps/vscode/tests/unit/stacWriterFs.commitPlotSave.test.ts` +- [x] T008 [P][test] IDB `commitPlotSave` atomicity tests — with `fake-indexeddb`, spy on `db.transaction` to assert exactly **one** transaction per save; abort it and assert the store is byte-identical to before; success commits item record + geojson payload together; covers contract C4 `apps/web-shell/src/services/__tests__/stacWriterIdb.commitPlotSave.test.ts` + +### Implementation + +- [x] T009 Implement `commitPlotSave` in the FS adaptor: build the updated `item.json` (existing thumbnail-asset logic + ensure the features asset entry), stage every artefact as `..tmp` via the existing `atomicWriteSync` temp step, atomically write the `SaveJournal` (the commit point), apply the `temp → final` renames, delete the journal; on any pre-journal failure delete the temps and throw (originals untouched) `apps/vscode/src/services/stacWriterFs.ts` +- [x] T010 Implement `commitPlotSave` in the IDB adaptor: open one `readwrite` transaction over `items` + `payloads` + `assets` + `meta`, enqueue the item record + geojson payload (+ any binary assets), `await tx.done`; route the bundled-plot edit case through the already-atomic `patchItem` logic inside the same transaction (resolves research open-item #1) `apps/web-shell/src/services/stacWriterIdb.ts` +- [x] T011 Route the VS Code save through `commitPlotSave`: replace the raw `storeFeatureCollection` (`fs.writeFileSync`) and the separate `storeThumbnails` call with a single `await writer.commitPlotSave({ ctx, stacItemPath, featureCollection, thumbnails? })`; thumbnail **capture** failure stays non-fatal (omit `thumbnails`). This moves the feature-collection write onto the boundary (FR-004 / Article IV.2) `apps/vscode/src/commands/saveSession.ts` +- [x] T012 Route the web-shell save through `commitPlotSave`: replace the sequential `writeItem()` + `writeAsset()` pair with one `commitPlotSave(...)` `apps/web-shell/src/mocks/stacService.ts` +- [x] T013 [P] Align `captureScene`'s feature-collection write onto the atomic boundary — have `defaultWriteFeatureCollection` delegate to `commitPlotSave` (thumbnails omitted), or record in the ADR why scene-capture is deferred (resolves research open-item #2) `apps/vscode/src/commands/captureScene.ts` + +### Story integration test + +- [x] T014 [P][test] saveSession integration — inject a writer failure (reuse `saveFaultInjection` + the `ReadOnlyFilesystemError` sim); assert the previously-persisted plot still opens unchanged and no partial is observable `apps/vscode/tests/unit/saveSession.commit.test.ts` + +**Checkpoint**: a save is all-or-nothing on both hosts; catchable failures never corrupt the plot. US1 independently testable. + + +## Phase 4: User Story 2 — Honest reporting and a safe retry (P2) + +**Goal**: Success is reported only after the whole save commits; on failure the analyst sees a clear failure, the unsaved-changes indicator stays set, and the previous version is intact (FR-005/006). + +**Independent test**: Inject a failure during a save and assert (a) a failure is surfaced, (b) the dirty indicator remains set, (c) no success message appears, (d) the previous plot still opens. On a clean save, assert success fires exactly once, after commit. + +**Depends on**: Phase 3 (the save now routes through `commitPlotSave`, which resolves/rejects honestly). + +### Tests (write first) + +- [x] T015 [P][test] saveSession reporting-order tests — assert `markClean()` and the "Plot saved" message fire **only after** `commitPlotSave` resolves, and that a rejected commit leaves the dirty flag set, surfaces a failure message, and shows no success message; covers contract C3 (SC-003) `apps/vscode/tests/unit/saveSession.reporting.test.ts` + +### Implementation + +- [x] T016 Move `markClean()` and the "Plot saved" `showInformationMessage` to **after** the `commitPlotSave` `await` resolves; in the catch path surface a clear failure (`showErrorMessage`) and leave the dirty state untouched so the analyst can retry `apps/vscode/src/commands/saveSession.ts` +- [x] T017 Honest reporting on the web-shell save path — the only `commitPlotSave` caller is `createStandaloneItem` (no live plot-dirty Save handler exists in `App.tsx`; `handleSaveResult` saves CSV result assets, not plots). It registers the item in the in-memory catalog **only after** `commitPlotSave` resolves and returns `null` on rejection (no partial 'saved' state) — honest reporting enforced at the service boundary `apps/web-shell/src/mocks/stacService.ts` + +**Checkpoint**: "Plot saved" is trustworthy; a failed save is honest and retryable on both hosts. US2 independently testable. + + +## Phase 5: User Story 3 — Coherent plot after an interrupted save (P3) + +**Goal**: An uncatchable interruption (crash / OOM / power loss) is healed on next open — `reconcilePlotSave` runs before the read, auto-restoring the last good version (pre-commit) or completing the new one (post-commit), with a non-blocking notice when it acts (FR-007/008, Clarifications Q2/Q3). + +**Independent test**: Seed the on-disk/on-store state in each mid-save condition (temps but no journal; journal present with renames pending; clean), open the plot, and assert it resolves to a single coherent state and that any recovery is reported non-blockingly and leaves no `.tmp`/journal behind. + +**Depends on**: Phase 2 (`SaveJournal` shape) and Phase 3 (FS commit writes the journal `reconcile` consumes). + +### Tests (write first) + +- [x] T018 [P][test] FS `reconcilePlotSave` tests — seed each leftover condition in a temp dir: temps + no journal → `rolled-back` (originals kept, temps gone); journal + pending renames → `rolled-forward` (new version, journal gone); clean → `clean` no-op; assert idempotency (second call is `clean`) and that nothing partial is ever read; covers contracts C3/C5 (SC-002) `apps/vscode/tests/unit/stacWriterFs.reconcile.test.ts` +- [x] T019 [P][test] IDB `reconcilePlotSave` tests — clean store → `{ recovered:false, outcome:'clean' }`, mutates nothing; optional orphan-overlay prune path `apps/web-shell/src/services/__tests__/stacWriterIdb.reconcile.test.ts` + +### Implementation + +- [x] T020 Implement `reconcilePlotSave` in the FS adaptor: inspect the item dir for a `SaveJournal` and/or stray `.tmp` files; no journal → delete temps, keep originals (`rolled-back`); journal present → re-apply pending `temp → final` renames idempotently then delete the journal (`rolled-forward`); clean → no-op; return `{ recovered, outcome }` `apps/vscode/src/services/stacWriterFs.ts` +- [x] T021 Implement `reconcilePlotSave` in the IDB adaptor: return `clean` (IndexedDB never exposes partial transaction state); optionally prune an orphaned overlay-only record and report `rolled-back` `apps/web-shell/src/services/stacWriterIdb.ts` +- [x] T022 Wire reconcile into the VS Code open path **before** `loadPlotData` (`openPlot.ts:155`); when `recovered`, show a non-modal `vscode.window.showWarningMessage('Recovered an interrupted save — opened the last good version of this plot.')` `apps/vscode/src/commands/openPlot.ts` +- [x] T023 Wire reconcile into the web-shell open path **before** the `catalogReadView` read; on `recovered`, surface the existing non-blocking toast `apps/web-shell/src/services/catalogReadView.ts` + +### Story integration test + +- [x] T024 [test] Open-path integration — seed an "interrupted save" fixture (staged temps + journal), open the plot, assert it hydrates coherently and the recovery notice fired once `apps/vscode/tests/unit/openPlot.reconcile.test.ts` + +**Checkpoint**: every interruption point resolves to a coherent plot on reopen, with a quiet notice. US3 independently testable. All three stories complete. + + +## Phase 6: Polish & Cross-Cutting Concerns + +**Depends on**: Phases 3–5 complete. + +### Verification + +- [x] T025 Run the full gate (`task verify` = ruff + pyright + ESLint + tsc + pytest + vitest); all green before evidence capture. Confirm SC-004 informally (a normal save still feels instant) and confirm no Article IV ESLint regression +- [x] T026 [test] Web-shell happy-path E2E smoke — edit a plot, **Save**, reopen, assert it loads coherently with the new state (regression guard for FR-011; no fault injection — that lives in unit/integration) `apps/web-shell/playwright/tests/save-atomicity.spec.ts` + +> **⚠️ PLAYWRIGHT WORKS IN CLOUD SESSIONS** — Do NOT skip T026. The project bundles Linux Chromium via `@sparticuz/chromium`; the CDN download 403 is expected and worked around. Run `cd apps/web-shell && node run-playwright.mjs save-atomicity`. Details: `docs/project_notes/playwright-installation-research.md`. + +### Evidence Collection + +- [x] T027 Capture test results using the template (`.specify/templates/evidence/test-summary-template.md`) — YAML front matter (`feature`, `captured_at`, `git_sha`, `tests_passed/failed/skipped`, `coverage_pct`); body notes the SC-004 save-duration observation `specs/268-save-atomicity/evidence/test-summary.md` +- [x] T028 Create the usage demonstration — a save that fails mid-write leaving the plot intact, and an interrupted save auto-recovering on open (code/test excerpts + expected behaviour) `specs/268-save-atomicity/evidence/usage-example.md` +- [x] T029 [P] Capture the fault-injection matrix — a table of each injected failure point (stage / journal / apply × VS Code fs and web-shell idb) → observed coherent outcome, citing the tests that prove it (SC-001/002/003/005) `specs/268-save-atomicity/evidence/fault-injection-matrix.md` + +### Media Content + +- [x] T030 Create the feature blog post via the Content Specialist agent — copy `What We're Building` / `How It Fits` / `Key Decisions` **verbatim** from `evidence/opening-context.md`, lift the mermaid Hook to the top, and write `By the Numbers` (from test-summary), `Lessons Learned`, `What's Next` from evidence; track `[credibility]` `specs/268-save-atomicity/media/shipped-post.md` + +### PR Creation + +- [x] T031 Feature PR #658 updated with the full implementation + evidence (title/body rewritten via GitHub MCP — in-session scope is `debrief/debrief-future` only). Blog publish to debrief.github.io is the dev's manual follow-up via `/publish-future-post` + +**Task T031 must run last** — it depends on every evidence and media task above being complete. + + +## Dependencies + +**Phase order**: Setup (1) → Foundation (2) → US1 (3) → US2 (4) → US3 (5) → Polish (6). + +**Hard dependencies**: +- **Foundation (T003–T006) blocks everything** — the interface members and `SaveJournal` type must exist (with stubs, T005) before any adaptor or call-site edit compiles. +- **US2 (P2) depends on US1 (P1)** — honest reporting (T015–T017) wraps the `commitPlotSave` call that US1 introduces. +- **US3 (P3) depends on US1 (P1)** — the FS `reconcilePlotSave` (T020) consumes the `SaveJournal` that FS `commitPlotSave` (T009) writes; they must share one journal format (defined in T004). +- **Within each story: tests precede implementation** (Article VII) — T007/T008 before T009–T013; T015 before T016/T017; T018/T019 before T020–T023. +- **Polish (Phase 6) depends on US1–US3**; **T031 (`/speckit.pr`) is last** and depends on all evidence + media tasks. + +**Story completion order**: US1 → US2 → US3 (priority order, which is also the natural build order: engine → honest reporting → recovery). Each story is independently testable at its checkpoint; US1 alone already delivers the core "a failed save never corrupts" guarantee for catchable failures. + +**Parallel opportunities**: +- Setup: T002 ‖ T001. +- Foundation: T004 ‖ T006 (T003 first; T005 after T003). +- US1 tests: T007 (fs) ‖ T008 (idb). US1 impl: T009 (fs) ‖ T010 (idb) — different files; the two call-site routings T011/T013 (vscode, different files) ‖ T012 (web-shell). T014 ‖ once T011 lands. +- US3 tests: T018 (fs) ‖ T019 (idb). US3 impl: T020 (fs) ‖ T021 (idb); T022 (vscode open) ‖ T023 (web-shell open). +- Polish: T029 ‖ T027/T028 (after tests pass). +- **Not parallel**: T011 and T016 both edit `saveSession.ts` (US1 routing then US2 reporting-order) — sequential. T009 and T020 both edit `stacWriterFs.ts` — sequential. T010 and T021 both edit `stacWriterIdb.ts` — sequential. + + +## Implementation Strategy + +**MVP = Foundation + US1.** Extending the boundary (Phase 2) and delivering `commitPlotSave` on both hosts (Phase 3) is the smallest shippable increment that closes the core defect: a save becomes all-or-nothing and the feature-collection write moves onto the boundary. It is independently testable and independently valuable — stop here and the headline corruption risk for *catchable* failures is gone. + +**Then layer US2, then US3.** US2 (honest reporting) is a thin, high-trust follow-up on top of US1 — small edits in the two host save handlers. US3 (reconcile-on-open) adds resilience to *uncatchable* interruptions; it is the rarer-but-nastier case (P3) and builds on the journal US1 already writes. + +**Test-first throughout.** The fault-injection acceptance tests *are* the definition of done (contracts C1–C5 ⇄ SC-001..005). Write each story's tests before its implementation; a story's checkpoint is "its tests are green and it is independently demonstrable." + +**Boundary discipline.** All persistence stays behind `StacWriter` (Article IV.4); the only frontend changes are *orchestration* (which writer call, when to report success, when to reconcile) — no frontend touches `fs`/IndexedDB directly. Keep `CommitPlotSaveInput.thumbnails` structurally derived (T006 guards this) so the save unit can't silently drop fields as the thumbnail input grows (Article IV.5 / ADR-033). + +**Durability target is deliberate.** Do not add fsync/durability machinery (Clarifications Q3) — the guarantee is atomicity/coherence; the existing best-effort temp→rename and IndexedDB transaction suffice. This keeps US3 simple: reconcile only ever has to choose *which coherent version*, never repair a torn one. + +**Incremental delivery checkpoints**: after Phase 2 (workspace green, interface extended) → after Phase 3 (MVP: atomic save, both hosts) → after Phase 4 (trustworthy success/failure) → after Phase 5 (crash-resilient) → after Phase 6 (evidence + blog + PR). +