Stop losing snapshots and source files on eight paths - #878
Merged
Merged
Conversation
AddInlineAsync read every launch outcome but Failed as Queued. That was right until the gate gained Capped, for a launch MaxInstance declined: nothing was started and nothing took the patch, yet the caller was told the snapshot had been queued, so it staged nothing and the snapshot was pending nowhere. With no tray running, every inline snapshot failing after the fifth diff tool of a run went that way, and with DiffEngine_MaxInstances=0 every one did. Only Launched and Taken now read as Queued. Capped joins Failed as NoViewerFound, the answer that has the caller stage the snapshot. OnlyALaunchOrAHandoverIsQueued pins the mapping for all four outcomes.
InlineApplier writes the patched source to a sibling temporary, swaps it in with File.Replace, and deletes the temporary in a finally. ReplaceFile can fail after it has already taken the destination away: with no backup name, ERROR_UNABLE_TO_MOVE_REPLACEMENT means the original no longer exists and the replacement is still under its temporary name - which the finally then deleted, leaving no copy of the source file anywhere. An antivirus or sync client holding the freshly written temporary is what produces it. When the swap fails with the destination gone and the temporary still there, the temporary is now moved into place. It is the whole patched file, so that completes the write. The temporary is only deleted while the destination exists, and if moving it back fails too, the error names where the source went. The swap is supplied by the tests, since the failure cannot be arranged on demand. AReplaceThatFailsAfterRemovingTheSourceStillLeavesOne deletes the destination and throws, and finds the patched content in place with nothing left beside it. AReplaceThatFailsCleanlyLeavesTheSourceAsItWas throws without touching either file, and finds the original.
A patch can reach the patcher again after it has been applied: a second target framework's identical patch arriving after the first was accepted, or each framework's test process applying the same Remove. The anchor has gone from the call it named by then, so the content search looked for it elsewhere, and a sibling holding the same literal - ordinary for a member verifying two values that serialise alike - is exactly where it found it. A Set rewrote the sibling and a Remove stripped its Snapshot call, and both reported Applied. A Set now stops at the recorded line once the call there already holds the new content, and reports AlreadyApplied. A Remove reports AlreadyApplied when the recorded line holds no Snapshot call and the verify statement it belongs to has none chained onto it - including a Snapshot call that had a line of its own, whose removal leaves the rest of its statement ending on the line above. The Reapplying tests apply each shape twice; the Set and the Remove were reproduced against the old code first. RemoveWithNoSnapshotCall expected NotFound for a verify call with no Snapshot, which is exactly what the second Remove finds, so it is now RemoveWithNoSnapshotCallIsAlreadyDone, and RemoveWithNoCallAtAll keeps NotFound for a line with no call on it.
The Linux binaries were built on the ubuntu-24.04 runners with nothing setting a glibc floor, and a library records the glibc symbol versions of the headers it was compiled against. The committed ones import __isoc23_sscanf, fmod and fmodf at GLIBC_2.38, so the loader refuses them on Ubuntu 22.04, Debian 12, and RHEL 8 and 9: the viewer cannot open its window there. The Linux jobs now build inside quay.io/pypa/manylinux_2_28, whose glibc 2.28 is the floor .NET 10 itself supports (RHEL 8), through native/build-linux.sh so the same build can be run locally under docker. A new step fails the job if objdump shows the library needing any glibc version above that floor. Stripping moved into the container, which owns the build directory. The committed binaries are unchanged here. This workflow rebuilds them and proposes them in a pull request of their own.
An owning viewer could lose a snapshot or a pair it had already told its sender it held. A window that would not open. The viewer binds the port before asking for its window, so its launcher saw an owner and reported the patch queued, but Run returned before PersistOwned and the patch existed nowhere. A native library that will not load and a Linux session with no display both do this, for every inline snapshot of a run. Run now persists on that path, and in a finally when the loop throws. RunInline stages its patch when handing it to an existing owner is refused, which it used to read as a hand over. And ViewerLauncher starts no viewer on Linux with neither DISPLAY nor WAYLAND_DISPLAY set, so the caller hears NoViewerFound and stages the snapshot itself. An arrival while the queue emptied. A settle that empties the queue sets Exit, and the loop acts on it a frame later. EnqueueInline and EnqueueTracked carried Exit across, so a patch or a pair arriving in between was answered and then left with the window. Enqueueing now clears Exit, the loop commits to leaving under the host's lock (CommitExit sets SessionState.Closing), and every other way out of the loop marks it closing too. A closing viewer refuses Inline, Diff, Move and Delete, so the sender stages or reports instead of believing the work queued. TrackMove and TrackDelete also read their files before taking the lock, as their comment already said they did. ViewerProgramTests hands Run a window that will not open and one that throws, and finds the patch staged both times. ViewerSessionTests pins the Exit and Closing transitions, IpcTests a closing viewer refusing each verb, and ViewerLauncherTests the display check.
A snapshot moving inline arrives as two unrelated entries: the patch that writes its literal, and a delete of the verified file it replaces. The tray's accept-all - the menu, both hot keys, and a displaying viewer's Accept all forwarded to it - ran the deletes first, so a patch refused afterwards (the source edited since the run, or a call site that cannot host a Snapshot) left the snapshot in neither place. The viewer's own batch has always applied snapshots first and held its deletes when one was not written. The tray, which owns the queue in the usual arrangement, did not. Every tray accept-all now applies the snapshots before the deletes, and holds every delete when a snapshot in the batch was not written, saying why. The owned host decides from the batch's tally. A tray driving a viewer's queue reads the refusal back out of the full listing, where a non-conflicted entry with a status is one the batch could not write, and an owner that cannot be asked counts as refused. The owned batch also applied from a copy of the queue taken when it started, so an entry settled, discarded, replaced or made a conflict of while earlier ones were applying was still written into the source. It now takes keys and looks each entry up again just before applying it, the way the viewer's batch claims its entries. Pinned from the menu (TrackerDeleteTest), the wire (OwnedInlineHostTest, including a discard during a held batch), the tracked files (TrackerTrackedFilesTest), and a tray driving an owning viewer (TrayViewerSyncTest). AnAcceptAllCountsTheFilesIntoItsProgress now expects no files done while the first snapshot applies.
Release builds treat CS1573 as an error, and the overload documented its replace parameter but not fullPath or output, which failed every CI job at the build. IInlineHost.AcceptAll had the same gap for message.
…snapshots Rebuild native renderer binaries
The manylinux_2_28 build's CMake prefers GLVND and linked libOpenGL.so.0, which Ubuntu's libgl1 does not provide, so the native CI job could not load the rebuilt library. OpenGL_GL_PREFERENCE=LEGACY links libGL.so.1, and build-native now fails if libOpenGL is a dependency.
…snapshots Rebuild native renderer binaries
SimonCropp
added a commit
that referenced
this pull request
Sep 23, 2026
Every item that was unverified, checked on c37bf9e: all of them hold, and none was fixed by #878, #881 or #884. Each now says whether it was reproduced, verified by reading, or needs a platform this machine lacks, with the current lines, the repro test names, and where the suggested fix turned out wrong or incomplete. An attached "Accept all in <solution>" deleting verified files whose snapshots were not written moves to Data loss. The repro tests are on a local branch, review-repros, rather than here: each fails until its item is fixed.
SimonCropp
added a commit
that referenced
this pull request
Sep 23, 2026
…885) * Make the Linux viewer draw frames, read input and wait between them raylib 6.0's CMake reads every SUPPORT_ flag in config.h into an option that defaults to ON, including the ones config.h sets to 0, and CUSTOMIZE_BUILD skips config.h's own values. So SUPPORT_CUSTOM_FRAME_CONTROL was on, and EndDrawing never swapped a frame onto the screen, never polled input and never waited for the next frame: the Linux window stayed blank and ignored every key and click while the loop spun. SUPPORT_BUSY_WAIT_LOOP was on the same way, and would have spun through the whole of every frame's wait once there was one. Switch both off, with every other flag config.h defaults to 0, and turn off SUPPORT_SCREEN_CAPTURE, whose F12 handler becomes reachable once input is read. PixelTests.PresentWaitsForTheNextFrame times sixty presents against the renderer the Ubuntu job builds. The pixel snapshots could never have noticed: a capture draws into a texture and never reaches EndDrawing. * Record what checking the review's unverified items found Every item that was unverified, checked on c37bf9e: all of them hold, and none was fixed by #878, #881 or #884. Each now says whether it was reproduced, verified by reading, or needs a platform this machine lacks, with the current lines, the repro test names, and where the suggested fix turned out wrong or incomplete. An attached "Accept all in <solution>" deleting verified files whose snapshots were not written moves to Data loss. The repro tests are on a local branch, review-repros, rather than here: each fails until its item is fixed. * Rebuild native renderer binaries (#886) Co-authored-by: SimonCropp <122666+SimonCropp@users.noreply.github.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: SimonCropp <122666+SimonCropp@users.noreply.github.com>
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.
Eight paths where a snapshot, or the source file it lives in, could be lost, found in a review of the tray, viewer, patcher and launch paths. Six commits, one per area, each with the tests that pin it.
Behaviour that changes on purpose:
DISPLAYnorWAYLAND_DISPLAYset, so the sender stages instead.The committed Linux binaries are unchanged. This push runs
build-native, which should open its own PR with binaries built in themanylinux_2_28container, and the new floor check fails that job if they still need anything above GLIBC_2.28. The container build has not been run anywhere before, so that job is its first test.Run locally on Windows (Debug,
DiffEngine_ToolOrdercleared), all passing: DiffEngine.Tests 700 on net10.0 and 684 on net48, DiffEngineViewer.Tests 356, DiffEngineTray.Tests 246, DiffEngineViewer.Windows.Tests 56.