From b694c512954281fe892a8b4744dbcabb7fadae7e Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Wed, 23 Sep 2026 14:14:52 +1000 Subject: [PATCH 1/2] Remove the done items from todo.md --- todo.md | 232 ++------------------------------------------------------ 1 file changed, 5 insertions(+), 227 deletions(-) diff --git a/todo.md b/todo.md index 77181614..3bbfd0e0 100644 --- a/todo.md +++ b/todo.md @@ -1,67 +1,15 @@ # Review todo -Findings from a review of `main` at 4244ebe6 (2026-09-23). A second pass checked every item left unverified, on c37bf9e1 (see "Bugs, second pass"). +Open findings from a review of `main` at 4244ebe6 (2026-09-23), rechecked on c37bf9e1. Done items are removed. -- **repro**: a test fails on the commit the item was checked on: the appendix for the first pass, the local branch `review-repros` for the second. +- **repro**: a test on the local branch `review-repros` fails on c37bf9e1. - **verified**: confirmed by reading the code (or running the API involved), no test yet. - **cannot verify here**: needs a platform this machine lacks; the item says what would settle it. ## Data loss -- [x] **Tray "Accept all" deletes verified files before applying inline patches** (verified) - - **Fixed:** the menu, hot keys and wire accept-all now run moves, then snapshots, then deletes, and hold every delete when a snapshot in the batch was not written (`AcceptAllTally.Refused` in the owned host; read back from the full listing in `RemoteInlineHost`). The user is told why (`Tracker.DeletesHeld`). Tests: `TrackerDeleteTest.AcceptAll{AppliesTheSnapshotsBeforeTheDeletes,HoldsTheDeletesWhenASnapshotWasNotWritten}`, `OwnedInlineHostTest.AnAcceptAllWhose{PatchIsRefusedHoldsTheDeletes,PatchesAllLandCarriesOutTheDeletes}`, `TrackerTrackedFilesTest.AcceptAllHoldingDeletesLeavesThemPending`, `TrayViewerSyncTest.TrayAcceptAll{HoldsItsDeletesWhenTheOwningViewerRefusedAPatch,CarriesOutItsDeletesWhenTheOwningViewerWroteEveryPatch}`. - - `src/DiffEngineTray/Tracker.cs:742` (`AcceptAll`, and `AcceptOpen` at :727) and `src/DiffEngineTray/OwnedInlineHost.cs:368` (`IQueueOwner.AcceptAll`, which an attached viewer's Accept all is forwarded to) sweep deletes, then moves, then snapshots, and never hold a delete. - - A snapshot moving inline arrives as an Append patch plus a delete of `X.verified.txt`. If the Append comes back NotFound (source edited since the run, or a call site that cannot host `Snapshot`), the verified file is already gone and the literal was never written. - - The viewer-owned batch does the opposite on purpose: snapshots first, deletes held once any snapshot was not written. Rationale on `InlineRefused` (`src/DiffEngineViewer/ViewerSession.cs:1101`), pinned by `HeldDeleteTests`. The tray is the default owner on Windows. - - Fix: snapshots first, then moves; run deletes only if no non-conflicted snapshot in this batch was not written or failed, decided from `AcceptAllTally` rather than `queue.Count`. `OwnedInlineHostTest.AnAcceptAllCountsTheFilesIntoItsProgress` pins files-first incidentally. - -- [x] **`AddInlineAsync` reports `Queued` when the viewer launch was capped** (verified) - - **Fixed:** `DiffRunner.InlineResultFor` maps only `Launched` and `Taken` to `Queued`. Test: `ViewerLaunchGateTests.OnlyALaunchOrAHandoverIsQueued`. The related slot spent by a failed launch is not fixed (not data loss). - - `src/DiffEngine/DiffRunner_Inline.cs:91`: `launched == ViewerLaunchOutcome.Failed ? NoViewerFound : Queued`. `Capped` (added in 26e79f0e, #855) falls through to `Queued`. `PendingFiles.Launched` already maps it correctly. - - No tray, no viewer: five diff tools opened for file snapshots use up `MaxInstance`, and every later inline failure is capped, reported `Queued`, and not staged by the caller. With `DiffEngine_MaxInstances=0` that is every inline failure. - - Fix: `Failed or Capped` → `NoViewerFound`. - - Related: `MaxInstance.Reached()` increments before `launch()` runs, so a launch that returns false still spends a slot. - -- [x] **Committed Linux native binaries need glibc 2.38** (verified) - - **Fixed:** the Linux jobs build inside `quay.io/pypa/manylinux_2_28_$(uname -m)` via `native/build-linux.sh`, and a "Check glibc floor" step fails the job if `objdump -T` shows anything above `GLIBC_2.28`. The binaries rebuilt that way (#882) need at most `GLIBC_2.27` and link `libGL.so.1` rather than `libOpenGL.so.0` (`readelf -V`/`-d` in WSL). - - Both `src/DiffEngineViewer.Linux/runtimes/linux-{x64,arm64}/native/libdiffengine_viewer.so` reference `GLIBC_2.38` (`__isoc23_sscanf`, `fmod`, `fmodf`), plus 2.35 and 2.34. They are built on `ubuntu-24.04` with no floor (`.github/workflows/build-native.yml:40`). - - `dlopen` fails on Ubuntu 22.04 (2.35), Debian 12 (2.36), RHEL 8/9 and Amazon Linux 2023. CI only loads them on 24.04. - - Fix: build in an old-glibc container (e.g. `quay.io/pypa/manylinux_2_28_*`) or with `zig cc -target x86_64-linux-gnu.2.28`, and fail the job if `objdump -T` shows a GLIBC version above the floor. - -- [x] **A viewer that cannot open its window drops what it was launched with** (verified) - - **Fixed:** `ViewerProgram.Run` persists on a window that will not open and in a `finally` around the loop; `RunInline` stages the patch when its forward is refused or fails; `ViewerLauncher` starts no viewer on Linux with neither `DISPLAY` nor `WAYLAND_DISPLAY`, so the caller hears `NoViewerFound` and stages. Tests: `ViewerProgramTests.AViewer{WithNoWindow,WhoseLoopThrows}StillStagesWhatItHolds`, `ViewerLauncherTests`. Not done: the launch gate still reports `Launched` for a process that exited during the bind wait (covered in practice by the viewer staging, except for a native crash). - - `src/DiffEngineViewer/ViewerProgram.cs:210-215`: `Run` returns 4 before `PersistOwned`. `RunInline`/`RunDelete`/`RunDiff` bound the port first, so the launch gate saw an owner, reported `Launched`, and `AddInlineAsync` returned `Queued`. Any exception out of the loop does the same, since the outer catch at :41 also skips `PersistOwned`. - - Hit by the glibc item above, and by any Linux machine with no display (SSH, devcontainer, WSL without WSLg). Nothing checks `DISPLAY`/`WAYLAND_DISPLAY` before launching. - - Also: `WaitForBind` giving up after 5 s still returns `Launched` even when the process has already exited (`src/DiffEngine/Viewer/ViewerLaunchGate.cs:175-178`), and `RunInline`'s forward path ignores `response.Ok` (`ViewerProgram.cs:74`). - - Fix: try/finally around everything after the bind that calls `PersistOwned`; have the launcher return the `Process` and report `Failed` once it has exited; consider not launching the viewer on Linux with no display. - -- [x] **Re-applying an applied Set or Remove patch edits a different call** (repro) - - **Fixed:** a Set stops at the recorded line once the call there already holds the new content (`InlinePatcher.IsOnHint`), and a Remove reports AlreadyApplied when the statement at the recorded line has no Snapshot left, including a chained line that was pulled up (`RemovedAtHint`). `RemoveWithNoSnapshotCall` now expects AlreadyApplied (renamed `RemoveWithNoSnapshotCallIsAlreadyDone`). Tests: `InlinePatcherTests.Reapplying*LeavesASiblingWithTheSameLiteral`, `RemoveWithNoCallAtAll`. - - `src/DiffEngine/Inline/InlinePatcher.cs:147-167` (Set by expression), `:177-203` (Set by value), `:686-719` (`TryFindAnchoredCall`, used by Remove). When the hinted call no longer matches the anchor, the outward walk in `FindCalls` (:801) takes any other call whose literal still equals the old anchor, so the "another process may have applied it" branch (:166) is never reached. - - Two `Verify(x).Snapshot("dup")` in one member, lines 5 and 6. Set with hint 6 makes line 6 "new". The same patch again returns `Applied` and makes line 5 "new" (should be `AlreadyApplied`). Remove twice strips both `Snapshot` calls. - - Happens when a second target framework's identical patch arrives after the first was accepted (it becomes a new queue entry), or when each framework's test process applies the same Remove. Without MemberName the reach is file wide. `ReapplyingWithIdenticalLiteralsIsAlreadyApplied` does not cover it, since its anchor matches nothing. - - Fix: before walking outward, if the recorded line is inside the member's span and its Snapshot value already equals `newContent`, return AlreadyApplied. For Remove, if that line holds an entry point with no Snapshot chained, report it as already removed. - -- [x] **Tray accept-all applies patches from a copy of the queue taken at the start** (verified) - - **Fixed:** `AcceptEvery` takes keys and re-finds each entry under the gate before applying it. Test: `OwnedInlineHostTest.AnEntryDiscardedDuringAnAcceptAllIsNotWritten`. - - `src/DiffEngineTray/OwnedInlineHost.cs:527-541` (`AcceptEvery`) applies `entry.Patch` for every captured entry with no re-check, and `AcceptInBatch` then ignores the result because the entry changed. - - Entries settled (the test now passes), discarded ("Discard (n)" mid-batch), replaced by a re-run, or made into a conflict during the batch are still written into source. - - Fix: iterate keys; under `gate` re-find the entry, skip it when gone or conflicted, and apply the current patch outside the gate, the way `ViewerSession.ClaimNext` does. Testable with `HeldApply`. - -- [x] **A failed `File.Replace` can delete the source file** (verified against the ReplaceFile docs, not reproduced) - - **Fixed:** when the swap fails with the destination gone and the temporary present, the temporary is moved into place; the temporary is only deleted while the destination exists, and a failed recovery names where the source went. Tests: `InlineApplierTests.AReplaceThat{FailsAfterRemovingTheSourceStillLeavesOne,FailsCleanlyLeavesTheSourceAsItWas}`. - - `src/DiffEngine/Inline/InlineApplier.cs:291-321`: `File.Replace(temporary, fullPath, null)`, then the `finally` deletes the temporary. ReplaceFile's `ERROR_UNABLE_TO_MOVE_REPLACEMENT` (1176) with no backup name leaves the original gone and the replacement under its temporary name, which the `finally` then deletes. - - Triggered by anything holding the fresh temporary: antivirus, sync clients, network shares. - - Fix: on failure, if `!File.Exists(fullPath) && File.Exists(temporary)`, move the temporary into place instead of deleting it. - -- [x] **An item arriving just after the queue empties is acknowledged, then dropped** (repro) - - **Fixed:** enqueues clear `Exit`; the loop commits to leaving under the lock (`ViewerSession.CommitExit` sets `SessionState.Closing`), as does every other way out of the loop; a closing viewer refuses Inline, Diff, Move and Delete, so the sender stages or reports instead. `TrackMove`/`TrackDelete` also read their files before taking the lock now. Tests: `ViewerSessionTests.{AnArrivalAfterTheQueueEmptiedKeepsTheWindow,NothingJoinsAQueueThatHasCommittedToLeaving,AnArrivalBeforeTheCommitCancelsIt}`, `IpcTests.AClosingViewerRefusesWhatWouldJoinTheQueue`. - - `src/DiffEngineViewer/ViewerSession.cs:1395`: `Remove` sets `Exit = queue.Count == 0`, and `EnqueueInline` (:50) and `EnqueueTracked` (:141) carry `Exit` forward. - - A settle empties the queue; before the next frame a Diff, Move or Delete arrives and is answered Ok; the loop then sees `Exit` and quits. `PersistOwned` stages only inline entries, so the tracked pair is in no window and no tray, and an inline one is staged rather than shown while its sender believes it is queued. - - Fix: `Exit = false` in both enqueue methods. Better, decide the exit under the lock and have the handler refuse once closing. - -- [ ] **An attached "Accept all in " deletes verified files whose snapshots were not written** (repro, second pass) +- [ ] **An attached "Accept all in " deletes verified files whose snapshots were not written** (repro) - `DispatchGroup` (`src/DiffEngineViewer/ViewerProgram.cs:663-697`) posts one `Accept` per member, deletes included (`:683`), before any answer comes back, and the owner carries each out as a single accept with no held-delete rule: the tray through `OwnedInlineHost.cs:302-304` and `Tracker.cs:956` to `File.Delete` at `:1061`, a viewer through `MessageHandler.cs:156`. #878 fixed the tray's accept-all and the AcceptAll verb, not this path. The owning viewer's own group accept does hold deletes (`ViewerSession.cs:664-675`). - Test (`review-repros`): `An_attached_accept_all_in_a_solution_holds_its_deletes_when_a_snapshot_was_not_written`. The patch comes back NotFound and the verified file is deleted anyway, so no copy of the snapshot is left. Its owner is a viewer; the tray owner's delete path was read, not run. - Fix: the client cannot know the patch outcomes when it posts, so the rule belongs with the owner, for example a group verb that carries the member keys through the owner's batch and its hold rule. @@ -69,66 +17,7 @@ Findings from a review of `main` at 4244ebe6 (2026-09-23). A second pass checked ## Bugs -- [x] **The Linux viewer never showed a frame or read input** (repro, found while verifying the items below) - - **Fixed:** `native/CMakeLists.txt` switches `SUPPORT_CUSTOM_FRAME_CONTROL` and `SUPPORT_BUSY_WAIT_LOOP` back off, with every other flag raylib's `config.h` defaults to 0. Test: `PixelTests.PresentWaitsForTheNextFrame`, which the Ubuntu job runs under xvfb against the renderer built from source. - - raylib 6.0's `cmake/ParseConfigHeader.cmake` turns every `#define SUPPORT_X ` in `config.h` into an option defaulting to ON, whatever the value (raysan5/raylib#5844 fixed it after 6.0), and `CUSTOMIZE_BUILD ON` skips `config.h`'s own values. The Linux configure output listed `SUPPORT_CUSTOM_FRAME_CONTROL=ON`, `SUPPORT_BUSY_WAIT_LOOP=ON` and every image format raylib has (build-native run 35812014703). - - With custom frame control, `EndDrawing` skips `SwapScreenBuffer`, the `SetTargetFPS` wait and `PollInputEvents`, the only callers of `glfwSwapBuffers` and `glfwPollEvents`, and `deview_present` calls none of them itself. Disassembly of both committed `.so` files shows neither has a call site. So the window never painted and ignored keys, mouse and its close button, while the managed loop, which relies on the present to pace it, spun; and it owned 3493 the whole time, accepting snapshots it could not show. - - Measured on the committed linux-x64 `.so` in WSL (python ctypes, hidden window): a bare `deview_present` loop ran at 41,659 fps across 11 cores. - - Every committed Linux binary since the first (#733) was built this way. The pixel snapshots never noticed, because `deview_capture` draws into a texture and never calls `EndDrawing`. - -- [x] **Attached viewer's right-click menu closes within 200 ms** (repro) - - `src/DiffEngineViewer/ViewerSession.cs:189`: `Sync` always sets `Menu = null`. `OwnerLink.List` calls it on every poll (`src/DiffEngineViewer/Ipc/OwnerLink.cs:120`), `ViewerForm.ApplyMenu` then closes the popup, and a later click is dropped (`ViewerProgram.cs:385`). Every viewer is attached when the tray owns the queue, which is the default on Windows. - - `EnqueueInline` also clears the menu when an identical snapshot is re-sent. - - Fix: when the new queue is element-wise `ReferenceEquals` to `state.Queue` (`Project` and `ReadChanges` already reuse unchanged entries), keep `Menu`; if the message is null and progress unchanged too, return `state` itself. `AQueueChangeClosesTheMenu` only covers a listing that changed. - -- [x] **.NET Framework: one quoted PATH entry breaks DiffTools for the whole process** (verified) - - `src/DiffEngine/OsSettingsResolver.cs:14` splits PATH with no cleanup, and `:152` `Path.Combine("\"C:\\Program Files\\Foo\\bin\"", name)` throws `ArgumentException: Illegal characters in path` on net4x (checked in Windows PowerShell 5.1). It runs in `DiffTools`' static constructor, so it is a permanent `TypeInitializationException`, and `DiffRunner.Kill` throws on every passing test. - - Fix: trim whitespace and quotes, and drop entries that are empty or contain invalid path characters. - -- [x] **`DiffEngineViewer left right` never exits while the tray runs** (verified) - - `src/DiffEngineViewer/ViewerProgram.cs:354`: hide-instead-of-exit has no mode check. File mode owns no port and the tray does not know the process, so X, Close, q and Esc hide it forever, and a blocking `git difftool` style caller hangs. - - Fix: add `host.State.Mode == ViewerMode.Inline &&`. - -- [x] **F# double-backtick test names get no MemberName narrowing** (repro) - - `src/DiffEngine/Inline/InlinePatcher.cs:991-994` (`MemberLine`) and `src/DiffEngine/Inline/FsLanguage.cs:120` (`IsDeclaration`): for `let ``test b`` () =` the character before the name is a backtick, so it is not a declaration, `MemberLine` returns null, and the search is hint only. `NextMemberLine` never sees backticked siblings either. Double backticks are the common F# test naming style. - - Repro: two tests ` ``test a`` ` and ` ``test b`` ` with identical literals, stale hint on test a, member "test b": test a's literal is rewritten. - - Fix: when the name is enclosed in double backticks, judge the declaration from before the opening pair. - -- [x] **An empty entry-point name hangs the patcher** (repro) - - `src/DiffEngine/Inline/InlinePatcher.cs:915`: `IndexOf("", i, n)` returns `i` and `index += 0`, so `CallsOnLine` spins (or grows `matches` until out of memory) while holding the per-file mutex. `InlinePatchFile.TryParse` produces `["VerifyDocx", ""]` from a payload of `"VerifyDocx,"`. - - Fix: skip null or empty (ideally any non-identifier) names in `EntryPoints()` (:78). - -- [x] **"Copy received" and "Copy expected" turn tabs into four spaces** (repro) - - `src/DiffEngineViewer/SelectionText.cs:119` (`All`) goes through `RowText.Flatten`. A whole-side copy has no columns to keep aligned. Pasting the result into a verified file changes it. - - Fix: `.Select(_ => _.Text)`. - -- [x] **Linux shortcuts follow physical key position rather than layout** (verified) - - `native/src/deview.cpp:526` (`ReadKey`) tests raylib/GLFW key tokens, which are US key positions. On AZERTY the key labelled Q sends `KEY_A`, which accepts (with Shift, accepts all), and the key labelled A sends `KEY_Q`, which quits; Ctrl+A and Ctrl+Q are swapped. The macOS and WinForms heads follow the layout. - - Fix: drain `GetKeyPressed()` and map letters through `GetKeyName(key)`, falling back to the position for non-Latin layouts. `IsKeyPressedRepeat` for navigation keys. - -- [x] **InlineApplier's mutex is session scoped on macOS and Linux** (verified against .NET semantics) - - `src/DiffEngine/Inline/InlineApplier.cs:84,355-365`: `DiffEngineInline_` has no prefix, so it is Local, which on Unix means per POSIX session, and every terminal is its own session. Rider's plugin and a viewer launched from a terminal test run do not exclude each other: both read, both swap, the later rename wins, and one literal is silently lost while both report Applied. - - Fix: `Global\` prefix off Windows, falling back to Local on `UnauthorizedAccessException` or `IOException`. - -- [x] **ProcessCleanup's process list is taken once and PIDs are not re-checked** (verified) - - `src/DiffEngine/Process/ProcessCleanup.cs:27` (the only `Refresh` call, in the static constructor), `:94` (`TryGetProcessInfo`), `src/DiffEngine/Process/WindowsProcess.cs:155` (`TryTerminateProcess` kills whatever holds the PID). - - A tool window from a previous run closed mid run: an AutoRefresh tool is reported `AlreadyRunningAndSupportsRefresh` and no window opens. If Windows has reused that PID, a passing test's `Kill`, or a replacement launch, terminates an unrelated process. Tools launched in this run never enter the list. - - Fix: on a hit, re-read that PID's command line and drop it if it no longer matches; remove entries once terminated; add `(command, pid)` after a launch. - -- [x] **Slow work runs inside SessionHost's lock, and the render loop takes that lock every frame** (verified) - - `src/DiffEngineViewer/ViewerProgram.cs:321`: `host.Mutate(_ => Apply(...))` every frame, even with no input. - - `src/DiffEngineViewer/Ipc/MessageHandler.cs:47-51`: `TrackedEntry.ForMove`/`ForDelete` (file reads, SHA-256 of images, the DiffPlex diff) is evaluated inside the `Mutate` lambda, contrary to the comment above it. `Act` (:102-133) runs `InlineApplier`, with its up to 10 s mutex wait, inside `Mutate`. - - The window stalls, which `SessionHost`'s doc says lock-free reads exist to prevent. - - Fix: build tracked entries before `Mutate`; skip the per-frame `Mutate` when the input is empty and the size unchanged; apply wire accepts outside the lock the way `AcceptAllRunner` does. - -- [x] **`MessageHandler.Act` checks the conflict refusal outside the lock** (verified) - - `src/DiffEngineViewer/Ipc/MessageHandler.cs:102-133` reads `host.State` twice, so `Queue[index]` can throw `ArgumentOutOfRangeException`, and the conflict check can pass just before a second framework's patch makes the entry conflicted, after which the accept picks a side. - - Fix: do the lookup and the refusal inside the `Mutate` lambda. - - -## Bugs, second pass - -Every item that was unverified, checked on c37bf9e1, after #878, #881 and #884. All of them hold, and none of the three fixed any. The repro tests are on the local branch `review-repros`, one class per area: `ReviewReproSessionTests` and `ReviewReproWatchTests` (viewer model), `ReviewReproWindowsTests`, `ReviewReproTrayTests`, `ReviewReproPatcherTests`, `ReviewReproLibraryTests`. Each test fails on c37bf9e1 except a control (`ControlSpaceIndentedLocalLeavesTheSiblingAlone`) and a measurement (`HowLongADecodeHoldsTheFile`). The attached "Accept all in " item moved to Data loss. +The repro tests are on the local branch `review-repros`, one class per area: `ReviewReproSessionTests` and `ReviewReproWatchTests` (viewer model), `ReviewReproWindowsTests`, `ReviewReproTrayTests`, `ReviewReproPatcherTests`, `ReviewReproLibraryTests`. Each test fails on c37bf9e1 except a control (`ControlSpaceIndentedLocalLeavesTheSiblingAlone`) and a measurement (`HowLongADecodeHoldsTheFile`). Viewer model @@ -254,10 +143,8 @@ Tray - Test: `AlwaysKillAppliesToAnAcceptArrivingOverTheSocket`. - Fix: read the setting in `ShouldKill` before the `NeverPrompt` branch. `ALockedMoveIsRefusedWithoutPrompting` requires that the resolver, which builds a dialog, is never consulted. -Native (the Linux items were unreachable until "The Linux viewer never showed a frame" was fixed; these are verdicts on the code as it behaves from there) +Native (the Linux items were unreachable until #885 made the Linux window draw and read input; these are verdicts on the code as it behaves since) -- [x] F12 in the Linux viewer writes `screenshotNNN.png` into the working directory (raylib `SUPPORT_SCREEN_CAPTURE` default; the string is in the committed `.so`). `set(SUPPORT_SCREEN_CAPTURE OFF CACHE BOOL "" FORCE)` in `native/CMakeLists.txt`. - - **Fixed** with the frame control above (verified: `EndDrawing` takes `IsKeyPressed(KEY_F12)` to `TakeScreenshot` in the working directory). It could not fire while nothing polled input, and could as soon as something did. - [ ] **Decoded image caches never evict on Linux or macOS** (verified) - `state.pictures` (`native/src/deview.cpp:140`) drops an entry only when that path is asked for again and has changed or gone (`:318-344`), or at shutdown; `Renderer.swift:60` likewise (`:436-452`). Bounded by one viewer session, which ends when the queue empties. @@ -355,115 +242,6 @@ Library and inline ## Perf -- [x] **Queue projection is O(n²) per frame and per mutation under the lock** (`src/DiffEngineViewer/QueueProjection.cs`). `Order` calls `TestGroup` (two string allocations) for every pair and runs twice per inline change and per `Sync`; `Rows`/`Labels`/`Collisions` run every frame with n² comparisons, and grouped entries always collide so all four passes run. Compute the group key once per entry, group with a dictionary, detect collisions with a `(solution, label)` count, and cache `Rows` per queue instance. - [ ] **An attached viewer polls `ListFull` five times a second, even while hidden.** The owner re-serialises every patch (base64 twice) inside its gate and the viewer re-parses all of it (`src/DiffEngineViewer/Ipc/OwnerLink.cs:100`, `src/DiffEngine/Protocol/ViewerListing.cs`, `src/DiffEngineTray/OwnedInlineHost.cs:259-284`). Add a generation or etag and answer "unchanged"; poll slower while hidden. -- [x] **On macOS and Linux every passing verification builds a string of every process's command line**, even with logging off (`src/DiffEngine/Process/ProcessCleanup.cs:66-71`; the Unix `FindAll` ignores the name filter). Guard with `Logging.enabled`, and keep only commands that start with a resolved tool's exe path. -- [x] `SelectionText.Summary` rebuilds the whole selected text every frame (`src/DiffEngineViewer/ScreenBuilder.cs:248`, `SelectionText.cs:131-142`), and a right-click builds both whole sides just to test for emptiness (`MenuState.cs:76`). Count from span lengths, once per selection. -- [x] DiffPlex computes word-level sub-diffs that are never read, has no cost cap for large wholly-different files, and runs under the lock for inline changes and tracked arrivals (`src/DiffEngineViewer/DiffRows.cs:17-22`, `QueueEntry.cs:59-63`). Use a whole-line chunker for the word pass; guard with an edit-distance lower bound. - [ ] macOS repaints the whole window every frame (`native/swift/Sources/Deview/Runtime.swift:139-140`). Redraw only when the frame, bounds or a picture stamp change, and cache scaled pictures. -- [x] Windows `Thread.Sleep(16)` sleeps about 30 ms at the default timer resolution, and a hidden process wakes about 34 times a second forever (`src/DiffEngineViewer.Windows/FormsViewerWindow.cs:65`). Use `MsgWaitForMultipleObjectsEx`, with a longer timeout while hidden. - [ ] Windows image panes rescale from full resolution and redraw the checkerboard on every paint (11 to 40 ms per image), and decode on the UI thread (`ViewerCanvas.cs:385-420`, `ImageCache.cs:56-71`). Cache the composited scaled bitmap per path, stamp and size. -- [x] All three heads lay out each row's full text though only about 35 cells fit (`ViewerCanvas.cs:503-508`, `src/DiffEngineViewer/Native/ScreenPayload.cs:164-177`); a 1 MB minified line costs about 0.8 s per paint on Windows. Truncate to the visible columns before drawing or marshalling. -- [x] raylib busy-waits the last 5% of every frame: `set(SUPPORT_PARTIALBUSY_WAIT_LOOP OFF CACHE BOOL "" FORCE)` in `native/CMakeLists.txt`. - - That line alone changed nothing: `build-native` after #884 produced byte-identical binaries. The misparse behind "The Linux viewer never showed a frame" had `SUPPORT_BUSY_WAIT_LOOP` on too, which takes precedence and would have spun through the whole of every frame's wait once frame control ran it. Both are off now. -- [x] `InlineStaging.Clear` walks the `obj` tree and re-reads and parses every staged `.inlinepatch` on each verification (`src/DiffEngine/Inline/InlineStaging.cs:94-192`). Cache per directory keyed on `LastWriteTimeUtc`. -- [x] Tray: `SafeMove`'s 8 × 400 ms retry runs on the UI thread even for failures that cannot clear, such as a read-only target or a missing directory (`src/DiffEngineTray/Tracker.cs:535-585`, `FileEx.cs:73-90`). Retry only sharing violations. -- [x] Tray: the 2 s scan re-reads every equal-size, different pair from scratch (`Tracker.cs:81-97`, `FileComparer.cs:18-57`). Cache length and write time with the last result. -- [x] `PiperClient` has no unowned-port memory for 3492, and `TrayAvailable` is cached at type init, so after the tray exits every send pays a refused connect (`src/DiffEngine/Tray/PiperClient.cs:138-153`, `PendingFiles.cs:47-49`). - - -## Appendix: repro tests - -Each test fails on 4244ebe6. `EmptyEntryPointDoesNotHang` leaves a spinning thread-pool thread behind when it fails. - -`src/DiffEngine.Tests/ReviewReproTests.cs`: - -```cs -public class ReviewReproTests -{ - static PatchStatus Cs(string source, int hint, InlinePatchMode mode, string? expression, string content, out string newSource, string? member = null) => - InlinePatcher.TryApply(SourceLanguage.CSharp, source, hint, mode, expression, null, member, null, false, content, out newSource, out _); - - const string twoDups = "class Tests\n{\n async Task Test()\n {\n await Verify(a).Snapshot(\"dup\");\n await Verify(b).Snapshot(\"dup\");\n }\n}\n"; - - [Test] - public async Task ReapplyingASetDoesNotRewriteTheSibling() - { - var first = Cs(twoDups, 6, InlinePatchMode.Set, "\"dup\"", "new", out var once, "Test"); - await Assert.That(first).IsEqualTo(PatchStatus.Applied); - await Assert.That(once).Contains("Verify(a).Snapshot(\"dup\")"); - await Assert.That(once).Contains("Verify(b).Snapshot(\"new\")"); - - var second = Cs(once, 6, InlinePatchMode.Set, "\"dup\"", "new", out var twice, "Test"); - await Assert.That(twice).Contains("Verify(a).Snapshot(\"dup\")"); - await Assert.That(second).IsEqualTo(PatchStatus.AlreadyApplied); - } - - [Test] - public async Task ReapplyingARemoveDoesNotRemoveTheSibling() - { - var first = Cs(twoDups, 6, InlinePatchMode.Remove, "\"dup\"", "", out var once, "Test"); - await Assert.That(first).IsEqualTo(PatchStatus.Applied); - - Cs(once, 6, InlinePatchMode.Remove, "\"dup\"", "", out var twice, "Test"); - await Assert.That(twice).Contains("Verify(a).Snapshot(\"dup\")"); - } - - [Test] - public async Task FsBacktickMemberNameBoundsTheSearch() - { - var source = SourceLanguage.NormalizeNewlines("module Tests\n\nlet ``test a`` () =\n Verifier.Verify(a).Snapshot(\"dup\").ToTask()\n\nlet ``test b`` () =\n Verifier.Verify(b).Snapshot(\"dup\").ToTask()\n"); - var status = InlinePatcher.TryApply(SourceLanguage.FSharp, source, 4, InlinePatchMode.Set, null, "dup", "test b", null, false, "new", out var newSource, out _); - await Assert.That(status).IsEqualTo(PatchStatus.Applied); - await Assert.That(newSource).Contains("Verify(a).Snapshot(\"dup\")"); - await Assert.That(newSource).Contains("Verify(b).Snapshot(\"new\")"); - } - - [Test] - public async Task EmptyEntryPointDoesNotHang() - { - var source = "class Tests\n{\n Task Test() =>\n Verify(a);\n}\n"; - var task = Task.Run(() => InlinePatcher.TryApply(SourceLanguage.CSharp, source, 4, InlinePatchMode.Append, null, null, null, ["VerifyDocx", ""], true, "x", out _, out _)); - var finished = await Task.WhenAny(task, Task.Delay(5000)) == task; - await Assert.That(finished).IsTrue(); - } -} -``` - -`src/DiffEngineViewer.Tests/ReviewReproViewerTests.cs`: - -```cs -public class ReviewReproViewerTests -{ - [Test] - public async Task AnUnchangedListingKeepsTheMenuOpen() - { - var state = Fixtures.Attached(Fixtures.Pending(Fixtures.Patch())); - var open = ViewerSession.OpenMenu(state, 0); - await Assert.That(open.Menu).IsNotNull(); - - // The next poll, 200ms later, with the owner reporting exactly the same queue - var synced = ViewerSession.Sync(open, Fixtures.Pending(Fixtures.Patch()), [], null); - await Assert.That(synced.Menu).IsNotNull(); - } - - [Test] - public async Task AnArrivalAfterTheQueueEmptiedClearsExit() - { - var state = Fixtures.Inline(Fixtures.Patch()); - var settled = ViewerSession.Settle(state, state.Queue[0].Key); - await Assert.That(settled.Exit).IsTrue(); - - var arrived = ViewerSession.EnqueueInline(settled, Fixtures.Patch("OtherTests.cs", 7)); - await Assert.That(arrived.Queue.Count).IsEqualTo(1); - await Assert.That(arrived.Exit).IsFalse(); - } - - [Test] - public async Task CopyingAWholeSideKeepsTabs() - { - var entry = Fixtures.Move(left: "a\tb", right: "a\tb"); - await Assert.That(SelectionText.All(entry, PaneSide.Left)).IsEqualTo("a\tb"); - } -} -``` From 38750ed9dbae42068ba622522e3e653afc4ffe3d Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Wed, 23 Sep 2026 14:15:06 +1000 Subject: [PATCH 2/2] Hold a group's deletes until its snapshots are written, from an attached viewer A snapshot moving inline arrives as a patch plus a delete of the verified file it replaces. "Accept all in " in a viewer attached to someone else's queue posted one accept per member, the delete included, and the owner carried each out as asked. A patch it could not write - the call site moved since the run - was dropped as a stale one is, and the verified file went anyway, leaving no copy of the snapshot. The owning viewer's group accept and every accept-all already hold deletes for this; this path did not. The attached viewer now sends the group as one ordered step: moves, then snapshots, then the deletes only once every snapshot is in the source. Whether one is cannot be read from the reply's ok, which a stale patch also gets, nor from a listing, where a stale patch is gone just as an applied one is. So an accept reply now says so: written, set by the tray and by an owning viewer from the applier's own answer. A reply without it, from an owner that predates it, holds the deletes, which stay queued to be accepted on their own. --- .../InlineQueueClientTests.cs | 10 +- src/DiffEngine.Tests/ViewerProtocolTests.cs | 38 +++++- src/DiffEngine/Protocol/IQueueOwner.cs | 8 +- .../Protocol/ViewerMessageHandler.cs | 27 ++++- src/DiffEngine/Protocol/ViewerResponse.cs | 19 ++- .../TrayViewerSyncTest.cs | 39 ++++++ src/DiffEngineTray/OwnedInlineHost.cs | 14 +-- .../AttachedViewerTests.cs | 113 ++++++++++++++++++ src/DiffEngineViewer.Tests/ServerFixture.cs | 25 +++- src/DiffEngineViewer/Ipc/MessageHandler.cs | 28 +++-- src/DiffEngineViewer/Ipc/OwnerLink.cs | 82 ++++++++++++- src/DiffEngineViewer/ViewerProgram.cs | 47 ++++---- todo.md | 8 -- 13 files changed, 394 insertions(+), 64 deletions(-) diff --git a/src/DiffEngine.Tests/InlineQueueClientTests.cs b/src/DiffEngine.Tests/InlineQueueClientTests.cs index 3f741b59..748ee871 100644 --- a/src/DiffEngine.Tests/InlineQueueClientTests.cs +++ b/src/DiffEngine.Tests/InlineQueueClientTests.cs @@ -412,16 +412,17 @@ bool IQueueOwner.Has(string key) } } - (bool ok, string? message) IQueueOwner.Accept(string key, string? origin) + (bool ok, string? message, bool written) IQueueOwner.Accept(string key, string? origin) { lock (gate) { if (queue.Find(key) is null) { - return (false, null); + return (false, null, false); } var before = queue; + var applied = Applied.Count; queue = origin is null ? queue.Accept(key, Record, out var message) : queue.Accept(key, origin, Record, out message); @@ -430,10 +431,11 @@ bool IQueueOwner.Has(string key) // refusal rather than an attempt and goes on the wire as an error. if (ReferenceEquals(before, queue)) { - return (false, message); + return (false, message, false); } - return (true, message); + // Record keeps only what landed + return (true, message, Applied.Count > applied); } } diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index d136f18b..168291a2 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -447,6 +447,38 @@ public async Task ARefusedAcceptGoesOnTheWireAsAnError() await Assert.That(done.Message).IsEqualTo("Applied Tests.cs:42"); } + /// + /// Whether an accept left its snapshot in the source, which ok cannot say: a patch whose call + /// site moved is attempted, so ok, and dropped unwritten. A surface accepting a group from + /// someone else's queue sends the group's deletes on it. A reply without it, from an owner + /// that predates it, reads as null, which that surface takes as not written. + /// + [Test] + public async Task AnAcceptSaysWhetherTheSnapshotWasWritten() + { + static ViewerResponse RoundTrip(ViewerResponse response) + { + if (!ViewerResponse.TryParse(response.Build(), out var parsed)) + { + throw new("Unreadable response."); + } + + return parsed; + } + + var written = RoundTrip(ViewerMessageHandler.Handle(new FakeOwner((true, "Applied Tests.cs:42"), written: true), new(ViewerVerb.Accept, "key"))); + await Assert.That(written.Written).IsTrue(); + + var stale = RoundTrip(ViewerMessageHandler.Handle(new FakeOwner((true, "Not written")), new(ViewerVerb.Accept, "key"))); + await Assert.That(stale.Ok).IsTrue(); + await Assert.That(stale.Written).IsFalse(); + + var discarded = RoundTrip(ViewerMessageHandler.Handle(new FakeOwner((true, null), written: true), new(ViewerVerb.Discard, "key"))); + await Assert.That(discarded.Written).IsNull(); + + await Assert.That(RoundTrip(ViewerResponse.Success("Applied Tests.cs:42")).Written).IsNull(); + } + /// /// A pending file with no tray running. The paths ride key and body rather than an encoded /// payload, because that is all a tracked move or delete is. @@ -539,7 +571,7 @@ public async Task AnAcceptForwardsItsOriginToTheOwner() await Assert.That(owner.AcceptedOrigin).IsEqualTo("net9.0"); } - class FakeOwner((bool ok, string? message) act) : + class FakeOwner((bool ok, string? message) act, bool written = false) : IQueueOwner { public string? AcceptedOrigin { get; private set; } @@ -562,10 +594,10 @@ public void TrackDelete(string file) => public bool Has(string key) => true; - public (bool ok, string? message) Accept(string key, string? origin) + public (bool ok, string? message, bool written) Accept(string key, string? origin) { AcceptedOrigin = origin; - return act; + return (act.ok, act.message, written); } public (bool ok, string? message) Discard(string key) => act; diff --git a/src/DiffEngine/Protocol/IQueueOwner.cs b/src/DiffEngine/Protocol/IQueueOwner.cs index f4979f1b..2768a740 100644 --- a/src/DiffEngine/Protocol/IQueueOwner.cs +++ b/src/DiffEngine/Protocol/IQueueOwner.cs @@ -49,8 +49,14 @@ interface IQueueOwner /// nothing was attempted, and the message says why — a conflicted entry with no origin to /// pick, or a locked tracked move. True means attempted, including a retryable apply failure, /// whose message says what went wrong while the entry stays pending. + /// + /// Written is whether a snapshot is in the source now: applied, or found already there. False + /// for anything else, a tracked file included, since that has no snapshot. It is what a surface + /// accepting a group from someone else's queue waits on before sending the group's deletes, + /// because ok cannot say it: a patch whose call site moved is attempted, and dropped unwritten. + /// /// - (bool ok, string? message) Accept(string key, string? origin); + (bool ok, string? message, bool written) Accept(string key, string? origin); (bool ok, string? message) Discard(string key); diff --git a/src/DiffEngine/Protocol/ViewerMessageHandler.cs b/src/DiffEngine/Protocol/ViewerMessageHandler.cs index 6d6a80b9..d8c8ee23 100644 --- a/src/DiffEngine/Protocol/ViewerMessageHandler.cs +++ b/src/DiffEngine/Protocol/ViewerMessageHandler.cs @@ -141,16 +141,33 @@ static ViewerResponse Act(IQueueOwner owner, string? key, string? body, ViewerVe return ViewerResponse.Error($"{verb} requires a key"); } + if (verb == ViewerVerb.Discard) + { + return Reply(key, owner.Discard(key)); + } + // The body is the variant origin a reviewer picked, and only an accept carries one. - var (ok, message) = verb == ViewerVerb.Accept - ? owner.Accept(key, body) - : owner.Discard(key); + var (ok, message, written) = owner.Accept(key, body); + var reply = Reply(key, (ok, message)); if (!ok) { - return ViewerResponse.Error(message ?? $"No pending snapshot for {key}"); + return reply; + } + + return reply with + { + Written = written + }; + } + + static ViewerResponse Reply(string key, (bool ok, string? message) result) + { + if (!result.ok) + { + return ViewerResponse.Error(result.message ?? $"No pending snapshot for {key}"); } - return ViewerResponse.Success(message); + return ViewerResponse.Success(result.message); } static ViewerResponse Focus(IQueueOwner owner, string? key) diff --git a/src/DiffEngine/Protocol/ViewerResponse.cs b/src/DiffEngine/Protocol/ViewerResponse.cs index d19ee9ba..5e766a8a 100644 --- a/src/DiffEngine/Protocol/ViewerResponse.cs +++ b/src/DiffEngine/Protocol/ViewerResponse.cs @@ -72,6 +72,13 @@ record ViewerResponse( /// public AcceptProgress? Progress { get; init; } + /// + /// On an accept: whether a snapshot is in the source now (see ). + /// Null on every other reply, and on any reply from an owner that predates it, which a reader + /// needing an answer takes as not written: what waits on it is a delete. + /// + public bool? Written { get; init; } + public static ViewerResponse Success(string? message = null) => new(true, message, []); @@ -111,6 +118,11 @@ public string Build() builder.Append($"progress: {Progress.Build()}\n"); } + if (Written is { } written) + { + builder.Append($"written: {(written ? "true" : "false")}\n"); + } + foreach (var item in Items) { var status = item.Status is null ? "" : ViewerPayload.Encode(item.Status); @@ -159,6 +171,7 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse? WindowCommand? window = null; string? windowKey = null; AcceptProgress? progress = null; + bool? written = null; var items = new List(); var moves = new List(); var deletes = new List(); @@ -191,6 +204,9 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse? return false; } + continue; + case "written": + written = value == "true"; continue; case "message": if (!ViewerPayload.TryDecode(value, out message)) @@ -272,7 +288,8 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse? { Moves = moves, Deletes = deletes, - Progress = progress + Progress = progress, + Written = written }; return true; } diff --git a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs index 30c9ee24..2fc12803 100644 --- a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs +++ b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs @@ -242,6 +242,45 @@ public async Task ViewerAcceptOfOneSnapshotReachesTheTray() await Assert.That(pair.Applied.Select(_ => _.LineHint)).IsEquivalentTo([1]); } + /// + /// "Accept all in" a group, from a window attached to the tray, whose patch the tray cannot + /// write because the call site moved since the run. The tray takes each accept as asked and a + /// stale patch goes as an applied one does, so only the reply says it was not written - and + /// the verified file the delete would remove is the one copy of that snapshot left. + /// + [Test] + public async Task AViewerGroupAcceptHoldsItsDeletesWhenTheTrayCouldNotWriteASnapshot() + { + await using var pair = new TrayOwned(_ => InlineApplyResult.NotFound("Could not locate the call")); + pair.Queue(sample, 1); + var delete = pair.AddDelete(); + pair.Pump(); + + pair.Link.PostAcceptGroup([], [Key(sample, 1)], [delete.Key]); + + var viewer = pair.Pump(); + await Assert.That(File.Exists(delete.File)).IsTrue(); + await Assert.That(pair.Tracker.Deletes).HasSingleItem(); + await Assert.That(viewer.Message).IsEqualTo(OwnerLink.DeletesHeld); + } + + [Test] + public async Task AViewerGroupAcceptCarriesOutItsDeletesOnceTheTrayWroteTheSnapshots() + { + await using var pair = new TrayOwned(); + pair.Queue(sample, 1); + var move = pair.AddMove(); + var delete = pair.AddDelete(); + pair.Pump(); + + pair.Link.PostAcceptGroup([move.Key], [Key(sample, 1)], [delete.Key]); + + await Assert.That(pair.Pump().Queue).IsEmpty(); + await Assert.That(pair.Applied.Select(_ => _.LineHint)).IsEquivalentTo([1]); + await Assert.That(File.Exists(delete.File)).IsFalse(); + await Assert.That(await File.ReadAllTextAsync(move.Target)).IsEqualTo("received"); + } + [Test] public async Task ViewerDiscardOfOneSnapshotReachesTheTray() { diff --git a/src/DiffEngineTray/OwnedInlineHost.cs b/src/DiffEngineTray/OwnedInlineHost.cs index f17df644..afdea9d1 100644 --- a/src/DiffEngineTray/OwnedInlineHost.cs +++ b/src/DiffEngineTray/OwnedInlineHost.cs @@ -297,34 +297,34 @@ bool IQueueOwner.Has(string key) } } - (bool ok, string? message) IQueueOwner.Accept(string key, string? origin) + (bool ok, string? message, bool written) IQueueOwner.Accept(string key, string? origin) { if (TrackedKeys.IsTracked(key)) { - var result = TrackedFiles?.Accept(key) ?? (false, null); - if (result.ok) + var (ok, text) = TrackedFiles?.Accept(key) ?? (false, null); + if (ok) { Changed?.Invoke(); } - return result; + return (ok, text, false); } var (outcome, message, refused) = AcceptOne(key, origin); if (outcome == AcceptOutcome.Unknown) { - return (false, null); + return (false, null, false); } if (refused) { // Nothing changed and nothing was attempted; the message says what a reviewer has to // do, and it goes on the wire as an error so a remote surface shows it as one. - return (false, message); + return (false, message, false); } Changed?.Invoke(); - return (true, message); + return (true, message, outcome == AcceptOutcome.Applied); } (bool ok, string? message) IQueueOwner.Discard(string key) diff --git a/src/DiffEngineViewer.Tests/AttachedViewerTests.cs b/src/DiffEngineViewer.Tests/AttachedViewerTests.cs index d09181f6..63591236 100644 --- a/src/DiffEngineViewer.Tests/AttachedViewerTests.cs +++ b/src/DiffEngineViewer.Tests/AttachedViewerTests.cs @@ -394,4 +394,117 @@ public async Task AKeyOnlyAcceptOfAConflictIsRefused() await Assert.That(host.State.Message) .IsEqualTo("Conflicting snapshots (net8.0 / net9.0), resolve in the viewer"); } + + /// + /// A snapshot moving inline is a patch plus a delete of the verified file it replaces. "Accept + /// all in SolutionA" from an attached window used to post an accept per member, the delete + /// included, and the owner carried each out as asked: a patch it could not write - the source + /// moved since the run - and the verified file went anyway, leaving no copy of the snapshot. + /// + [Test] + public async Task AGroupAcceptHoldsItsDeletesWhenASnapshotWasNotWritten() + { + using var owner = new ServerFixture(applier: _ => InlineApplyResult.NotFound("Could not locate the call")); + var (verified, host, link) = AttachToSolutionA(owner); + try + { + AcceptAllInSolutionA(host, link); + + await Assert.That(owner.Actions).IsEquivalentTo([$"apply {solutionA.SourceFile}"]); + await Assert.That(host.State.Message).IsEqualTo(OwnerLink.DeletesHeld); + // Still pending, to be accepted on its own once the reviewer has seen why + await Assert.That(host.State.Queue.Select(_ => _.Kind)).Contains(QueueEntryKind.Delete); + } + finally + { + File.Delete(verified); + } + } + + /// + /// The deletes still go once every snapshot has landed, and only then. + /// + [Test] + public async Task AGroupAcceptDeletesOnceItsSnapshotsLanded() + { + using var owner = new ServerFixture(); + var (verified, host, link) = AttachToSolutionA(owner); + try + { + AcceptAllInSolutionA(host, link); + + // In this order: a delete sent before the patch landed is what this is about + await Assert.That(string.Join(" | ", owner.Actions)) + .IsEqualTo($"apply {solutionA.SourceFile} | delete {verified}"); + await Assert.That(host.State.Queue.Select(_ => _.Key)).IsEquivalentTo([QueueEntry.KeyForInline(solutionB.SourceFile, solutionB.LineHint)]); + } + finally + { + File.Delete(verified); + } + } + + static readonly InlinePatch solutionA = Fixtures.Patch(Fixtures.SolutionFile("SolutionA", "Tests", "ATests.cs"), 10); + static readonly InlinePatch solutionB = Fixtures.Patch(Fixtures.SolutionFile("SolutionB", "Tests", "BTests.cs"), 10); + + /// + /// An owner holding a snapshot and a pending delete in SolutionA, and a snapshot in SolutionB + /// so the queue groups, with a window attached to it. + /// + static (string verified, SessionHost host, OwnerLink link) AttachToSolutionA(ServerFixture owner) + { + var verified = Fixtures.SolutionFile("SolutionA", "Tests", $"Group{Guid.NewGuid():N}.verified.txt"); + File.WriteAllText(verified, "the verified file the snapshot is moving inline from"); + owner.Send(Inline(solutionA)); + owner.Host.Mutate(_ => ViewerSession.EnqueueTracked(_, TrackedEntry.ForDelete(verified))); + owner.Send(Inline(solutionB)); + var (host, link) = Attach(owner); + link.Pump(); + return (verified, host, link); + } + + /// + /// Right-click SolutionA's header and choose "Accept all in SolutionA", as the reader would. + /// + static void AcceptAllInSolutionA(SessionHost host, OwnerLink link) + { + var visible = QueueProjection.Visible(host.State, ScreenBuilder.BodyRows(host.State), out _).ToList(); + var header = visible.FindIndex(_ => _.GroupName == "SolutionA"); + host.Mutate(_ => ViewerSession.OpenMenu(_, header)); + var item = host.State.Menu!.Items.ToList().FindIndex(_ => _.Label == "Accept all in SolutionA"); + var click = new ViewerInput(CommandKind.None, -1, -1, 0, false, Fixtures.Columns, Fixtures.Rows) + { + ClickedMenuItem = item + }; + host.Mutate(_ => ViewerProgram.Apply(_, click, link, new NoWindow())); + link.Pump(); + } + + sealed class NoWindow : IViewerWindow + { + public bool Present(Screen screen) => + true; + + public ViewerInput Poll() => + default; + + public void SetHidden(bool hidden) + { + } + + public void Focus() + { + } + + public void SetClipboard(string text) + { + } + + public bool Capture(Screen screen, int width, int height, string pngPath) => + false; + + public void Dispose() + { + } + } } diff --git a/src/DiffEngineViewer.Tests/ServerFixture.cs b/src/DiffEngineViewer.Tests/ServerFixture.cs index 7aa99792..d7f28833 100644 --- a/src/DiffEngineViewer.Tests/ServerFixture.cs +++ b/src/DiffEngineViewer.Tests/ServerFixture.cs @@ -27,12 +27,29 @@ public ServerFixture(ViewerMode mode = ViewerMode.Inline, Func { }, - _ => { }); + _ => { }) + { + MoveFile = (temp, _) => + { + lock (Applied) + { + Actions.Add($"move {temp}"); + } + }, + DeleteFile = path => + { + lock (Applied) + { + Actions.Add($"delete {path}"); + } + } + }; var handler = new MessageHandler(Host, actions, Windows.Add); listening = server.Listen(handler.Handle, cancel.Token); } @@ -40,6 +57,12 @@ public ServerFixture(ViewerMode mode = ViewerMode.Inline, Func Applied { get; } = []; + + /// + /// Every apply, move and delete the owner carried out, in order. Nothing is written: the moves + /// and deletes are recorded, not performed. + /// + public List Actions { get; } = []; public List Windows { get; } = []; public ViewerResponse Send(ViewerMessage message, TimeSpan? wait = null) diff --git a/src/DiffEngineViewer/Ipc/MessageHandler.cs b/src/DiffEngineViewer/Ipc/MessageHandler.cs index b7350753..810264c2 100644 --- a/src/DiffEngineViewer/Ipc/MessageHandler.cs +++ b/src/DiffEngineViewer/Ipc/MessageHandler.cs @@ -114,14 +114,25 @@ ViewerResponse IQueueOwner.Listing(bool withPatches) bool IQueueOwner.Has(string key) => IndexOf(host.State, key) >= 0; - (bool ok, string? message) IQueueOwner.Accept(string key, string? origin) => + (bool ok, string? message, bool written) IQueueOwner.Accept(string key, string? origin) => Act(key, CommandKind.Accept, origin); - (bool ok, string? message) IQueueOwner.Discard(string key) => - Act(key, CommandKind.Discard); + (bool ok, string? message) IQueueOwner.Discard(string key) + { + var (ok, message, _) = Act(key, CommandKind.Discard); + return (ok, message); + } - (bool ok, string? message) Act(string key, CommandKind command, string? origin = null) + (bool ok, string? message, bool written) Act(string key, CommandKind command, string? origin = null) { + // What the applier answered, for whoever sent this to know whether the snapshot is in the + // source now. The session drops a patch whose call site moved just as it drops one that + // landed, so the queue afterwards cannot tell the two apart. + InlineApplyResult? applied = null; + var recording = actions with + { + ApplyInline = _ => applied = actions.ApplyInline(_) + }; // Looked up and refused inside the same mutation that acts, rather than on a read taken // before it: the queue can change in between, which threw on an index that had gone, and // let an accept through on an entry a second framework had just made a conflict of. @@ -153,20 +164,21 @@ origin is null && selected = ViewerSession.SelectVariant(selected, origin); } - return ViewerSession.Apply(selected, command, actions); + return ViewerSession.Apply(selected, command, recording); }); if (!found) { - return (false, null); + return (false, null, false); } if (refusal is not null) { - return (false, refusal); + return (false, refusal, false); } - return (true, state.Message); + var written = applied?.Status is InlineApplyStatus.Applied or InlineApplyStatus.AlreadyApplied; + return (true, state.Message, written); } /// diff --git a/src/DiffEngineViewer/Ipc/OwnerLink.cs b/src/DiffEngineViewer/Ipc/OwnerLink.cs index dfab9b72..24f249c3 100644 --- a/src/DiffEngineViewer/Ipc/OwnerLink.cs +++ b/src/DiffEngineViewer/Ipc/OwnerLink.cs @@ -44,7 +44,12 @@ sealed class OwnerLink(SessionHost host, int port) /// public static TimeSpan SendWait { get; set; } = TimeSpan.FromMinutes(5); - readonly ConcurrentQueue outbound = new(); + /// + /// What is waiting to be sent, each as the step that sends it and says what came of it. A step + /// rather than a message, because a group accept is several messages whose last ones depend on + /// what the first ones did. + /// + readonly ConcurrentQueue> outbound = new(); /// /// Set by a post and by a send finishing, so the loop answers either at once rather than on @@ -54,12 +59,71 @@ sealed class OwnerLink(SessionHost host, int port) record Outbound(ViewerVerb Verb, string? Key, string? Body); - public void Post(ViewerVerb verb, string? key, string? body = null) + public void Post(ViewerVerb verb, string? key, string? body = null) => + Enqueue(() => Send(new(verb, key, body))); + + /// + /// "Accept all in" a group of someone else's queue: its moves, then its snapshots, then - only + /// once every snapshot has been written - its deletes. + /// + /// A snapshot moving inline arrives as a patch plus a delete of the verified file it replaces. + /// Posting an accept per member sent the delete whether or not the patch landed, and the owner + /// carries out a single accept as asked, so a patch it could not write cost the snapshot both + /// copies. The owning viewer's group accept and every owner's accept-all hold the deletes for + /// that; this is the same rule on the side of a viewer that owns nothing. + /// + /// + /// Whether each snapshot landed is the reply's , not ok and + /// not a listing: a patch whose call site moved is attempted, so ok, and dropped, so gone from + /// the listing just as an applied one is. A reply without it - an owner that predates it, or + /// none at all - holds the deletes too, because a delete is the one thing not safe to guess + /// about. The deletes held are still queued, to be accepted on their own. + /// + /// + public void PostAcceptGroup(IReadOnlyList moves, IReadOnlyList snapshots, IReadOnlyList deletes) => + Enqueue(() => AcceptGroup(moves, snapshots, deletes)); + + public const string DeletesHeld = "Deletes kept: a snapshot in this group was not written, and a file being deleted may be the only copy of it left. Accept them on their own to delete them anyway."; + + void Enqueue(Func send) { - outbound.Enqueue(new(verb, key, body)); + outbound.Enqueue(send); wake.Set(); } + string AcceptGroup(IReadOnlyList moves, IReadOnlyList snapshots, IReadOnlyList deletes) + { + var message = ""; + foreach (var key in moves) + { + message = Send(new(ViewerVerb.Accept, key, null)); + } + + var landed = true; + foreach (var key in snapshots) + { + message = Send(new(ViewerVerb.Accept, key, null), out var written); + landed &= written; + } + + if (deletes.Count == 0) + { + return message; + } + + if (!landed) + { + return DeletesHeld; + } + + foreach (var key in deletes) + { + message = Send(new(ViewerVerb.Accept, key, null)); + } + + return message; + } + public bool Pump() => Pump(out _); @@ -82,10 +146,10 @@ public bool Pump(out bool sent) => { sent = false; string? message = null; - while (outbound.TryDequeue(out var command)) + while (outbound.TryDequeue(out var send)) { sent = true; - message = Send(command); + message = send(); } return message; @@ -211,8 +275,13 @@ void Pump(Cancel cancel) } } - string Send(Outbound command) + string Send(Outbound command) => + Send(command, out _); + + /// The reply's , false when it had none. + string Send(Outbound command, out bool written) { + written = false; // The long wait matters most here: an accept is the command that takes ten seconds, and // failing it at three used to report the owner dead while it was mid apply. if (!ViewerClient.TrySend(new(command.Verb, command.Key, command.Body), out var response, port, SendWait)) @@ -220,6 +289,7 @@ string Send(Outbound command) return "The queue owner is no longer running."; } + written = response.Written == true; return response.Message ?? (response.Ok ? "" : $"{command.Verb} was refused."); } diff --git a/src/DiffEngineViewer/ViewerProgram.cs b/src/DiffEngineViewer/ViewerProgram.cs index b3ce0796..2847f3e1 100644 --- a/src/DiffEngineViewer/ViewerProgram.cs +++ b/src/DiffEngineViewer/ViewerProgram.cs @@ -656,9 +656,13 @@ static SessionState Copy(SessionState state, CommandKind kind, IViewerWindow win } /// - /// A group command against someone else's queue: one accept or discard per member, by key, - /// with conflicted entries skipped the way every bulk accept skips them. The results come - /// back on the next listing like any other forwarded command. + /// A group command against someone else's queue, by key, with conflicted entries skipped the + /// way every bulk accept skips them. The results come back on the next listing like any other + /// forwarded command. + /// + /// A discard is one per member. An accept goes as one ordered step, deletes last and only once + /// the snapshots have landed: see . + /// /// static SessionState DispatchGroup(SessionState state, CommandKind kind, OwnerLink link) { @@ -667,26 +671,23 @@ static SessionState DispatchGroup(SessionState state, CommandKind kind, OwnerLin return state; } - foreach (var index in menu.Members) + var members = menu.Members + .Where(_ => _ >= 0 && _ < state.Queue.Count) + .Select(_ => state.Queue[_]) + .ToList(); + if (kind == CommandKind.AcceptGroup) { - if (index < 0 || - index >= state.Queue.Count) - { - continue; - } - - var entry = state.Queue[index]; - if (kind == CommandKind.AcceptGroup) + link.PostAcceptGroup( + KeysOf(members, QueueEntryKind.Move), + KeysOf(members.Where(_ => !_.Conflicted), QueueEntryKind.Inline), + KeysOf(members, QueueEntryKind.Delete)); + } + else + { + foreach (var entry in members) { - if (!entry.Conflicted) - { - link.Post(ViewerVerb.Accept, entry.Key); - } - - continue; + link.Post(ViewerVerb.Discard, entry.Key); } - - link.Post(ViewerVerb.Discard, entry.Key); } return state with @@ -696,6 +697,12 @@ static SessionState DispatchGroup(SessionState state, CommandKind kind, OwnerLin }; } + static List KeysOf(IEnumerable entries, QueueEntryKind kind) => + entries + .Where(_ => _.Kind == kind) + .Select(_ => _.Key) + .ToList(); + static ViewerVerb? Remote(CommandKind kind) => kind switch { diff --git a/todo.md b/todo.md index 3bbfd0e0..ffd5dfc7 100644 --- a/todo.md +++ b/todo.md @@ -7,14 +7,6 @@ Open findings from a review of `main` at 4244ebe6 (2026-09-23), rechecked on c37 - **cannot verify here**: needs a platform this machine lacks; the item says what would settle it. -## Data loss - -- [ ] **An attached "Accept all in " deletes verified files whose snapshots were not written** (repro) - - `DispatchGroup` (`src/DiffEngineViewer/ViewerProgram.cs:663-697`) posts one `Accept` per member, deletes included (`:683`), before any answer comes back, and the owner carries each out as a single accept with no held-delete rule: the tray through `OwnedInlineHost.cs:302-304` and `Tracker.cs:956` to `File.Delete` at `:1061`, a viewer through `MessageHandler.cs:156`. #878 fixed the tray's accept-all and the AcceptAll verb, not this path. The owning viewer's own group accept does hold deletes (`ViewerSession.cs:664-675`). - - Test (`review-repros`): `An_attached_accept_all_in_a_solution_holds_its_deletes_when_a_snapshot_was_not_written`. The patch comes back NotFound and the verified file is deleted anyway, so no copy of the snapshot is left. Its owner is a viewer; the tray owner's delete path was read, not run. - - Fix: the client cannot know the patch outcomes when it posts, so the rule belongs with the owner, for example a group verb that carries the member keys through the owner's batch and its hold rule. - - ## Bugs The repro tests are on the local branch `review-repros`, one class per area: `ReviewReproSessionTests` and `ReviewReproWatchTests` (viewer model), `ReviewReproWindowsTests`, `ReviewReproTrayTests`, `ReviewReproPatcherTests`, `ReviewReproLibraryTests`. Each test fails on c37bf9e1 except a control (`ControlSpaceIndentedLocalLeavesTheSiblingAlone`) and a measurement (`HowLongADecodeHoldsTheFile`).