fix(host): release the write lock after a throwing applyEditSettled transition - #409
Merged
Merged
Conversation
A throwing applyEditSettled transition unwinds before the panel commits the state that releases the write lock, so the lock stayed held for the rest of the panel's life and every later edit was stashed into a pendingEdit that could never drain. Add the reducer arm that releases the lock, drops the stash, and reposts the authoritative Document so the webview's single flight un-parks with its replay buffer intact. The throw also abandoned the settlement's own save-failure toast, so the arm always raises one outcome-blind internal-error toast in its place, adding a dropped-edit clause only post-dispose, where the replay buffer went with the iframe and the stash really was the only carrier. The arm is a pure field reset by design: decideEdit and the outcome switch are throw sources of the transition being recovered from, so the recovery must not re-enter them. Wiring follows. Also order the two withheld-ack effect lists so the user-visible showResyncFailure precedes the logWarn the executor runs unguarded.
…catch PR #406's rescue released the deferred side channels' at-receipt guards but never the write lock, so the payment was one-shot: a retried side channel re-deferred behind a lock nothing would release. The catch now commits the recovery transition first and drops the side channels second, so the drop is durable and a retry runs. The recovery re-bases on the throwing settlement's own observed version, so no second guarded version reader is introduced. Also re-anchor the release-site claims in this module and broaden the onSettleError channel, which now carries recovery failures too.
The panel hands the step its own state-committing lambda, so the recovery releases the lock THROUGH the reducer and the panel never patches reducer state itself. Pinned end to end against the real core + step + barrier + dispatcher composition, including that the next edit is accepted and persisted. Non-vacuity measured: stubbing commitWriteLockRecovery to return no effects reddens the lock-release assertion.
…tude The recovery arm makes "applyEditSettled is the only live release site" false. LEARNING 2026-09-09 recorded that this claim lives at several altitudes and that reviews twice caught one copy left stale, so all of them are updated together and the roster is pinned in the plan's inventory step. Comment-only, plus the stale "STAYS locked" narration on the barrier-drop test, which now says what the stub is isolating and points at the composition test for the actual release.
…ery's receiving side The recovery's data-preservation claim rests on this test, but nothing in this file said so — an unrelated webview-sync refactor could have deleted it and left the claim silently unverified.
…ontracts
Review-cycle findings on the write-lock recovery, in four groups.
1. runEffects' `logWarn` was the one effect case with no `try` of its own, and
several reducer arms put a triage log AHEAD of the effect that pays the
incident — the drain `accept` arm returns `[...staleReBaseWarn, applyEdit]`.
A throw there unwound the loop and abandoned the applyEdit, and since the
committed state had already re-acquired the write lock, no settlement was
ever dispatched: the same stranded lock this branch exists to repair, except
the recovery hangs off the TRANSITION catch and never sees a runEffects
throw. Guarded at that one seam rather than by reordering each effect list.
2. The recovery's loss clause was gated on `disposed` alone, so the alive
withheld-ack branch dropped the stash and told the user nothing was lost.
There is no ack Document in that branch, so the webview's retained replay
buffer is never replayed; the recovery then clears the lock, the next
lock-free resync bumps `externalEpoch`, and the buffer is dropped on
`recordedEpoch > buf.epoch`. The gate is now "will any ack go out?".
3. Three contracts that the comments declared and nothing enforced:
- `releasesWriteLockOnCommit` returns the SETTLEMENT rather than a boolean,
so the version reaches the recovery through the same decision that
selected it. A future member mis-placed in that arm was silently handed
the caller's fabricated `null`; it is now TS2322.
- `HostSessionInputEvent` excludes `settlementTransitionFailed` from both
dispatch surfaces. A dispatched recovery would queue behind a sibling
whose lock-held stash arm then loses its stash to it; injecting that
dispatch used to type-check clean. Derived with `Exclude` so a new member
is carried automatically. commitTransition / commitWriteLockRecovery keep
the wide type — committing the recovery is the legitimate path.
- `commitWriteLockRecovery` staying REQUIRED is pinned in
types-equality.test.ts, the one test program `pnpm compile` checks.
4. Comment accuracy at every altitude where this branch left a claim behind
(LEARNING 2026-09-09). The rescue's `settle(false)` rationale said `true`
would take the barrier's WAIT arm; since the recovery releases the lock
first, `true` would DRAIN and RUN the thunks — the choice is DROP-vs-RUN.
Also: the ack-site census (two, now three), the withhold pair's scope and
post-dispose route count (three, now four), the accepted residual's throw
source, `refused`'s effect order, "unbounded" recursion, and the test
comment that presented its own stub as the production mechanism.
Non-vacuity measured for every group. Reverting the logWarn guard reddens the
new containment test; restoring the old `lostStash` reddens the new loss claim;
gating the toast on `ackLabelObserved` reddens three core tests (it was green
before, because the ordering test read presence through `indexOf` and absorbed
absence as -1); gating the rescue on the recovery's success reddens exactly one
of 335 step tests. For the two type mechanisms the probes were run BEFORE the
narrowing as well as after: both were tsc-clean beforehand, which is the
measurement that the contracts were prose-only. Adding `?` plus a
`?? (() => [])` default to commitWriteLockRecovery leaves all 335 runtime tests
passing and reddens only the type pin.
… its test fixtures The recovery arm spelled `toast` and `triage` out on both branches of its return, so an ordering fix — the kind this PR made twice — could land on one branch and miss the other. Only the ack half is conditional now, and the order the surrounding comments justify lives in one list. Also: two comment corrections (SettlementEvent's doc claimed a nullability that belongs to the function's return type; a rationale restated 120 lines from the doc that owns it is now a pointer, keeping the clause that is not duplicated), three comment reflows left over 100 cols by earlier edits, and three local fixtures in the new describe block. `messageOf` deliberately keeps its cast so a missing effect still throws instead of reading as an empty string, which is what would vacate the not.toContain pins.
The per-effect `logWarn` guard reported its own failure through a bare `console.error`, so a correlated console failure — VS Code patches the console as one IPC family — still unwound `runEffects` from inside the guard meant to contain it. On the drain `accept` arm that costs the WRITE and strands the write lock for the panel's life. Route every report made from a position where a throw must not unwind through one owner, `reportContained`. It takes a thunk so argument evaluation is contained too, and each site keeps its own console level and payload (the per-site triage tokens are unchanged). Five sites: the `showError` and `logWarn` per-effect guards, plus three that were never guarded and each needed only a SINGLE console failure to strand the lock — `readVersionGuarded`'s and `readCanWrite`'s catches (both evaluated while building the settlement event) and the rejection arm's leading log, which sat outside that arm's own `try`. Both guarded catches now carry `effect.message`, so triage keeps the identity of the toast or log line that was lost. Correct the `logWarn` header: the "ONLY effect with no try of its own" claim was written in the present tense in the hunk that added the try, and the "fixes the whole class at one seam" claim was wrong twice over — containment is best-effort, and three effects still evaluate their injected builder outside every try (`post` guards only `deps.send`). That gap is named where it stays findable.
…ests on The write-lock recovery arm is outcome-blind, so "A later unsaved edit was dropped." was an over-claim on the alive path: with no version advance the epoch does not move, the next same-epoch Document replays the retained buffer, and the bytes land. Split the clause by branch — definite post-dispose, where the stash really was the edit's only carrier, and hedged alive with a withheld ack, using the same "may not have been saved" phrase RESYNC_FAILURE_MESSAGE uses so the two toasts cannot disagree on certainty. The alive branch also names the remedy that preserves the bytes (copy before reloading), which composes with the resync toast's reload as an order rather than a conflict, and stands alone because that toast is latched per panel. Each branch carries its own closing instruction instead of sharing an appended one. Pin the clause on the SHARED `ackLabelObserved` gate: keying it on the raw `event.settledVersion` was undetectable by the whole session suite, because the two directions already covered keep the disjunction intact. The new test drives the state that separates them — alive, stash present, unobserved settled version, label already raised by a lock-held resync — and asserts BOTH clauses absent, since an absence check on the definite wording alone stays green against the hedge. Close the fail-OPEN derive: `HostSessionInputEvent`'s `Exclude` returns the original union when the excluded literal does not match, so a one-character typo re-admitted the forbidden dispatch with tsc silent, while its sibling `Extract` is fail-CLOSED. One pin in the only type-checked test location observes the exclusion expression itself. Comment corrections, all measured against the code: the effect ORDER is defence in depth, not a consequence of an unguarded `logWarn` (7 sites); the widened loss gate is NOT the settlement arm's gate, and the divergence is now stated outright with its reason; the four-step loss chain's third step is conditional; the alive stash's survival is bounded by the ack; `isEditApplied`'s heading is no longer a biconditional its own tail breaks; and a cross-reference drops a line number that went stale inside the PR that added it.
Self-review of the previous commit: `withholdAckEffects`' header quoted the
DEFINITE clause ("A later unsaved edit was dropped.") as what the recovery arm
says on this same withheld-ack condition. That path is now the HEDGED branch —
the definite wording belongs to post-dispose, where this pair is never built at
all. Name the hedge, and say why it reuses RESYNC_FAILURE_MESSAGE's phrasing.
The recovery arm's own note called both no-ack branches "losses" flatly, which is
what the hedge exists to qualify: they differ in certainty, and that difference is
exactly what the two wordings encode.
…n it The three-way loss clause was a nested ternary, which the house style forbids and which the same file already solves one arm up with an if/else chain. Unnesting it also puts each branch's commentary on its branch instead of in a header list the reader has to re-index. Tests: the two logWarn-containment cases carried a verbatim-duplicated 20-line fixture, so they could drift apart; `expectNoLossClaim` replaces three copies of the same not-dropped / not-hedge / Reopen triple, which makes this cycle's rule structural rather than remembered — checking only "dropped" reads the hedge as a pass, and a future no-loss site can no longer half-apply the check. Comments: two in-PR second derivations became pointers to the note that owns each argument. The standing "this does NOT close the class" guard stays; the history of how the claim was wrong belongs in the PR description, not the source. Non-vacuity re-measured after the refactor: reverting each fix still reddens the tests that were extracted, so the shared fixtures detect the original defects.
…ntained owner `reportContained` already existed for reports made from a position where a throw must not unwind, but six such positions still reported through a bare console, so one console fault could unwind `runEffects` from inside the guard meant to contain it: - `sendEditRejected`'s three recovery dispatches. A throw here skipped `editRejectedDeliveryFailed` entirely (measured `dispatched: []`), leaving the rejection `pending` with the webview's single flight parked and visible-edge resync suppressed. The host makes no further attempt of its own; recovery waits on an external `documentChanged` or a webview reload. - `post`'s sync-throw catch, which `case "postRejectedDraft"` follows immediately with `sendEditRejected` — so an escape took the banner delivery, its recovery dispatch and the arm's `Cannot save:` toast with it. - the reseed build-failure guard, which additionally had the bare log AHEAD of the toast: with a throwing console it produced no toast attempt at all (`toastAttempts: 0`), making silent the exact state its own comment says must not be. The signal now goes first, matching the three reducer-side sites. - `reportResyncFailure`'s own fallback, the most correlated position of all since it runs only because `showError` just threw. The delivery-refused site also moves its two injected seam reads inside the thunk; argument evaluation happens before the call, so leaving them out contained only half the site. Each new wrap gets its own test: an unpinned guard is the one that regresses, measured on the `showError` fallback, which was the only `reportContained` site without a pin and survived reversion to a bare console with the file green. That gap is closed here too. Also makes this increment's own claims match what is measured: - the exclusion type pin was fail-OPEN on its own literal — `Extract<T, U>` answers `never` both when the union excludes the member and when the literal matches nothing. It now asks two questions of ONE shared literal, so the pair is a biconditional rather than a convention about keeping two copies in sync. - `reportContained`'s header conflated two different limits: the throw is fully contained, the report is not. Its list of calls that bypass the owner is now a rule rather than a roster. - the withhold pair's ordering rationale no longer rests on a failing fallback console call, which the guard absorbs; it rests on the guard being removable. - exports `RESYNC_FAILURE_MESSAGE` and pins the cross-module wording contract the recovery arm's hedge depends on. Rewording it previously left everything green. - corrects the `false`-arm count in `isEditApplied`'s contract, the ALIVE replay claim that a withheld ack falsifies, the "NO LOSS" branch that is really "no stash loss", and a toast-ordering claim that rested on a notification stack order this repo cannot verify.
`reportContained`'s header carries two standing claims — the best-effort limit and the effect-order rule — and two call sites pointed back at it by the same marker, so one of them resolved to the wrong paragraph. Naming the order rule gives each pointer a target it cannot miss, which is the failure mode that replacing duplication with pointers introduces. Also: a group comment counted three tests where there are four, and two headers were left mid-sentence by earlier edits. Comment-only — no statement, assertion, or test was added or removed. Measured while here: the header's roster of console calls that deliberately do NOT route through the helper matches a grep of every console call in the module, so that completeness claim is now verified rather than asserted.
…case with no stash An alive `settlementTransitionFailed` recovery whose ack is withheld and whose `pendingEdit` is null fell into the no-loss branch, because `lostStash` requires a stash. That state told the user to "Reopen the file" — which destroys the webview buffer still holding the at-risk bytes. The recovery arm is outcome-blind, so those in-flight bytes sit under the same conditional loss as a stash: no ack means no replay, the next lock-free advance bumps the epoch, and edit-sync drops the buffer. Re-key the no-loss branch on `disposed || ackLabelObserved` so every alive recovery with an unobserved label takes the copy-first hedge, and generalise the hedge's wording so it no longer presupposes a stash that may not exist. Also correct three claims this branch introduced earlier and one type-level fail-open: - the `case "logWarn"` roster claimed ONE remaining unguarded position class "and it is not a console call". `case "openExternal"` is a second one, and its delegate's open positions ARE console calls (bare `console.warn` twice, plus bare `console.error` and `deps.showError` outside its own `try`). - the named ORDER RULE was stated as a universal that two reducer arms in this same repo contradict; restated in the count form the file already uses, with both exceptions named. - the withhold pair's order was justified partly as a defence against the unguarded builder positions. No effect list reaching that helper evaluates a builder, so the order cannot cover them; only the guard-removal half stands. - `RecoveryEventType = never` degenerated both event-union assertions to trivially true. One assertion asks the alias about itself, closing it. Pinned in both directions: restoring the old condition reddens the new no-stash hedge test, and inverting the new conjunct reddens all three no-loss sites plus the post-dispose definite claim.
Branch 1's header claimed exactly two states reach it, but its first bullet was keyed on `ackLabelObserved` alone — which is computed independently of `state.disposed`, so a disposed arm with an ack and a stash satisfied the bullet while landing in branch 2. The bullet's stated reason was also false post-dispose, where there is no replay buffer left to replay. Qualifying it as the ALIVE case makes the two bullets the branch predicate verbatim. Also rephrases "asserts no stash", which read as the opposite of its intent. Comment-only, zero line delta.
…le claim - effect-executor.ts's ORDER RULE comment (and its second, duplicate reference to the same set later in the same file) asserted an exact count of reducer-side sites that put a user-visible signal ahead of a triage log. The count under-counted in both directions: a disposed signal-first arm was missing from it, and it omitted deliberate log-first exceptions. Replaced both with wording that names the closed set of sites citing the rule by name and explicitly disclaims closure in either direction, rather than asserting a count that goes stale on the next edit. - Narrowed two "the panel's `step` settles unconditionally" claims (also in effect-executor.ts) to name the one case where that does not hold: `isEditApplied` itself throwing before `runEffects` runs (host-session-step.ts:390). - test/extension/session/host-session-core.test.ts: replaced a comment claiming a specific wording assertion is uniquely the one that fails against the old condition with one describing which assertion actually distinguishes right from wrong, since `expect` throws at the first failure and the earlier framing no longer held once measured. Comment-only; no behaviour, assertion, or type change.
Pure reflow after the census de-quantification left one line at 93 columns while its neighbours sat at 65-85. No word, claim, or code changed.
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.
What
A throwing
applyEditSettledtransition left the host write lock held for the rest of the panel's life.quoll-editor-panel.tscommits reducer state asstate = result.stateinsidecommitTransition, so when the transition throws that assignment never runs:pendingApplyBaseVersionstays non-null, andapplyEditSettledis the only event that releases it.PR #406 paid the barrier half (
settle(false)releases the deferred side channels' at-receipt guards) but that payment was one-shot — a retried side channel re-enterededitSettledBarrier.run(), found the lock still held, and re-deferred. Meanwhile every later inbound edit was stashed intopendingEditwhose only drain is the settlement that already threw, and at dispose that stash was lost silently (thedisposedarm clears the lock witheffects: [], and revert-rescue snapshots the held lock →NO_RESCUE).How
One new reducer-owned event,
settlementTransitionFailed, committed fromhost-session-step.ts's existing transitioncatch— before the barrier settle, so the guard release is durable.The arm is a pure field reset plus effects by design:
decideEdit(through the injected validator) and the outcome switch (failureToasts'default) are the transition's own throw sources, so the recovery must not re-enter them. That is also why the stash is dropped rather than re-run —canDrain's safety condition needs an OBSERVED canonical snapshot this arm must not go and read.settledVersionthrough. It is the label the settlement itself would have used, nothing can interleave before the throw, and it keepseffect-executor.ts's "ONE guarded version reader" contract true — the recovery adds no read seam.resyncLiveVersion's lock-free foreign-advance branch cannot fire on the alive path: no epoch bump ⇒ the webview's replay buffer survives and replays over the reposted Document. Reversed, the in-flight apply's own late echo would read as a foreign advance and drop those keystrokes.okoutcome) began with the save-failure toast, so the recovery raises one outcome-blind internal-error toast in its place. The "a later unsaved edit was dropped" clause is added only post-dispose, whereedit-sync.ts's retained replay buffer went with the iframe and the stash really was the only carrier.showError/postDocumentper effect but runslogWarnas a bareconsole.warn, so the triage log goes last. The two withheld-ack lists (withholdAckEffectsandeditRejectedDeliveryFailed's inline copy) are reordered for the same reason.Intended side consequence: with the lock released, a later dispose snapshots
writeInFlight: false, so revert-rescue is eligible again — correct, since the settlement has already resolved.Scope notes
settle(false)rescue into an unconditional one (dropAll()before consultingisLockedremains a separate bug).logWarnfirst (settlementEffects'refusedarm; the undrainable-stash alive path). Left out to keep this to one purpose — fixing them requires rewritingrefused's indexedeffects[0] === logWarnassertion — and filed as a follow-up TODO.Verification
pnpm compile— green;pnpm test:unit— 279 files / 5400 tests green (24 new).commitWriteLockRecoveryto() => []reddens the composition test's lock-release assertion; movingsettlementTransitionFailedintoreleasesWriteLockOnCommit'struearm reddens "no rescue of the rescue" (the false-side pin LEARNING 2026-09-09 asked for). Both exhaustive-switch tests carry aconsole.errorguard, because the default arms already answerfalse— the guard is what proves an explicit arm answered.stateclosure) pins the lock release, the stash disposition, the durable side-channel release, and that the next edit is accepted and persisted.cm-edit-sync.test.ts's "(h) replays a buffer on a same-generation, same-epoch settlement" (receiving side), now commented as such so the coupling is discoverable from that file.applyEditSettledis the only live release site" claim is re-anchored at all twelve sites where it appeared, enumerated against a measured sweep.