A mirror cut short at a size or time cap reported Success - #163
Merged
Conversation
back_checkmirror() asks for a smooth stop when maxsite or maxtime is reached (htsback.c:4841-4853). That sets state.stop and no abort flag, and the user tapped nothing, so the pane saw a clean run and said so. The line above it disagreed: pendingWork already counted the run as resumable and offered to continue it. The stop source is read once now, as MirrorOutcome.Stop, and feeds both the pane and the resume offer, so they cannot contradict each other again. A user stop and a real abort still outrank a cap, because reaching a cap also makes the engine report itself stopped. InterruptedLockTest pinned the old expression verbatim, which made a behaviour-preserving rewrite fail it. It pins the property instead: the resume offer must not read the user's stop alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
The review found the assertion I wrote to replace #158's guard was strictly weaker than the one it replaced. Rewriting the production line was fair, since stop != NONE is equivalent to the old expression, but contains("engine.wasStopped()") is satisfied by the ternary anywhere in runInternal, so leavesPendingWork(stop == Stop.USER, code) brought #158 back with both assertions green. The argument is now taken with balanced parentheses instead of up to the first comma, and it must read the run's verdict without narrowing it to one kind of stop. Changing false to Stop.ENGINE in two existing rows also left no row pairing Stop.NONE with a live abort code, so an abort that never set the stop flag could read as a success. Those rows are back alongside the new ones. Three more the same pass found: transposing the ternary's branches was invisible, so each branch is pinned to its source; the pane's first argument is pinned by position rather than by a four-character substring a comment could satisfy; and the new Stopped! wording was pinned to nothing. STOPPED_AT_LIMIT moves after the aborts, because declaration order read as a precedence and of() ranks it last. Stop.ENGINE's comment claimed the cap, which is only what of() decides later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.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.
A mirror the engine cut short at a size or time cap still reported "Success!".
back_checkmirror()asks for a smooth stop whenmaxsiteormaxtimeis reached. That sets the engine's stop flag but no abort flag, and the user tapped nothing, so the pane saw a clean run. The line above it disagreed:pendingWorkalready counted the run as resumable and offered to continue it.The stop source is read once now, as
MirrorOutcome.Stop, and feeds both the pane and the resume offer. A cap reports "Stopped!" and says a limit was reached. A user stop and a real abort still outrank it, because reaching a cap also makes the engine report itself stopped. Reading the source once closes a second gap:interruptedis volatile and was read twice, so a stop landing between the two reads could leave the pane and the resume offer disagreeing.InterruptedLockTestpinned the old expression verbatim, so a behaviour-preserving rewrite failed it. It pins the property instead: the resume offer must read the run's verdict, without narrowing it to one kind of stop.