fix(execution): cancelling twice must stay idempotent - #267
Merged
Merged
Conversation
Self-review of #266 found a regression it introduced. Making "cancelling" a one-way state left it unreachable from itself, but request_cancel() writes status="cancelling" -- so a second Stop, or a recovery pass re-requesting a cancel, raised ExecutionTransitionConflict. The execution router maps that to 404, so pressing Stop twice told the user "Execution job not found". No existing test double-cancels, which is why CI stayed green. Allow cancelling -> cancelling while keeping the invariant that nothing non-terminal may follow it, and pin the idempotence with a test that fails against the previous frozenset. Three smaller corrections from the same review: - The read of the launcher credentials was writing the handoff secret to sessionStorage from inside a useState initialiser -- which the comment immediately below it explains must stay pure, because StrictMode double-invokes initialisers. Moved the write into the effect that already scrubs the URL, where the other side effect lives. - #266's commit message claimed a Stop pressed during translation returns the finished answer. It does not: the check was skipped only when translation had also failed, and the API runner discards the result whenever the cancel event is set, so nothing user-visible changed either way. Restored the unconditional check and replaced the comment with what actually happens, and where keeping work across a cancellation would have to be fixed instead. - The legacy JSON migration's fallback timestamp was still naive, and storage._utc_now() was left behind with no callers -- a trap that would quietly reintroduce the zone-less writes #266 removed. Both gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.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
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.
The problem
Self-review of #266 found a regression that PR introduced, plus three smaller
things it got wrong. CI was green on all seven jobs, so none of this was caught
by the existing suite.
The regression. #266 made
cancellinga one-way state in the executionstore. It left
cancellingunreachable from itself -- butrequest_cancel()writes
status="cancelling", so a second Stop raisedExecutionTransitionConflict.routers/execution.pymaps that whole errorfamily to 404, so pressing Stop twice told the user "Execution job not
found". Clicking Stop twice is an ordinary thing to do when a local model is
slow to react.
No existing test double-cancels, which is exactly why it slipped through.
Reproduced before the fix:
What changed
_CANCELLING_EXITSgainscancelling, so a repeated cancel is idempotentwhile nothing non-terminal may still follow a committed cancellation. The new
test fails against the previous frozenset and passes with it, and it re-asserts
the original invariant (a
runningwrite is still refused) so the fix cannotbe widened by accident later.
Three smaller corrections from the same review:
An impure React initialiser. fix: nine correctness defects in cancellation, consent, time, and startup #266 wrote the handoff secret to
sessionStoragefrom insidereadLauncherCredentials, which runs from auseStateinitialiser. The comment immediately below that function explainswhy the URL scrub was moved out of the initialiser: React documents
initialisers as pure and StrictMode double-invokes them to surface impure
ones. The write now happens in the effect that already performs the scrub,
and the docstring no longer claims a purity it did not have.
An overstated claim. fix: nine correctness defects in cancellation, consent, time, and startup #266's message said a Stop pressed during
translation returns the finished answer. It did not: the cancellation check
was skipped only when translation had also failed, and the API runner
discards the result whenever the cancel event is set, so nothing
user-visible changed either way. The check is unconditional again and the
comment now says what actually happens, and where keeping finished work
across a cancellation would genuinely have to be fixed.
A trap left behind. The legacy JSON migration's fallback timestamp was
still naive, and
storage._utc_now()survived with no callers -- ready toquietly reintroduce the zone-less writes fix: nine correctness defects in cancellation, consent, time, and startup #266 removed. Both are gone; read
normalisation already covered the display side.
Verification
The idempotence test was confirmed by reverting the one-line fix and watching
it fail, then restoring it.
Risk
Narrower than #266. The store invariant is relaxed in exactly one direction
(
cancellingto itself); recovery writes its cancelled rows through direct SQLrather than
transition(), so it was never affected by either version. Rollbackis a revert of this single commit, which restores #266's behaviour including the
double-cancel defect.
🤖 Generated with Claude Code