fix(host): gate the settlement ack on an observed version label - #403
Merged
Conversation
…djudicate unobserved-content settlements from version-delta evidence
…atch so a transient failure cannot withhold the ack
…etry liveness, lock-held observation, shared latch)
…served 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.
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.
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.
… 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.
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.
…lds them 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.
…ricating ack evidence 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.
… and pin the ack clause on both arrangements 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.
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 ...)'.
…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.
… exclusion 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.
…-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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Gate the settlement ack Document on an OBSERVED post-apply version label. A settlement where neither the settle-time/dispatch-retry
readVersionnor a lock-held resync observeddocument.versionused to post live V+1 bytes labelleddocVersion: V; the webview would base its next Edit on that stale label, ending in a stale verdict → lock-free forward advance →externalEpochbump → the replay buffer dropped (buffered-keystroke loss). Now the ack posts only when the label is backed by a real observation; otherwise it is withheld with a per-incident latched toast, and the content-unobserved epoch verdict falls back to positive version-delta evidence instead of "not foreign".Changes
host-session-core.ts: one version representation —ApplyEditOutcome.ok.documentVersiondeleted; theapplyEditSettledevent carries a requiredsettledVersion: number | null, applied tolastAppliedDocVersionviaMath.maxfor every outcome kind. NewackLabelObservedgate threaded throughsettlementEffects(every arm) and the drain's readonly/stale/no-op repost; withheld acks emitlogWarn+ a newshowResyncFailureeffect. Content-unobserved foreign verdict:lastAppliedDocVersion > heldBase + (ok ? 1 : 0)(positive evidence only).editRejectedDeliveryFailed.documentVersionis nullable — an unobserved recovery read clears the rejection but withholds the reseed.effect-executor.ts:readVersionGuarded()— the single guarded version reader (number | null, never fabricated) serving the settlement-dispatch retry (restores liveness on transient settle-read failures) andsendEditRejected's three recovery dispatches (previously unguarded throw sources).showResyncFailureshares the per-incident resync-failure latch with the reseed-build guard (one latch, two triggers).execute-write.ts: caller-contract comments narrowed to CONTENT (the guarded version retry is the single documented exception). No code change.Math.maxadvance, the version-delta epoch verdict, the null recovery arm (+ id-guard + convergence), the retry (transient/persistent, both arms), the three guarded recovery sites, latch sharing, and four end-to-end pins over the real pipeline (with a fault-injectiondropLockHeldDocumentChangedharness arm and anarmVersionFailureseam). Gate and latch non-vacuity measured via revert-checks.Related
buildSeedDocumentは live bytes を読む" (2026-09-07, split from the settle-total PR's review). Residuals (quiet-document liveness backstop; unobserved-label parse-failed drain corner) are recorded in a follow-up TODO entry.Test Plan
pnpm compile(all 5 tsconfigs) greenpnpm test:unitgreen (279 files / 5354 tests)ackLabelObservedtotruereddens the withhold pins; splitting the shared latch reddens the latch-sharing pinpnpm biome checkclean on changed files;pnpm buildgreen;pnpm check:todo-hygiene/check:todo-markersOK