From b3e1b8bb9a0dc8bed09ace667031a6a080af1cbe Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Tue, 8 Sep 2026 21:10:48 +1000 Subject: [PATCH 01/22] fix(host): gate the settlement ack on an observed version label and adjudicate unobserved-content settlements from version-delta evidence --- src/extension/session/effect-executor.ts | 67 ++-- src/extension/session/host-session-core.ts | 241 +++++++++++--- src/extension/session/host-session-step.ts | 4 +- .../extension/session/effect-executor.test.ts | 66 +++- .../session/host-session-core.test.ts | 307 ++++++++++++++---- .../session/host-session-step.test.ts | 9 +- 6 files changed, 552 insertions(+), 142 deletions(-) diff --git a/src/extension/session/effect-executor.ts b/src/extension/session/effect-executor.ts index 643befc8..657c1d50 100644 --- a/src/extension/session/effect-executor.ts +++ b/src/extension/session/effect-executor.ts @@ -43,6 +43,13 @@ 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. +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 +111,18 @@ 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; // Alias for the injected open-external delegate (see the `openExternal` effect // case for why it is called via this local rather than `deps.openExternal(...)`). @@ -265,12 +274,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": @@ -361,6 +369,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef deps.dispatch({ type: "applyEditSettled", outcome: toApplyEditOutcome(result), + settledVersion: result.settledVersion, canWrite: readCanWrite(), currentContent: result.settledContent, preApplyContent: result.preApplyContent, @@ -470,6 +479,8 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef deps.dispatch({ type: "applyEditSettled", outcome: { kind: "rejected", message: errorMessage(err) }, + // NOT OBSERVED on this arm until the Task-2 dispatch retry lands here. + settledVersion: null, canWrite: false, // NOT OBSERVED — nothing was read, so say nothing rather than // fabricating an empty document. The non-ok foreign-bytes check reads @@ -565,7 +576,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef "[quoll] failed to build the Document to post; skipping this reseed", err ); - if (!reseedBuildFailureReported) { + if (!resyncFailureReported) { // 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 @@ -582,7 +593,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // repo alone. // Spam is the worse failure, and this placement removes it // structurally rather than by argument about VS Code internals. - reseedBuildFailureReported = true; + resyncFailureReported = 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 @@ -595,9 +606,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // 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)." - ); + deps.showError(RESYNC_FAILURE_MESSAGE); } catch (toastErr) { console.error("[quoll] failed to report the reseed build failure", toastErr); } @@ -613,7 +622,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // above (which bounds a SINGLE incident); 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. @@ -701,6 +710,22 @@ 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). + if (!resyncFailureReported) { + resyncFailureReported = true; + try { + deps.showError(RESYNC_FAILURE_MESSAGE); + } catch (err) { + console.error("[quoll] failed to report the withheld settlement ack", err); + } + } + 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..f1e76ab2 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,14 @@ 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: the executor's settle-time read AND its + // guarded settlement-dispatch retry both failed. 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 +139,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 @@ -192,7 +199,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 +235,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; @@ -276,8 +290,9 @@ function contentMatches(a: string, b: string | null): boolean { * 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). */ + * other write is the settlement advance (`advanced` — `Math.max` over + * `event.settledVersion` for EVERY outcome kind, clamp-consistent with this + * helper, no longer an exemption — fenced by the invariant test). */ function resyncLiveVersion(state: HostSessionState, liveVersion: number): HostSessionState { const raised = Math.max(state.lastAppliedDocVersion, liveVersion); const foreignAdvance = @@ -300,10 +315,36 @@ 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: the logWarn is the triage +// record (fires every time), 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. +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 (settle-time read and dispatch retry 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" }, + ]; +} + +// 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` @@ -321,11 +362,15 @@ function settlementEffects( outcome: ApplyEditOutcome, settled: HostSessionState, heldBase: number | null, - context: HostSessionContext + context: HostSessionContext, + ackLabelObserved: boolean ): HostSessionEffect[] { + const ack = ackLabelObserved + ? [postDoc(settled, settled.lastAppliedDocVersion)] + : withholdAckEffects(settled, heldBase, context); switch (outcome.kind) { case "ok": - return [postDoc(settled, settled.lastAppliedDocVersion)]; + return ack; case "refused": return [ { @@ -337,15 +382,12 @@ function settlementEffects( type: "showError", message: `Quoll could not save ${context.fsPath}. Reload the file or try again.`, }, - postDoc(settled, settled.lastAppliedDocVersion), + ...ack, ]; case "constructThrew": case "applyThrew": case "rejected": - return [ - { type: "showError", message: `Failed to save: ${outcome.message}` }, - postDoc(settled, settled.lastAppliedDocVersion), - ]; + return [{ type: "showError", message: `Failed to save: ${outcome.message}` }, ...ack]; default: { const _exhaustive: never = outcome; throw new Error( @@ -563,6 +605,11 @@ 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) { + // `ackLabelObserved` is passed the literal `true` here — the + // `.filter(e => e.type === "showError")` below keeps only toasts + // either way, so the argument is inert. `true` is chosen so the + // withhold pair (a webview-facing signal) is never even constructed + // for a disposed panel. const effects = event.outcome.kind === "ok" ? [] @@ -570,7 +617,8 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes event.outcome, state, state.pendingApplyBaseVersion, - state.context + state.context, + true ).filter((e) => e.type === "showError"); return { state, effects }; } @@ -584,14 +632,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 +676,54 @@ 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). + 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. + 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 @@ -659,8 +744,11 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // 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. const canDrain = stash !== null && event.outcome.kind === "ok" && @@ -677,7 +765,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 ?? ""`: @@ -846,6 +940,18 @@ 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 above, this arm is + // DELIBERATELY NOT gated on `ackLabelObserved` (round-2 reversal — + // see the plan's dispositions table): in the unobserved-label + // corner (version unread twice + 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. 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}` }] : [ @@ -866,10 +972,17 @@ 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). return { state: settled, - effects: state.disposed ? [] : [postDoc(settled, settled.lastAppliedDocVersion)], + effects: state.disposed + ? [] + : ackLabelObserved + ? [postDoc(settled, settled.lastAppliedDocVersion)] + : withholdAckEffects(settled, heldBase, state.context), }; default: { const _exhaustive: never = verdict; @@ -892,6 +1005,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: diff --git a/src/extension/session/host-session-step.ts b/src/extension/session/host-session-step.ts index 38dc3611..0cf20606 100644 --- a/src/extension/session/host-session-step.ts +++ b/src/extension/session/host-session-step.ts @@ -87,8 +87,8 @@ 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 `settledVersion` is null): + // the write completed and only the verification read broke (PR #399). return true; case "refused": case "constructThrew": diff --git a/test/extension/session/effect-executor.test.ts b/test/extension/session/effect-executor.test.ts index 5fd7896f..a2266e67 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,6 +318,9 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect.objectContaining({ type: "applyEditSettled", outcome: expect.objectContaining({ kind: "rejected", message: "boom-read" }), + // TASK-1-ONLY value: nothing was read BY THE PIPELINE (Task 2's guarded + // dispatch retry — not present yet — is what later labels this arm). + settledVersion: null, // NOT OBSERVED — nothing was 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 @@ -340,7 +346,8 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect(dispatch).toHaveBeenCalledWith( expect.objectContaining({ type: "applyEditSettled", - outcome: { kind: "ok", documentVersion: 7 }, + outcome: { kind: "ok" }, + settledVersion: 7, currentContent: null, divergedAfterApply: false, }) @@ -363,7 +370,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 }) ); @@ -499,6 +507,8 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect.objectContaining({ type: "applyEditSettled", outcome: expect.objectContaining({ kind: "rejected", message: "boom-read" }), + // TASK-1-ONLY value; see the "pipeline rejection … STILL settles" test. + settledVersion: null, canWrite: false, }) ); @@ -520,7 +530,8 @@ 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, }) ); @@ -536,7 +547,7 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { // Exactly one version read — the executor's verify. The wrapper adds none. expect(readVersion).toHaveBeenCalledTimes(1); expect(dispatch).toHaveBeenCalledWith( - expect.objectContaining({ outcome: { kind: "ok", documentVersion: 5 } }) + expect.objectContaining({ outcome: { kind: "ok" }, settledVersion: 5 }) ); }); }); @@ -842,3 +853,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..9db78789 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,14 @@ 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,12 +325,15 @@ 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.) + // 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({ outcome: { kind: "ok", documentVersion: null }, currentContent: null }) + 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 @@ -351,7 +355,7 @@ describe("host-session-core: applyEditSettled", () => { }); const r = core.transition( inFlight, - settled({ outcome: { kind: "ok", documentVersion: null }, currentContent: null }) + settled({ settledVersion: null, currentContent: null }) ); expect(r.state.externalEpoch).toBe(inFlight.externalEpoch); expect(r.state.lastAppliedDocVersion).toBe(1); @@ -364,7 +368,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 +392,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 +409,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 +420,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 +446,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 +471,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 +487,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 +495,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 +526,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 +556,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 +576,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 +587,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 +596,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 +605,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 +617,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 +639,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 +662,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 +676,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 +691,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 +703,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 +712,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 +847,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 +863,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 +880,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 +896,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 +906,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 +1031,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([]); @@ -1168,7 +1185,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { }); const r = core.transition( locked, - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "applied" }) + settled({ settledVersion: 2, currentContent: "applied" }) ); expect(r.state.externalEpoch).toBe(0); expect(r.effects).toEqual([pDoc(2)]); @@ -1187,7 +1204,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { }); const r = core.transition( locked, - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "a\r\nb" }) + settled({ settledVersion: 2, currentContent: "a\r\nb" }) ); expect(r.state.externalEpoch).toBe(0); expect(r.effects).toEqual([pDoc(2)]); @@ -1203,6 +1220,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { locked, settled({ outcome: { kind: "refused" }, + settledVersion: 1, currentContent: "a\r\nb", preApplyContent: "a\nb", }) @@ -1219,7 +1237,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { }); const r = core.transition( locked, - settled({ outcome: { kind: "ok", documentVersion: 2 }, currentContent: "foreign" }) + settled({ settledVersion: 2, currentContent: "foreign" }) ); expect(r.state.externalEpoch).toBe(1); expect(r.effects).toEqual([pDoc(2, 1)]); @@ -1235,6 +1253,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { locked, settled({ outcome: { kind: "refused" }, + settledVersion: 1, currentContent: "pre-apply", preApplyContent: "pre-apply", }) @@ -1253,6 +1272,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { locked, settled({ outcome: { kind: "refused" }, + settledVersion: 1, currentContent: "foreign-bytes", preApplyContent: "pre-apply", }) @@ -1273,7 +1293,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 +1326,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 +1363,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 +1387,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 +1406,177 @@ 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("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. What this guards is the early + // return's `.filter(showError)` — it strips the ack AND the withhold pair + // alike, so the literal `ackLabelObserved` argument at that call site is + // deliberately unobservable from here (it is inert by construction). + 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); + }); +}); + +describe("host-session-core: the drain's repost arm carries the same gate", () => { + it("a drained no-op verdict withholds its repost when the label is unobserved", () => { + // Stash present; ok; content OBSERVED equal to inFlight (canDrain passes); + // version unobserved and no lock-held advance. decideEdit(no-op: stash bytes + // === settled bytes) would repost — the gate withholds it. + const s = base({ + pendingApplyBaseVersion: 1, + inFlightContent: "edit1", + pendingEdit: { content: "edit1", baseDocVersion: 1 }, + }); + const r = core.transition(s, 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("a drained ACCEPT still applies (the drain is a new write, not an ack — never withheld)", () => { + const s = base({ + pendingApplyBaseVersion: 1, + inFlightContent: "edit1", + pendingEdit: { content: "edit1-more", baseDocVersion: 1 }, + }); + const r = core.transition(s, settled({ settledVersion: null, currentContent: "edit1" })); + expect(r.effects.some((e) => e.type === "applyEdit")).toBe(true); + }); + +}); diff --git a/test/extension/session/host-session-step.test.ts b/test/extension/session/host-session-step.test.ts index fe852ada..f63474f7 100644 --- a/test/extension/session/host-session-step.test.ts +++ b/test/extension/session/host-session-step.test.ts @@ -31,9 +31,10 @@ 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 +91,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 +198,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(); }); From 2caaabc742daeabb9ae444bd114a111b1d39b57c Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Tue, 8 Sep 2026 21:18:05 +1000 Subject: [PATCH 02/22] fix(host): retry the settle-time version read once at settlement dispatch so a transient failure cannot withhold the ack --- src/extension/document-write/execute-write.ts | 13 +- src/extension/session/effect-executor.ts | 94 +++++--- .../extension/session/effect-executor.test.ts | 216 ++++++++++++++++-- 3 files changed, 277 insertions(+), 46 deletions(-) diff --git a/src/extension/document-write/execute-write.ts b/src/extension/document-write/execute-write.ts index 1ebe9a52..270495c5 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 diff --git a/src/extension/session/effect-executor.ts b/src/extension/session/effect-executor.ts index 657c1d50..5a0f505c 100644 --- a/src/extension/session/effect-executor.ts +++ b/src/extension/session/effect-executor.ts @@ -186,6 +186,39 @@ 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 withholds. 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 BOTH the settlement-dispatch retry above and + // `sendEditRejected`'s recovery dispatches below. + const readVersionGuarded = (): number | null => { + try { + return deps.applyEditSeam.readVersion(); + } catch (err) { + console.warn( + "[quoll] guarded readVersion failed; the label stays UNOBSERVED (null — never a fabricated number)", + 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)` @@ -197,9 +230,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; @@ -219,7 +253,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef deps.dispatch({ type: "editRejectedDeliveryFailed", id, - documentVersion: deps.applyEditSeam.readVersion(), + documentVersion: readVersionGuarded(), }); return; } @@ -243,7 +277,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef deps.dispatch({ type: "editRejectedDeliveryFailed", id, - documentVersion: deps.applyEditSeam.readVersion(), + documentVersion: readVersionGuarded(), }); }, (err: unknown) => { @@ -254,7 +288,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef deps.dispatch({ type: "editRejectedDeliveryFailed", id, - documentVersion: deps.applyEditSeam.readVersion(), + documentVersion: readVersionGuarded(), }); } ); @@ -354,22 +388,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, + settledVersion: result.settledVersion ?? readVersionGuarded(), canWrite: readCanWrite(), currentContent: result.settledContent, preApplyContent: result.preApplyContent, @@ -462,25 +498,27 @@ 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 + authoritative reseed as any other failed + // write, instead of a panel that has quietly stopped saving. // - // ⚠️ 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) }, - // NOT OBSERVED on this arm until the Task-2 dispatch retry lands here. - settledVersion: null, + settledVersion: readVersionGuarded(), canWrite: false, // NOT OBSERVED — nothing was read, so say nothing rather than // fabricating an empty document. The non-ok foreign-bytes check reads diff --git a/test/extension/session/effect-executor.test.ts b/test/extension/session/effect-executor.test.ts index a2266e67..7dc8ec4e 100644 --- a/test/extension/session/effect-executor.test.ts +++ b/test/extension/session/effect-executor.test.ts @@ -318,14 +318,17 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect.objectContaining({ type: "applyEditSettled", outcome: expect.objectContaining({ kind: "rejected", message: "boom-read" }), - // TASK-1-ONLY value: nothing was read BY THE PIPELINE (Task 2's guarded - // dispatch retry — not present yet — is what later labels this arm). - settledVersion: null, - // 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, @@ -507,8 +510,9 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect.objectContaining({ type: "applyEditSettled", outcome: expect.objectContaining({ kind: "rejected", message: "boom-read" }), - // TASK-1-ONLY value; see the "pipeline rejection … STILL settles" test. - settledVersion: null, + // The guarded dispatch retry labels this too; see the + // "pipeline rejection … STILL settles" test. + settledVersion: 1, canWrite: false, }) ); @@ -537,19 +541,106 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { ); }); - // 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" }, 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 }) + ); + } 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, + }) + ); + } finally { + warnSpy.mockRestore(); + } + }); }); const rejErr = { code: "unsafe_url", message: "bad" } as const; @@ -697,6 +788,101 @@ 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, + }); + } 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, + }); + } 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, + }); + } finally { + errorSpy.mockRestore(); + warnSpy.mockRestore(); + } + }); }); describe("effect-executor runEffects other cases", () => { From 3818fcdcafa75f4245ff529dd448de5bd59786c1 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Tue, 8 Sep 2026 21:22:03 +1000 Subject: [PATCH 03/22] test(host): pin the settlement ack-label gate end to end (withhold, retry liveness, lock-held observation, shared latch) --- .../apply-edit-settle-rejection.test.ts | 121 +++++++++++++++++- 1 file changed, 116 insertions(+), 5 deletions(-) diff --git a/test/extension/session/apply-edit-settle-rejection.test.ts b/test/extension/session/apply-edit-settle-rejection.test.ts index dd09cdc7..bc08c214 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,16 @@ // 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 a withheld-ack arrangement (`armVersionFailure`); filter for the message you +// mean. import { describe, expect, it, vi } from "vitest"; @@ -76,6 +85,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 +119,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 +193,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 +232,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 +269,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 +725,81 @@ 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(); + } + }); +}); From 5b6d0a3813492fa2b3a66f984f14747d11bfbc94 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Tue, 8 Sep 2026 21:26:47 +1000 Subject: [PATCH 04/22] style: format the ack-label-gate changes per Biome (no logic change) --- src/extension/session/host-session-core.ts | 3 +- .../apply-edit-settle-rejection.test.ts | 12 +++- .../session/host-session-core.test.ts | 55 ++++++++++--------- .../session/host-session-step.test.ts | 5 +- 4 files changed, 44 insertions(+), 31 deletions(-) diff --git a/src/extension/session/host-session-core.ts b/src/extension/session/host-session-core.ts index f1e76ab2..e12b8f6d 100644 --- a/src/extension/session/host-session-core.ts +++ b/src/extension/session/host-session-core.ts @@ -722,7 +722,8 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // the webview base its next Edit on it → stale verdict → lock-free // forward advance → epoch bump → replay buffer dropped. const ackLabelObserved = - event.settledVersion !== null || (heldBase !== null && settled.lastAppliedDocVersion > heldBase); + 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 diff --git a/test/extension/session/apply-edit-settle-rejection.test.ts b/test/extension/session/apply-edit-settle-rejection.test.ts index bc08c214..3ce390ac 100644 --- a/test/extension/session/apply-edit-settle-rejection.test.ts +++ b/test/extension/session/apply-edit-settle-rejection.test.ts @@ -741,7 +741,9 @@ describe("applyEdit settlement: the ack-label gate end to end", () => { 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("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 { @@ -786,7 +788,9 @@ describe("applyEdit settlement: the ack-label gate end to end", () => { h.armVersionFailure(2); h.type("a"); await flushSettle(); - expect(h.errors.filter((m) => m.includes("could not update the editor view"))).toHaveLength(1); + 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; @@ -797,7 +801,9 @@ describe("applyEdit settlement: the ack-label gate end to end", () => { h.armVersionFailure(2); h.type("recovered!"); await flushSettle(); - expect(h.errors.filter((m) => m.includes("could not update the editor view"))).toHaveLength(2); + expect(h.errors.filter((m) => m.includes("could not update the editor view"))).toHaveLength( + 2 + ); } finally { warnSpy.mockRestore(); } diff --git a/test/extension/session/host-session-core.test.ts b/test/extension/session/host-session-core.test.ts index 9db78789..4a6952a3 100644 --- a/test/extension/session/host-session-core.test.ts +++ b/test/extension/session/host-session-core.test.ts @@ -304,7 +304,10 @@ 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" }, settledVersion: 1 })); + 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)]); @@ -331,10 +334,7 @@ describe("host-session-core: applyEditSettled", () => { // 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 }) - ); + 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 @@ -353,10 +353,7 @@ describe("host-session-core: applyEditSettled", () => { lastAppliedDocVersion: 1, inFlightContent: "edit1", }); - const r = core.transition( - inFlight, - settled({ settledVersion: 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); }); @@ -1183,10 +1180,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { lastAppliedDocVersion: 1, inFlightContent: "applied", }); - const r = core.transition( - locked, - settled({ settledVersion: 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)]); }); @@ -1202,10 +1196,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { lastAppliedDocVersion: 1, inFlightContent: "a\nb", }); - const r = core.transition( - locked, - settled({ settledVersion: 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)]); }); @@ -1235,10 +1226,7 @@ describe("host-session-core: externalEpoch (S3a)", () => { lastAppliedDocVersion: 1, inFlightContent: "target", }); - const r = core.transition( - locked, - settled({ settledVersion: 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)]); }); @@ -1422,7 +1410,11 @@ describe("host-session-core: settlement ack-label gate (ackLabelObserved)", () = 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 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); @@ -1476,7 +1468,11 @@ describe("host-session-core: settlement ack-label gate (ackLabelObserved)", () = rejection: { kind: "pending", id: 7, content: "draft", error: unsafe }, nextRejectionId: 8, }); - const r = core.transition(s, { type: "editRejectedDeliveryFailed", id: 7, documentVersion: null }); + 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); @@ -1488,7 +1484,11 @@ describe("host-session-core: settlement ack-label gate (ackLabelObserved)", () = rejection: { kind: "pending", id: 9, content: "draft", error: unsafe }, nextRejectionId: 10, }); - const r = core.transition(s, { type: "editRejectedDeliveryFailed", id: 7, documentVersion: null }); + const r = core.transition(s, { + type: "editRejectedDeliveryFailed", + id: 7, + documentVersion: null, + }); expect(r.state).toBe(s); expect(r.effects).toEqual([]); }); @@ -1527,7 +1527,11 @@ describe("host-session-core: unified settledVersion advance (every outcome, Math }); it("Math.max never rewinds: an observed settledVersion LOWER than lastApplied leaves it untouched", () => { - const s = base({ pendingApplyBaseVersion: 2, inFlightContent: "edit1", lastAppliedDocVersion: 3 }); + 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); }); @@ -1578,5 +1582,4 @@ describe("host-session-core: the drain's repost arm carries the same gate", () = const r = core.transition(s, settled({ settledVersion: null, currentContent: "edit1" })); expect(r.effects.some((e) => e.type === "applyEdit")).toBe(true); }); - }); diff --git a/test/extension/session/host-session-step.test.ts b/test/extension/session/host-session-step.test.ts index f63474f7..2d2d2ad2 100644 --- a/test/extension/session/host-session-step.test.ts +++ b/test/extension/session/host-session-step.test.ts @@ -31,7 +31,10 @@ import { } from "../../../src/extension/session/host-session-step.js"; // The executor's real settlement event, minus the optional `divergedAfterApply`. -const settled = (outcome: ApplyEditOutcome, settledVersion: number | null = null): HostSessionEvent => ({ +const settled = ( + outcome: ApplyEditOutcome, + settledVersion: number | null = null +): HostSessionEvent => ({ type: "applyEditSettled", outcome, settledVersion, From 7ad052274f246e2307f4097ea2d2ec60b4966442 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Tue, 8 Sep 2026 22:12:05 +1000 Subject: [PATCH 05/22] fix(host): withhold the stash drain when no post-apply version was observed The drain re-bases the stash onto the settled label. Without an observation behind that label the re-acquired base is a known-stale lower bound, so the NEXT settlement's version-delta fallback counts our own unobserved increment as foreign, bumps the epoch, and the resulting reseed drops the webview replay buffer holding the keystrokes the failure toast asks the user to retry. Require the same observation the ack requires. Post-dispose is exempt and stays as it was: the drain does not re-acquire the lock there, so no later settlement ever reads the re-based label, while the stash is the dropped keystroke's only carrier. Gating it there would trade a structurally unreachable epoch bump for real data loss. The newly blocked ALIVE drop gets its own triage log; the webview replay buffer still carries the edit, so no toast. Side effect: the parse-failed drain arm can no longer redeliver a draft Document at an unobserved label, which closes that accepted residual by construction rather than by a local gate. Also here: extract ackEffects so both ack sites share one owner for the gate, pin the two evidence paths the ack label can come from (a lock-held raise licensing the ack, and that raise supplying the version-delta verdict when the settlement itself observed nothing), and correct the settlement comments that described superseded behaviour - the toast-before-reseed rationale, the withhold pair's surface, the count of version-raising paths, the ownContribution invariant, and one inverted cross-reference. --- src/extension/session/host-session-core.ts | 184 ++++++++++++++---- .../session/host-session-core.test.ts | 118 +++++++++-- 2 files changed, 250 insertions(+), 52 deletions(-) diff --git a/src/extension/session/host-session-core.ts b/src/extension/session/host-session-core.ts index e12b8f6d..d403ad4b 100644 --- a/src/extension/session/host-session-core.ts +++ b/src/extension/session/host-session-core.ts @@ -122,8 +122,11 @@ export type HostSessionEvent = // 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: the executor's settle-time read AND its - // guarded settlement-dispatch retry both failed. Never a fabricated number — + // 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; @@ -289,10 +292,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 write is the settlement advance (`advanced` — `Math.max` over - * `event.settledVersion` for EVERY outcome kind, clamp-consistent with this - * helper, no longer an 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 = @@ -316,11 +320,14 @@ const postDoc = (s: HostSessionState, docVersion: number): HostSessionEffect => }); // The withhold pair — what a settlement emits INSTEAD of its ack Document when -// no source observed a post-apply version. Not silent: the logWarn is the triage -// record (fires every time), 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. +// 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 both are stripped by the undrainable arm's showError filter +// below, deliberately: 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, @@ -330,7 +337,7 @@ function withholdAckEffects( { type: "logWarn", message: - "[quoll] settlement ack withheld: no post-apply document version was observed (settle-time read and dispatch retry failed; no lock-held resync arrived) — posting would pair live bytes with a stale label", + "[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, @@ -341,6 +348,22 @@ function withholdAckEffects( ]; } +// 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); +} + // 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 @@ -351,13 +374,17 @@ function withholdAckEffects( // 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 — the sibling test says so explicitly +// ("the toast arrives regardless of order; do not put an order assert back +// here"). 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, @@ -365,9 +392,7 @@ function settlementEffects( context: HostSessionContext, ackLabelObserved: boolean ): HostSessionEffect[] { - const ack = ackLabelObserved - ? [postDoc(settled, settled.lastAppliedDocVersion)] - : withholdAckEffects(settled, heldBase, context); + const ack = ackEffects(ackLabelObserved, settled, heldBase, context); switch (outcome.kind) { case "ok": return ack; @@ -608,8 +633,14 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // `ackLabelObserved` is passed the literal `true` here — the // `.filter(e => e.type === "showError")` below keeps only toasts // either way, so the argument is inert. `true` is chosen so the - // withhold pair (a webview-facing signal) is never even constructed - // for a disposed panel. + // withhold pair is never even constructed for a disposed panel. Note + // what that pair actually is: 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). It is suppressed here for a different reason than the ack: + // 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. const effects = event.outcome.kind === "ok" ? [] @@ -696,6 +727,16 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // 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 || @@ -721,6 +762,23 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // 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 "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). In the second reading the ack pairs live bytes + // with a label that is one edit behind, the same mislabel class the gate + // narrows elsewhere. It is not closable HERE: no predicate can separate + // states that are identical, 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); @@ -750,11 +808,25 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // 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 gate applies HERE TOO, for a different reason than the + // ack: re-basing the stash onto the settled label is only sound when that + // label is a real observation. Without one the re-acquired `heldBase` is a + // known-stale LOWER BOUND, so the NEXT settlement's version-delta fallback + // counts our own unobserved increment as foreign (spurious epoch bump → + // the webview drops the replay buffer holding the very keystrokes the + // failure toast tells the user to retry). + // POST-DISPOSE is exempt, and deliberately so: the drain does NOT + // re-acquire the lock there (the `accept` arm below returns `settled` + // unchanged), so no later settlement ever reads that base — there is no + // stale re-base to be unsound, no webview to mislabel, and the stash is + // the dropped keystroke's ONLY carrier. Gating it there would trade a + // structurally-unreachable epoch bump for real data loss. const canDrain = stash !== null && event.outcome.kind === "ok" && inFlight !== null && observed !== null && + (ackLabelObserved || state.disposed) && contentMatches(observed, inFlight); if (!canDrain) { @@ -788,6 +860,11 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // "external edit won the race" without having read the document // would be a fabricated diagnosis, and an unobserved snapshot is not // a mismatch. + // 4. The settled document IS edit #1's exact result — the drain was + // safe on the CONTENT and only the ack LABEL was never observed, so + // `canDrain` refused rather than re-base onto a stale one. ALIVE + // only: post-dispose the gate is lifted (see `canDrain`), so there + // the keystroke is written instead of logged. // None is a save failure, so none adds a showError (the ok baseEffects // 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 @@ -838,6 +915,19 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes detail, }, ]; + } else if (!ackLabelObserved) { + // Arm 4 — no toast even though a keystroke was dropped: while the + // panel is ALIVE the webview's replay buffer (which this settlement + // does not invalidate) still carries it and re-posts once an + // observed Document lands. Same reasoning as arm 2's ALIVE half. + extraEffects = [ + { + type: "logWarn", + message: + "[quoll] unlabelled settle: pending stash dropped because no post-apply document version was observed (re-basing it onto the stale label would make the next settlement read our own increment as foreign)", + detail, + }, + ]; } } if (state.disposed) { @@ -942,17 +1032,20 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // live base instead of stale-rejecting). Mirrors the `ready`-arm // redelivery precedent. // - // UNLIKE the readonly/stale/no-op repost above, this arm is - // DELIBERATELY NOT gated on `ackLabelObserved` (round-2 reversal — - // see the plan's dispositions table): in the unobserved-label - // corner (version unread twice + 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. A durable fix needs rejection-state provenance + - // observed-version catch-up — its own slice, tracked in the - // follow-up TODO entry. + // UNLIKE the readonly/stale/no-op repost BELOW (the next case in + // this switch), this arm carries NO `ackLabelObserved` ternary of + // its own — and no longer needs one. `canDrain` requires an + // observed label while the panel is alive, so this draft Document + // cannot be reached at an unobserved one; post-dispose, where the + // drain gate is lifted, the arm posts no Document at all. The + // unobserved-label corner this comment used to accept as a residual + // (version unread + no lock-held resync + the stash parse-failing) + // is therefore closed BY CONSTRUCTION rather than by a local gate — + // which is what the reviewed local variants, a Document-free + // degrade included, all failed to achieve (a `ready` replay + // redelivers at the STORED label with no resync regardless). What + // remains is the ack gate's own residual, stated at its definition: + // an observed label is not proof of a POST-APPLY observation. effects: state.disposed ? [{ type: "showError", message: `Cannot save: ${verdict.error.message}` }] : [ @@ -976,14 +1069,16 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // 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). + // not version evidence). Since `canDrain` itself now requires an + // observed label while the panel is alive, the withhold branch is + // structurally unreachable from here — the shared `ackEffects` is + // kept anyway so the two ack sites cannot drift apart if that + // relationship ever changes. return { state: settled, effects: state.disposed ? [] - : ackLabelObserved - ? [postDoc(settled, settled.lastAppliedDocVersion)] - : withholdAckEffects(settled, heldBase, state.context), + : ackEffects(ackLabelObserved, settled, heldBase, state.context), }; default: { const _exhaustive: never = verdict; @@ -1069,8 +1164,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/test/extension/session/host-session-core.test.ts b/test/extension/session/host-session-core.test.ts index 4a6952a3..368af7f9 100644 --- a/test/extension/session/host-session-core.test.ts +++ b/test/extension/session/host-session-core.test.ts @@ -1420,6 +1420,31 @@ describe("host-session-core: settlement ack-label gate (ackLabelObserved)", () = 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" })); @@ -1556,30 +1581,101 @@ describe("host-session-core: content-unobserved epoch verdict is positive versio 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: the drain's repost arm carries the same gate", () => { - it("a drained no-op verdict withholds its repost when the label is unobserved", () => { - // Stash present; ok; content OBSERVED equal to inFlight (canDrain passes); - // version unobserved and no lock-held advance. decideEdit(no-op: stash bytes - // === settled bytes) would repost — the gate withholds it. - const s = base({ +describe("host-session-core: an unobserved ack label refuses the drain (ALIVE)", () => { + // The stash re-bases onto `settled.lastAppliedDocVersion`. Without an observed + // label that base is a known-stale LOWER BOUND, so the NEXT settlement's + // version-delta fallback reads our own unobserved increment as foreign and + // bumps the epoch — which drops the webview replay buffer holding the very + // keystrokes the user is being told to retry. So `canDrain` requires the same + // observation the ack does, and the stash is dropped exactly as it is for an + // unobserved CONTENT. Alive, the keystroke survives in the replay buffer. + const lockedStash = (stash: string) => + base({ pendingApplyBaseVersion: 1, inFlightContent: "edit1", - pendingEdit: { content: "edit1", baseDocVersion: 1 }, + pendingEdit: { content: stash, baseDocVersion: 1 }, }); - const r = core.transition(s, settled({ settledVersion: null, currentContent: "edit1" })); + const unobserved = settled({ settledVersion: null, currentContent: "edit1" }); + + it("an accept-shaped stash is NOT applied: the stash is dropped and the ack withheld", () => { + const r = core.transition(lockedStash("edit1-more"), unobserved); + expect(r.effects.some((e) => e.type === "applyEdit")).toBe(false); + expect(r.state.pendingEdit).toBeNull(); + expect(r.state.pendingApplyBaseVersion).toBeNull(); // lock NOT re-acquired + expect(r.effects.find((e) => e.type === "postDocument")).toBeUndefined(); + expect(r.effects.some((e) => e.type === "showResyncFailure")).toBe(true); + // Not silent: the drop is its own triage record, distinct from the + // unobserved-CONTENT one (here the content WAS read and matched). + expect( + r.effects.some((e) => e.type === "logWarn" && e.message.includes("unlabelled settle")) + ).toBe(true); + expect(r.effects.some((e) => e.type === "showError")).toBe(false); // not a failed save + }); + + it("a parse-failing stash never reaches decideEdit: no rejected draft is posted at a stale label", () => { + // NEGATIVE pin for the parse-failed drain arm, which carries no gate of its + // own. Ungated it would redeliver `postRejectedDraft` stamped with the stored + // (unobserved) version; the drain gate is what makes that unreachable while + // the panel is alive. The BEHAVIOUR of the arm is unchanged — what this pins + // is that the arm is not entered. + const r = core.transition(lockedStash("hasBAD"), unobserved); + expect(r.effects.find((e) => e.type === "postRejectedDraft")).toBeUndefined(); + expect(r.state.rejection).toEqual({ kind: "none" }); + expect(r.state.nextRejectionId).toBe(1); // no delivery id was minted + expect(r.effects.some((e) => e.type === "showResyncFailure")).toBe(true); + }); + + it("a no-op-shaped stash withholds the repost the drain arm would have made", () => { + const r = core.transition(lockedStash("edit1"), unobserved); expect(r.effects.find((e) => e.type === "postDocument")).toBeUndefined(); expect(r.effects.some((e) => e.type === "showResyncFailure")).toBe(true); }); - it("a drained ACCEPT still applies (the drain is a new write, not an ack — never withheld)", () => { + it("an OBSERVED label drains normally: the accept still applies (the drain is a write, not an ack)", () => { + // Positive control for all three above — only the observation differs. + const r = core.transition( + lockedStash("edit1-more"), + settled({ settledVersion: 2, currentContent: "edit1" }) + ); + expect(r.effects).toEqual([{ type: "applyEdit", content: "edit1-more", baseDocVersion: 2 }]); + }); + + it("POST-DISPOSE the gate is LIFTED: the stash still drains at an unobserved label", () => { + // Post-dispose the drain does not re-acquire the lock, so no later settlement + // reads the re-based label — the stale-re-base harm is structurally absent — + // while the stash is the dropped keystroke's ONLY carrier (no webview, no + // replay buffer). Gating it there would trade an unreachable epoch bump for + // real data loss. Dropping the `|| state.disposed` disjunct turns this red. const s = base({ - pendingApplyBaseVersion: 1, + 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" })); - expect(r.effects.some((e) => e.type === "applyEdit")).toBe(true); + expect(r.effects).toEqual([{ type: "applyEdit", content: "edit1-more", baseDocVersion: 1 }]); }); }); From 01ed7da5ab24a8b5a557fcb05a6e683bf799c71b Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Tue, 8 Sep 2026 22:12:13 +1000 Subject: [PATCH 06/22] refactor(host): name the call site in the guarded version read readVersionGuarded serves three call families with different consequences - the resolved settlement's retry, the pipeline-rejection arm's only version read, and the three edit-rejected recovery dispatches - but its warn named none of them, so a withheld ack and a withheld recovery reseed logged the same line. Take a literal site and put it on the warn. The doc counted two families and called the rejection arm's read a retry; it reaches no settle-time read at all. Correct that, and state at the rejection arm that the reseed it promises is conditional on the label being observed. --- src/extension/session/effect-executor.ts | 39 ++++++++++++++----- .../extension/session/effect-executor.test.ts | 8 ++++ 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/src/extension/session/effect-executor.ts b/src/extension/session/effect-executor.ts index 5a0f505c..26ef23e0 100644 --- a/src/extension/session/effect-executor.ts +++ b/src/extension/session/effect-executor.ts @@ -205,14 +205,29 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // // Name it `readVersionGuarded` (NOT "retry"): it is the ONE guarded version // reader, with ONE contract — `number | null`, null ⇔ unobserved, never a - // fabricated value — serving BOTH the settlement-dispatch retry above and - // `sendEditRejected`'s recovery dispatches below. - const readVersionGuarded = (): number | null => { + // 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". Pass a literal — this runs on a failure path and must not evaluate + // anything that can throw. + const readVersionGuarded = (site: string): 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; @@ -253,7 +268,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef deps.dispatch({ type: "editRejectedDeliveryFailed", id, - documentVersion: readVersionGuarded(), + documentVersion: readVersionGuarded("edit-rejected-recovery:sync-throw"), }); return; } @@ -277,7 +292,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef deps.dispatch({ type: "editRejectedDeliveryFailed", id, - documentVersion: readVersionGuarded(), + documentVersion: readVersionGuarded("edit-rejected-recovery:refused"), }); }, (err: unknown) => { @@ -288,7 +303,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef deps.dispatch({ type: "editRejectedDeliveryFailed", id, - documentVersion: readVersionGuarded(), + documentVersion: readVersionGuarded("edit-rejected-recovery:rejected"), }); } ); @@ -405,7 +420,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef deps.dispatch({ type: "applyEditSettled", outcome: toApplyEditOutcome(result), - settledVersion: result.settledVersion ?? readVersionGuarded(), + settledVersion: result.settledVersion ?? readVersionGuarded("settlement-retry"), canWrite: readCanWrite(), currentContent: result.settledContent, preApplyContent: result.preApplyContent, @@ -501,8 +516,12 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // `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 + authoritative reseed as any other failed - // write, instead of a panel that has quietly stopped saving. + // `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 CONTENT or `canWrite()` — // those seams are the candidate throw sources, and a throw HERE strands @@ -518,7 +537,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef deps.dispatch({ type: "applyEditSettled", outcome: { kind: "rejected", message: errorMessage(err) }, - settledVersion: readVersionGuarded(), + 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 diff --git a/test/extension/session/effect-executor.test.ts b/test/extension/session/effect-executor.test.ts index 7dc8ec4e..cd386b08 100644 --- a/test/extension/session/effect-executor.test.ts +++ b/test/extension/session/effect-executor.test.ts @@ -593,6 +593,14 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { 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(); } From f9ca6209b0d2d932b14a9da53b77a500489c8faf Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Tue, 8 Sep 2026 22:12:24 +1000 Subject: [PATCH 07/22] docs(host): correct three settlement contracts the ack gate outdated execute-write's null-contract claimed a numeric version sentinel would be assigned verbatim and rewind the label. The settlement clamps with Math.max, so it could not - the real hazard is worse and now stated: any fabricated number satisfies settledVersion !== null and fabricates the observation the ack gate tests. The consequences are also per missing observation, not one flat list. The barrier's ok arm attributed an unverified landing to a null settledVersion; it is a null currentContent that downgrades the tag, and a version-only failure leaves the write verified. The settle-rejection suite's header banned asserting h.errors is empty under any armVersionFailure, which contradicts the two tests where an ack IS licensed - a transient failure the retry recovers, and a persistent one a lock-held documentChanged labels. Limit the rule to the arrangement that actually withholds. --- src/extension/document-write/execute-write.ts | 14 +++++++++----- src/extension/session/host-session-step.ts | 8 ++++++-- .../session/apply-edit-settle-rejection.test.ts | 13 +++++++++++-- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/extension/document-write/execute-write.ts b/src/extension/document-write/execute-write.ts index 270495c5..a24181e0 100644 --- a/src/extension/document-write/execute-write.ts +++ b/src/extension/document-write/execute-write.ts @@ -214,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/host-session-step.ts b/src/extension/session/host-session-step.ts index 0cf20606..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 (the event's `settledVersion` is 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 3ce390ac..5947ed03 100644 --- a/test/extension/session/apply-edit-settle-rejection.test.ts +++ b/test/extension/session/apply-edit-settle-rejection.test.ts @@ -40,8 +40,17 @@ // `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 a withheld-ack arrangement (`armVersionFailure`); filter for the message you -// mean. +// 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"; From 08c5295891720af6fe9fed64cbcb122a99b6dd1a Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Tue, 8 Sep 2026 22:23:25 +1000 Subject: [PATCH 08/22] refactor(host): give the resync-failure latch one owner (reportResyncFailure) --- src/extension/session/effect-executor.ts | 103 ++++++++++++----------- 1 file changed, 53 insertions(+), 50 deletions(-) diff --git a/src/extension/session/effect-executor.ts b/src/extension/session/effect-executor.ts index 26ef23e0..abac094c 100644 --- a/src/extension/session/effect-executor.ts +++ b/src/extension/session/effect-executor.ts @@ -46,7 +46,12 @@ import type { // 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. +// 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)."; @@ -124,6 +129,43 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // 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: the reseed caller 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`. + 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(...)`). const runOpenExternal = deps.openExternal; @@ -633,41 +675,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef "[quoll] failed to build the Document to post; skipping this reseed", err ); - if (!resyncFailureReported) { - // 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. - resyncFailureReported = 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(RESYNC_FAILURE_MESSAGE); - } 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) { @@ -675,8 +683,9 @@ 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. resyncFailureReported = false; @@ -729,9 +738,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 — @@ -773,15 +782,9 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // 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). - if (!resyncFailureReported) { - resyncFailureReported = true; - try { - deps.showError(RESYNC_FAILURE_MESSAGE); - } catch (err) { - console.error("[quoll] failed to report the withheld settlement ack", err); - } - } + // 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 From 4976a9bdbca771a6b839bdb96cb82f85940cd67d Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 10:05:18 +1000 Subject: [PATCH 09/22] fix(host): drain the stash at an unobserved label instead of dropping the keystroke Reverts the ALIVE half of the `canDrain` ack-label gate added earlier in this branch, restoring the reviewed drain shape: the drain gates on CONTENT evidence only, because it is a new WRITE rather than an ack. The gate refused the drain whenever no post-apply version was observed, which dropped the keystroke and handed it to the webview's replay buffer. That buffer does not survive the ordinary continuation: the apply DID move the document, so its later onDidChangeTextDocument arrives with the lock already released, reads as a lock-free forward advance, bumps the epoch, and edit-sync drops the buffer on recordedEpoch > buf.epoch. No second fault is needed, so the refusal was a deterministic loss of the keystroke the failure toast asks the user to retry. Draining self-heals on that same continuation instead: the accept arm re-acquires the lock at the settled base, so the echo lands lock-held, bumps nothing, and its raise licenses the next ack. What the drain accepts is the stale re-base residual - the re-acquired base is a known-stale lower bound, so a later settlement can score our own increment as foreign and spend one spurious epoch bump. Reaching it needs a second, independent read failure (that settlement must also miss its content read while observing a version beyond heldBase + ownContribution), and by then the drained keystroke is already on the document. Strictly shallower harm at a strictly deeper fault; the residual is documented at canDrain and in the liveness-backstop follow-up entry. The diagnostic arm the gate needed ("unlabelled settle") is removed with it - that configuration now drains, so the arm was unreachable. Tests are rewritten to the new contract and pin both halves: the write happens, and the reachable residual (a parse-failing stash redelivering its draft at the stored label) is pinned literally rather than asserted away. Also here, three comment corrections from the same review: - settlementEffects cited a verbatim quotation that exists in no test and inverted the sibling's instruction; it now names apply-edit-settle-rejection's CONTAINMENT measurement and points the order pin at expectToastBeforeReseed. - the ack gate's ACCEPTED RESIDUAL paragraph claimed the two readings are indistinguishable outright; that holds only when the content is also unobserved - with an observed content the byte mismatch already bumps. - withholdAckEffects named only one of the two post-dispose suppression routes. --- src/extension/session/host-session-core.ts | 129 +++++++++--------- .../session/host-session-core.test.ts | 127 ++++++++++++----- 2 files changed, 158 insertions(+), 98 deletions(-) diff --git a/src/extension/session/host-session-core.ts b/src/extension/session/host-session-core.ts index d403ad4b..d0cf3b39 100644 --- a/src/extension/session/host-session-core.ts +++ b/src/extension/session/host-session-core.ts @@ -325,9 +325,13 @@ const postDoc = (s: HostSessionState, docVersion: number): HostSessionEffect => // 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 both are stripped by the undrainable arm's showError filter -// below, deliberately: there is no view left to resync, and the only loss worth -// reporting there (a dropped stash) has its own toast. +// POST-DISPOSE the pair never reaches the executor, by TWO different routes: +// the no-stash arm (`state.disposed && state.pendingEdit === null`, the early +// return in the `applyEditSettled` case) passes `ackLabelObserved: true` so the +// pair is not even +// constructed, and the undrainable arm keeps only `showError`s from the +// settlement effects. Deliberate in both: 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, @@ -378,9 +382,11 @@ function ackEffects( // 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 — the sibling test says so explicitly -// ("the toast arrives regardless of order; do not put an order assert back -// here"). The correlated failure is also narrower since `settle()` became +// 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 @@ -766,12 +772,15 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // 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 "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). In the second reading the ack pairs live bytes - // with a label that is one edit behind, the same mislabel class the gate - // narrows elsewhere. It is not closable HERE: no predicate can separate - // states that are identical, and every narrowing considered also withholds + // 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 → @@ -808,25 +817,37 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // 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 gate applies HERE TOO, for a different reason than the - // ack: re-basing the stash onto the settled label is only sound when that - // label is a real observation. Without one the re-acquired `heldBase` is a - // known-stale LOWER BOUND, so the NEXT settlement's version-delta fallback - // counts our own unobserved increment as foreign (spurious epoch bump → - // the webview drops the replay buffer holding the very keystrokes the - // failure toast tells the user to retry). - // POST-DISPOSE is exempt, and deliberately so: the drain does NOT - // re-acquire the lock there (the `accept` arm below returns `settled` - // unchanged), so no later settlement ever reads that base — there is no - // stale re-base to be unsound, no webview to mislabel, and the stash is - // the dropped keystroke's ONLY carrier. Gating it there would trade a - // structurally-unreachable epoch bump for real data loss. + // 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. + // One review cycle added `(ackLabelObserved || state.disposed)` here and + // it was reverted after two independent advisors traced the fault depth. + // 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. The durable fix is the liveness + // backstop tracked in the follow-up TODO entry. const canDrain = stash !== null && event.outcome.kind === "ok" && inFlight !== null && observed !== null && - (ackLabelObserved || state.disposed) && contentMatches(observed, inFlight); if (!canDrain) { @@ -860,11 +881,9 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // "external edit won the race" without having read the document // would be a fabricated diagnosis, and an unobserved snapshot is not // a mismatch. - // 4. The settled document IS edit #1's exact result — the drain was - // safe on the CONTENT and only the ack LABEL was never observed, so - // `canDrain` refused rather than re-base onto a stale one. ALIVE - // only: post-dispose the gate is lifted (see `canDrain`), so there - // the keystroke is written instead of logged. + // 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. // None is a save failure, so none adds a showError (the ok baseEffects // 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 @@ -915,19 +934,6 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes detail, }, ]; - } else if (!ackLabelObserved) { - // Arm 4 — no toast even though a keystroke was dropped: while the - // panel is ALIVE the webview's replay buffer (which this settlement - // does not invalidate) still carries it and re-posts once an - // observed Document lands. Same reasoning as arm 2's ALIVE half. - extraEffects = [ - { - type: "logWarn", - message: - "[quoll] unlabelled settle: pending stash dropped because no post-apply document version was observed (re-basing it onto the stale label would make the next settlement read our own increment as foreign)", - detail, - }, - ]; } } if (state.disposed) { @@ -1033,19 +1039,19 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // redelivery precedent. // // UNLIKE the readonly/stale/no-op repost BELOW (the next case in - // this switch), this arm carries NO `ackLabelObserved` ternary of - // its own — and no longer needs one. `canDrain` requires an - // observed label while the panel is alive, so this draft Document - // cannot be reached at an unobserved one; post-dispose, where the - // drain gate is lifted, the arm posts no Document at all. The - // unobserved-label corner this comment used to accept as a residual - // (version unread + no lock-held resync + the stash parse-failing) - // is therefore closed BY CONSTRUCTION rather than by a local gate — - // which is what the reviewed local variants, a Document-free - // degrade included, all failed to achieve (a `ready` replay - // redelivers at the STORED label with no resync regardless). What - // remains is the ack gate's own residual, stated at its definition: - // an observed label is not proof of a POST-APPLY observation. + // 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}` }] : [ @@ -1069,11 +1075,10 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // 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). Since `canDrain` itself now requires an - // observed label while the panel is alive, the withhold branch is - // structurally unreachable from here — the shared `ackEffects` is - // kept anyway so the two ack sites cannot drift apart if that - // relationship ever changes. + // 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 diff --git a/test/extension/session/host-session-core.test.ts b/test/extension/session/host-session-core.test.ts index 368af7f9..f3c12016 100644 --- a/test/extension/session/host-session-core.test.ts +++ b/test/extension/session/host-session-core.test.ts @@ -1604,14 +1604,21 @@ describe("host-session-core: content-unobserved epoch verdict is positive versio }); }); -describe("host-session-core: an unobserved ack label refuses the drain (ALIVE)", () => { - // The stash re-bases onto `settled.lastAppliedDocVersion`. Without an observed - // label that base is a known-stale LOWER BOUND, so the NEXT settlement's - // version-delta fallback reads our own unobserved increment as foreign and - // bumps the epoch — which drops the webview replay buffer holding the very - // keystrokes the user is being told to retry. So `canDrain` requires the same - // observation the ack does, and the stash is dropped exactly as it is for an - // unobserved CONTENT. Alive, the keystroke survives in the replay buffer. +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, @@ -1620,42 +1627,90 @@ describe("host-session-core: an unobserved ack label refuses the drain (ALIVE)", }); const unobserved = settled({ settledVersion: null, currentContent: "edit1" }); - it("an accept-shaped stash is NOT applied: the stash is dropped and the ack withheld", () => { + it("an accept-shaped stash IS applied: the keystroke is written at the stale base", () => { const r = core.transition(lockedStash("edit1-more"), unobserved); - expect(r.effects.some((e) => e.type === "applyEdit")).toBe(false); + expect(r.effects).toEqual([{ type: "applyEdit", content: "edit1-more", baseDocVersion: 1 }]); expect(r.state.pendingEdit).toBeNull(); - expect(r.state.pendingApplyBaseVersion).toBeNull(); // lock NOT re-acquired - expect(r.effects.find((e) => e.type === "postDocument")).toBeUndefined(); - expect(r.effects.some((e) => e.type === "showResyncFailure")).toBe(true); - // Not silent: the drop is its own triage record, distinct from the - // unobserved-CONTENT one (here the content WAS read and matched). + // 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); + // NEGATIVE pin on the diagnostic the reverted gate needed: with the drain + // running there is no dropped keystroke to log, so the arm must stay gone. expect( r.effects.some((e) => e.type === "logWarn" && e.message.includes("unlabelled settle")) - ).toBe(true); - expect(r.effects.some((e) => e.type === "showError")).toBe(false); // not a failed save + ).toBe(false); }); - it("a parse-failing stash never reaches decideEdit: no rejected draft is posted at a stale label", () => { - // NEGATIVE pin for the parse-failed drain arm, which carries no gate of its - // own. Ungated it would redeliver `postRejectedDraft` stamped with the stored - // (unobserved) version; the drain gate is what makes that unreachable while - // the panel is alive. The BEHAVIOUR of the arm is unchanged — what this pins - // is that the arm is not entered. + 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:288` then drops 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")).toBeUndefined(); - expect(r.state.rejection).toEqual({ kind: "none" }); - expect(r.state.nextRejectionId).toBe(1); // no delivery id was minted - expect(r.effects.some((e) => e.type === "showResyncFailure")).toBe(true); + 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); }); - it("a no-op-shaped stash withholds the repost the drain arm would have made", () => { + it("a no-op-shaped stash withholds the repost the drain arm makes", () => { + // 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. const r = core.transition(lockedStash("edit1"), unobserved); expect(r.effects.find((e) => e.type === "postDocument")).toBeUndefined(); expect(r.effects.some((e) => e.type === "showResyncFailure")).toBe(true); }); - it("an OBSERVED label drains normally: the accept still applies (the drain is a write, not an ack)", () => { - // Positive control for all three above — only the observation differs. + 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" }) @@ -1663,12 +1718,11 @@ describe("host-session-core: an unobserved ack label refuses the drain (ALIVE)", expect(r.effects).toEqual([{ type: "applyEdit", content: "edit1-more", baseDocVersion: 2 }]); }); - it("POST-DISPOSE the gate is LIFTED: the stash still drains at an unobserved label", () => { - // Post-dispose the drain does not re-acquire the lock, so no later settlement - // reads the re-based label — the stale-re-base harm is structurally absent — - // while the stash is the dropped keystroke's ONLY carrier (no webview, no - // replay buffer). Gating it there would trade an unreachable epoch bump for - // real data loss. Dropping the `|| state.disposed` disjunct turns this red. + 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 @@ -1677,5 +1731,6 @@ describe("host-session-core: an unobserved ack label refuses the drain (ALIVE)", }); const r = core.transition(s, settled({ settledVersion: null, currentContent: "edit1" })); expect(r.effects).toEqual([{ type: "applyEdit", content: "edit1-more", baseDocVersion: 1 }]); + expect(r.state.pendingApplyBaseVersion).toBeNull(); // lock NOT re-acquired }); }); From e77e73b2bb0f689bd14918c80b79ae5fda7403aa Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 10:05:27 +1000 Subject: [PATCH 10/22] refactor(host): close the guarded version-read site roster as a union readVersionGuarded took `site: string`, so the roster of triage tokens was enforced only by a comment asking readers to pass a literal, and just one of the five labels was observed anywhere in the tests. Type it as a closed union instead: the compiler now rejects an off-roster token and any computed non-literal expression, which matters because this runs on a failure path and must not evaluate anything that can throw. The union cannot catch a copy-paste that stamps one valid token onto the wrong arm - the three adjacent edit-rejected-recovery sites are exactly that shape - so the four unpinned tests each assert the site their name claims. Also here: - reportResyncFailure's GUARDED rationale named only the reseed caller while the doc-comment above it says both triggers report through the helper; it now names both, which is why one guard suffices. - the settle-read-failure warn listed only "no version advance" as the consequence of an unread version. This branch added a second one: the ack Document is withheld unless some source observed a post-apply version. --- src/extension/session/effect-executor.ts | 27 ++++++++++++++----- .../extension/session/effect-executor.test.ts | 22 +++++++++++++++ 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/src/extension/session/effect-executor.ts b/src/extension/session/effect-executor.ts index abac094c..c3b3430d 100644 --- a/src/extension/session/effect-executor.ts +++ b/src/extension/session/effect-executor.ts @@ -149,8 +149,9 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // Spam is the worse failure, and this placement removes it structurally rather // than by argument about VS Code internals. // - // GUARDED: the reseed caller sits INSIDE the boundary that exists to stop a - // throw from escaping `runEffects`, and `window.showErrorMessage`'s SYNCHRONOUS + // 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`. @@ -261,9 +262,23 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // 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". Pass a literal — this runs on a failure path and must not evaluate - // anything that can throw. - const readVersionGuarded = (site: string): number | null => { + // withheld". + // + // The roster IS the contract: a closed set of triage tokens, one per call + // family. Typing it as a union (not `string`) makes both invariants the + // compiler's job — an off-roster token is rejected, and so is any computed + // non-literal expression, which matters because this runs on a failure path + // and must not evaluate anything that can throw. What the union CANNOT catch + // is 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) { @@ -514,7 +529,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // 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 (settledContent !== null), no version advance unless the VERSION was read (settledVersion !== null), 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 ); diff --git a/test/extension/session/effect-executor.test.ts b/test/extension/session/effect-executor.test.ts index cd386b08..d4e14779 100644 --- a/test/extension/session/effect-executor.test.ts +++ b/test/extension/session/effect-executor.test.ts @@ -645,6 +645,13 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { 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(); } @@ -826,6 +833,11 @@ describe("effect-executor sendEditRejected (via postEditRejected effect)", () => 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(); @@ -856,6 +868,11 @@ describe("effect-executor sendEditRejected (via postEditRejected effect)", () => id: 42, documentVersion: null, }); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("guarded readVersion failed"), + { site: "edit-rejected-recovery:refused" }, + expect.anything() + ); } finally { errorSpy.mockRestore(); warnSpy.mockRestore(); @@ -886,6 +903,11 @@ describe("effect-executor sendEditRejected (via postEditRejected effect)", () => id: 42, documentVersion: null, }); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("guarded readVersion failed"), + { site: "edit-rejected-recovery:rejected" }, + expect.anything() + ); } finally { errorSpy.mockRestore(); warnSpy.mockRestore(); From ebabd24a3a87acba35f19823cbe7d0a3c3d7e02b Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 10:15:16 +1000 Subject: [PATCH 11/22] test(host): pin the ack-withhold clause on the settle-read-failure warn --- test/extension/session/effect-executor.test.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/extension/session/effect-executor.test.ts b/test/extension/session/effect-executor.test.ts index d4e14779..b2ddde54 100644 --- a/test/extension/session/effect-executor.test.ts +++ b/test/extension/session/effect-executor.test.ts @@ -404,6 +404,14 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { expect.stringContaining("no stash drain"), 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( From b07d3874ec41083768ef29ac320b3eaae679ca2c Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 10:24:18 +1000 Subject: [PATCH 12/22] docs(host): reflow and disambiguate the canDrain commentary after the gate revert --- src/extension/session/host-session-core.ts | 41 ++++++++++++---------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/src/extension/session/host-session-core.ts b/src/extension/session/host-session-core.ts index d0cf3b39..3764ab09 100644 --- a/src/extension/session/host-session-core.ts +++ b/src/extension/session/host-session-core.ts @@ -328,10 +328,10 @@ const postDoc = (s: HostSessionState, docVersion: number): HostSessionEffect => // POST-DISPOSE the pair never reaches the executor, by TWO different routes: // the no-stash arm (`state.disposed && state.pendingEdit === null`, the early // return in the `applyEditSettled` case) passes `ackLabelObserved: true` so the -// pair is not even -// constructed, and the undrainable arm keeps only `showError`s from the -// settlement effects. Deliberate in both: there is no view left to resync, and -// the only loss worth reporting there (a dropped stash) has its own toast. +// pair is not even constructed, and the undrainable arm keeps only `showError`s +// from the settlement effects. Deliberate in both: 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, @@ -386,11 +386,11 @@ function ackEffects( // 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. +// 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, @@ -780,10 +780,10 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // 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 → + // 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 @@ -805,7 +805,8 @@ 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 @@ -874,20 +875,22 @@ 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 // would be a fabricated diagnosis, and an unobserved snapshot is not // a mismatch. - // 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. // None is a save failure, so none adds a showError (the ok baseEffects // 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 && From a7832de8f795d65480928176c3a54600a55db8cf Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 10:57:11 +1000 Subject: [PATCH 13/22] docs(host): correct the settle-path rationales that overstate what holds them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment-only. Seven claims that a reader could act on but that do not survive checking: - the guarded-version-read roster does NOT make the literal rule a compiler guarantee — a helper returning the union, or a ternary over two valid tokens, type-checks fine, so the literal rule is stated as the convention it is; - a VERSION-only read failure puts the self-advance AT RISK rather than suppressing it, since the guarded dispatch retry rescues the transient case (two source sites plus the test-side rationale); - the withhold pair is unreachable post-dispose by THREE routes, not two — a stash that drains post-dispose never calls ackEffects at all; - a persistent version-read failure withholds UNLESS a lock-held resync already raised the label (the second disjunct of ackLabelObserved); - "saved your change" holds on the no-op route by precondition, but the landed-but-unverified route only has the apply's ok, which is why the toast keeps its "could not verify it" hedge; - the canDrain revert warning now names the test that enforces it; - the stale-re-base residual is tracked in the follow-up TODO entry but is NOT closed by that entry's ack-timeout / reseed-retry half: the mis-scoring happens inside the next settlement's version-delta fallback, which reads heldBase as exact and has no input a timeout or retry can reach. Also replaces a rot-prone `edit-sync.ts:288` line reference with the symbolic `recordedEpoch > buf.epoch` form the same describe block already uses. --- src/extension/session/effect-executor.ts | 39 ++++++++++-------- src/extension/session/host-session-core.ts | 40 +++++++++++++------ .../extension/session/effect-executor.test.ts | 5 ++- .../session/host-session-core.test.ts | 14 ++++--- 4 files changed, 62 insertions(+), 36 deletions(-) diff --git a/src/extension/session/effect-executor.ts b/src/extension/session/effect-executor.ts index c3b3430d..4461ac2b 100644 --- a/src/extension/session/effect-executor.ts +++ b/src/extension/session/effect-executor.ts @@ -241,10 +241,11 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // 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 withholds. 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). + // 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 @@ -265,13 +266,15 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // withheld". // // The roster IS the contract: a closed set of triage tokens, one per call - // family. Typing it as a union (not `string`) makes both invariants the - // compiler's job — an off-roster token is rejected, and so is any computed - // non-literal expression, which matters because this runs on a failure path - // and must not evaluate anything that can throw. What the union CANNOT catch - // is 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`. + // family. 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" @@ -493,9 +496,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; @@ -522,9 +526,10 @@ 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( diff --git a/src/extension/session/host-session-core.ts b/src/extension/session/host-session-core.ts index 3764ab09..57262936 100644 --- a/src/extension/session/host-session-core.ts +++ b/src/extension/session/host-session-core.ts @@ -325,13 +325,15 @@ const postDoc = (s: HostSessionState, docVersion: number): HostSessionEffect => // 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 TWO different routes: +// 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) passes `ackLabelObserved: true` so the -// pair is not even constructed, and the undrainable arm keeps only `showError`s -// from the settlement effects. Deliberate in both: there is no view left to -// resync, and the only loss worth reporting there (a dropped stash) has its own -// toast. +// 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, @@ -820,8 +822,10 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // 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. - // One review cycle added `(ackLabelObserved || state.disposed)` here and - // it was reverted after two independent advisors traced the fault depth. + // ⛔ 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 @@ -842,8 +846,14 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // 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. The durable fix is the liveness - // backstop tracked in the follow-up TODO entry. + // 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" && @@ -958,9 +968,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. diff --git a/test/extension/session/effect-executor.test.ts b/test/extension/session/effect-executor.test.ts index b2ddde54..9d6d9584 100644 --- a/test/extension/session/effect-executor.test.ts +++ b/test/extension/session/effect-executor.test.ts @@ -359,7 +359,10 @@ describe("effect-executor runApplyEdit (wrapper mapping)", () => { // 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(() => {}); diff --git a/test/extension/session/host-session-core.test.ts b/test/extension/session/host-session-core.test.ts index f3c12016..652739a8 100644 --- a/test/extension/session/host-session-core.test.ts +++ b/test/extension/session/host-session-core.test.ts @@ -1636,8 +1636,12 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" expect(r.state.pendingApplyBaseVersion).toBe(1); expect(r.state.inFlightContent).toBe("edit1-more"); expect(r.state.externalEpoch).toBe(0); - // NEGATIVE pin on the diagnostic the reverted gate needed: with the drain - // running there is no dropped keystroke to log, so the arm must stay gone. + // 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); @@ -1647,9 +1651,9 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" // 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:288` then drops the replay buffer holding the - // keystroke the refusal had just dropped. Re-adding the conjunct to - // `canDrain` turns this red. + // 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 From 8aa943179a77a101180304a10e51bb38ca722e3e Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 11:01:10 +1000 Subject: [PATCH 14/22] fix(host): record the unlabelled drain, and stop the disposed arm fabricating ack evidence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stash drain is the one degraded path in this file that carried no triage record. It is also the only one that WRITES at a base it knows to be a lower bound, so a later spurious epoch bump could not be attributed to the drain that caused it. Emit a `logWarn` naming the stale re-base and both of its consequences, on the accept and parse-failed arms. Scoped three ways, each pinned: - only when `ackLabelObserved` is false — there is no residual to report when the base rests on an observation; - never on the readonly/stale/no-op arm, which already logs the same incident through `withholdAckEffects`; - never post-dispose, where the accept arm deliberately does not re-acquire the lock, so neither named consequence can occur (`heldBase` is null there, and the record would cite a base that was never held). Also pins the accepted residual itself: a second content-unobserved settlement takes the version-delta fallback, which reads the re-acquired base as exact and scores our own increment as foreign — exactly one epoch bump, with the drained bytes already on the document. Separately, the disposed-no-stash arm asked `settlementEffects` for the full effect set and filtered it down to `showError`, which meant handing the ack gate a literal `true` for an observation it did not have and depending on the filter for its correctness. It now builds its toasts directly through `failureToasts`, which `settlementEffects` also uses so the toast text keeps one owner. --- src/extension/session/host-session-core.ts | 93 +++++++++++++------ .../session/host-session-core.test.ts | 42 ++++++++- 2 files changed, 107 insertions(+), 28 deletions(-) diff --git a/src/extension/session/host-session-core.ts b/src/extension/session/host-session-core.ts index 57262936..145ec1c0 100644 --- a/src/extension/session/host-session-core.ts +++ b/src/extension/session/host-session-core.ts @@ -393,6 +393,32 @@ function ackEffects( // 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. +// 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.`, + }, + ]; + default: + return [{ type: "showError", message: `Failed to save: ${outcome.message}` }]; + } +} + function settlementEffects( outcome: ApplyEditOutcome, settled: HostSessionState, @@ -411,16 +437,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.`, - }, + ...failureToasts(outcome, context), ...ack, ]; case "constructThrew": case "applyThrew": case "rejected": - return [{ type: "showError", message: `Failed to save: ${outcome.message}` }, ...ack]; + return [...failureToasts(outcome, context), ...ack]; default: { const _exhaustive: never = outcome; throw new Error( @@ -638,28 +661,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) { - // `ackLabelObserved` is passed the literal `true` here — the - // `.filter(e => e.type === "showError")` below keeps only toasts - // either way, so the argument is inert. `true` is chosen so the - // withhold pair is never even constructed for a disposed panel. Note - // what that pair actually is: 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). It is suppressed here for a different reason than the ack: - // 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. - const effects = - event.outcome.kind === "ok" - ? [] - : settlementEffects( - event.outcome, - state, - state.pendingApplyBaseVersion, - state.context, - true - ).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; @@ -1012,6 +1025,30 @@ 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). + const staleReBaseWarn: HostSessionEffect[] = ackLabelObserved + ? [] + : [ + { + type: "logWarn", + message: + "[quoll] unlabelled drain: the pending stash was re-based onto an UNOBSERVED settlement label (a known-stale lower bound). 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), and a parse-failed 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 @@ -1026,6 +1063,7 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes inFlightContent: stash.content, }, effects: [ + ...(state.disposed ? [] : staleReBaseWarn), { type: "applyEdit", content: stash.content, @@ -1072,6 +1110,7 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes effects: state.disposed ? [{ type: "showError", message: `Cannot save: ${verdict.error.message}` }] : [ + ...staleReBaseWarn, { type: "postRejectedDraft", content: stash.content, diff --git a/test/extension/session/host-session-core.test.ts b/test/extension/session/host-session-core.test.ts index 652739a8..e3875680 100644 --- a/test/extension/session/host-session-core.test.ts +++ b/test/extension/session/host-session-core.test.ts @@ -1629,7 +1629,17 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" it("an accept-shaped stash IS applied: the keystroke is written at the stale base", () => { const r = core.transition(lockedStash("edit1-more"), unobserved); - expect(r.effects).toEqual([{ type: "applyEdit", content: "edit1-more", baseDocVersion: 1 }]); + // 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 }, + ]); 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. @@ -1647,6 +1657,24 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" ).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: @@ -1703,6 +1731,11 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" 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. + expect( + r.effects.some((e) => e.type === "logWarn" && e.message.includes("unlabelled drain")) + ).toBe(true); }); it("a no-op-shaped stash withholds the repost the drain arm makes", () => { @@ -1719,6 +1752,9 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" 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 }]); }); @@ -1734,6 +1770,10 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" 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 }); From 7a98f049ba9b1a4be677243a977c48ddb9ecd0e3 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 11:08:47 +1000 Subject: [PATCH 15/22] fix(host): state the settle-read warn's version clause conditionally, and pin the ack clause on both arrangements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The warn's parentheticals named a mix of result fields and event fields, so a reader binding both to the pipeline result read "no version advance unless the VERSION was read" as a verdict on this settlement — false whenever the guarded dispatch retry rescued a transient failure. Name the EVENT fields, and say that either source can supply the observation. The ack clause was pinned only on the arrangement where it is true (a version-only failure, where the ack really is withheld). The mirror — content unread, version observed, ack posted — asserted nothing about the warn at all, so an unconditional reword of that clause would have stayed green while the warn claimed the opposite of what happened. That test now spies on console.warn and pins the clause's conditional form. Both existing substring pins are preserved verbatim. --- src/extension/session/effect-executor.ts | 2 +- .../extension/session/effect-executor.test.ts | 43 ++++++++++++------- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/src/extension/session/effect-executor.ts b/src/extension/session/effect-executor.ts index 4461ac2b..531a93c5 100644 --- a/src/extension/session/effect-executor.ts +++ b/src/extension/session/effect-executor.ts @@ -534,7 +534,7 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // 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), and the ack Document is withheld unless some source observed a post-apply version" + ? "[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 ); diff --git a/test/extension/session/effect-executor.test.ts b/test/extension/session/effect-executor.test.ts index 9d6d9584..bb22b179 100644 --- a/test/extension/session/effect-executor.test.ts +++ b/test/extension/session/effect-executor.test.ts @@ -340,21 +340,34 @@ 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" }, - settledVersion: 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 From 4cb7e870a89236afa6f085db9da22ae5b480de42 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 11:14:11 +1000 Subject: [PATCH 16/22] test(host): pin the settle-read warn's version-clause attribution --- test/extension/session/effect-executor.test.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/extension/session/effect-executor.test.ts b/test/extension/session/effect-executor.test.ts index bb22b179..22d360b4 100644 --- a/test/extension/session/effect-executor.test.ts +++ b/test/extension/session/effect-executor.test.ts @@ -420,6 +420,14 @@ 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 From 31d61ddd7aafbc0e12df6bdda2e6d361ac2f81a8 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 12:21:27 +1000 Subject: [PATCH 17/22] refactor(host): keep the staleReBaseWarn gate in one place and re-home failureToasts --- src/extension/session/host-session-core.ts | 78 ++++++++++++---------- 1 file changed, 41 insertions(+), 37 deletions(-) diff --git a/src/extension/session/host-session-core.ts b/src/extension/session/host-session-core.ts index 145ec1c0..02876676 100644 --- a/src/extension/session/host-session-core.ts +++ b/src/extension/session/host-session-core.ts @@ -370,29 +370,6 @@ function ackEffects( : withholdAckEffects(settled, heldBase, context); } -// 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 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. // 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 @@ -419,6 +396,29 @@ function failureToasts( } } +// 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 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, @@ -1035,20 +1035,24 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes // 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). - const staleReBaseWarn: HostSessionEffect[] = ackLabelObserved - ? [] - : [ - { - type: "logWarn", - message: - "[quoll] unlabelled drain: the pending stash was re-based onto an UNOBSERVED settlement label (a known-stale lower bound). 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), and a parse-failed draft goes out stamped with this stale label", - detail: { - uri: state.context.uriString, - heldBase, - lastAppliedDocVersion: settled.lastAppliedDocVersion, + // 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", + message: + "[quoll] unlabelled drain: the pending stash was re-based onto an UNOBSERVED settlement label (a known-stale lower bound). 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), and a parse-failed 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 @@ -1063,7 +1067,7 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes inFlightContent: stash.content, }, effects: [ - ...(state.disposed ? [] : staleReBaseWarn), + ...staleReBaseWarn, { type: "applyEdit", content: stash.content, From 7c30161f9c5b401c869ba9d2741e03821beba0d2 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 12:38:59 +1000 Subject: [PATCH 18/22] docs(host): fix GuardedVersionReadSite roster comment (site, not family) The doc comment claimed the union is 'one per call family', contradicting the very next sentence and the roster itself: family 3 (sendEditRejected's three recovery dispatches) alone owns three of the five tokens. Corrected to 'one per call site (not one per family ...)'. --- src/extension/session/effect-executor.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/extension/session/effect-executor.ts b/src/extension/session/effect-executor.ts index 531a93c5..53d44f56 100644 --- a/src/extension/session/effect-executor.ts +++ b/src/extension/session/effect-executor.ts @@ -266,12 +266,13 @@ export function createEffectExecutor(deps: EffectExecutorDeps): Ef // withheld". // // The roster IS the contract: a closed set of triage tokens, one per call - // family. 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). + // 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`. From 864ecd7f48e897b1ddd5796edd020e6946541ebf Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 12:39:34 +1000 Subject: [PATCH 19/22] fix(host): close failureToasts exhaustively and split the unlabelled-drain warn per arm failureToasts switched over ApplyEditOutcome with a bare default reading outcome.message, unlike every other outcome-kind switch in this file. Safe only because all three folded-in variants happen to share a message field; a future 6th variant would compile silently through the direct call site (applyEditSettled's disposed-no-stash arm has no exhaustive switch upstream to catch it). Switched to explicit cases plus a never-typed default, same idiom as settlementEffects. staleReBaseWarn's message asserted consequences that are false on one of its two arms: the accept arm's 'the bytes land' clause is true but its 'a parse-failed draft goes out' clause is not (only an applyEdit is emitted there), while the parse-failed arm is the reverse (no applyEdit, only the rejected draft). Split the message per verdict.kind so each arm states only what actually happens, keeping the shared 'unlabelled drain' prefix the existing stringContaining pins key off. Strengthened both existing pins to assert the arm-specific clause in addition to the shared prefix, so a regression that collapses the two messages back into one inaccurate string is caught. --- src/extension/session/host-session-core.ts | 14 ++++++++++++-- test/extension/session/host-session-core.test.ts | 8 ++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/extension/session/host-session-core.ts b/src/extension/session/host-session-core.ts index 02876676..0c73cb7e 100644 --- a/src/extension/session/host-session-core.ts +++ b/src/extension/session/host-session-core.ts @@ -391,8 +391,16 @@ function failureToasts( message: `Quoll could not save ${context.fsPath}. Reload the file or try again.`, }, ]; - default: + 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}` + ); + } } } @@ -1045,7 +1053,9 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes { type: "logWarn", message: - "[quoll] unlabelled drain: the pending stash was re-based onto an UNOBSERVED settlement label (a known-stale lower bound). 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), and a parse-failed draft goes out stamped with this stale label", + verdict.kind === "accept" + ? "[quoll] unlabelled drain: the pending stash was re-based onto an UNOBSERVED settlement label (a known-stale lower bound). 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)" + : "[quoll] unlabelled drain: the pending stash was re-based onto an UNOBSERVED settlement label (a known-stale lower bound). 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, diff --git a/test/extension/session/host-session-core.test.ts b/test/extension/session/host-session-core.test.ts index e3875680..952130b1 100644 --- a/test/extension/session/host-session-core.test.ts +++ b/test/extension/session/host-session-core.test.ts @@ -1640,6 +1640,9 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" }, { 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. + expect((r.effects[0] as { message: string }).message).toContain("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. @@ -1736,6 +1739,11 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" expect( r.effects.some((e) => e.type === "logWarn" && e.message.includes("unlabelled drain")) ).toBe(true); + // ARM-SPECIFIC clause: this is the `parse-failed` verdict, so NO bytes land — + // pinned separately from the `accept` arm's "The bytes land" wording. + 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", () => { From 2b4bd1c41f47b9e854c5b0617d829bcf87e7eee9 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 12:40:02 +1000 Subject: [PATCH 20/22] test(host): pin the readonly/stale/no-op drain arm's unobserved-label exclusion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The drain's readonly/stale/no-op arm deliberately does not spread staleReBaseWarn into its effects (that residual is already logged through withholdAckEffects). No test pinned that exclusion exhaustively, and the readonly sub-case at an UNOBSERVED ack label had no test at all — the only existing canWrite:false drain test uses an OBSERVED label. Strengthened the existing no-op test to a full toEqual and added the missing readonly case, both with a negative pin against a stray 'unlabelled drain' log. --- .../session/host-session-core.test.ts | 45 +++++++++++++++++-- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/test/extension/session/host-session-core.test.ts b/test/extension/session/host-session-core.test.ts index 952130b1..bff5fa14 100644 --- a/test/extension/session/host-session-core.test.ts +++ b/test/extension/session/host-session-core.test.ts @@ -1746,13 +1746,50 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" ).toEqual(expect.objectContaining({ message: expect.stringContaining("no bytes land") })); }); - it("a no-op-shaped stash withholds the repost the drain arm makes", () => { + 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. + // 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` below) — 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.find((e) => e.type === "postDocument")).toBeUndefined(); - expect(r.effects.some((e) => e.type === "showResyncFailure")).toBe(true); + expect(r.effects).toEqual([ + { + 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("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([ + { + 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" }, + ]); + // 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", () => { From e5627210007f7558dbaa41bc2ecacde964b9870a Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 13:19:51 +1000 Subject: [PATCH 21/22] refactor(host): share the unlabelled-drain sentence and align the arm pins --- src/extension/session/host-session-core.ts | 10 +++- .../session/host-session-core.test.ts | 56 +++++++++---------- 2 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/extension/session/host-session-core.ts b/src/extension/session/host-session-core.ts index 0c73cb7e..5c707f0e 100644 --- a/src/extension/session/host-session-core.ts +++ b/src/extension/session/host-session-core.ts @@ -1052,10 +1052,14 @@ export function createHostSessionCore(context: HostSessionContext, deps: HostSes : [ { 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: - verdict.kind === "accept" - ? "[quoll] unlabelled drain: the pending stash was re-based onto an UNOBSERVED settlement label (a known-stale lower bound). 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)" - : "[quoll] unlabelled drain: the pending stash was re-based onto an UNOBSERVED settlement label (a known-stale lower bound). The stash did not validate, so no bytes land; the residual is that its rejected draft goes out stamped with this stale label", + "[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, diff --git a/test/extension/session/host-session-core.test.ts b/test/extension/session/host-session-core.test.ts index bff5fa14..4ab71d7c 100644 --- a/test/extension/session/host-session-core.test.ts +++ b/test/extension/session/host-session-core.test.ts @@ -1626,6 +1626,19 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" 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); @@ -1641,8 +1654,11 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" { 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. - expect((r.effects[0] as { message: string }).message).toContain("The bytes 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. @@ -1735,12 +1751,9 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" 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. - expect( - r.effects.some((e) => e.type === "logWarn" && e.message.includes("unlabelled drain")) - ).toBe(true); - // ARM-SPECIFIC clause: this is the `parse-failed` verdict, so NO bytes land — - // pinned separately from the `accept` arm's "The bytes land" wording. + // 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") })); @@ -1752,19 +1765,11 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" // 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` below) — a `toEqual` is what would - // catch a future "for consistency" regression that spreads it in anyway. + // 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([ - { - 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" }, - ]); + expect(r.effects).toEqual(withheldAck); }); it("a readonly-shaped stash withholds the repost the drain arm makes, at an UNOBSERVED label", () => { @@ -1776,16 +1781,7 @@ describe("host-session-core: an unobserved ack label still DRAINS (bytes first)" lockedStash("edit1-more-ro"), settled({ settledVersion: null, currentContent: "edit1", canWrite: false }) ); - expect(r.effects).toEqual([ - { - 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" }, - ]); + 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")) From 3acb2ec8e8dc994c1e64e37619e2c0f2fe9d7e04 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 9 Sep 2026 13:35:57 +1000 Subject: [PATCH 22/22] docs(host): correct two settlement comments that drifted from the ack-label gate The divergedAfterApply field doc described the ok-but-mismatch convergence shape as epoch++ plus an authoritative resync plus a diverged log, which reads as if all three are unconditional. Only two are: execute-write's settle() guards the content read and the version read separately, so the diverged tag proves the content was observed and says nothing about the version. With an unobserved label the settlement swaps the ack Document for the withhold pair while the epoch bump and the diverged log run regardless. The POST-DISPOSE withhold-pair test still described the disposed no-stash arm as filtering settlement effects down to showError. That arm now calls failureToasts directly and never reaches settlementEffects, so neither the ack nor the withhold pair is constructed at all -- which is what the test actually pins. --- src/extension/session/host-session-core.ts | 6 +++++- test/extension/session/host-session-core.test.ts | 10 ++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/extension/session/host-session-core.ts b/src/extension/session/host-session-core.ts index 5c707f0e..f5e736d4 100644 --- a/src/extension/session/host-session-core.ts +++ b/src/extension/session/host-session-core.ts @@ -193,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. diff --git a/test/extension/session/host-session-core.test.ts b/test/extension/session/host-session-core.test.ts index 4ab71d7c..83afdb1b 100644 --- a/test/extension/session/host-session-core.test.ts +++ b/test/extension/session/host-session-core.test.ts @@ -1470,10 +1470,12 @@ describe("host-session-core: settlement ack-label gate (ackLabelObserved)", () = }); it("POST-DISPOSE the withhold pair is suppressed with the rest of the webview-bound effects", () => { - // Disposed + no stash + unobserved version. What this guards is the early - // return's `.filter(showError)` — it strips the ack AND the withhold pair - // alike, so the literal `ackLabelObserved` argument at that call site is - // deliberately unobservable from here (it is inert by construction). + // 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([]);