Skip to content

Fix BL-16350 Cancel App Builder build when moving away from the Apps screen - #8119

Closed
StephenMcConnel wants to merge 9 commits into
masterfrom
BL-16350-CancelAppBuildWhenMoveAway
Closed

Fix BL-16350 Cancel App Builder build when moving away from the Apps screen#8119
StephenMcConnel wants to merge 9 commits into
masterfrom
BL-16350-CancelAppBuildWhenMoveAway

Conversation

@StephenMcConnel

@StephenMcConnel StephenMcConnel commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Cancels an in-progress App Builder (RAB) prepare / build / install when the user
navigates away from the Apps publish screen, so a long RAB build no longer keeps
running in the background after the user has moved on.

What it does

  • Adds a per-action CancellationTokenSource in RabProjectService, claimed in
    TryBeginAction and torn down in ClearAction.
  • New publish/rab/cancel endpoint (requiresSync: false, so it can run
    concurrently with the background action) that trips the token.
  • Cancellation kills the running child-process tree (cmd.exe → rab.bat → Gradle/Java) so WaitForExit returns promptly, and stage-boundary checks
    stop the workflow between external processes.
  • A cancelled action is reported as an informational "cancelled" message rather
    than a build failure.
  • Front-end (useAppBuilderPublisherScreen) posts the cancel on the
    active→inactive sub-tab transition and on unmount (leaving the Publish tab).

Testing

  • Added RabAppProjectTests covering cancel-stops-the-action and
    cancel-is-a-safe-no-op-when-nothing-is-running.
  • Full RabAppProjectTests suite green (80/80).

Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16350

Devin review


This change is Reviewable

StephenMcConnel and others added 2 commits July 28, 2026 11:16
…screen

https://issues.bloomlibrary.org/youtrack/issue/BL-16350

Previously a Reading App Builder prepare/build/install ran as a fire-and-forget
background task with no way to stop it, so it kept running (holding the RAB
toolchain, Gradle/Java, and the collection's build folder) even after the user
switched publish sub-tabs or left the Publish tab. The UI even reconnected to the
still-running build on return. This change cancels the in-progress action when the
user moves away.

Backend (RabProjectService):
- TryBeginAction now creates a CancellationTokenSource for the claimed action;
  ClearAction disposes it. New CancelActiveAction() trips it (safe no-op when idle
  or racing action completion).
- RunProcess / RunProcessCapturingOutput register a cancellation callback that kills
  the running process and its child tree (cmd.exe -> rab.bat -> Gradle/Java) via
  Process.Kill(entireProcessTree: true), then throw OperationCanceledException so the
  workflow unwinds. ThrowIfActionCancelled checkpoints at stage boundaries stop a
  cancellation that arrives between processes.
- New ReportCancelled logs cancellation as informational rather than an error.

API (RabPublishApi):
- New publish/rab/cancel endpoint (requiresSync: false) calls CancelActiveAction.
- The prepare/build/install background tasks catch OperationCanceledException and
  report it as a cancellation instead of a failure.

Front-end (useAppBuilderPublisherScreen):
- When the Apps screen deactivates (sub-tab switch) or unmounts (leaving the Publish
  tab) with an action running, POST publish/rab/cancel and clear local busy state.

Testing: build/agent-dotnet.sh test with filter RabAppProjectTests -> 80/80 passed
(includes two new tests: CancelActiveAction cancels the active action so BuildAsync
stops, and CancelActiveAction is a safe no-op when nothing is running). Front-end
pnpm typecheck and eslint on the changed file both pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…L-16350)

ClearAction freed the action slot (_activeProgressAction = null) before
disposing the CancellationTokenSource. Because the RAB endpoints run with
requiresSync: false, a finishing action's ClearAction can race a new action's
TryBeginAction: once the slot was freed, the new action's CompareExchange could
succeed and install its own CancellationTokenSource, which the finishing
ClearAction would then null out and dispose -- leaving the new action
uncancellable (or throwing ObjectDisposedException from its token access).

Free the slot last, after tearing down our own CTS. While _activeProgressAction
is still non-null, no concurrent TryBeginAction can install a CTS, so ClearAction
only ever disposes the source it owns.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@StephenMcConnel

Copy link
Copy Markdown
Contributor Author

[Claude Opus 4.8] Consulted Devin on 2026-07-28 17:30 UTC up to commit 3c23233e2.

Result: clean — 0 Bugs, 0 Investigate flags. Devin raised 4 Informational items (not posted as threads; all assessed as non-actionable):

  1. Cancel arriving just after a successful process exit skips the post-build state-save, leaving BuildNeeded true next time. Benign — the user navigated away, so discarding the unsaved build is acceptable (at worst a redundant rebuild if they return).
  2. CancelActiveAction.Cancel() racing ClearAction.Dispose() beyond the guarded ObjectDisposedException. Practically nonexistent: the RunProcess cancellation registration's dispose already waits for any in-flight kill callback, so ClearAction runs only after the action has fully unwound.
  3. Brief 'busy' flicker if the user returns to the Apps tab mid-cancellation, ending in the correct 'cancelled' message. UX only, not a correctness issue.
  4. ~1-instruction window in TryBeginAction between claiming the slot and creating the CancellationTokenSource where a cancel would be a no-op. Requires starting an action and navigating away in the same microsecond; benign (the action simply runs).

Note: the more serious version of #2/#4ClearAction disposing a newly started action's CancellationTokenSource — was fixed in 3c23233e2 (free the slot last, after tearing down our own CTS), which is the commit Devin reviewed.

StephenMcConnel and others added 3 commits July 28, 2026 14:57
Exporting a BloomPUB per book in ExportBookInfos is in-process work, so the
process-tree kill in RunProcess can't stop it and the only cancellation checks
were at stage boundaries. A cancel during a large multi-book export therefore
kept building every remaining BloomPUB and only took effect when the export
stage finished. Check ThrowIfActionCancelled() at the top of each iteration so
leaving the Apps screen stops the export promptly. Any BloomPUBs already
exported are cached and reused on the next build, so throwing mid-loop is safe.

(Not separately unit-tested: TestRabProjectService overrides the export step, so
the real ExportBookInfos loop isn't exercised by the suite; the
ThrowIfActionCancelled mechanism is covered by the stage-boundary cancel test.)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
react-tabs renders only the selected panel's children (BloomTabs doesn't set
forceRenderTabPanel), so the Apps screen is mounted only while its sub-tab is
selected: isActive is always true for the component's whole lifetime and never
transitions true->false in place. The effect that watched for that transition to
cancel a running action was therefore dead code, and its comment wrongly implied
a sub-tab switch keeps the screen mounted.

Both leaving the Publish tab and switching to another publish sub-tab unmount the
screen, so the unmount cleanup already cancels a running action in both cases
(verified over CDP: switching sub-tab posts publish/rab/cancel and gets 200).
Remove the dead effect and its wasActiveRef, and correct the cleanup's comment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@StephenMcConnel

Copy link
Copy Markdown
Contributor Author

[Claude Opus 4.8] Consulted Devin on 2026-07-28 21:25 UTC up to commit 133c4c8f1.

Result: clean — 0 Bugs, 0 Investigate flags. Devin raised 9 Informational items, all assessed as non-actionable:

  • Confirmations (not issues): a cancellation-triggered OperationCanceledException reliably reaches the ReportCancelled branch rather than being reported as a build failure; and the single unmount-cleanup cancel trigger correctly covers both leaving Publish and switching sub-tabs (react-tabs unmounts the non-selected panel).
  • Repeats of previously-assessed benign micro-window races: Cancel() vs ClearAction().Dispose() beyond the guarded ObjectDisposedException; the ~1-instruction gap in TryBeginAction before the token exists; a cancel arriving just after a successful process exit skipping finalization; and a brief 'busy' flicker if the user returns mid-cancellation. All require sub-microsecond timing or a deliberate quick return; worst case is a redundant rebuild. No user-visible harm.
  • Minor/cosmetic (new): ReportCancelled sends the progress message with ProgressKind.Warning while its doc comment calls it 'informational' (styling vs wording mismatch); and a cancelled action still emits an actionComplete:failure websocket event — harmless because the Apps screen has already unmounted, so nothing is listening.

None warrant a code change.

…re (BL-16350)

Two small cancel-path polish items from the Devin review:

(a) ReportCancelled sends the progress message with ProgressKind.Warning, but its
doc comment called it "informational". Align the comment with the code: it is
surfaced as a warning (not an error) so an intentional cancellation doesn't look
like a build failure.

(b) On cancellation the action handlers left succeeded=false, so the
actionComplete websocket event was "{action}:failure" -- a cancel mislabeled as a
build failure. Add a distinct "cancelled" outcome: SendActionCompleteEvent now
takes a cancelled flag and emits "{action}:cancelled". The Apps screen already
treats any non-"success" outcome as "clear busy and refresh" (no settings init),
so behavior is unchanged; the event is just honest now. (Harmless in practice
because the screen has unmounted by the time a cancel fires, but correct is
better than harmless.)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread src/BloomExe/Publish/Rab/RabProjectService.cs
@StephenMcConnel

Copy link
Copy Markdown
Contributor Author

[Claude Opus 4.8] Consulted Devin on 2026-07-28 21:48 UTC up to commit 98e0d88f5.

Result: 0 Bugs, 1 Investigate, 9 Informational.

  • Investigate (posted, thread left open for a decision): the RAB installer download during a first-run Prepare isn't cancellable — #discussion_r3669416271. Real gap (the HTTP byte-copy loop has no token check), but a first-run-only path outside this PR's original build-cancellation scope; flagged for a fix-here-or-follow-up decision.
  • Informational — no action: most repeat previously-assessed benign micro-window races and confirmations. Note that two of them (ReportCancelled Warning-vs-comment, and cancelled action emitting failure) describe code this very commit fixed — Devin's analysis lagged 98e0d88f5; both are now resolved. One more references the transition effect already removed in 8d995a10e.
  • CI: pr-automation and track/set-waiting both green.

When Reading App Builder isn't installed yet, the first Prepare downloads the
installer over HTTP. The byte-copy loop in CopyRabInstallerDownloadStream had no
cancellation check and the download wasn't passed the action token, so leaving
the Apps screen mid-download didn't stop it -- it ran to completion or the
HttpClient's 30-minute timeout. That's the same "keeps working after the user
moved on" problem this card targets, on the first-run download path.

Check ThrowIfActionCancelled() at the top of each copy iteration, matching the
per-book export fix. A partial download is discarded/re-fetched on the next
attempt, so stopping mid-copy is safe. Added a unit test that cancels the action
and asserts the copy throws OperationCanceledException without copying the whole
stream. (Found by the Devin review of 98e0d88.)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@StephenMcConnel

Copy link
Copy Markdown
Contributor Author

[Claude Opus 4.8] Consulted Devin on 2026-07-28 22:00 UTC up to commit f81d47ad7. CI (pr-automation, track/set-waiting) green.

0 Bugs, 1 Investigate, 10 Informational. The Investigate flag is the same installer-download item (thread, resolved) that f81d47ad7 fixed — Devin's pattern match re-fires because the fix checks ThrowIfActionCancelled() in the copy loop rather than threading the token into HttpClient/GetAsync. Functionally the download is now cancellable during active streaming; the only residual is a stalled-connection / blocked-read edge case still bounded by the 30-min timeout. Deciding with the developer whether to leave the loop-check fix as sufficient or thread the token fully. Informational items are the previously-assessed benign repeats/confirmations (several describe already-fixed/removed code — Devin lagging the recent commits).

StephenMcConnel and others added 2 commits July 28, 2026 16:13
…16350)

Follow-up to the loop-check fix: pass ActionCancellationToken into the download
itself so cancellation also interrupts a request that is stalled connecting or a
read blocked waiting on the network -- not just one actively streaming chunks
(which the loop-top check already covered). GetAsync now receives the token,
Content.ReadAsStreamAsync receives the token, and the copy loop reads via
ReadAsync(token). Previously those blocking points were bounded only by the
HttpClient's 30-minute timeout, so a leave during a stalled first-run download
wouldn't take effect promptly.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…350)

Making the installer download cancellable introduced a regression (caught by the
Devin review of e62c313): cancelling mid-download left a partial installer file
at the download path, and FindRabSetupInstallerPath reuses whatever file is
there, so every later Prepare picked up the truncated installer and failed.

Write the download to a temporary "<installer>.partial" file and move it into
place only after a complete copy; delete the temp file if the copy is cancelled
or fails. Extracted this atomic-write into WriteRabInstallerDownloadToFile so it
can be unit-tested without the network, and added a test asserting a cancelled
download leaves neither a truncated installer nor an orphaned .partial behind.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread src/BloomExe/Publish/Rab/RabProjectService.cs
@StephenMcConnel

Copy link
Copy Markdown
Contributor Author

[Claude Opus 4.8] Consulted Devin on 2026-07-28 22:34 UTC up to commit e1941e3b0. CI green; full suites green (C# 2946 passed/0 failed, front-end 568 passed/0 failed).

Devin reports 1 Bug + 1 Investigate, but both are lagging re-reports of issues already fixed in e1941e3b0 itself — its bug text still describes the file being 'left on disk (CopyRabInstallerDownloadStream)', but that commit moved the write into WriteRabInstallerDownloadToFile with a .partial temp file that is moved into place only on a complete download and deleted on cancel/failure (regression test WriteRabInstallerDownloadToFile_CancelledMidDownload_LeavesNoLeftoverFile proves nothing is left behind). Both have resolved threads already (bug, investigate); not re-posting duplicates. Stopping the re-review loop here since the findings are addressed and verified.

@StephenMcConnel
StephenMcConnel marked this pull request as ready for review July 28, 2026 23:14
@hatton

hatton commented Jul 29, 2026

Copy link
Copy Markdown
Member

I had written:

while it's building you can't do anything except cancel.

But Claude says

This change makes Bloom cancel that in-progress action the moment you leave the Apps screen

By my instruction, I did not mean to auto-cancel. I meant that the user's only option should be to explicitly cancel the process. In the absence of a button, they can click on the "X" in the upper right. Can we do that?

@StephenMcConnel
StephenMcConnel marked this pull request as draft July 30, 2026 19:07
@StephenMcConnel

Copy link
Copy Markdown
Contributor Author

I misunderstood the instructions. I'm starting over on another branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants