Repository navigation
fix(session): drop deferred side channels on dispose from the step verdict - #453
Merged
Merged
Conversation
…rdict isEditApplied answered true for the disposed event and left the drop to the barrier's isDisposed check, which reads the panel's local flag. That made the drop depend on the flag being set before the dispatch in onDidDispose. disposed now answers false, so the barrier drops on that step regardless of the line order. Observable behaviour is unchanged with the current wiring. The e2e that pinned the line order is removed: with the dependency gone, no single mutation turns only that test red. A unit pin beside the settlementTransitionFailed one covers the new arm.
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
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
isEditAppliednow answersfalse(DROP) for thedisposedevent, so the edit-settled barrier drops deferred side channels on dispose from the step's own verdict instead of relying on the panel having set its localdisposedflag before dispatching. The e2e that pinned that line order (#452) is removed in the same change, because the dependency it guarded no longer exists.Changes
host-session-step.ts:disposedmoves out of thereturn truegroup into its ownfalsearm. LikesettlementTransitionFailed, it releases the write lock without looking at an outcome, so it must not claim the edit landed.isEditApplieddoc, the barrier'ssettledoc, the panel's step recap andonDidDisposenote, and the two step-module paragraphs that read as if the barrier needed the flag-first order).settlementTransitionFailedone:isEditApplied({ type: "disposed" })isfalseand reaches an explicit arm (noconsole.error).drops a deferred context-handoff when the panel is disposed while the lock is heldfromhandoff-edit-applied-barrier.test.ts.Behaviour
No observable change with the current wiring:
settleevaluatesisDisposed() || !applied, andisDisposed()already reads true on that step, so the samedropAllruns. The difference only shows if the two lines inonDidDisposewere ever swapped: deferred save / clipboard / command / editor-switch thunks are now still dropped.Not changed: the order of the two lines in
onDidDispose(flag-first stays as the defence for adisposedtransition that throws; that double-failure path still depends on it).Why the e2e goes
After this change no single-point mutation turns only that test red. Removing
isDisposed()from the drop condition is caught byedit-settled-barrier.test.tsand the real-barrier test inhost-session-step.test.ts; reverting thedisposedarm is caught by the new unit pin. The e2e cost two 1.2 s "nothing happened" windows and about 70 lines of setup duplicated from its neighbours.Known gap, left as is: no test pins on its own that the panel's
isDisposedwiring or thedisposedstep's verdict reachesbarrier.settle. The removed e2e did not isolate those either.Test Plan
expected true to be false), green afterpnpm compilepnpm test:unit(5789 passed)env -u ELECTRON_RUN_AS_NODE pnpm test:e2e(114 passing)