diff --git a/src/extension/document-write/execute-write.ts b/src/extension/document-write/execute-write.ts index 1ebe9a52..a24181e0 100644 --- a/src/extension/document-write/execute-write.ts +++ b/src/extension/document-write/execute-write.ts @@ -29,8 +29,13 @@ // (see the adapter's doc comment). Every read/build/apply is injected; the module // never re-reads outside the adapter, and — the caller contract — the returned // outcome CARRIES its verification-time snapshots so callers map from those -// fields and NEVER re-read the document (a wrapper re-read can observe a later -// edit and mis-attribute divergence). +// fields and never re-read the document CONTENT (a wrapper content re-read can +// observe a later edit and mis-attribute divergence). The session wrapper's +// guarded settlement-dispatch `readVersion()` retry is the single documented +// exception — the version labels the settlement and feeds the reducer's +// version-delta epoch verdict, never the byte-level divergence compare; it is +// safe because no document event can interleave between the pipeline's settle +// and the dispatch on the single-threaded extension host. import { perfNow, perfRecord } from "../../shared/perf.js"; import type { MinimalEditSpan } from "./minimal-edit.js"; @@ -102,7 +107,9 @@ export type DocumentWriteTag = | "applyRejected"; // apply() promise rejected → reducer `rejected` /** Immutable verified-write outcome. Carries the four verification-time - * snapshots so callers map WITHOUT re-reading the document. Contents are + * snapshots so callers map WITHOUT re-reading the document CONTENT (the + * session wrapper's guarded version read is the documented exception — see + * the module header). Contents are * canonical (EOL-normalised to the document's EOL). EVERY terminal outcome — * including `buildThrew`, which never touched the document — populates all four * fields, but the two SETTLE-time ones are NULLABLE: `null` means the read threw @@ -207,12 +214,16 @@ export async function executeDocumentWrite( // settled document IS edit #1's exact result") pass with no observation // behind it, so a stash could clobber an external edit that the verified // path deliberately lets win. - // - A numeric version sentinel (`-1`) would be assigned VERBATIM by the - // settlement `ok` self-advance and REWIND the version. + // - A numeric version sentinel (`-1`) is CLAMPED AWAY by the settlement's + // `Math.max` advance, so it could not rewind the label — it would do + // something worse: ANY fabricated number satisfies `settledVersion !== null` + // and FABRICATES the `ackLabelObserved` observation, so the settlement acks + // LIVE bytes under a made-up label. // Every consumer is therefore forced by the compiler to answer for `null`, and - // each answers conservatively: no self-advance, no epoch bump ("missing - // snapshot ⇒ foreign" is a REJECTED variant — it drops the webview's replay - // buffer), no drain. + // each answers conservatively PER MISSING OBSERVATION: content unobserved ⇒ no + // drain, and the epoch verdict falls back to POSITIVE version-delta evidence + // ("missing evidence ⇒ foreign" stays the REJECTED variant); version unobserved + // ⇒ no advance and the ack is WITHHELD, never posted at a stale label. const settle = (tag: DocumentWriteTag, message?: string): DocumentWriteOutcome => { const verifyStart = QUOLL_PERF ? perfNow() : 0; const readFailures: string[] = []; diff --git a/src/extension/session/effect-executor.ts b/src/extension/session/effect-executor.ts index 643befc8..53d44f56 100644 --- a/src/extension/session/effect-executor.ts +++ b/src/extension/session/effect-executor.ts @@ -43,6 +43,18 @@ import type { HostSessionState, } from "./host-session-core.js"; +// One user-facing message for BOTH "the webview could not be resynced" families +// (reseed build failure / withheld settlement ack) — same incident semantics, +// same latch, so the wording must stay true for both: the view could not be +// updated, and unsaved changes MAY not have been saved. It must also stay true +// on both paths the RESEED trigger serves: the reducer emits `postDocument` for +// the FIRST SEED too (host-session-core's `ready` arm), not only for settlement +// acks, and the executor cannot tell them apart — so an unconditional "Recent +// edits may not be saved" would tell a user their edits might be lost at first +// load, before they had typed anything. +const RESYNC_FAILURE_MESSAGE = + "Quoll could not update the editor view. If you have unsaved changes they may not have been saved — reload the window (Developer: Reload Window)."; + /** The VS Code build+apply+verify seam for the write executor (Plan S6). The * pipeline itself lives in `document-write/execute-write.ts`; this alias keeps * the panel's inline wiring + the executor deps stable. `TEdit` is the edit @@ -104,16 +116,56 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // first. let hostMountReported = false; - // Per-panel latch for the reseed-build failure notification (see the - // `postDocument` guard). One notification ATTEMPT per INCIDENT — a persistently - // broken document seam can fire once per settlement, and a toast per settlement - // is user-visible spam. The latch is RE-ARMED by a successful build (see the - // `postDocument` case): a success proves the seam recovered, so the next failure - // is a new incident and deserves its own signal. Without that, one transient - // hiccup early in a panel's life would consume the session's only user-visible - // signal for a state this module documents as one that must NOT be silent — and - // panels live for hours. - let reseedBuildFailureReported = false; + // Per-panel latch shared by TWO triggers that both mean "the webview could not + // be resynced": the `postDocument` reseed-build failure guard, and + // `showResyncFailure` (a withheld settlement ack — host-session-core's + // ackLabelObserved gate). One notification ATTEMPT per INCIDENT — either + // failure mode can recur once per settlement, and a toast per settlement is + // user-visible spam. The latch is RE-ARMED by a successful `postDocument` + // build (see that case): a success proves the seam recovered, so the next + // failure is a new incident and deserves its own signal. Without that, one + // transient hiccup early in a panel's life would consume the session's only + // user-visible signal for a state this module documents as one that must NOT + // be silent — and panels live for hours. + let resyncFailureReported = false; + + // The ONE place that spends that latch — both triggers report their incident + // through here, so the protocol below cannot drift between them. `failureLog` + // is the only thing the two paths differ in: the trigger-specific label for a + // toast that itself throws. + // + // The latch is set BEFORE the attempt, so the guarantee is "at most ONE + // notification attempt per incident" — not "exactly one toast". + // ⛔ Do NOT move this to latch-after-success. Both placements lose something + // and this is the safer loss: + // - latch-before: a single synchronous failure leaves only the caller's log + // line. Bounded, and by then the window API is broken. + // - latch-after-success: a `showError` that DISPLAYS and then throws is never + // latched, so a persistently broken seam re-toasts on every failure — + // user-visible spam on a path that can fire once per settlement. + // `showError` evaluates `window.showErrorMessage(message)` BEFORE + // `showSafely` wraps it, and `showSafely` only absorbs the Thenable's async + // rejection, so display-then-throw cannot be ruled out from this repo alone. + // Spam is the worse failure, and this placement removes it structurally rather + // than by argument about VS Code internals. + // + // GUARDED: BOTH callers sit INSIDE the boundary that exists to stop a throw + // from escaping `runEffects` (the `postDocument` build-failure guard and the + // `showResyncFailure` effect), and `window.showErrorMessage`'s SYNCHRONOUS + // throw is not absorbed by the panel's wrapper — an unguarded call here would + // re-open the exact hole this closes. Same discipline as + // revert-rescue-wiring's per-dep `runGuarded`. + const reportResyncFailure = (failureLog: string): void => { + if (resyncFailureReported) { + return; + } + resyncFailureReported = true; + try { + deps.showError(RESYNC_FAILURE_MESSAGE); + } catch (err) { + console.error(failureLog, err); + } + }; // Alias for the injected open-external delegate (see the `openExternal` effect // case for why it is called via this local rather than `deps.openExternal(...)`). @@ -177,6 +229,72 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef ); }; + // Guarded ONE-SHOT retry of the version read at settlement DISPATCH — the + // single documented exception to "callers never re-read the document + // CONTENT" (the caller contract is narrowed to content by this change). Be + // precise about what the version is used for: it LABELS the ack, feeds the + // Math.max advance, and is an input to the reducer's content-unobserved + // version-delta epoch verdict. It never enters the byte-level divergence + // compare. Re-reading it here is sound because this callback runs microtasks + // after the pipeline's own settle-time read with no possibility of an + // interleaved document event on the single-threaded extension host — the + // retry observes the same live version the settle read would have, so it + // cannot attribute a LATER edit to this settlement. The retry is what keeps a + // TRANSIENT settle-time failure on the normal path (ack at the live version) + // instead of the withhold branch; a PERSISTENT failure yields null, and the + // reducer then withholds UNLESS a lock-held resync already raised the label + // (the second disjunct of `ackLabelObserved`). Guarded because this runs while + // BUILDING the settlement event — an unguarded throw here would skip the + // dispatch and strand the write lock (same placement rule as readCanWrite). + // + // Name it `readVersionGuarded` (NOT "retry"): it is the ONE guarded version + // reader, with ONE contract — `number | null`, null ⇔ unobserved, never a + // fabricated value — serving THREE call families, only one of which is a + // retry: + // 1. the resolved settlement's RETRY (`runApplyEdit`'s fulfilment arm), the + // one the paragraph above describes: the pipeline already read the version + // once and that read threw. + // 2. the pipeline-REJECTION arm's FIRST read: that settlement never reached a + // settle-time read at all, so this is the only version read it ever makes + // (not a second chance at one). + // 3. `sendEditRejected`'s three recovery dispatches below (sync throw / + // delivery refused / delivery rejected), labelling the reseed that clears + // a stuck rejection. + // The consequences differ per family, so the call site is NAMED and travels on + // the warn: without it three unrelated outcomes collapse into one log line and + // triage cannot tell "the retry lost a transient" from "the recovery reseed was + // withheld". + // + // The roster IS the contract: a closed set of triage tokens, one per call + // SITE (not one per family — family 3 alone owns three of the five, one per + // recovery dispatch). Typing it as a union (not `string`) makes ONE of the two + // invariants the compiler's job: an off-roster token is rejected. The other + // stays a CONVENTION the compiler cannot hold — pass a LITERAL, never a + // computed expression, because this runs on a failure path and must not + // evaluate anything that can throw (a helper returning the union, or a ternary + // over two valid tokens, type-checks fine and would re-open exactly that hole). + // The union also cannot catch a copy-paste that stamps one VALID token onto + // the wrong arm (the three adjacent recovery sites are exactly that shape), so + // each site also has a per-site assertion in `effect-executor.test.ts`. + type GuardedVersionReadSite = + | "settlement-retry" + | "rejection-arm-first-read" + | "edit-rejected-recovery:sync-throw" + | "edit-rejected-recovery:refused" + | "edit-rejected-recovery:rejected"; + const readVersionGuarded = (site: GuardedVersionReadSite): number | null => { + try { + return deps.applyEditSeam.readVersion(); + } catch (err) { + console.warn( + "[quoll] guarded readVersion failed; the label stays UNOBSERVED (null — never a fabricated number)", + { site }, + err + ); + return null; + } + }; + // Edit-rejected delivery with a resync fallback re-entering the core, // carrying the per-delivery `id` (Codex N2/N6). If the webview refuses, // detaches, or `send()` throws, dispatching `editRejectedDeliveryFailed(id)` @@ -188,9 +306,10 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // clobber the live banner nor force an unsolicited reseed. When the clear // DOES fire, the user's typed content is overwritten — same "external wins" // semantics as for an `onDidChangeTextDocument` race. The event carries the - // LIVE document version (readVersion at this dispatch, read synchronously with - // the reseed's live bytes) so the recovery Document's version matches its - // bytes — never the possibly-stale stored version. + // document version read via the guarded `readVersionGuarded`; if that read + // fails the event carries `null` and the reducer clears the rejection + // WITHOUT reseeding — no Document at an unobserved label — signalling + // through the shared resync-failure latch. const sendEditRejected = (error: MarkdownError, id: number): void => { if (deps.isDisposed()) { return; @@ -210,7 +329,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef deps.dispatch({ type: "editRejectedDeliveryFailed", id, - documentVersion: deps.applyEditSeam.readVersion(), + documentVersion: readVersionGuarded("edit-rejected-recovery:sync-throw"), }); return; } @@ -234,7 +353,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef deps.dispatch({ type: "editRejectedDeliveryFailed", id, - documentVersion: deps.applyEditSeam.readVersion(), + documentVersion: readVersionGuarded("edit-rejected-recovery:refused"), }); }, (err: unknown) => { @@ -245,7 +364,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef deps.dispatch({ type: "editRejectedDeliveryFailed", id, - documentVersion: deps.applyEditSeam.readVersion(), + documentVersion: readVersionGuarded("edit-rejected-recovery:rejected"), }); } ); @@ -265,12 +384,11 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // completed (an apply resolved ok, or the no-op short-circuit submitted // nothing) and only the verification read failed. Mapping it to a failure // kind would toast "Failed to save" for a write that did not fail, and - // skip the self-advance. `documentVersion` rides through as `null` when the - // version was not observed — the reducer then leaves the version alone (no - // fabrication, no rewind); when it WAS observed the normal self-advance - // applies. + // skip the self-advance. The settled version rides on the EVENT, not the + // outcome (one representation for every outcome kind — v9 unification), so + // this outcome carries no version at all. case "appliedUnverified": - return { kind: "ok", documentVersion: result.settledVersion }; + return { kind: "ok" }; case "applyRefused": return { kind: "refused" }; case "buildThrew": @@ -346,21 +464,24 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // write pipeline. The lock is already set by the `accept` transition; the // pipeline (snapshot → span → build → apply → post-apply verify) lives in // `executeDocumentWrite`, and this only MAPS the immutable tagged outcome onto - // an `applyEditSettled` event. It NEVER re-reads the document — `currentContent` - // / `preApplyContent` / the settled version all come from the outcome's - // verify-time snapshots (a re-read could observe a later edit and mis-attribute - // divergence). `canWrite` is read here (an FS/config read, not a document read) - // for the stash-drain re-gate. The settlement lands in a fresh drain (the - // pipeline is async) and fires EVEN post-dispose: a stashed one-more-char edit - // can only drain on settlement, which fires AFTER onDidDispose (the core stays - // a strict no-op post-dispose unless a stash is waiting; webview-bound posts - // self-suppress via post()'s disposed guard). + // an `applyEditSettled` event. It never re-reads the document CONTENT — + // `currentContent` / `preApplyContent` come from the outcome's verify-time + // snapshots (a content re-read could observe a later edit and mis-attribute + // divergence); the guarded `readVersionGuarded` version read is the single + // documented exception, sound for the reasons at its definition. `canWrite` + // is read here (an FS/config read, not a document read) for the stash-drain + // re-gate. The settlement lands in a fresh drain (the pipeline is async) and + // fires EVEN post-dispose: a stashed one-more-char edit can only drain on + // settlement, which fires AFTER onDidDispose (the core stays a strict no-op + // post-dispose unless a stash is waiting; webview-bound posts self-suppress + // via post()'s disposed guard). const runApplyEdit = (content: string): void => { void executeDocumentWrite(deps.applyEditSeam, content).then( (result) => { deps.dispatch({ type: "applyEditSettled", outcome: toApplyEditOutcome(result), + settledVersion: result.settledVersion ?? readVersionGuarded("settlement-retry"), canWrite: readCanWrite(), currentContent: result.settledContent, preApplyContent: result.preApplyContent, @@ -376,9 +497,10 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // rejection. // Keyed on `settleReadFailure` rather than on the single // `appliedUnverified` tag, because a VERSION-only read failure keeps the - // tag `applied` (the content was verified) while still suppressing the - // self-advance — exactly the partial verification loss triage needs to - // see, and tag-keyed logging would make it silent. + // tag `applied` (the content was verified) while still putting the + // self-advance at risk — it is suppressed only if the guarded dispatch + // retry above ALSO fails. That partial verification loss is what triage + // needs to see, and tag-keyed logging would make it silent. // // Bounded to the ok-mapping family on purpose. A failure tag already // reports itself through its own message and its "Failed to save" toast; @@ -405,14 +527,15 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // It must not deliver a VERDICT on the save either ("treating it as an // UNVERIFIED save" was the old wording): on the VERSION-only path the // CONTENT was read, the divergence compare ran and the tag stayed - // `applied` — the save WAS verified, and only the self-advance is - // suppressed. So name WHICH observation is missing and let each one gate - // its own consequence. Naming the tag here would mislead symmetrically: + // `applied` — the save WAS verified, and only the self-advance is at + // risk (suppressed only if the guarded dispatch retry also failed). So + // name WHICH observation is missing and let each one gate its own + // consequence. Naming the tag here would mislead symmetrically: // `diverged` is only reachable WITH an observed content, so "the tag is // now appliedUnverified" is false for part of this very family. console.warn( okFamily - ? "[quoll] the write pipeline completed (no failure) but a settle-time verification read failed. Each missing observation gates only its OWN consequence: no drain unless the settled CONTENT was read (settledContent !== null), no version advance unless the VERSION was read (settledVersion !== null)" + ? "[quoll] the write pipeline completed (no failure) but a settle-time verification read failed. Each missing observation gates only its OWN consequence: no drain unless the settled CONTENT was read (the event's currentContent !== null), no version advance unless SOME source observed the version (the event's settledVersion !== null — the pipeline's settle read or the guarded dispatch retry), and the ack Document is withheld unless some source observed a post-apply version" : `[quoll] the settlement verification read also failed on a ${result.tag} outcome; the outcome itself is unchanged`, result.settleReadFailure ); @@ -453,23 +576,31 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // sole other path) — stays held for the session: every later inbound edit is // stashed behind a bare warn and never saved. Silent, toast-free data loss. // Settling with a NON-OK outcome is what makes it safe: `canDrain` requires - // `ok`, so the unobserved snapshot below never reaches `decideEdit`, and the - // non-ok foreign-bytes check reads a `null` `currentContent` as NOT OBSERVED - // ⇒ not foreign, so no spurious epoch bump. The - // user gets the same `Failed to save:` toast + authoritative reseed as any - // other failed write, instead of a panel that has quietly stopped saving. + // `ok`, so the unobserved snapshot below never reaches `decideEdit`, and + // the content check reads `null` as unobserved; foreign evidence, if any, + // comes from the version-delta fallback. The user gets the same + // `Failed to save:` toast as any other failed write, instead of a panel + // that has quietly stopped saving. The reseed that normally follows it is + // CONDITIONAL, and this arm is the likeliest place to lose it: the guarded + // read below is the settlement's ONLY version read, so if it also fails and + // no lock-held resync arrived, the reducer withholds the ack Document and + // reports through the shared resync-failure latch instead. // - // ⚠️ This arm MUST NOT re-read the document or `canWrite()` — those seams are - // the candidate throw sources, and a throw HERE strands the lock exactly as - // before (the "fix" would reintroduce the bug on its own recovery path). - // `canWrite` is unused for a non-ok settlement, so pass the conservative - // `false` rather than reading it. + // ⚠️ This arm MUST NOT re-read the document CONTENT or `canWrite()` — + // those seams are the candidate throw sources, and a throw HERE strands + // the lock exactly as before (the "fix" would reintroduce the bug on its + // own recovery path). The guarded `readVersionGuarded` is the one + // permitted read: it cannot throw, and the version it returns labels the + // settlement (see its definition for why that use is sound). `canWrite` + // is unused for a non-ok settlement, so pass the conservative `false` + // rather than reading it. (err: unknown) => { console.error("[quoll] verified write pipeline rejected; releasing the write lock", err); try { deps.dispatch({ type: "applyEditSettled", outcome: { kind: "rejected", message: errorMessage(err) }, + settledVersion: readVersionGuarded("rejection-arm-first-read"), canWrite: false, // NOT OBSERVED — nothing was read, so say nothing rather than // fabricating an empty document. The non-ok foreign-bytes check reads @@ -565,43 +696,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef "[quoll] failed to build the Document to post; skipping this reseed", err ); - if (!reseedBuildFailureReported) { - // The latch is set BEFORE the attempt, so the guarantee is "at most - // ONE notification attempt per incident" — not "exactly one toast". - // ⛔ Do NOT move this to latch-after-success. Both placements lose - // something and this is the safer loss: - // - latch-before: a single synchronous failure leaves only the log - // line above. Bounded, and by then the window API is broken. - // - latch-after-success: a `showError` that DISPLAYS and then - // throws is never latched, so a persistently broken seam - // re-toasts on every failed reseed — user-visible spam on a path - // that can fire once per settlement. `showError` evaluates - // `window.showErrorMessage(message)` BEFORE `showSafely` wraps - // it, and `showSafely` only absorbs the Thenable's async - // rejection, so display-then-throw cannot be ruled out from this - // repo alone. - // Spam is the worse failure, and this placement removes it - // structurally rather than by argument about VS Code internals. - reseedBuildFailureReported = true; - // GUARDED: this call sits INSIDE the boundary that exists to stop a - // throw from escaping `runEffects`, and `window.showErrorMessage`'s - // SYNCHRONOUS throw is not absorbed by the panel's wrapper — an - // unguarded call here would re-open the exact hole this closes. Same - // discipline as revert-rescue-wiring's per-dep `runGuarded`. - try { - // Wording that is true on BOTH paths this effect serves. The - // reducer emits `postDocument` for the FIRST SEED too - // (host-session-core's `ready` arm), not only for settlement acks, - // and the executor cannot tell them apart — so an unconditional - // "Recent edits may not be saved" would tell a user their edits - // might be lost at first load, before they had typed anything. - deps.showError( - "Quoll could not update the editor view. If you have unsaved changes they may not have been saved — reload the window (Developer: Reload Window)." - ); - } catch (toastErr) { - console.error("[quoll] failed to report the reseed build failure", toastErr); - } - } + reportResyncFailure("[quoll] failed to report the reseed build failure"); break; } if (QUOLL_PERF) { @@ -609,11 +704,12 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef } // RE-ARM the notification latch: the build just succeeded, so the seam // recovered and any later failure is a NEW incident, not a repeat of the - // one already reported. Orthogonal to the latch-before-attempt decision - // above (which bounds a SINGLE incident); without this, one transient + // one already reported. Orthogonal to `reportResyncFailure`'s + // latch-before-attempt decision (which bounds a SINGLE incident, for + // BOTH triggers); without this, one transient // early hiccup would leave every later real incident structurally // silent for the life of the panel. - reseedBuildFailureReported = false; + resyncFailureReported = false; post(documentMessage); // First postDocument is the seed; report once it (and its // host:postMessage) is recorded so host:mount carries both stages. @@ -663,9 +759,9 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef runApplyEdit(effect.content); break; case "showError": - // GUARDED, and NOT redundant with the `deps.showError` guard inside the - // `postDocument` builder catch above: that one protects the executor's - // OWN reseed-build notification, this one the REDUCER's settlement toast, + // GUARDED, and NOT redundant with the `deps.showError` guard inside + // `reportResyncFailure`: that one protects the executor's OWN + // resync-failure notification, this one the REDUCER's settlement toast, // which every non-ok settlement emits BEFORE its `postDocument` (see // `settlementEffects`' ORDER note). The containment matters here since // `execute-write.ts`'s `settle()` became total: the correlated case — @@ -701,6 +797,16 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // guard able to flag a future raw binding call added here by mistake. runOpenExternal(effect.href); break; + case "showResyncFailure": + // A withheld settlement ack (host-session-core's ackLabelObserved gate). + // Same latch as the postDocument build-failure guard: both report "the + // webview could not be resynced", and one incident must not toast twice. + // Latch-before-attempt + guarded showError for the same reasons as the + // build guard (a throwing toast must neither escape runEffects nor + // retry within the incident) — which is why both go through the one + // `reportResyncFailure` above rather than each keeping a copy. + reportResyncFailure("[quoll] failed to report the withheld settlement ack"); + break; default: { // Exhaustiveness guard — a new HostSessionEffect variant without // a case here is flagged as `never` at compile time. diff --git a/src/extension/session/host-session-core.ts b/src/extension/session/host-session-core.ts index 954fe891..f5e736d4 100644 --- a/src/extension/session/host-session-core.ts +++ b/src/extension/session/host-session-core.ts @@ -95,12 +95,7 @@ export function isWriteLockHeld(state: HostSessionState): boolean { } export type ApplyEditOutcome = - // `documentVersion: null` ⇔ the settle-time version read threw, so no - // post-apply version was OBSERVED. The settlement then leaves - // `lastAppliedDocVersion` alone: a fabricated value (a `-1` sentinel) would be - // assigned VERBATIM here — the self-advance is the documented exemption from - // `resyncLiveVersion`'s `max` clamp — and REWIND the version. - | { readonly kind: "ok"; readonly documentVersion: number | null } + | { readonly kind: "ok" } | { readonly kind: "refused" } | { readonly kind: "constructThrew"; readonly message: string } | { readonly kind: "applyThrew"; readonly message: string } @@ -124,6 +119,17 @@ export type HostSessionEvent = | { readonly type: "applyEditSettled"; readonly outcome: ApplyEditOutcome; + // The settle-time OBSERVED document version — ONE representation for every + // outcome kind (v9 unification: `outcome.documentVersion` and a separate + // event field would let the self-advance and the ack gate read different + // values). `null` ⇔ NOT OBSERVED: EVERY guarded version read this + // settlement made failed. How many that is depends on the producer — TWO + // on the resolved path (the pipeline's settle-time read plus the executor's + // guarded dispatch retry), ONE on the pipeline-rejection arm, which never + // reaches a settle read at all. Never a fabricated number — + // the advance below is inside a `!== null` guard, so `null` cannot reach a + // version assignment. + readonly settledVersion: number | null; // Fresh live snapshots taken by the executor at settlement time so the // stash drain can re-run the FULL decideEdit gates (canWrite + canonical // current text) AND the epoch foreign-bytes check (site 2). Since S3a the @@ -136,19 +142,23 @@ export type HostSessionEvent = // landed), and the pipeline-rejection arm, which has no trustworthy // snapshot at all and MUST NOT re-read (that would strand the lock on the // recovery path — the read seams are the candidate throw sources). - // `null` is deliberate rather than a stand-in value: the foreign-bytes - // check below reads it as NOT FOREIGN, and `canDrain` refuses to drain - // without an OBSERVED equality. Sending fabricated bytes instead would flip - // `foreignAtSettle`, bump the epoch, and the resulting reseed would + // `null` is deliberate rather than a stand-in value: without OBSERVED + // bytes the foreign-bytes check below falls back to the version-delta + // evidence at site 2, and `canDrain` refuses to drain without an OBSERVED + // equality. Sending fabricated bytes instead would flip `foreignAtSettle` + // on a clean save, bump the epoch, and the resulting reseed would // invalidate the webview's replay buffer — silently dropping the very // keystrokes the failure toast tells the user to retry. - // ACCEPTED RESIDUAL RISK, now TYPED rather than accidental: with no - // observation the foreign-bytes check cannot fire, so a foreign edit that - // raced the apply is NOT detected here and the epoch does not advance. (It - // used to fall out of the rejection arm's two equal empties; the same - // verdict is now the explicit answer for "not observed".) The webview - // therefore keeps its replay buffer live and can re-post over that foreign - // edit on the user's retry. This is deliberate — the alternative + // ACCEPTED RESIDUAL RISK, now TYPED rather than accidental and NARROWER + // than before: the version-delta fallback (site 2) supplies positive + // foreign evidence whenever the version moved beyond our own + // contribution, so the residual narrows to "content unobserved AND delta + // ≤ own contribution" — a foreign edit raced the apply but left the + // version exactly where our own contribution would have. (It used to + // fall out of the rejection arm's two equal empties; the same verdict is + // now the explicit answer for "not observed".) In that narrower residual + // the webview keeps its replay buffer live and can re-post over the + // foreign edit on the user's retry. This is deliberate — the alternative // (re-reading the document to get honest bytes) goes through the seam that // just threw and strands the write lock, which is strictly worse. Note this // is about the EPOCH only; `null` reaching `decideEdit` is separately @@ -183,7 +193,11 @@ export type HostSessionEvent = // `false` here as proof of a clean apply and drop it as redundant. When true the // settlement routes through the ok-but-mismatch convergence shape (epoch++ // + authoritative resync + a distinct diverged log, NO error toast — a - // deliberate conflict resolution must not read as "save failed"). It is a + // deliberate conflict resolution must not read as "save failed"). The resync + // half still rides the ack-label gate: `diverged` proves the CONTENT read + // succeeded, NOT the version read (`settle()` guards the two separately), so + // an unobserved label swaps that Document for the withhold pair — the epoch + // bump and the diverged log run either way. It is a // belt-and-braces annotation: a genuine divergence ALSO trips the byte // compare below, but driving convergence off the explicit flag keeps the // reducer honest even if the compare is inconclusive. @@ -192,7 +206,11 @@ export type HostSessionEvent = | { readonly type: "editRejectedDeliveryFailed"; readonly id: number; - readonly documentVersion: number; + // `null` ⇔ the recovery read was unobserved (the executor's guarded + // readVersionGuarded failed) — the arm clears the rejection (a stuck + // pending rejection is the deadlock this event exists to break) but + // WITHHOLDS the recovery reseed rather than fabricating a label. + readonly documentVersion: number | null; } | { readonly type: "disposed" }; @@ -224,7 +242,10 @@ export type HostSessionEffect = | { readonly type: "applyEdit"; readonly content: string; readonly baseDocVersion: number } | { readonly type: "showError"; readonly message: string } | { readonly type: "logWarn"; readonly message: string; readonly detail: Record } - | { readonly type: "openExternal"; readonly href: string }; + | { readonly type: "openExternal"; readonly href: string } + // User-visible signal for a withheld settlement ack; the executor latches it + // per incident together with the reseed-build failure. + | { readonly type: "showResyncFailure" }; export interface HostSessionResult { readonly state: HostSessionState; @@ -275,9 +296,11 @@ function contentMatches(a: string, b: string | null): boolean { * lock-free advance is FOREIGN by construction (no self-apply is in flight, so * the webview did not produce it), whereas a lock-HELD advance is usually the * in-flight apply's own echo and is adjudicated by the settlement check - * instead. This is the SINGLE version-raising path in the reducer; the only - * other `lastAppliedDocVersion` write is the settlement `ok` self-advance (the - * sole documented exemption, fenced by the invariant test). */ + * instead. This is ONE of the reducer's TWO version-raising paths; the other is + * the settlement advance (`advanced` — `Math.max` over `event.settledVersion` + * for EVERY outcome kind since this PR, clamp-consistent with this helper and + * no longer an ok-only exemption). There are exactly two, and the invariant + * test's allowed-RHS roster is what fences a third from appearing. */ function resyncLiveVersion(state: HostSessionState, liveVersion: number): HostSessionState { const raised = Math.max(state.lastAppliedDocVersion, liveVersion); const foreignAdvance = @@ -300,32 +323,125 @@ const postDoc = (s: HostSessionState, docVersion: number): HostSessionEffect => epochGeneration: s.epochGeneration, }); -// Per-outcome settlement effects: the ack Document (+ non-ok diagnostics). -// Extracted so the applyEditSettled arm can SUPPRESS these wholesale when -// disposed (the webview is gone) and REPLACE them with drain effects when a -// stash drains. +// The withhold pair — what a settlement emits INSTEAD of its ack Document when +// no source observed a post-apply version. Not silent WHILE THE PANEL IS ALIVE: +// the logWarn is the triage record and showResyncFailure is the user-visible +// signal, latched per incident by the EXECUTOR (the reducer is pure and cannot +// hold a latch) — the same latch as the reseed-build failure, so the two +// "webview could not be resynced" families cannot double-toast one incident. +// POST-DISPOSE the pair never reaches the executor, by THREE different routes: +// the no-stash arm (`state.disposed && state.pendingEdit === null`, the early +// return in the `applyEditSettled` case) builds only failure toasts, so the pair +// is not even constructed; the undrainable arm keeps only `showError`s from the +// settlement effects; and a stash that DRAINS post-dispose never calls +// `ackEffects` at all (the drain's readonly/stale/no-op arm returns `[]` when +// disposed, and its accept / parse-failed arms post no Document). Deliberate in +// all three: there is no view left to resync, and the only loss worth reporting +// there (a dropped stash) has its own toast. +function withholdAckEffects( + settled: HostSessionState, + heldBase: number | null, + context: HostSessionContext +): HostSessionEffect[] { + return [ + { + type: "logWarn", + message: + "[quoll] settlement ack withheld: no post-apply document version was observed (every guarded version read for this settlement failed; no lock-held resync arrived) — posting would pair live bytes with a stale label", + detail: { + uri: context.uriString, + heldBase, + lastAppliedDocVersion: settled.lastAppliedDocVersion, + }, + }, + { type: "showResyncFailure" }, + ]; +} + +// The ack a settlement posts: the authoritative Document when the label rests on +// a real observation, the withhold pair when it does not. ONE owner for that +// choice — both ack sites (the per-outcome effects below and the drain's +// readonly/stale/no-op repost) call this, so a third site cannot grow its own +// copy of the ternary and quietly diverge from the gate. +function ackEffects( + ackLabelObserved: boolean, + settled: HostSessionState, + heldBase: number | null, + context: HostSessionContext +): HostSessionEffect[] { + return ackLabelObserved + ? [postDoc(settled, settled.lastAppliedDocVersion)] + : withholdAckEffects(settled, heldBase, context); +} + +// A settlement's user-visible FAILURE toasts, and the ONE owner of their text. +// Split out because the disposed-no-stash arm wants exactly these and nothing +// else: asking `settlementEffects` for the full set and filtering it down to +// `showError` made that arm depend on the filter for its correctness, and +// forced it to hand the ack gate a fabricated `ackLabelObserved: true` for an +// ack it never wanted built. `ok` has no toast — a settlement that succeeded is +// not a failure, and an unverified landing is not a failed save. +function failureToasts( + outcome: ApplyEditOutcome, + context: HostSessionContext +): HostSessionEffect[] { + switch (outcome.kind) { + case "ok": + return []; + case "refused": + return [ + { + type: "showError", + message: `Quoll could not save ${context.fsPath}. Reload the file or try again.`, + }, + ]; + case "constructThrew": + case "applyThrew": + case "rejected": + return [{ type: "showError", message: `Failed to save: ${outcome.message}` }]; + default: { + const _exhaustive: never = outcome; + throw new Error( + `[quoll] unhandled ApplyEditOutcome: ${(_exhaustive as { kind: string }).kind}` + ); + } + } +} + +// Per-outcome settlement effects: the ack Document (or its withhold pair, gated +// on `ackLabelObserved`) + non-ok diagnostics. Extracted so the applyEditSettled +// arm can SUPPRESS these wholesale when disposed (the webview is gone) and +// REPLACE them with drain effects when a stash drains. // // ORDER IS LOAD-BEARING on every non-ok arm: the failure `showError` comes // BEFORE the ack `postDocument`. The two are independent surfaces — `showError` // is a VS Code window toast, `postDocument` a webview-bound message — so there // is no coupling to respect (the "Document before edit-rejected" constraint on // `postRejectedDraft` is a different pair, both webview-bound and read by the -// same webview reducer). Toast-first matters because the reseed is the effect -// MOST likely to unwind `runEffects`: `buildSeedDocument` bottoms out in -// `canonicalDocumentText(document)`, the very seam whose throw produces a -// `rejected` outcome, so on that correlated failure the reseed re-runs the -// broken read and throws. Emitted second, the toast — the only user-visible -// signal that the save failed — would go down with it. `ok` has no toast to -// order, so its single effect is unchanged. +// same webview reducer). Toast-first is DEFENCE IN DEPTH, not the only guard: +// the reseed is the effect most likely to throw (`buildSeedDocument` bottoms out +// in `canonicalDocumentText(document)`), but the executor catches a builder +// throw and continues the effect loop, and its `showError` call is guarded too, +// so a later toast still reaches the user — `apply-edit-settle-rejection.test.ts` +// measures that CONTAINMENT and deliberately keeps no ordering assert of its own. +// The ORDER itself IS still pinned, in `host-session-core.test.ts` +// (`expectToastBeforeReseed`) — keep it there. The correlated failure is also +// narrower since `settle()` became total: a throwing `readCanonical` now +// resolves as an UNVERIFIED ok, and only the pipeline's synchronous prefix still +// produces `rejected`. Keep the order anyway — it costs nothing and removes the +// dependency on those guards. `ok` has no toast to order, so its single effect +// is unchanged. function settlementEffects( outcome: ApplyEditOutcome, settled: HostSessionState, heldBase: number | null, - context: HostSessionContext + context: HostSessionContext, + ackLabelObserved: boolean ): HostSessionEffect[] { + const ack = ackEffects(ackLabelObserved, settled, heldBase, context); switch (outcome.kind) { case "ok": - return [postDoc(settled, settled.lastAppliedDocVersion)]; + return ack; case "refused": return [ { @@ -333,19 +449,13 @@ function settlementEffects( message: "[quoll] applyEdit returned false", detail: { uri: context.uriString, baseDocVersion: heldBase }, }, - { - type: "showError", - message: `Quoll could not save ${context.fsPath}. Reload the file or try again.`, - }, - postDoc(settled, settled.lastAppliedDocVersion), + ...failureToasts(outcome, context), + ...ack, ]; case "constructThrew": case "applyThrew": case "rejected": - return [ - { type: "showError", message: `Failed to save: ${outcome.message}` }, - postDoc(settled, settled.lastAppliedDocVersion), - ]; + return [...failureToasts(outcome, context), ...ack]; default: { const _exhaustive: never = outcome; throw new Error( @@ -563,16 +673,18 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // told). A clean `ok` settle (incl. the applyEdit no-op on a GC'd // sole-editor document) has no showError, so no false alarm. if (state.disposed && state.pendingEdit === null) { - const effects = - event.outcome.kind === "ok" - ? [] - : settlementEffects( - event.outcome, - state, - state.pendingApplyBaseVersion, - state.context - ).filter((e) => e.type === "showError"); - return { state, effects }; + // Toasts ONLY, built directly rather than filtered out of the full + // settlement effects: this arm has no ack to gate, so it must not have + // to name an ack-label observation it does not have. What it drops + // along the way is the withhold pair — a logWarn plus + // `showResyncFailure`, which the executor turns into a VS Code WINDOW + // TOAST, not a webview-bound message (same distinction as the + // `showError` note above). That suppression is for a DIFFERENT reason + // than the ack's: post-dispose there is no view left to resync, so + // telling the user it could not be resynced is noise. A dropped stash + // is the one loss that still matters post-dispose, and it gets its own + // toast below. + return { state, effects: failureToasts(event.outcome, state.context) }; } const heldBase = state.pendingApplyBaseVersion; const stash = state.pendingEdit; @@ -584,14 +696,18 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes pendingEdit: null, inFlightContent: null, }; - // NOT OBSERVED (`documentVersion === null`) ⇒ NO advance. This assignment - // is verbatim (the documented exemption from `resyncLiveVersion`'s `max` - // clamp), so any stand-in value would REWIND the version rather than be - // clamped away. - const versioned: HostSessionState = - event.outcome.kind === "ok" && event.outcome.documentVersion !== null - ? { ...released, lastAppliedDocVersion: event.outcome.documentVersion } - : released; + // Unified settle-time version advance — EVERY outcome kind, one source + // (`event.settledVersion`), raised via Math.max so an observed version can + // never REWIND the label (unlike the old ok-only verbatim assignment, this + // is clamp-consistent with `resyncLiveVersion`; deliberately NOT routed + // through that helper — the lock was just released above, so its lock-free + // foreign-advance branch would double-count the epoch against site 2 + // below). NOT OBSERVED (`null`) ⇒ NO advance, no fabrication, no rewind. + const advanced = + event.settledVersion !== null + ? Math.max(released.lastAppliedDocVersion, event.settledVersion) + : released.lastAppliedDocVersion; + const versioned: HostSessionState = { ...released, lastAppliedDocVersion: advanced }; // Site 2 — settlement foreign-bytes check ⇒ epoch++, baseline per // outcome. OK: baseline is `inFlightContent` (the apply's target); a @@ -624,21 +740,85 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // this disjunct is belt-and-braces — but it keeps the convergence driven // by the executor's authoritative verdict, not a re-derived heuristic. const divergedAfterApply = event.outcome.kind === "ok" && event.divergedAfterApply === true; - // NOT OBSERVED (`currentContent === null`) ⇒ NOT foreign. Treating a - // missing snapshot as foreign is the REJECTED variant: it bumps the epoch - // and edit-sync drops the webview's replay buffer, destroying buffered - // keystrokes for a write that most likely landed exactly as intended. + // content unobserved ⇒ the verdict falls back to POSITIVE version-delta + // evidence (below); treating MISSING evidence as foreign remains the + // rejected variant. const observed = event.currentContent; + // Content-unobserved fallback: POSITIVE version-delta evidence only. Our + // own write contributes exactly one version increment on an ok apply and + // zero otherwise. "+1 per content change" is de facto, NOT an API + // contract — VS Code guarantees only that `TextDocument.version` + // strictly increases per change; it holds for Quoll's write shape (a + // single-replace WorkspaceEdit producing one content change event). If + // it ever drifts, the failure direction is bounded and stated honestly: + // a multi-increment OWN edit is misread as foreign → one spurious epoch + // bump → the webview reseeds and its replay buffer is dropped (buffered- + // keystroke loss, never corruption); a foreign edit batched into zero + // extra increments is MISSED → no bump, which is exactly the + // pre-existing accepted residual for an unobserved settlement. A delta + // BEYOND our contribution proves something foreign also moved the + // document. "No advance" is NOT evidence (`!ownEditOnly` would + // re-import the rejected "missing ⇒ foreign" through the back door and + // drop the replay buffer for a write that landed exactly as intended). + // ⚠️ `ok` does NOT mean "+1" in every case: a real apply contributes one + // increment, but the no-op short-circuit (execute-write.ts) settles + // `applied` WITHOUT submitting an edit, so its contribution is 0. The + // outcome kind cannot tell them apart, so this takes the LARGER of the + // two — an over-stated allowance, which errs towards "not foreign" and + // therefore towards keeping the replay buffer. That over-statement is + // harmless rather than a missed foreign +1: the no-op path returns BEFORE + // execute-write's only `await`, so the whole heldBase → settle-read window + // is one synchronous tick on the single-threaded host and no external edit + // can interleave to spend the extra allowance. + const ownContribution = event.outcome.kind === "ok" ? 1 : 0; const foreignAtSettle = divergedAfterApply || - (observed !== null && - (event.outcome.kind === "ok" + (observed !== null + ? event.outcome.kind === "ok" ? inFlight !== null && !contentMatches(observed, inFlight) - : !contentMatches(observed, event.preApplyContent))); + : !contentMatches(observed, event.preApplyContent) + : heldBase !== null && versioned.lastAppliedDocVersion > heldBase + ownContribution); const settled: HostSessionState = foreignAtSettle ? { ...versioned, externalEpoch: versioned.externalEpoch + 1 } : versioned; + // The ack-label gate. The ack Document pairs LIVE bytes (buildSeedDocument + // reads the document at effect time) with the reducer's version label, so + // the label must be backed by a real `document.version` OBSERVATION: + // - the settle-time/retry read succeeded (`settledVersion !== null`), or + // - the version advanced under the lock (a lock-held documentChanged / + // edit resync — the wiring snapshots the live version into those + // events, so the raised label IS an observation; withholding here + // would break the lock-held deferral contract and leave a quiet + // document with no repost ever). + // Byte equality is deliberately NOT evidence: an undone foreign edit + // leaves identical bytes at a HIGHER version, and acking that label lets + // the webview base its next Edit on it → stale verdict → lock-free + // forward advance → epoch bump → replay buffer dropped. + // ACCEPTED RESIDUAL on the second disjunct: it proves the version was + // OBSERVED, not that the observation is POST-APPLY. `resyncLiveVersion` + // raises the label for whoever moved the document — `onDidChangeTextDocument` + // carries no producer and the lock-held wiring snapshots only the version — + // so when the CONTENT is also unobserved, "our own echo arrived" and "a + // FOREIGN edit landed while our echo never did" are the SAME reducer state + // (heldBase V, label V+1, ok, settledVersion null, currentContent null) + // and no predicate can separate them. (With an OBSERVED content the + // foreign reading shows up as a byte mismatch and `foreignAtSettle` + // already bumps the epoch — that half is diagnosed, not residual.) In the + // residual reading the ack pairs live bytes with a label one edit behind, + // the same mislabel class the gate narrows elsewhere. It is not closable + // HERE, and every narrowing considered also withholds the central case + // (own echo, delta exactly 1), which is the receiving end of the + // lock-held deferral contract — withhold it and a quiet document never + // gets a repost at all. The failure stays bounded (stale verdict → + // epoch bump → replay-buffer drop, no corruption), needs a triple + // coincidence to reach, and is strictly better than the pre-gate + // behaviour, which posted the STORED label unconditionally. The durable + // fix is the liveness backstop tracked in the follow-up TODO entry. + const ackLabelObserved = + event.settledVersion !== null || + (heldBase !== null && settled.lastAppliedDocVersion > heldBase); + // Drain is SAFE only when a stash is waiting, edit #1 applied cleanly // (`ok`), and the settled document is EXACTLY edit #1's result // (currentContent === inFlightContent). The last check keeps an @@ -652,15 +832,53 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // misread a plain edit on a CRLF-eol single-line doc as "external won" // and DROP the stash instead of draining it (the webview's OWN acked // lineage, not a foreign edit). - // NOT OBSERVED ⇒ NO drain. The drain's safety condition is an OBSERVED + // CONTENT NOT OBSERVED ⇒ NO drain (the ack LABEL is a separate + // question, taken up next). The drain's safety condition is an OBSERVED // equality — "the settled document IS edit #1's exact result" — which is // what keeps an external edit that won the apply→settle race from being // clobbered by the stash. Without the observation that condition cannot // be established, so the stash is dropped exactly as it is for a failed // save. While the panel is alive the keystroke still survives in the // webview's replay buffer (which this settlement deliberately does not - // invalidate) and is re-posted after the ack; post-dispose the stash is - // its only carrier, which is why the drop is LOGGED below. + // invalidate) — re-posted once a later OBSERVED Document arrives; when + // the ack itself is withheld (unobserved label) the single flight stays + // parked until an observed documentChanged/ready Document lands (the + // quiet-document residual the follow-up TODO entry carries). Post-dispose + // the stash is its only carrier, which is why the drop is LOGGED below. + // The ACK LABEL is deliberately NOT a conjunct below: the drain is a new + // WRITE, not an ack, and its safety rests on the CONTENT evidence above. + // ⛔ Do NOT re-add an `(ackLabelObserved || state.disposed)` conjunct here + // (added in one review cycle and reverted after two independent advisors + // traced the fault depth; `host-session-core.test.ts`'s "the drain + // re-acquires the lock, so the label's catch-up is LOCK-HELD" goes red). + // REFUSING at an unobserved label drops the keystroke, and the only + // carrier left — the webview's replay buffer — is destroyed by the + // ORDINARY continuation: the apply DID move the document, so its + // `documentChanged` almost always arrives, and with the lock already + // released it reads as a lock-free forward advance, bumps the epoch, and + // `edit-sync.ts`'s `recordedEpoch > buf.epoch` drops the buffer. No + // second fault is needed, so the refusal is a DETERMINISTIC loss. + // DRAINING self-heals on that same continuation instead: the `accept` arm + // re-acquires the lock at the settled base, so the late echo lands + // LOCK-HELD (no bump) and its raise is itself the observation that + // licenses the next ack. + // ACCEPTED RESIDUAL — the stale re-base. Without an observation the + // re-acquired base is a known-stale LOWER BOUND, so a later settlement's + // version-delta fallback can read our own increment as foreign. Reaching + // that needs a SECOND, independent read failure: the DRAINED apply's own + // settlement must ALSO miss its CONTENT read (with the content observed, + // `contentMatches` scores the increment correctly as ours) while + // observing a version beyond `heldBase + ownContribution`. The cost is + // then one spurious epoch bump — a replay-buffer drop, never corruption — + // with the drained keystroke ALREADY on the document. Strictly shallower + // harm at a strictly deeper fault. Tracked in the follow-up TODO entry — + // but NOT closed by that entry's ack-timeout / reseed-retry half, which + // only unsticks a quiet document that can no longer post: the mis-scoring + // happens inside the NEXT settlement's version-delta fallback above, + // which reads `heldBase` as EXACT and has no input a timeout or a retry + // can reach. Closing it needs the entry's OTHER half — base provenance + // (observed vs. lower bound), or an observed-version catch-up before the + // re-base. const canDrain = stash !== null && event.outcome.kind === "ok" && @@ -677,7 +895,13 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // not less. A clean `ok` settle has no showError → []; a failure → // [showError]; an ok-but-mismatch (external won) is a valid // resolution, not a failure → also []. Alive: full effects. - const baseEffects = settlementEffects(event.outcome, settled, heldBase, state.context); + const baseEffects = settlementEffects( + event.outcome, + settled, + heldBase, + state.context, + ackLabelObserved + ); // Diagnostic log for the post-apply divergence. THREE arms, in this // order — and the `null` semantics are written LITERALLY here so nobody // "fixes" a condition with `observed ?? ""`: @@ -686,8 +910,9 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // must be visible for triage regardless of whether a keystroke was // queued. // 2. An UNOBSERVED settlement holding a stash: `canDrain` refused for - // want of an observation, so the keystroke is dropped. Post-dispose - // the stash was its only carrier, so the loss must be observable. + // want of a CONTENT observation, so the keystroke is dropped. + // Post-dispose the stash was its only carrier, so the loss must be + // observable. // 3. The narrower ok-but-mismatch log (external edit won a stash's // apply→settle race), which now REQUIRES an observation: claiming // "external edit won the race" without having read the document @@ -697,6 +922,10 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // carry none) — an unverified landing is not a failed save. Arm 2 // POST-DISPOSE is the exception and gets its own toast below: there the // log is the whole signal and nobody is left to read it. + // + // There is deliberately no fourth arm for "the content matched but the + // ack label was never observed": that configuration DRAINS (see + // `canDrain`), so it never reaches this branch at all. const unobservedStashDrop = !divergedAfterApply && stash !== null && @@ -764,9 +993,13 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // WITHOUT submitting an edit at all, so `appliedUnverified` is not // a landing claim and its ⚠️ note at `settle` binds caller text // too. The TOAST BODY below is unaffected and stays as written: - // that path's own precondition is that the document already holds - // the intended bytes, so "saved your change" is true for the user - // on every route into this arm. + // on the no-op route the document already holds the intended + // bytes, and on the landed-but-unverified route the apply + // resolved ok — which is why the toast pairs "saved your change" + // with "could not verify it" and tells the user to reopen and + // check. Do not drop that hedge: without the settle-time read a + // misplaced splice (execute-write.ts's S5 escape) cannot be ruled + // out. // ALIVE deliberately stays toast-free — there the webview's // single-flight replay buffer (which this settlement does not // invalidate) still holds the edit and re-posts it after the ack. @@ -804,6 +1037,40 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes currentContent: observed, markdownValidator: validateForWrite, }); + // The drain's own triage record for the STALE RE-BASE residual the + // `canDrain` comment accepts. Every other degraded path in this file + // logs; without this the one path that WRITES at a base it knows to be a + // lower bound would be the exception, and a later spurious epoch bump + // could not be attributed to the drain that caused it. NOT added to the + // readonly/stale/no-op arm below — that arm already logs the same + // incident through `withholdAckEffects`. NOT emitted post-dispose + // either: there the `accept` arm deliberately does not re-acquire the + // lock, so neither consequence named below can occur (no later + // settlement reads this base, and no draft goes out). + // Both exclusions live HERE, in the one place the record is built, so no + // call site can carry half the gate: an arm that spreads it emits it + // exactly when it is warranted. + const staleReBaseWarn: HostSessionEffect[] = + ackLabelObserved || state.disposed + ? [] + : [ + { + type: "logWarn", + // Shared incident sentence, then the arm's own consequence — + // written as ONE owner for the shared half so the two arms + // cannot drift apart on what the incident WAS. + message: + "[quoll] unlabelled drain: the pending stash was re-based onto an UNOBSERVED settlement label (a known-stale lower bound). " + + (verdict.kind === "accept" + ? "The bytes land; the residual is that a later settlement which also misses its CONTENT read can score our own increment as foreign (one spurious epoch bump → replay-buffer drop)" + : "The stash did not validate, so no bytes land; the residual is that its rejected draft goes out stamped with this stale label"), + detail: { + uri: state.context.uriString, + heldBase, + lastAppliedDocVersion: settled.lastAppliedDocVersion, + }, + }, + ]; switch (verdict.kind) { case "accept": // Re-acquire the lock + track the drained content as the new @@ -818,6 +1085,7 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes inFlightContent: stash.content, }, effects: [ + ...staleReBaseWarn, { type: "applyEdit", content: stash.content, @@ -846,9 +1114,25 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // webview's docVersion bookkeeping (so the next retry lands on a // live base instead of stale-rejecting). Mirrors the `ready`-arm // redelivery precedent. + // + // UNLIKE the readonly/stale/no-op repost BELOW (the next case in + // this switch), this arm is DELIBERATELY NOT gated on + // `ackLabelObserved` (round-2 reversal — see the plan's + // dispositions table): in the unobserved-label corner (version + // unread + no lock-held resync + the stash parse-failing) every + // reviewed local variant, a Document-free degrade included, + // converges to the same terminal state anyway — without an + // observation no correct label-advance exists, and a `ready` replay + // redelivers at the STORED label with no resync regardless. So this + // draft CAN go out stamped with a stale label; that is an ACCEPTED + // RESIDUAL, reachable because `canDrain` gates on CONTENT evidence + // only (see its comment). A durable fix needs rejection-state + // provenance + observed-version catch-up — its own slice, tracked in + // the follow-up TODO entry. effects: state.disposed ? [{ type: "showError", message: `Cannot save: ${verdict.error.message}` }] : [ + ...staleReBaseWarn, { type: "postRejectedDraft", content: stash.content, @@ -866,10 +1150,18 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes case "stale": case "no-op": // Nothing to write. Repost the authoritative (settled) Document so - // the webview reseeds — suppressed post-dispose. + // the webview reseeds — suppressed post-dispose, and WITHHELD when + // the label is unobserved (same gate as settlementEffects: this + // repost is an ack Document too, and canDrain's observed CONTENT is + // not version evidence). Both branches are LIVE: `canDrain` gates on + // content, not on the label, so a drain that lands here at an + // unobserved label takes the withhold arm. The shared `ackEffects` is + // what keeps this site and `settlementEffects` from drifting apart. return { state: settled, - effects: state.disposed ? [] : [postDoc(settled, settled.lastAppliedDocVersion)], + effects: state.disposed + ? [] + : ackEffects(ackLabelObserved, settled, heldBase, state.context), }; default: { const _exhaustive: never = verdict; @@ -892,6 +1184,28 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes if (state.rejection.kind !== "pending" || state.rejection.id !== event.id) { return { state, effects: [] }; } + if (event.documentVersion === null) { + // UNOBSERVED recovery read (the executor's guarded readVersionGuarded + // failed): same answer as the settlement ack gate — the recovery + // reseed pairs LIVE bytes with the version label, so no observation ⇒ + // no Document. The rejection is still CLEARED (a stuck pending + // rejection suppresses visible-edge resync — the deadlock this arm + // exists to break); the user is signalled through the shared latch, + // and the next OBSERVED Document (documentChanged / ready / edit + // resync) converges. + return { + state: { ...state, rejection: NONE }, + effects: [ + { + type: "logWarn", + message: + "[quoll] edit-rejected recovery reseed withheld: the live document version could not be read; rejection cleared, awaiting an observed Document", + detail: { uri: state.context.uriString, id: event.id }, + }, + { type: "showResyncFailure" }, + ], + }; + } // Resync to the live snapshot before the recovery reseed (see the // `ready` arm) — the reseed posts live bytes, so it must carry the // matching live version (and a bumped epoch if the live version moved: @@ -933,8 +1247,15 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // Promise settles. Posting here would emit a Document at the new // version while the lock is still held (Codex N1). Defer the post: // record the observed version and let the settlement repost the - // authoritative version EXACTLY ONCE (its `ok`/`refused`/throw arms - // all postDocument from the released state). + // authoritative version EXACTLY ONCE. + // That repost is CONDITIONAL now — every settlement arm's ack is gated on + // `ackLabelObserved`, and a draining stash replaces it with an applyEdit — + // and the deferral stays safe because the raise recorded HERE is itself + // the observation that licenses the ack: the wiring snapshots a real + // `document.version` into this event, so a settlement that never manages + // a read of its own still finds `lastAppliedDocVersion > heldBase` and + // posts. Withholding on that disjunct would leave this deferred post with + // no receiver at all. if (resynced.pendingApplyBaseVersion !== null) { return { state: resynced, effects: [] }; } diff --git a/src/extension/session/host-session-step.ts b/src/extension/session/host-session-step.ts index 38dc3611..56dac43d 100644 --- a/src/extension/session/host-session-step.ts +++ b/src/extension/session/host-session-step.ts @@ -87,8 +87,12 @@ export function isEditApplied(event: HostSessionEvent): boolean { } switch (event.outcome.kind) { case "ok": - // Includes the UNVERIFIED landing (`documentVersion: null`): the write - // completed and only the verification read broke (PR #399). + // Includes the UNVERIFIED landing (the event's `currentContent` is null — + // the settle-time CONTENT read is what downgrades `applied` to + // `appliedUnverified`): the write completed and only the verification read + // broke (PR #399). A version-only read failure is NOT that case: it leaves + // the tag `applied` and only gates the ack label, so `settledVersion` says + // nothing about whether the edit was applied. return true; case "refused": case "constructThrew": diff --git a/test/extension/session/apply-edit-settle-rejection.test.ts b/test/extension/session/apply-edit-settle-rejection.test.ts index dd09cdc7..5947ed03 100644 --- a/test/extension/session/apply-edit-settle-rejection.test.ts +++ b/test/extension/session/apply-edit-settle-rejection.test.ts @@ -23,7 +23,8 @@ // verification read broke → an UNVERIFIED-ok // settlement: the lock is released, a triage warn is logged, and there is NO // "Failed to save" toast (reporting a write that succeeded as failed is the -// defect this file now pins against). +// defect this file now pins against) — but see the ack-label-gate caveat below: +// the ABSENCE of a version observation is its own, separate signal. // - the write genuinely did NOT land (a refusal / a rejected apply / a throw in // the synchronous prefix) → the failure family is unchanged: toast, then the // authoritative reseed. @@ -31,8 +32,25 @@ // broken seam also makes `buildSeedDocument` throw, the ack Document cannot be // built, and the executor emits one latched "could not update the editor view" // notification (latched per INCIDENT — a successful build re-arms it). That is a -// reseed-delivery failure at another layer — never assert `h.errors` is empty -// under `armSettleFailure(true)`; filter for the message you mean. +// reseed-delivery failure at another layer. +// A THIRD trigger for that SAME latched toast (the ack-label gate, +// host-session-core's `ackLabelObserved`): when no source observed a post-apply +// version — the settle-time read AND the executor's dispatch retry both failed, +// AND no lock-held `documentChanged` arrived (`armVersionFailure` + +// `dropLockHeldDocumentChanged` below) — the settlement withholds its ack rather +// than pairing live bytes with a stale label, and reports through the SAME +// shared latch. So: never assert `h.errors` is empty under `armSettleFailure(true)` +// OR under a WITHHELD-ACK arrangement — `armVersionFailure(2+)` (persistent: the +// settle read and the dispatch retry both fail) TOGETHER WITH +// `dropLockHeldDocumentChanged`, which is what removes the other observation +// source. Filter for the message you mean instead. +// ⚠️ `armVersionFailure` ALONE is not that arrangement, and two tests below turn +// on the difference: `armVersionFailure(1)` is a TRANSIENT failure the dispatch +// retry recovers, and `armVersionFailure(2)` WITHOUT the drop still gets its +// label from the lock-held `documentChanged`. Both ack normally, so +// `expect(h.errors).toEqual([])` is exactly the assertion there — "the ack was +// licensed" means no toast of any kind. Weakening those two to a filtered check +// would stop pinning the recovery. import { describe, expect, it, vi } from "vitest"; @@ -76,6 +94,15 @@ interface HarnessOptions { * API failing while the host tears down). `errorAttempts` still counts it, so a * test can distinguish "attempted" from "displayed". */ showErrorThrows?: boolean; + /** EVENT-DELIVERY-LOSS FAULT INJECTION, not production equivalence (Codex r2 + * 88): the apply LANDS (buffer + version bump) but the lock-held + * `documentChanged` is DROPPED. Production wiring dispatches that event + * IMMEDIATELY while the lock is held (revert-rescue-wiring bypasses the + * trailing debounce), so the usual case is covered by a lock-held resync — + * but that mitigation is incidental, not a contract (the prior plan's + * Established fact 2), and the ack-label gate exists for the fault where + * the event never arrives. This arm injects that fault. */ + dropLockHeldDocumentChanged?: boolean; } // `armSettleFailure` arms two different seams: @@ -101,6 +128,7 @@ function harness(options: HarnessOptions = {}) { // this file can reach `runApplyEdit`'s rejection arm — the write lock's sole // release valve on that path. let readTextFailure = false; + let versionFailures = 0; // remaining readVersion calls that will throw (0 = healthy) // The span the last `build` produced — `apply` replays it against the live // buffer so the fake document really LANDS the edit (version bump included), // which is what makes an ok settlement carry a live version. @@ -174,7 +202,13 @@ function harness(options: HarnessOptions = {}) { } return doc.text; }, - readVersion: () => doc.version, + readVersion: () => { + if (versionFailures > 0) { + versionFailures -= 1; + throw new Error("boom-version"); + } + return doc.version; + }, // The settle-time verification read. execute-write GUARDS it individually, // so a broken seam (a disposed document, a broken canonicaliser) yields an // UNVERIFIED settlement rather than rejecting the whole pipeline. @@ -207,7 +241,9 @@ function harness(options: HarnessOptions = {}) { pendingSpan = null; } doc.version += 1; - dispatchEvent({ type: "documentChanged", documentVersion: doc.version }); + if (!options.dropLockHeldDocumentChanged) { + dispatchEvent({ type: "documentChanged", documentVersion: doc.version }); + } return true; }, }, @@ -242,6 +278,12 @@ function harness(options: HarnessOptions = {}) { armReadTextFailure: (on: boolean) => { readTextFailure = on; }, + /** Arm the NEXT n readVersion calls to throw (settle read = 1st, dispatch + * retry = 2nd). n=1 models a TRANSIENT failure the retry recovers; n>=2 a + * PERSISTENT one that reaches the withhold branch. */ + armVersionFailure: (n: number) => { + versionFailures = n; + }, /** A FOREIGN edit, on the panel's real path for one: mutate the buffer, bump * the version, and dispatch `documentChanged` LOCK-FREE. This is the only * honest way to re-trigger a reseed after a correlated failure — the webview @@ -692,3 +734,87 @@ describe("applyEdit settlement: a landed write is acked, not toasted", () => { expect(posted).toHaveLength(1); // no replay }); }); + +describe("applyEdit settlement: the ack-label gate end to end", () => { + it("an UNOBSERVABLE version withholds the mislabelled ack: no Document, one latched toast, lock released", async () => { + const h = harness({ dropLockHeldDocumentChanged: true }); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + h.armVersionFailure(2); // settle read AND dispatch retry + const postedBefore = h.documents.length; + h.type("a"); + await flushSettle(); + // The apply LANDED (live doc at v2) but no source observed a version — the + // OLD behaviour posted live "a" bytes labelled docVersion 1, which the + // webview would base an Edit on → stale → epoch bump → replay buffer drop. + expect(h.docVersion()).toBe(2); + expect(h.documents.length).toBe(postedBefore); // WITHHELD + expect(h.state().lastAppliedDocVersion).toBe(1); // no fabricated advance + expect(h.errors.filter((m) => m.includes("could not update the editor view"))).toHaveLength( + 1 + ); + expect(h.errors.filter((m) => m.includes("Failed to save"))).toEqual([]); // the write did not fail + expect(isWriteLockHeld(h.state())).toBe(false); + } finally { + warnSpy.mockRestore(); + } + }); + + it("a TRANSIENT version-read failure recovers through the dispatch retry: the ack posts at the live version", async () => { + const h = harness({ dropLockHeldDocumentChanged: true }); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + h.armVersionFailure(1); // settle read throws; the dispatch retry succeeds + h.type("a"); + await flushSettle(); + expect(h.documents.at(-1)?.docVersion).toBe(h.docVersion()); // ack at LIVE v2 + expect(h.state().externalEpoch).toBe(0); // own +1 delta is not foreign + expect(h.errors).toEqual([]); // no toast of any kind + } finally { + warnSpy.mockRestore(); + } + }); + + it("a lock-held documentChanged licenses the ack even when every version read fails", async () => { + const h = harness(); // fault NOT injected: apply dispatches the lock-held documentChanged + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + h.armVersionFailure(2); + h.type("a"); + await flushSettle(); + expect(h.documents.at(-1)?.docVersion).toBe(h.docVersion()); // label from the lock-held resync + expect(h.errors).toEqual([]); // observed → no withhold toast + } finally { + warnSpy.mockRestore(); + } + }); + + it("the withhold latch is per incident and shared: a recovered reseed re-arms it", async () => { + const h = harness({ dropLockHeldDocumentChanged: true }); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + // Incident 1: withheld ack → one toast. + h.armVersionFailure(2); + h.type("a"); + await flushSettle(); + expect(h.errors.filter((m) => m.includes("could not update the editor view"))).toHaveLength( + 1 + ); + // The seam recovers; a REAL host-side path (foreign edit → lock-free + // documentChanged) posts a Document successfully, which re-arms the latch. + const postedBefore = h.documents.length; + h.externalEdit("recovered"); + expect(h.documents.length).toBeGreaterThan(postedBefore); + // Incident 2: the webview reseeded (its single flight cleared), so a second + // keystroke is a sequence a real webview can produce. + h.armVersionFailure(2); + h.type("recovered!"); + await flushSettle(); + expect(h.errors.filter((m) => m.includes("could not update the editor view"))).toHaveLength( + 2 + ); + } finally { + warnSpy.mockRestore(); + } + }); +}); diff --git a/test/extension/session/effect-executor.test.ts b/test/extension/session/effect-executor.test.ts index 5fd7896f..22d360b4 100644 --- a/test/extension/session/effect-executor.test.ts +++ b/test/extension/session/effect-executor.test.ts @@ -204,7 +204,8 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect(dispatch).toHaveBeenCalledWith( expect.objectContaining({ type: "applyEditSettled", - outcome: { kind: "ok", documentVersion: 8 }, + outcome: { kind: "ok" }, + settledVersion: 8, currentContent: "new", // from the outcome's settledContent, not a re-read preApplyContent: "old", // canonical pre-apply, populated for ok too divergedAfterApply: false, @@ -217,7 +218,8 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect(dispatch).toHaveBeenCalledWith( expect.objectContaining({ type: "applyEditSettled", - outcome: { kind: "ok", documentVersion: 8 }, + outcome: { kind: "ok" }, + settledVersion: 8, currentContent: "CORRUPTED", divergedAfterApply: true, }) @@ -284,7 +286,8 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect(dispatch).toHaveBeenCalledWith( expect.objectContaining({ type: "applyEditSettled", - outcome: { kind: "ok", documentVersion: 9 }, + outcome: { kind: "ok" }, + settledVersion: 9, }) ); }); @@ -315,11 +318,17 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect.objectContaining({ type: "applyEditSettled", outcome: expect.objectContaining({ kind: "rejected", message: "boom-read" }), - // NOT OBSERVED — nothing was read, so the settlement says so rather than - // fabricating bytes. Safe because the outcome is non-ok (`canDrain` + // Nothing was read BY THE PIPELINE — the guarded dispatch retry + // (readVersionGuarded) labelled the settlement instead, against the + // seam's default healthy `readVersion: () => 1`. The unobserved case + // (the retry itself fails) is pinned by the dedicated persistent- + // failure test below. + settledVersion: 1, + // NOT OBSERVED — content was not read, so the settlement says so rather + // than fabricating bytes. Safe because the outcome is non-ok (`canDrain` // requires `ok`, so it never reaches `decideEdit`) and because the - // foreign-bytes check reads `null` as "not foreign" → no spurious epoch - // bump. + // foreign-bytes check reads `null` as unobserved; foreign evidence, if + // any, comes from the version-delta fallback. currentContent: null, preApplyContent: "", canWrite: false, @@ -331,25 +340,42 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { // pipeline resolves, and mapping it to a failure kind would toast "Failed to // save" for a write that succeeded. it("a settle-time read throw settles as ok/UNVERIFIED, never as a rejection", async () => { - const dispatch = await runApply({ - readCanonical: () => { - throw new Error("boom-settle"); - }, - readVersion: () => 7, - }); - expect(dispatch).toHaveBeenCalledWith( - expect.objectContaining({ - type: "applyEditSettled", - outcome: { kind: "ok", documentVersion: 7 }, - currentContent: null, - divergedAfterApply: false, - }) - ); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + const dispatch = await runApply({ + readCanonical: () => { + throw new Error("boom-settle"); + }, + readVersion: () => 7, + }); + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ + type: "applyEditSettled", + outcome: { kind: "ok" }, + settledVersion: 7, + currentContent: null, + divergedAfterApply: false, + }) + ); + // MIRROR of the version-only test's "no stash drain" negative pin. Here a + // VERSION was observed, so `ackLabelObserved` is true and the ack Document + // IS posted — the clause must stay CONDITIONAL rather than deliver a + // verdict on this event. + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("the ack Document is withheld unless some source observed"), + expect.anything() + ); + } finally { + warnSpy.mockRestore(); + } }); // The verification-loss warn is keyed on `settleReadFailure`, NOT on the // `appliedUnverified` tag: a VERSION-only failure keeps the tag `applied` (the - // content WAS verified) while still suppressing the self-advance, so a + // content WAS verified) while still putting the self-advance at risk — it is + // suppressed only when the guarded dispatch retry ALSO fails, which is the + // arrangement below (`readVersion` throws on every call; the transient + // counterpart is the retry test further down, where the event carries 9). A // tag-keyed warn would make that partial loss silent. it("a VERSION-only read failure still warns, though the tag stays applied", async () => { const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); @@ -363,7 +389,8 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect(dispatch).toHaveBeenCalledWith( expect.objectContaining({ type: "applyEditSettled", - outcome: { kind: "ok", documentVersion: null }, + outcome: { kind: "ok" }, + settledVersion: null, currentContent: "new", // the CONTENT was observed }) ); @@ -393,6 +420,22 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect.stringContaining("no stash drain"), expect.anything() ); + // ...and the version clause must attribute WHICH sources can supply the + // observation (settle read OR the guarded dispatch retry) rather than a + // flat "the VERSION was read" — this is the delta a revert of the + // version clause's reword must turn red. + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("the pipeline's settle read or the guarded dispatch retry"), + expect.anything() + ); + // ...and it must name the ACK consequence too: `settledVersion` is the ONE + // signal `ackLabelObserved` reads off this event (host-session-core.ts), so + // a VERSION-only failure is exactly the case where the ack Document is + // withheld absent that observation. + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("the ack Document is withheld"), + expect.anything() + ); // The no-op short-circuit reaches this same family without submitting an // edit, so the warn must not claim a landing either. expect(warnSpy).not.toHaveBeenCalledWith( @@ -499,6 +542,9 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect.objectContaining({ type: "applyEditSettled", outcome: expect.objectContaining({ kind: "rejected", message: "boom-read" }), + // The guarded dispatch retry labels this too; see the + // "pipeline rejection … STILL settles" test. + settledVersion: 1, canWrite: false, }) ); @@ -520,25 +566,128 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect(dispatch).toHaveBeenCalledWith( expect.objectContaining({ type: "applyEditSettled", - outcome: { kind: "ok", documentVersion: 8 }, + outcome: { kind: "ok" }, + settledVersion: 8, canWrite: false, }) ); }); - // Contract: the wrapper maps from the OUTCOME and does not re-read the - // document. For an ok settlement the executor reads the settled version once - // (inside verify); the wrapper must NOT read it again (a re-read could observe - // a later edit and mis-version the settlement). - it("does NOT re-read the document version after the outcome (maps from settledVersion)", async () => { + // Contract: the wrapper maps from the OUTCOME's settled version. The dispatch + // retry (readVersionGuarded) fires ONLY when the settle-time read failed — a + // healthy seam like this one's never does, so the version is read exactly + // once. An UNCONDITIONAL re-read would make this call count 2 and redden. + it("maps from settledVersion; the dispatch retry fires only when the settle read failed", async () => { const readVersion = vi.fn(() => 5); const dispatch = await runApply({ readVersion }); - // Exactly one version read — the executor's verify. The wrapper adds none. + // Exactly one version read — the executor's verify. The retry is conditional. expect(readVersion).toHaveBeenCalledTimes(1); expect(dispatch).toHaveBeenCalledWith( - expect.objectContaining({ outcome: { kind: "ok", documentVersion: 5 } }) + expect.objectContaining({ outcome: { kind: "ok" }, settledVersion: 5 }) ); }); + + // The settlement-dispatch retry (liveness): a TRANSIENT version-read failure + // must not withhold the ack. The retried value is a LABEL and an input to the + // reducer's content-unobserved version-delta verdict; that is sound because + // the retry runs microtasks after the settle-time read with no possibility of + // an interleaved document event on the single-threaded extension host — it + // observes the same live version the settle read would have. + it("retries readVersion once at dispatch when the settle read failed, and the event carries the retried value", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + let calls = 0; + const readVersion = vi.fn(() => { + calls += 1; + if (calls === 1) { + throw new Error("boom-version-transient"); + } + return 9; + }); + const dispatch = await runApply({ readVersion, readCanonical: () => "new" }); + expect(readVersion).toHaveBeenCalledTimes(2); // settle read (threw) + ONE dispatch retry + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ type: "applyEditSettled", settledVersion: 9 }) + ); + } finally { + warnSpy.mockRestore(); + } + }); + + it("a PERSISTENT version-read failure settles with settledVersion null (the retry is guarded, no throw)", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + const readVersion = vi.fn(() => { + throw new Error("boom-version"); + }); + const dispatch = await runApply({ readVersion, readCanonical: () => "new" }); + expect(readVersion).toHaveBeenCalledTimes(2); // one settle read + one retry, never more + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ type: "applyEditSettled", settledVersion: null }) + ); + // The guarded reader serves three call families with different + // consequences, so its warn NAMES the site — without it this line is + // indistinguishable from a withheld edit-rejected recovery reseed. + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("guarded readVersion failed"), + { site: "settlement-retry" }, + expect.anything() + ); + } finally { + warnSpy.mockRestore(); + } + }); + + it("the REJECTION arm also retries (guarded): a working version seam labels even a rejected pipeline", async () => { + const readVersion = vi.fn(() => 4); + const dispatch = await runApply({ + readText: () => { + throw new Error("boom-read"); + }, + readVersion, + }); + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ + outcome: expect.objectContaining({ kind: "rejected" }), + settledVersion: 4, + }) + ); + }); + + it("the REJECTION arm's retry failing does not strand the lock (settles with settledVersion null)", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + const readVersion = vi.fn(() => { + throw new Error("boom-version"); + }); + const dispatch = await runApply({ + readText: () => { + throw new Error("boom-read"); + }, + readVersion, + }); + // The retry WAS attempted, exactly once (the pipeline itself never reads + // the version on a synchronous-prefix rejection). This is what makes the + // test red before the retry exists — the value assertion alone is + // already satisfied by Task 1's hardcoded null (Explore r2 New-1). + expect(readVersion).toHaveBeenCalledTimes(1); + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ + outcome: expect.objectContaining({ kind: "rejected" }), + settledVersion: null, + }) + ); + // The site token is per-ARM, and the union type cannot catch a valid token + // stamped onto the wrong arm — only this assertion can. + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("guarded readVersion failed"), + { site: "rejection-arm-first-read" }, + expect.anything() + ); + } finally { + warnSpy.mockRestore(); + } + }); }); const rejErr = { code: "unsafe_url", message: "bad" } as const; @@ -686,6 +835,116 @@ describe("effect-executor sendEditRejected (via postEditRejected effect)", () => documentVersion: 11, }); }); + + // sendEditRejected's recovery dispatch shares this PR's failure model: its + // three dispatch sites run exactly when delivery is failing, and readVersion + // is a documented throw source (Fable r2 85 + error-handler r2 85, + // independently). The dispatch MUST still fire (a stuck pending rejection + // suppresses visible-edge resync) — but with `documentVersion: null`, NEVER a + // fabricated number (Codex r3 99: a stored-version fallback would ship live + // bytes at a stale label through the recovery reseed). + it("recovery dispatch survives a broken readVersion at the SYNC-throw site: dispatches with an UNOBSERVED version", () => { + const dispatch = vi.fn(); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + runReject({ + send: () => { + throw new Error("sync transport throw"); + }, + dispatch, + applyEditSeam: { + ...seamFor(), + readVersion: () => { + throw new Error("boom-version"); + }, + }, + }); + expect(dispatch).toHaveBeenCalledWith({ + type: "editRejectedDeliveryFailed", + id: 42, + documentVersion: null, + }); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("guarded readVersion failed"), + { site: "edit-rejected-recovery:sync-throw" }, + expect.anything() + ); + } finally { + errorSpy.mockRestore(); + warnSpy.mockRestore(); + } + }); + + // The .then sites are the more insidious mode: unguarded, their throw became + // an UNHANDLED REJECTION and the dispatch never fired (Explore r3 T2). + it("recovery dispatch survives a broken readVersion at the delivery-REFUSED site: dispatches with an UNOBSERVED version", async () => { + const dispatch = vi.fn(); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + runReject({ + send: vi.fn(async () => false), + dispatch, + applyEditSeam: { + ...seamFor(), + readVersion: () => { + throw new Error("boom-version"); + }, + }, + }); + await Promise.resolve(); + await Promise.resolve(); + expect(dispatch).toHaveBeenCalledWith({ + type: "editRejectedDeliveryFailed", + id: 42, + documentVersion: null, + }); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("guarded readVersion failed"), + { site: "edit-rejected-recovery:refused" }, + expect.anything() + ); + } finally { + errorSpy.mockRestore(); + warnSpy.mockRestore(); + } + }); + + // The THIRD site (the .then onRejected arm) — same shape, pinned for + // completeness so no dispatch site is unguarded-by-regression (Codex r4 93). + it("recovery dispatch survives a broken readVersion at the delivery-REJECTED site: dispatches with an UNOBSERVED version", async () => { + const dispatch = vi.fn(); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + runReject({ + send: () => Promise.reject(new Error("detached")), + dispatch, + applyEditSeam: { + ...seamFor(), + readVersion: () => { + throw new Error("boom-version"); + }, + }, + }); + await Promise.resolve(); + await Promise.resolve(); + expect(dispatch).toHaveBeenCalledWith({ + type: "editRejectedDeliveryFailed", + id: 42, + documentVersion: null, + }); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("guarded readVersion failed"), + { site: "edit-rejected-recovery:rejected" }, + expect.anything() + ); + } finally { + errorSpy.mockRestore(); + warnSpy.mockRestore(); + } + }); }); describe("effect-executor runEffects other cases", () => { @@ -842,3 +1101,44 @@ describe("effect-executor runEffects other cases", () => { expect(seen).toEqual(["light", "dark"]); }); }); + +describe("effect-executor showResyncFailure (withheld settlement ack)", () => { + it("toasts once, and shares its latch with the postDocument build-failure guard", () => { + const showError = vi.fn(); + const buildSeedDocument = vi.fn(() => { + throw new Error("boom-seed"); + }); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + const { runEffects } = createEffectExecutor(makeDeps({ showError, buildSeedDocument })); + runEffects([{ type: "showResyncFailure" }]); + runEffects([{ type: "showResyncFailure" }]); // same incident → latched + expect(showError).toHaveBeenCalledTimes(1); + // The OTHER trigger is latched by the SAME flag: a failing reseed build in + // the same incident must not toast a second time. + runEffects([{ type: "postDocument", docVersion: 1, externalEpoch: 0, epochGeneration: 7 }]); + expect(showError).toHaveBeenCalledTimes(1); + } finally { + errorSpy.mockRestore(); + } + }); + + it("a THROWING toast is contained and spends the latch", () => { + const showError = vi.fn(() => { + throw new Error("toast failed"); + }); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + const { runEffects } = createEffectExecutor(makeDeps({ showError })); + expect(() => runEffects([{ type: "showResyncFailure" }])).not.toThrow(); + runEffects([{ type: "showResyncFailure" }]); + expect(showError).toHaveBeenCalledTimes(1); + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining("failed to report the withheld settlement ack"), + expect.anything() + ); + } finally { + errorSpy.mockRestore(); + } + }); +}); diff --git a/test/extension/session/host-session-core.test.ts b/test/extension/session/host-session-core.test.ts index a9b7c4f7..83afdb1b 100644 --- a/test/extension/session/host-session-core.test.ts +++ b/test/extension/session/host-session-core.test.ts @@ -62,7 +62,8 @@ const edit = (over: Partial> = {}) = const settled = (over: Partial> = {}) => ({ type: "applyEditSettled", - outcome: { kind: "ok", documentVersion: 2 }, + outcome: { kind: "ok" }, + settledVersion: 2, canWrite: true, currentContent: "cur", // Canonical pre-apply snapshot (non-ok epoch baseline). Defaults equal to @@ -278,14 +279,14 @@ describe("host-session-core: applyEditSettled", () => { rejection: { kind: "pending", id: 1, content: "d", error: unsafe }, }); it("ok → release lock, advance version, clear rejection, postDocument(newV)", () => { - const r = core.transition(locked, settled({ outcome: { kind: "ok", documentVersion: 2 } })); + const r = core.transition(locked, settled({ settledVersion: 2 })); expect(r.state.pendingApplyBaseVersion).toBeNull(); expect(r.state.lastAppliedDocVersion).toBe(2); expect(r.state.rejection).toEqual({ kind: "none" }); expect(r.effects).toEqual([pDoc(2)]); }); it("refused → release lock, logWarn(heldBase) + showError(fsPath) + postDocument", () => { - const r = core.transition(locked, settled({ outcome: { kind: "refused" } })); + const r = core.transition(locked, settled({ outcome: { kind: "refused" }, settledVersion: 1 })); expect(r.state.pendingApplyBaseVersion).toBeNull(); expectToastBeforeReseed(r.effects); expect(r.effects[0]).toMatchObject({ @@ -303,14 +304,17 @@ describe("host-session-core: applyEditSettled", () => { "applyThrew", "rejected", ] as const)("%s → release lock, showError(message) + postDocument", (kind) => { - const r = core.transition(locked, settled({ outcome: { kind, message: "boom" } })); + const r = core.transition( + locked, + settled({ outcome: { kind, message: "boom" }, settledVersion: 1 }) + ); expect(r.state.pendingApplyBaseVersion).toBeNull(); expectToastBeforeReseed(r.effects); expect(r.effects).toEqual([{ type: "showError", message: "Failed to save: boom" }, pDoc(1)]); }); it("settle after dispose → no effects, state unchanged", () => { const disposed = base({ disposed: true, pendingApplyBaseVersion: null }); - const r = core.transition(disposed, settled({ outcome: { kind: "ok", documentVersion: 9 } })); + const r = core.transition(disposed, settled({ settledVersion: 9 })); expect(r.effects).toEqual([]); expect(r.state).toEqual(disposed); }); @@ -324,13 +328,13 @@ describe("host-session-core: applyEditSettled", () => { // NO documentChanged is injected here on purpose: the production resync that // usually raises the version is another module's incidental behaviour, so the // reducer must be correct without it. What it must NOT do is move the version - // on an unobserved read. (A `-1` sentinel would be assigned VERBATIM — the - // settlement self-advance is exempt from `resyncLiveVersion`'s `max` clamp — - // and rewind the version.) - const r = core.transition( - locked, - settled({ outcome: { kind: "ok", documentVersion: null }, currentContent: null }) - ); + // on an unobserved read. (The settlement advance is now `Math.max`-clamped — + // the hoisted `advanced` const applies for EVERY outcome kind, no longer an + // ok-only verbatim exemption from `resyncLiveVersion`'s clamp — so a fabricated + // LOW sentinel could not rewind it either way; `null` is used instead of any + // sentinel because a fabricated HIGH value would wrongly read as an observed + // advance and license the ack gate.) + const r = core.transition(locked, settled({ settledVersion: null, currentContent: null })); expect(r.state.lastAppliedDocVersion).toBe(1); // unchanged — not rewound, not invented expect(r.state.externalEpoch).toBe(locked.externalEpoch); // unobserved is NOT foreign expect(isWriteLockHeld(r.state)).toBe(false); // the lock is still released @@ -349,10 +353,7 @@ describe("host-session-core: applyEditSettled", () => { lastAppliedDocVersion: 1, inFlightContent: "edit1", }); - const r = core.transition( - inFlight, - settled({ outcome: { kind: "ok", documentVersion: null }, currentContent: null }) - ); + const r = core.transition(inFlight, settled({ settledVersion: null, currentContent: null })); expect(r.state.externalEpoch).toBe(inFlight.externalEpoch); expect(r.state.lastAppliedDocVersion).toBe(1); }); @@ -364,7 +365,7 @@ describe("host-session-core: applyEditSettled", () => { const r = core.transition( locked, settled({ - outcome: { kind: "ok", documentVersion: null }, + settledVersion: null, currentContent: "other", divergedAfterApply: true, }) @@ -388,7 +389,7 @@ describe("host-session-core: applyEditSettled drain", () => { it("ALIVE ok, currentContent === inFlightContent → drain accept: applyEdit(stash) re-based, NO ack Document", () => { const r = core.transition( lockedWithStash("edit1", "edit1plus"), - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "edit1" }) + settled({ settledVersion: 2, currentContent: "edit1" }) ); expect(r.state.pendingEdit).toBeNull(); expect(r.state.pendingApplyBaseVersion).toBe(2); // re-acquired (alive) @@ -405,7 +406,7 @@ describe("host-session-core: applyEditSettled drain", () => { // (contentMatches). Reproduces the pre-fix skew: red without contentMatches. const r = core.transition( lockedWithStash("a\nb", "a\nb-plus"), - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "a\r\nb" }) + settled({ settledVersion: 2, currentContent: "a\r\nb" }) ); expect(r.state.pendingEdit).toBeNull(); expect(r.state.pendingApplyBaseVersion).toBe(2); // re-acquired (alive) = drained @@ -416,7 +417,7 @@ describe("host-session-core: applyEditSettled drain", () => { it("EXTERNAL edit raced (currentContent !== inFlightContent) → NO drain, logWarn + repost authoritative Document (external wins)", () => { const r = core.transition( lockedWithStash("edit1", "edit1plus"), - settled({ outcome: { kind: "ok", documentVersion: 5 }, currentContent: "external-content" }) + settled({ settledVersion: 5, currentContent: "external-content" }) ); expect(r.state.pendingEdit).toBeNull(); expect(r.state.pendingApplyBaseVersion).toBeNull(); @@ -442,7 +443,7 @@ describe("host-session-core: applyEditSettled drain", () => { // isolates the CONTENT being unobserved. const r = core.transition( lockedWithStash("edit1", "edit1plus"), - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: null }) + settled({ settledVersion: 2, currentContent: null }) ); expect(r.state.pendingEdit).toBeNull(); // released expect(r.effects.some((e) => e.type === "applyEdit")).toBe(false); // but NOT written @@ -467,7 +468,12 @@ describe("host-session-core: applyEditSettled drain", () => { // Clean failure: the doc is still at the pre-apply snapshot (currentContent // === preApplyContent), so NO foreign bytes intervened → epoch unchanged // (0). The retry buffer must stay replayable. - settled({ outcome: { kind: "refused" }, currentContent: "edit1", preApplyContent: "edit1" }) + settled({ + outcome: { kind: "refused" }, + settledVersion: 1, + currentContent: "edit1", + preApplyContent: "edit1", + }) ); expect(r.state.pendingEdit).toBeNull(); expect(reseedIn(r.effects)).toEqual(pDoc(1)); @@ -478,7 +484,7 @@ describe("host-session-core: applyEditSettled drain", () => { it("drain no-op (stash content === settled currentContent) → repost Document only", () => { const r = core.transition( lockedWithStash("same", "same"), - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "same" }) + settled({ settledVersion: 2, currentContent: "same" }) ); expect(r.effects).toEqual([pDoc(2)]); }); @@ -486,7 +492,7 @@ describe("host-session-core: applyEditSettled drain", () => { it("drain parse-failed (ALIVE) → postRejectedDraft(draft, settled version) + showError, rejection pending", () => { const r = core.transition( lockedWithStash("edit1", "hasBAD"), - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "edit1" }) + settled({ settledVersion: 2, currentContent: "edit1" }) ); expect(r.state.rejection).toMatchObject({ kind: "pending", id: 1, content: "hasBAD" }); // The draft is redelivered as a Document at the SETTLED version so the @@ -517,7 +523,7 @@ describe("host-session-core: applyEditSettled drain", () => { // base and is NOT stale-rejected. const drained = core.transition( lockedWithStash("edit1", "hasBAD"), - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "edit1" }) + settled({ settledVersion: 2, currentContent: "edit1" }) ); const draftDoc = drained.effects.find((e) => e.type === "postRejectedDraft"); expect(draftDoc).toBeDefined(); @@ -547,7 +553,7 @@ describe("host-session-core: applyEditSettled drain", () => { it("drain parse-failed (ALIVE) round-trip NEGATIVE: a retry still on the PRE-drain version IS stale-rejected (reproduces finding #2 without the fix)", () => { const drained = core.transition( lockedWithStash("edit1", "hasBAD"), - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "edit1" }) + settled({ settledVersion: 2, currentContent: "edit1" }) ); // Simulate the OLD (pre-fix) webview: it never learned the settled // version, so it retries with the stale pre-drain base (1) while the @@ -567,7 +573,7 @@ describe("host-session-core: applyEditSettled drain", () => { const r = core.transition( lockedWithStash("edit1", "edit1plus"), settled({ - outcome: { kind: "ok", documentVersion: 2 }, + settledVersion: 2, canWrite: false, currentContent: "edit1", }) @@ -578,7 +584,7 @@ describe("host-session-core: applyEditSettled drain", () => { it("POST-DISPOSE ok drain accept → applyEdit only, NO lock re-acquired, NO webview post", () => { const r = core.transition( lockedWithStash("edit1", "edit1plus", { disposed: true }), - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "edit1" }) + settled({ settledVersion: 2, currentContent: "edit1" }) ); expect(r.state.pendingApplyBaseVersion).toBeNull(); // NOT re-acquired (Codex #5) expect(r.effects).toEqual([{ type: "applyEdit", content: "edit1plus", baseDocVersion: 2 }]); @@ -587,7 +593,7 @@ describe("host-session-core: applyEditSettled drain", () => { it("POST-DISPOSE drain parse-failed → showError only (postEditRejected suppressed)", () => { const r = core.transition( lockedWithStash("edit1", "hasBAD", { disposed: true }), - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "edit1" }) + settled({ settledVersion: 2, currentContent: "edit1" }) ); expect(r.effects).toHaveLength(1); expect(r.effects[0]).toMatchObject({ type: "showError" }); @@ -596,7 +602,11 @@ describe("host-session-core: applyEditSettled drain", () => { it("POST-DISPOSE non-ok WITH a stash → showError only (failed save still surfaced), NO webview post", () => { const r = core.transition( lockedWithStash("edit1", "edit1plus", { disposed: true }), - settled({ outcome: { kind: "rejected", message: "boom" }, currentContent: "edit1" }) + settled({ + outcome: { kind: "rejected", message: "boom" }, + settledVersion: 1, + currentContent: "edit1", + }) ); expect(r.effects).toEqual([{ type: "showError", message: "Failed to save: boom" }]); }); @@ -604,7 +614,7 @@ describe("host-session-core: applyEditSettled drain", () => { it("POST-DISPOSE ok but external-mismatch WITH a stash → logWarn only, no toast (external won, webview-bound effects suppressed)", () => { const r = core.transition( lockedWithStash("edit1", "edit1plus", { disposed: true }), - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "external" }) + settled({ settledVersion: 2, currentContent: "external" }) ); expect(r.effects).toEqual([ { @@ -626,7 +636,7 @@ describe("host-session-core: applyEditSettled drain", () => { // post-dispose there is no webview replay buffer left to carry it. const r = core.transition( lockedWithStash("edit1", "edit1plus", { disposed: true }), - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: null }) + settled({ settledVersion: 2, currentContent: null }) ); const toasts = r.effects.filter((e) => e.type === "showError"); expect(toasts).toHaveLength(1); @@ -649,7 +659,7 @@ describe("host-session-core: applyEditSettled drain", () => { // `state.disposed` gate above turns this red. const r = core.transition( lockedWithStash("edit1", "edit1plus"), - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: null }) + settled({ settledVersion: 2, currentContent: null }) ); expect(r.effects.some((e) => e.type === "showError")).toBe(false); expect( @@ -663,7 +673,7 @@ describe("host-session-core: applyEditSettled drain", () => { // "any post-dispose stash drop" turns this red. const r = core.transition( lockedWithStash("edit1", "edit1plus", { disposed: true }), - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "external" }) + settled({ settledVersion: 2, currentContent: "external" }) ); expect(r.effects.some((e) => e.type === "showError")).toBe(false); }); @@ -678,7 +688,11 @@ describe("host-session-core: applyEditSettled drain", () => { // save left to describe as unverified. const r = core.transition( lockedWithStash("edit1", "edit1plus", { disposed: true }), - settled({ outcome: { kind: "rejected", message: "boom" }, currentContent: null }) + settled({ + outcome: { kind: "rejected", message: "boom" }, + settledVersion: 1, + currentContent: null, + }) ); const toasts = r.effects.filter((e) => e.type === "showError"); expect(toasts).toEqual([{ type: "showError", message: "Failed to save: boom" }]); @@ -686,7 +700,7 @@ describe("host-session-core: applyEditSettled drain", () => { it("POST-DISPOSE settle with NO stash → strict no-op, state unchanged", () => { const disposed = base({ disposed: true }); - const r = core.transition(disposed, settled({ outcome: { kind: "ok", documentVersion: 9 } })); + const r = core.transition(disposed, settled({ settledVersion: 9 })); expect(r.effects).toEqual([]); expect(r.state).toEqual(disposed); }); @@ -695,7 +709,7 @@ describe("host-session-core: applyEditSettled drain", () => { const disposed = base({ disposed: true }); const r = core.transition( disposed, - settled({ outcome: { kind: "rejected", message: "boom" } }) + settled({ outcome: { kind: "rejected", message: "boom" }, settledVersion: 1 }) ); expect(r.effects).toEqual([{ type: "showError", message: "Failed to save: boom" }]); expect(r.state).toEqual(disposed); @@ -830,7 +844,7 @@ describe("host-session-core: traces", () => { edit({ content: "good", currentContent: "cur", baseDocVersion: 1, documentVersion: 1 }), // Clean settlement: the settled doc IS the applied bytes ("good"), so the // epoch does NOT advance (site 2 baseline = inFlightContent). - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "good" }) + settled({ settledVersion: 2, currentContent: "good" }) ); expect(batches[0]).toEqual([{ type: "applyEdit", content: "good", baseDocVersion: 1 }]); expect(batches[1]).toEqual([pDoc(2)]); @@ -846,7 +860,7 @@ describe("host-session-core: traces", () => { // The deferred documentChanged is the in-flight apply's OWN echo (lock // held → no epoch bump), and the settled doc IS the applied bytes → clean, // epoch 0. - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "good" }) + settled({ settledVersion: 2, currentContent: "good" }) ); expect(batches[0]).toEqual([{ type: "applyEdit", content: "good", baseDocVersion: 1 }]); expect(batches[1]).toEqual([]); // <-- deferred: NO post while the lock is held @@ -863,7 +877,7 @@ describe("host-session-core: traces", () => { base({ lastAppliedDocVersion: 1 }), edit({ content: "good", currentContent: "cur", baseDocVersion: 1, documentVersion: 1 }), { type: "documentChanged", documentVersion: 2 }, // fires before the Promise settles, lock still held - settled({ outcome: { kind: "refused" } }) + settled({ outcome: { kind: "refused" }, settledVersion: 1 }) ); expect(batches[1]).toEqual([]); // deferred: NO post while the lock is held // The refused arm reseeds from released.lastAppliedDocVersion — which MUST be @@ -879,7 +893,7 @@ describe("host-session-core: traces", () => { base({ lastAppliedDocVersion: 1 }), edit({ content: "good", currentContent: "cur", baseDocVersion: 1, documentVersion: 1 }), { type: "ready", documentVersion: 1 }, - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "good" }) + settled({ settledVersion: 2, currentContent: "good" }) ); expect(batches[1].map((e) => e.type)).toEqual(["logWarn"]); // ready dropped while locked expect(batches[2]).toEqual([pDoc(2)]); @@ -889,7 +903,7 @@ describe("host-session-core: traces", () => { const { state, batches } = run( base({ lastAppliedDocVersion: 1 }), edit({ content: "good", currentContent: "cur", baseDocVersion: 1, documentVersion: 1 }), - settled({ outcome: { kind: "constructThrew", message: "lineAt blew up" } }) + settled({ outcome: { kind: "constructThrew", message: "lineAt blew up" }, settledVersion: 1 }) ); expect(batches[0]).toEqual([{ type: "applyEdit", content: "good", baseDocVersion: 1 }]); expectToastBeforeReseed(batches[1]); @@ -1014,7 +1028,7 @@ describe("host-session-core: traces", () => { const { batches, state } = run( base({ pendingApplyBaseVersion: 1, lastAppliedDocVersion: 1 }), { type: "disposed" }, - settled({ outcome: { kind: "ok", documentVersion: 2 } }) + settled({ settledVersion: 2 }) ); expect(batches[0]).toEqual([]); expect(batches[1]).toEqual([]); @@ -1166,10 +1180,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { lastAppliedDocVersion: 1, inFlightContent: "applied", }); - const r = core.transition( - locked, - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "applied" }) - ); + const r = core.transition(locked, settled({ settledVersion: 2, currentContent: "applied" })); expect(r.state.externalEpoch).toBe(0); expect(r.effects).toEqual([pDoc(2)]); }); @@ -1185,10 +1196,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { lastAppliedDocVersion: 1, inFlightContent: "a\nb", }); - const r = core.transition( - locked, - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "a\r\nb" }) - ); + const r = core.transition(locked, settled({ settledVersion: 2, currentContent: "a\r\nb" })); expect(r.state.externalEpoch).toBe(0); expect(r.effects).toEqual([pDoc(2)]); }); @@ -1203,6 +1211,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { locked, settled({ outcome: { kind: "refused" }, + settledVersion: 1, currentContent: "a\r\nb", preApplyContent: "a\nb", }) @@ -1217,10 +1226,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { lastAppliedDocVersion: 1, inFlightContent: "target", }); - const r = core.transition( - locked, - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "foreign" }) - ); + const r = core.transition(locked, settled({ settledVersion: 2, currentContent: "foreign" })); expect(r.state.externalEpoch).toBe(1); expect(r.effects).toEqual([pDoc(2, 1)]); }); @@ -1235,6 +1241,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { locked, settled({ outcome: { kind: "refused" }, + settledVersion: 1, currentContent: "pre-apply", preApplyContent: "pre-apply", }) @@ -1253,6 +1260,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { locked, settled({ outcome: { kind: "refused" }, + settledVersion: 1, currentContent: "foreign-bytes", preApplyContent: "pre-apply", }) @@ -1273,7 +1281,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { const r = core.transition( locked, settled({ - outcome: { kind: "ok", documentVersion: 2 }, + settledVersion: 2, currentContent: "misplaced-splice", divergedAfterApply: true, }) @@ -1306,7 +1314,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { const r = core.transition( locked, settled({ - outcome: { kind: "ok", documentVersion: 2 }, + settledVersion: 2, currentContent: "target", divergedAfterApply: true, }) @@ -1343,13 +1351,15 @@ describe("host-session-core: externalEpoch (S3a)", () => { }); // Structural backstop: the ONLY writes to `lastAppliedDocVersion` are the - // single `resyncLiveVersion` helper, `initialState`, and the settlement `ok` - // self-advance (the sole documented exemption). A future hand-rolled arm that - // raises the version directly (bypassing the helper, re-opening the epoch - // under-advance that reintroduces finding #4 silently) adds a new RHS token - // here and reddens. Comments are stripped first so a rule-shaped literal in a - // doc-comment cannot vacuate the guard (LEARNING: source-contract grep). - it("INVARIANT: lastAppliedDocVersion is only written by resyncLiveVersion / initialState / settlement-ok", () => { + // single `resyncLiveVersion` helper, `initialState`, and the settlement + // advance. The advance is now the hoisted `advanced` const applied via + // `Math.max` to EVERY outcome kind (no longer an ok-only verbatim exemption). + // A future hand-rolled arm that raises the version directly (bypassing the + // helper, re-opening the epoch under-advance that reintroduces finding #4 + // silently) adds a new RHS token here and reddens. Comments are stripped + // first so a rule-shaped literal in a doc-comment cannot vacuate the guard + // (LEARNING: source-contract grep). + it("INVARIANT: lastAppliedDocVersion is only written by resyncLiveVersion / initialState / settlement-advance", () => { const source = readFileSync( new URL("../../../src/extension/session/host-session-core.ts", import.meta.url), "utf8" @@ -1365,16 +1375,17 @@ describe("host-session-core: externalEpoch (S3a)", () => { m[1].trim().replace(/[;,}\s]+$/, "") ); // Allowed RHS tokens: - // - `raised` → resyncLiveVersion (the one helper) - // - `docVersion` → initialState seed - // - `event.outcome.documentVersion` → settlement `ok` self-advance (exempt) - // - `number` → the readonly field type declaration + // - `raised` → resyncLiveVersion (the one helper) + // - `docVersion` → initialState seed + // - `advanced` → the settlement advance, Math.max over event.settledVersion + // for EVERY outcome kind (no longer an ok-only exemption) + // - `number` → the readonly field type declaration // - `resynced.lastAppliedDocVersion` / `settled.lastAppliedDocVersion` - // → decideEdit ARGS (reads, not writes) + // → decideEdit ARGS (reads, not writes) const allowed = new Set([ "raised", "docVersion", - "event.outcome.documentVersion", + "advanced", "number", "resynced.lastAppliedDocVersion", "settled.lastAppliedDocVersion", @@ -1383,3 +1394,430 @@ describe("host-session-core: externalEpoch (S3a)", () => { expect(disallowed).toEqual([]); }); }); + +describe("host-session-core: settlement ack-label gate (ackLabelObserved)", () => { + const locked = base({ pendingApplyBaseVersion: 1, inFlightContent: "edit1" }); + + it("WITHHOLDS the ack when no source observed a post-apply version (ok, unobserved settle + no lock-held advance)", () => { + const r = core.transition(locked, settled({ settledVersion: null, currentContent: null })); + expect(r.effects.find((e) => e.type === "postDocument")).toBeUndefined(); + expect(r.effects.some((e) => e.type === "showResyncFailure")).toBe(true); + expect(r.effects.some((e) => e.type === "logWarn")).toBe(true); + // No fabricated advance, no spurious epoch bump. + expect(r.state.lastAppliedDocVersion).toBe(1); + expect(r.state.externalEpoch).toBe(0); + }); + + it("POSTS the ack when the version advanced under the lock (lock-held documentChanged was a real observation)", () => { + // documentChanged during the lock raised lastApplied 1→2 (no epoch bump, lock-held branch). + const s = base({ + pendingApplyBaseVersion: 1, + inFlightContent: "edit1", + lastAppliedDocVersion: 2, + }); + const r = core.transition(s, settled({ settledVersion: null, currentContent: null })); + expect(reseedIn(r.effects)).toEqual(pDoc(2)); + expect(r.effects.some((e) => e.type === "showResyncFailure")).toBe(false); + }); + + it("a lock-held raise licenses the ack even though the raise's PRODUCER is unattributable (accepted residual)", () => { + // Same disjunct as the test above, but composed from the two events that + // produce it instead of a hand-placed `lastAppliedDocVersion`, so the trace + // is the real one: a `documentChanged` arrives while the lock is held and + // raises the label, then the settlement observes nothing at all. + // ⚠️ The reducer cannot tell WHO raised it. `resyncLiveVersion` takes no + // producer (VS Code's change event carries none, and the lock-held wiring + // snapshots only the version), so this same state is reached both by our own + // apply's echo — the central, correct case the deferral contract depends on — + // and by a FOREIGN edit landing while our echo never arrived, where the ack + // then labels live bytes one version behind. Today we ack in BOTH: the states + // are identical, so no predicate separates them, and withholding would kill + // the deferral contract's only receiver. Accepted residual; the backstop is + // the liveness TODO entry. Narrowing this disjunct turns this test red on + // purpose — that is the conversation it exists to force. + const raised = core.transition(locked, { type: "documentChanged", documentVersion: 2 }); + const r = core.transition( + raised.state, + settled({ settledVersion: null, currentContent: null }) + ); + expect(reseedIn(r.effects)).toEqual(pDoc(2)); + expect(r.effects.some((e) => e.type === "showResyncFailure")).toBe(false); + expect(r.state.externalEpoch).toBe(0); // delta 1 === our own contribution + }); + + it("byte equality does NOT license the ack (undone foreign edit leaves identical bytes at a higher version)", () => { + // Content observed and EQUAL to the in-flight bytes — still withheld without a version observation. + const r = core.transition(locked, settled({ settledVersion: null, currentContent: "edit1" })); + expect(r.effects.find((e) => e.type === "postDocument")).toBeUndefined(); + expect(r.effects.some((e) => e.type === "showResyncFailure")).toBe(true); + }); + + it("non-ok arms get the same gate: refused + unobserved version withholds the ack but KEEPS the failure toast", () => { + const r = core.transition( + locked, + settled({ outcome: { kind: "refused" }, settledVersion: null, currentContent: null }) + ); + expect(r.effects.find((e) => e.type === "postDocument")).toBeUndefined(); + expect(r.effects.some((e) => e.type === "showError")).toBe(true); + expect(r.effects.some((e) => e.type === "showResyncFailure")).toBe(true); + }); + + it("the no-op short-circuit's UNCHANGED observed version licenses the ack (delta 0 is not foreign)", () => { + // settledVersion === heldBase: nothing was applied, the observation confirms the label. + const r = core.transition(locked, settled({ settledVersion: 1, currentContent: null })); + expect(reseedIn(r.effects)).toEqual(pDoc(1)); + expect(r.state.externalEpoch).toBe(0); + }); + + it("POST-DISPOSE the withhold pair is suppressed with the rest of the webview-bound effects", () => { + // Disposed + no stash + unobserved version. The early return builds + // `failureToasts(outcome, context)` directly and never reaches + // `settlementEffects`, so neither the ack nor the withhold pair is + // CONSTRUCTED here at all — there is no ack-label gate on this path to + // observe. What this pins is that non-construction: `ok` leaves no effects + // at all, `refused` leaves toasts and nothing else. + const disposed = base({ disposed: true, pendingApplyBaseVersion: null }); + const ok = core.transition(disposed, settled({ settledVersion: null, currentContent: null })); + expect(ok.effects).toEqual([]); + const refused = core.transition( + disposed, + settled({ outcome: { kind: "refused" }, settledVersion: null, currentContent: null }) + ); + expect(refused.effects.every((e) => e.type === "showError")).toBe(true); + expect(refused.effects.length).toBeGreaterThan(0); + }); + + it("editRejectedDeliveryFailed with an UNOBSERVED version clears the rejection but WITHHOLDS the recovery reseed", () => { + // The recovery reseed pairs LIVE bytes with the version label, so an + // unobserved version gets the same answer as the settlement ack gate. + // (`unsafe` is the file's existing MarkdownError fixture, :18.) + const s = base({ + rejection: { kind: "pending", id: 7, content: "draft", error: unsafe }, + nextRejectionId: 8, + }); + const r = core.transition(s, { + type: "editRejectedDeliveryFailed", + id: 7, + documentVersion: null, + }); + expect(r.state.rejection).toEqual({ kind: "none" }); // no deadlock: pending is cleared + expect(r.effects.find((e) => e.type === "postDocument")).toBeUndefined(); // no fabricated label + expect(r.effects.some((e) => e.type === "showResyncFailure")).toBe(true); + expect(r.state.lastAppliedDocVersion).toBe(1); // nothing observed, nothing advanced + }); + + it("editRejectedDeliveryFailed null-version STILL respects the id guard (stale failure is a no-op)", () => { + const s = base({ + rejection: { kind: "pending", id: 9, content: "draft", error: unsafe }, + nextRejectionId: 10, + }); + const r = core.transition(s, { + type: "editRejectedDeliveryFailed", + id: 7, + documentVersion: null, + }); + expect(r.state).toBe(s); + expect(r.effects).toEqual([]); + }); + + it("after a null-version recovery the next OBSERVED event reseeds normally (convergence)", () => { + // Two steps: the withheld recovery clears the rejection, so a later + // lock-free documentChanged takes the normal resync path — observed label + // + the foreign-advance epoch bump ride the reseed. (Codex r4 90.) + const s = base({ + rejection: { kind: "pending", id: 7, content: "draft", error: unsafe }, + nextRejectionId: 8, + }); + const withheld = core.transition(s, { + type: "editRejectedDeliveryFailed", + id: 7, + documentVersion: null, + }); + const r = core.transition(withheld.state, { type: "documentChanged", documentVersion: 2 }); + expect(reseedIn(r.effects)).toEqual(pDoc(2, 1)); + }); +}); + +describe("host-session-core: unified settledVersion advance (every outcome, Math.max)", () => { + const locked = base({ pendingApplyBaseVersion: 1, inFlightContent: "edit1" }); + + it("a NON-OK settlement with an observed version advances via Math.max and acks at the observed label", () => { + // refused + a foreign edit raced the failed apply: doc moved 1→2, content unobserved. + const r = core.transition( + locked, + settled({ outcome: { kind: "refused" }, settledVersion: 2, currentContent: null }) + ); + expect(r.state.lastAppliedDocVersion).toBe(2); + // delta 1 > heldBase + 0 → positive foreign evidence → epoch bump rides the ack. + expect(r.state.externalEpoch).toBe(1); + expect(reseedIn(r.effects)).toEqual(pDoc(2, 1)); + }); + + it("Math.max never rewinds: an observed settledVersion LOWER than lastApplied leaves it untouched", () => { + const s = base({ + pendingApplyBaseVersion: 2, + inFlightContent: "edit1", + lastAppliedDocVersion: 3, + }); + const r = core.transition(s, settled({ settledVersion: 2, currentContent: null })); + expect(r.state.lastAppliedDocVersion).toBe(3); + }); +}); + +describe("host-session-core: content-unobserved epoch verdict is positive version-delta evidence only", () => { + const locked = base({ pendingApplyBaseVersion: 1, inFlightContent: "edit1" }); + + it("ok + unobserved content: delta === own contribution (+1) is NOT foreign", () => { + const r = core.transition(locked, settled({ settledVersion: 2, currentContent: null })); + expect(r.state.externalEpoch).toBe(0); + expect(reseedIn(r.effects)).toEqual(pDoc(2)); + }); + + it("ok + unobserved content: delta BEYOND own contribution IS foreign (epoch++ rides the ack)", () => { + const r = core.transition(locked, settled({ settledVersion: 3, currentContent: null })); + expect(r.state.externalEpoch).toBe(1); + expect(reseedIn(r.effects)).toEqual(pDoc(3, 1)); + }); + + it("no advance at all stays NOT foreign (missing ⇒ foreign is the rejected variant)", () => { + const r = core.transition(locked, settled({ settledVersion: null, currentContent: null })); + expect(r.state.externalEpoch).toBe(0); + }); + + it("a label RAISED under the lock supplies the delta even when the settlement observed nothing", () => { + // Composed from two events rather than hand-placed state, because the point + // is WHERE the evidence comes from: the settlement itself observed neither + // the version nor the content, and the only number the verdict can use is the + // one a lock-held `documentChanged` wrote into `lastAppliedDocVersion`. + // heldBase 1 → raised to 3 (delta 2) → beyond our own +1 → foreign. + // A verdict that read `event.settledVersion` instead of the reducer's label + // would score 0 here and leave the epoch at 0, while the sibling tests above + // (which DO observe a version) stay green — this is the arm that catches it. + const raised = core.transition(locked, { type: "documentChanged", documentVersion: 3 }); + expect(raised.effects).toEqual([]); // deferred: the lock is still held + expect(raised.state.externalEpoch).toBe(0); // a lock-held advance never bumps + const r = core.transition( + raised.state, + settled({ settledVersion: null, currentContent: null }) + ); + expect(r.state.lastAppliedDocVersion).toBe(3); + expect(r.state.externalEpoch).toBe(1); + expect(reseedIn(r.effects)).toEqual(pDoc(3, 1)); + }); +}); + +describe("host-session-core: an unobserved ack label still DRAINS (bytes first)", () => { + // `canDrain` gates on CONTENT evidence, never on the ack label: the drain is a + // new WRITE, not an ack. One review cycle added an `ackLabelObserved` conjunct + // and it was reverted — refusing the drain drops the keystroke, and the only + // carrier left (the webview replay buffer) is destroyed by the ORDINARY + // continuation, because the apply DID move the document and its later + // `documentChanged` then reads as a lock-free forward advance ⇒ epoch++ ⇒ + // `edit-sync.ts`'s `recordedEpoch > buf.epoch` drop. Draining instead + // self-heals: the `accept` arm re-acquires the lock, so that same echo lands + // LOCK-HELD and bumps nothing. + // What the drain accepts is the STALE RE-BASE residual — the re-acquired base + // is a lower bound, so a later settlement that ALSO misses its content read can + // score our own increment as foreign (one spurious bump, bytes already landed). + // The tests below pin BOTH halves: the write happens, and the residual is + // stated rather than asserted away. + const lockedStash = (stash: string) => + base({ + pendingApplyBaseVersion: 1, + inFlightContent: "edit1", + pendingEdit: { content: stash, baseDocVersion: 1 }, + }); + const unobserved = settled({ settledVersion: null, currentContent: "edit1" }); + // The EXACT pair `withholdAckEffects` builds at an unobserved label, shared by + // the two readonly/stale/no-op tests below so their exhaustive `toEqual`s + // cannot drift apart. `lockedStash` fixes both numbers in the detail. + const withheldAck = [ + { + type: "logWarn", + message: expect.stringContaining( + "settlement ack withheld: no post-apply document version was observed" + ), + detail: { uri: ctx.uriString, heldBase: 1, lastAppliedDocVersion: 1 }, + }, + { type: "showResyncFailure" }, + ]; + + it("an accept-shaped stash IS applied: the keystroke is written at the stale base", () => { + const r = core.transition(lockedStash("edit1-more"), unobserved); + // EXHAUSTIVE: the write, preceded by the drain's own record of the residual + // it is accepting. The record is what lets a later spurious epoch bump be + // attributed to the drain that caused it. + expect(r.effects).toEqual([ + { + type: "logWarn", + message: expect.stringContaining("unlabelled drain"), + detail: { uri: ctx.uriString, heldBase: 1, lastAppliedDocVersion: 1 }, + }, + { type: "applyEdit", content: "edit1-more", baseDocVersion: 1 }, + ]); + // ARM-SPECIFIC clause: this is the `accept` verdict, so the bytes DID land — + // pinned separately from the `parse-failed` arm's "no bytes land" wording, + // and in the same shape that arm uses. + expect( + r.effects.find((e) => e.type === "logWarn" && e.message.includes("unlabelled drain")) + ).toEqual(expect.objectContaining({ message: expect.stringContaining("The bytes land") })); + expect(r.state.pendingEdit).toBeNull(); + // The lock IS re-acquired — this is what makes the label's catch-up + // lock-HELD in the test below, and so what keeps the epoch still. + expect(r.state.pendingApplyBaseVersion).toBe(1); + expect(r.state.inFlightContent).toBe("edit1-more"); + expect(r.state.externalEpoch).toBe(0); + // The reverted arm-4 token, kept NAMED rather than kept as a guard: with the + // drain's own "unlabelled drain" record now in the array above, the + // exhaustive `toEqual` is what would catch arm 4 coming back (a third + // effect). This line survives so the two tokens cannot be confused — arm 4's + // "unlabelled settle" reported a REFUSED drain's dropped keystroke, and with + // the drain running there is no dropped keystroke to report. + expect( + r.effects.some((e) => e.type === "logWarn" && e.message.includes("unlabelled settle")) + ).toBe(false); + }); + + it("ACCEPTED RESIDUAL: a second content-unobserved settlement scores our own increment as foreign — ONE bump, bytes already landed", () => { + // The residual the describe header STATES, measured rather than asserted + // away. It takes a SECOND independent read failure to reach: the drained + // apply's own settlement must also miss its CONTENT read, so the epoch + // verdict falls back to the version delta — which reads the re-acquired + // base as EXACT while it is really a lower bound. + const drained = core.transition(lockedStash("edit1-more"), unobserved); + expect(drained.state.pendingApplyBaseVersion).toBe(1); // the stale lower bound + const second = core.transition( + drained.state, + settled({ settledVersion: 3, currentContent: null }) + ); + expect(second.state.externalEpoch).toBe(1); // exactly ONE spurious bump + expect(second.state.lastAppliedDocVersion).toBe(3); + expect(second.state.pendingEdit).toBeNull(); // nothing further dropped + expect(reseedIn(second.effects)).toEqual(pDoc(3, 1)); // the ack still goes out + }); + + it("the drain re-acquires the lock, so the label's catch-up is LOCK-HELD and spends no epoch", () => { + // The validator's cycle-2 trace, pinned in the direction the adjudication + // chose. Under the reverted gate this state had `pendingApplyBaseVersion: + // null`, so this same `documentChanged` was a lock-FREE forward advance: + // epoch 1, and `edit-sync.ts`'s `recordedEpoch > buf.epoch` drop check then + // discards the replay buffer holding the keystroke the refusal had just + // dropped. Re-adding the conjunct to `canDrain` turns this red. + const r = core.transition(lockedStash("edit1-more"), unobserved); + const after = core.transition(r.state, { type: "documentChanged", documentVersion: 2 }); + expect(after.effects).toEqual([]); // deferred: the lock is held + expect(after.state.externalEpoch).toBe(0); + expect(after.state.lastAppliedDocVersion).toBe(2); + }); + + it("the drained apply's own settlement catches the label up; the late echo is then a no-op", () => { + // The other half of the convergence: the drain's applyEdit settles WITH an + // observation, which advances the label to the live version and acks there. + // The delayed `documentChanged` for that same edit is then version-identical + // and no-ops, so the epoch is invariant across the whole catch-up — no + // spurious bump anywhere on this path. + const r = core.transition(lockedStash("edit1-more"), unobserved); + // PREMISE, pinned so it cannot be vacated silently: step 2 is the DRAINED + // apply's settlement. Without the drain the lock is free and `inFlightContent` + // null, and everything below still passes while measuring a different event. + expect(r.state.inFlightContent).toBe("edit1-more"); + const s2 = core.transition( + r.state, + settled({ settledVersion: 2, currentContent: "edit1-more" }) + ); + expect(reseedIn(s2.effects)).toEqual(pDoc(2)); + expect(s2.state.externalEpoch).toBe(0); + const after = core.transition(s2.state, { type: "documentChanged", documentVersion: 2 }); + expect(after.effects).toEqual([]); + expect(after.state.externalEpoch).toBe(0); + }); + + it("a parse-failing stash DOES reach decideEdit: the draft is redelivered at the STORED label", () => { + // The ACCEPTED RESIDUAL, pinned LITERALLY rather than asserted away: with no + // observation the draft Document carries `docVersion: 1` — the stored label, + // which may be one edit behind the live document. Every LOCAL gate for this + // was reviewed and rejected (a `ready` replay redelivers at the stored label + // with no resync regardless); the durable fix is the liveness-backstop TODO + // entry. If that entry lands, this expectation is what must change. + const r = core.transition(lockedStash("hasBAD"), unobserved); + expect(r.effects.find((e) => e.type === "postRejectedDraft")).toEqual({ + type: "postRejectedDraft", + content: "hasBAD", + error: unsafe, + docVersion: 1, + externalEpoch: 0, + epochGeneration: GEN, + id: 1, + }); + expect(r.state.rejection).toEqual({ kind: "pending", id: 1, content: "hasBAD", error: unsafe }); + expect(r.state.nextRejectionId).toBe(2); // a delivery id WAS minted + expect(r.effects.some((e) => e.type === "showError")).toBe(true); + // The stale label the draft carries is exactly what the drain's record + // names, so this arm carries it too — and its ARM-SPECIFIC clause says NO + // bytes land, pinned separately from the `accept` arm's "The bytes land" + // wording. One assertion covers both: a missing record fails the `toEqual`. + expect( + r.effects.find((e) => e.type === "logWarn" && e.message.includes("unlabelled drain")) + ).toEqual(expect.objectContaining({ message: expect.stringContaining("no bytes land") })); + }); + + it("a no-op-shaped stash withholds the repost the drain arm makes (EXHAUSTIVE: no stray 'unlabelled drain' log)", () => { + // The drain RUNS here and reaches the `no-op` verdict; what withholds the + // repost is the ACK gate (`ackEffects`), not `canDrain`. This is the pin that + // keeps that withhold branch from being deleted as unreachable. EXHAUSTIVE + // now (not just a partial find/some pair): this readonly/stale/no-op arm + // deliberately does NOT spread `staleReBaseWarn` (that residual is already + // logged through `withholdAckEffects`, the `withheldAck` pair above) — a + // `toEqual` is what would catch a future "for consistency" regression that + // spreads it in anyway. + const r = core.transition(lockedStash("edit1"), unobserved); + expect(r.effects).toEqual(withheldAck); + }); + + it("a readonly-shaped stash withholds the repost the drain arm makes, at an UNOBSERVED label", () => { + // The `readonly` sibling of the test above: `canWrite: false` also lands in + // the readonly/stale/no-op arm, and the only existing `canWrite: false` + // drain test uses an OBSERVED label (line ~572) — this is the missing + // UNOBSERVED-label case named by the describe header. + const r = core.transition( + lockedStash("edit1-more-ro"), + settled({ settledVersion: null, currentContent: "edit1", canWrite: false }) + ); + expect(r.effects).toEqual(withheldAck); + // NEGATIVE: no "unlabelled drain" record leaks into this arm. + expect( + r.effects.some((e) => e.type === "logWarn" && e.message.includes("unlabelled drain")) + ).toBe(false); + }); + + it("an OBSERVED label drains the same way — only the re-base is not stale", () => { + const r = core.transition( + lockedStash("edit1-more"), + settled({ settledVersion: 2, currentContent: "edit1" }) + ); + // NEGATIVE pin, by exhaustive equality: no "unlabelled drain" record here. + // There is no residual to report when the base rests on an observation, so + // an unconditional record would cry wolf on the ordinary path. + expect(r.effects).toEqual([{ type: "applyEdit", content: "edit1-more", baseDocVersion: 2 }]); + }); + + it("POST-DISPOSE drains the same way — there the stash is the keystroke's ONLY carrier", () => { + // Same drain, different stakes: no webview means no replay buffer, so the + // stash is the sole carrier. The `accept` arm deliberately does NOT re-acquire + // the lock here (no more edits arrive), which is why no later settlement ever + // reads this base. + const s = base({ + disposed: true, + pendingApplyBaseVersion: null, // the dispose transition already cleared it + inFlightContent: "edit1", + pendingEdit: { content: "edit1-more", baseDocVersion: 1 }, + }); + const r = core.transition(s, settled({ settledVersion: null, currentContent: "edit1" })); + // NEGATIVE pin on the "unlabelled drain" record, by exhaustive equality: the + // label is unobserved here too, but with no lock re-acquired neither + // consequence that record names can occur (no later settlement reads this + // base, and no draft goes out), so reporting one would be a false claim. + expect(r.effects).toEqual([{ type: "applyEdit", content: "edit1-more", baseDocVersion: 1 }]); + expect(r.state.pendingApplyBaseVersion).toBeNull(); // lock NOT re-acquired + }); +}); diff --git a/test/extension/session/host-session-step.test.ts b/test/extension/session/host-session-step.test.ts index fe852ada..2d2d2ad2 100644 --- a/test/extension/session/host-session-step.test.ts +++ b/test/extension/session/host-session-step.test.ts @@ -31,9 +31,13 @@ import { } from "../../../src/extension/session/host-session-step.js"; // The executor's real settlement event, minus the optional `divergedAfterApply`. -const settled = (outcome: ApplyEditOutcome): HostSessionEvent => ({ +const settled = ( + outcome: ApplyEditOutcome, + settledVersion: number | null = null +): HostSessionEvent => ({ type: "applyEditSettled", outcome, + settledVersion, canWrite: true, currentContent: null, preApplyContent: "", @@ -90,12 +94,12 @@ describe("createHostSessionStep", () => { throw boom; }, }); - expect(() => h.step(settled({ kind: "ok", documentVersion: 3 }))).toThrow(boom); + expect(() => h.step(settled({ kind: "ok" }, 3))).toThrow(boom); expect(h.settles).toEqual([true]); }); it("treats an UNVERIFIED landing as applied", () => { - expect(isEditApplied(settled({ kind: "ok", documentVersion: null }))).toBe(true); + expect(isEditApplied(settled({ kind: "ok" }))).toBe(true); }); it("treats every non-ok outcome as NOT applied", () => { @@ -197,7 +201,7 @@ describe("createHostSessionStep", () => { }, settleEditBarrier: (applied) => barrier.settle(applied), }); - expect(() => step(settled({ kind: "ok", documentVersion: 3 }))).toThrow(); + expect(() => step(settled({ kind: "ok" }, 3))).toThrow(); expect(ran).toHaveBeenCalledTimes(1); expect(dropped).not.toHaveBeenCalled(); });