From 6e7d540d994d1b96d9a00747a1478c2a85441036 Mon Sep 17 00:00:00 2001 From: Steve McConnel Date: Thu, 30 Jul 2026 15:22:15 -0600 Subject: [PATCH 1/3] Provide explicit Cancel for Publish>Apps and make it modal while running (BL-16350) While a Reading App Builder prepare/build/try-on-phone action is running (its Cancel button showing), the Apps operation is now modal: the user's only option is the explicit Cancel button, and Bloom prevents navigating away rather than auto-cancelling. - Modal navigation: disable the main workspace tabs from C# for the duration of an action (RabPublishApi injects PublishView -> WorkspaceView.SetTabsEnabled), and block the publish-tool switcher in React (PublishTabPane vetoes onSelect and disables the other tools while the Apps screen reports busy via onBusyChange). - Responsive cancellation: check for cancellation between building each BloomPUB in the export loop, and thread a CancellationToken through the RAB installer HTTP download so a stalled download aborts promptly instead of waiting for the timeout. - No debris on interrupt: - Installer download writes to a temp .part file and is moved into place only on success; a cancelled/failed download leaves no partial installer at the real name (which FindRabSetupInstallerPath would otherwise try to run). - A cancelled/failed build deletes intermediate .apk files under BuildRoot so a partial/unsigned APK cannot be mistaken for a finished app; the previously built signed APK in SafeApkRoot is preserved. - The update-incompatible install recovery (uninstall + reinstall) is made atomic so a cancel cannot leave the phone with no app installed. - Tests covering the new cancellation/cleanup paths. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../publish/Apps/AppPublisherScreen.tsx | 119 ++++-- .../Apps/useAppBuilderPublisherScreen.ts | 6 + .../publish/PublishTab/PublishTabPane.tsx | 21 + src/BloomExe/Publish/Rab/RabProjectService.cs | 369 ++++++++++++++++-- src/BloomExe/Publish/Rab/RabPublishApi.cs | 48 ++- .../Publish/Rab/RabAppProjectTests.cs | 288 ++++++++++++++ 6 files changed, 785 insertions(+), 66 deletions(-) diff --git a/src/BloomBrowserUI/publish/Apps/AppPublisherScreen.tsx b/src/BloomBrowserUI/publish/Apps/AppPublisherScreen.tsx index 40c14e3e71ff..6f29be2a329c 100644 --- a/src/BloomBrowserUI/publish/Apps/AppPublisherScreen.tsx +++ b/src/BloomBrowserUI/publish/Apps/AppPublisherScreen.tsx @@ -87,6 +87,7 @@ const AppActionButton: React.FunctionComponent<{ // Keep this component mostly declarative. The hook owns websocket/API state so the JSX can stay focused on the workflow. const AppPublisherScreenContents: React.FunctionComponent<{ isActive: boolean; + onBusyChange?: (busy: boolean) => void; }> = (props) => { const screenState = useAppBuilderPublisherScreen(props.isActive); const [showSettingsDialog, setShowSettingsDialog] = React.useState(false); @@ -94,6 +95,15 @@ const AppPublisherScreenContents: React.FunctionComponent<{ React.useState(false); const [showUsbDebuggingHelpDialog, setShowUsbDebuggingHelpDialog] = React.useState(false); + // Report the running/Cancel-showing state up to the publish-tab host so it can make the + // operation modal: while an action runs, the host blocks switching to another publish tool + // (and C# blocks the main workspace tabs). The cleanup resets it to false so leaving or + // unmounting never leaves the publish tools stuck disabled. + React.useEffect(() => { + props.onBusyChange?.(!!screenState.busyAction); + return () => props.onBusyChange?.(false); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [screenState.busyAction]); const prepareTooltip = useL10n( "Create the Reading App Builder project in this collection's Bloom App Data folder.", "PublishTab.Apps.Prepare.TooltipBloomAppData", @@ -355,17 +365,39 @@ const AppPublisherScreenContents: React.FunctionComponent<{ > - - screenState.runAction("prepare") - } - size="large" - tooltip={prepareTooltipToShow} +
- {prepareButtonLabel} - + + screenState.runAction("prepare") + } + size="large" + tooltip={prepareTooltipToShow} + > + {prepareButtonLabel} + + {busyAction === "prepare" && ( + + screenState.cancelAction() + } + size="large" + variant="outlined" + > + Cancel + + )} +
- - screenState.runAction("build") - } - size="large" - tooltip={buildTooltipToShow} - iconBeforeText={ - - } +
- Build - + + screenState.runAction("build") + } + size="large" + tooltip={buildTooltipToShow} + iconBeforeText={ + + } + > + Build + + {busyAction === "build" && ( + + screenState.cancelAction() + } + size="large" + variant="outlined" + > + Cancel + + )} +
Try on phone + {busyAction === "install" && ( + + screenState.cancelAction() + } + size="large" + variant="outlined" + > + Cancel + + )} void; }> = (props) => { const optionsPanel = ( @@ -696,7 +764,10 @@ export const AppPublisherScreen: React.FunctionComponent<{ bannerDescriptionMarkdown="Create an app that you can install on your Android phone, share with others, and publish on the Google Play Store." optionsPanelContents={optionsPanel} > - + ); diff --git a/src/BloomBrowserUI/publish/Apps/useAppBuilderPublisherScreen.ts b/src/BloomBrowserUI/publish/Apps/useAppBuilderPublisherScreen.ts index ca79f9861267..74f5ad2447e4 100644 --- a/src/BloomBrowserUI/publish/Apps/useAppBuilderPublisherScreen.ts +++ b/src/BloomBrowserUI/publish/Apps/useAppBuilderPublisherScreen.ts @@ -51,6 +51,7 @@ export interface IAppBuilderPublisherScreenState { progressStageCode?: string; sizeEstimates: IAppSizeEstimates; runAction: (action: AppBuilderAction) => void; + cancelAction: () => void; showApkInExplorerInShell: () => void; markConfigurationChanged: () => void; } @@ -448,6 +449,10 @@ export function useAppBuilderPublisherScreen( void postJson("fileIO/showInFolder", { folderPath: status.apkPath }); } + function cancelAction(): void { + post("publish/rab/cancel"); + } + function markConfigurationChanged(): void { setPendingBuildNeeded(true); void refreshStatus(); @@ -467,6 +472,7 @@ export function useAppBuilderPublisherScreen( progressStageCode, sizeEstimates, runAction, + cancelAction, showApkInExplorerInShell, markConfigurationChanged, }; diff --git a/src/BloomBrowserUI/publish/PublishTab/PublishTabPane.tsx b/src/BloomBrowserUI/publish/PublishTab/PublishTabPane.tsx index 96e1ae3e0706..a2413784eaf2 100644 --- a/src/BloomBrowserUI/publish/PublishTab/PublishTabPane.tsx +++ b/src/BloomBrowserUI/publish/PublishTab/PublishTabPane.tsx @@ -108,6 +108,9 @@ export const PublishTabPane: React.FunctionComponent = () => { const [tabIndex, setTabIndex] = React.useState( kWaitForUserToChooseTabIndex, ); + // True while the Apps tool has a Reading App Builder action running (its Cancel button is + // showing). While busy, switching to another publish tool is blocked so the operation is modal. + const [appsBusy, setAppsBusy] = React.useState(false); const appBuilderFeatureStatus = useGetFeatureStatus("AppBuilder"); const setup = () => { setTabIndex(kWaitForUserToChooseTabIndex); @@ -239,6 +242,13 @@ export const PublishTabPane: React.FunctionComponent = () => { labelBackgroundColor={kPanelBackground} selectedIndex={tabIndex} onSelect={(newIndex) => { + // While a Reading App Builder action is running (its Cancel button + // is showing), the Apps operation is modal: veto switching to another + // publish tool until it finishes or is cancelled. The main workspace + // tabs are locked from C# (RabPublishApi) to match. + if (appsBusy) { + return false; + } post("publish/switchingPublishMode"); logPublishTabSelected(newIndex); setTabIndex(newIndex); @@ -300,12 +310,22 @@ export const PublishTabPane: React.FunctionComponent = () => { .invisible_tab { display: none; } + // Doubled class for enough specificity to override the tab color + // rule above, so tools disabled during a modal Apps action read as + // greyed out (react-tabs already makes them non-clickable). + .react-tabs__tab--disabled.react-tabs__tab--disabled { + opacity: 0.4; + cursor: default; + } `} > {publishTabs.map((tab, index) => ( { isActive={ publishTabs[tabIndex]?.id === "apps" } + onBusyChange={setAppsBusy} /> diff --git a/src/BloomExe/Publish/Rab/RabProjectService.cs b/src/BloomExe/Publish/Rab/RabProjectService.cs index cde439e3a83e..ae5d4a418e00 100644 --- a/src/BloomExe/Publish/Rab/RabProjectService.cs +++ b/src/BloomExe/Publish/Rab/RabProjectService.cs @@ -93,6 +93,23 @@ string packageSegment private List _rabOutputCapture; private readonly object _rabOutputCaptureLock = new object(); + // Cancellation state for the currently running action (prepare/build/install). + private volatile bool _cancelRequested; + private readonly object _currentProcessLock = new object(); + private Process _currentProcess; + + // While true, the RAB/adb subprocess helpers run to completion without being killed by a + // cancellation and without throwing on a pending cancel. Used to make the "uninstall the old + // copy, then reinstall" recovery in Install atomic, so a cancel arriving mid-recovery cannot + // leave the phone with the old app removed and nothing put back in its place. + private volatile bool _protectCurrentProcessFromCancellation; + + // Cancels in-process async I/O (currently the installer download) that killing a subprocess + // would not interrupt, so a stalled HTTP request aborts immediately on cancel instead of + // hanging until the HttpClient timeout. Guarded by _currentProcessLock, which serializes all + // of the current action's cancellation state. + private CancellationTokenSource _actionCancellationSource; + public RabProjectService( CollectionModel collectionModel, BookSelection bookSelection, @@ -467,9 +484,80 @@ internal bool TryBeginAction(string action) // before the first ReportProgressStage call doesn't serve stale values. _lastLoggedProgressStage = null; _lastLoggedProgressPercent = -1; + _cancelRequested = false; + lock (_currentProcessLock) + { + _actionCancellationSource?.Dispose(); + _actionCancellationSource = new CancellationTokenSource(); + } return true; } + /// + /// Requests cancellation of the currently running prepare/build/install action. + /// Kills the active RAB subprocess (if any) so the background thread unblocks and can clean up, + /// and cancels the action's token so any in-flight async I/O (e.g. a stalled installer + /// download) aborts immediately instead of blocking until its timeout. + /// + internal void RequestCancellation() + { + _cancelRequested = true; + Process processToKill; + CancellationTokenSource sourceToCancel; + lock (_currentProcessLock) + { + processToKill = _currentProcess; + sourceToCancel = _actionCancellationSource; + } + // Cancel outside the lock so any read-abort continuations the token fires don't run while + // we hold it. The source can be disposed by a concurrent ClearAction, so tolerate that. + try + { + sourceToCancel?.Cancel(); + } + catch (ObjectDisposedException) { } + if (processToKill != null) + { + try + { + processToKill.Kill(true); + } + catch (Exception) { } // Process may have already exited + } + } + + /// + /// The current action's cancellation token, or when no + /// action is running. Used to make blocking async I/O (the installer download) abort promptly + /// when the user cancels. + /// + private CancellationToken CurrentCancellationToken + { + get + { + lock (_currentProcessLock) + return _actionCancellationSource?.Token ?? CancellationToken.None; + } + } + + /// + /// Throws if the user has requested cancellation of + /// the current action, so callers can bail at a safe point. + /// + private void ThrowIfCancellationRequested() + { + if (_cancelRequested) + throw new OperationCanceledException(); + } + + /// + /// Logs a user-facing "cancelled" message to the progress channel. + /// + public void ReportCancellation(string action) + { + _progress.MessageWithoutLocalizing($"{action} cancelled."); + } + /// /// Releases the action slot. Called by just before /// so the slot remains claimed until the @@ -479,6 +567,11 @@ internal bool TryBeginAction(string action) internal void ClearAction() { _activeProgressAction = null; + lock (_currentProcessLock) + { + _actionCancellationSource?.Dispose(); + _actionCancellationSource = null; + } } /// @@ -741,25 +834,42 @@ private void Build() _rabOutputCapture = buildOutput; try { - RunRabCommand( - BuildRabArgsForProjectUpdate( - paths, - state, - Array.Empty(), - supportFiles, - true - ), - paths.RabRoot - ); + try + { + RunRabCommand( + BuildRabArgsForProjectUpdate( + paths, + state, + Array.Empty(), + supportFiles, + true + ), + paths.RabRoot + ); + } + catch (ApplicationException e) + { + // RAB exited with an error (RunProcess throws here on a non-zero exit code). + // Its own stdout/stderr (collected in buildOutput) usually explains why — e.g. a + // missing font — so surface those diagnostics alongside the exit-code summary + // instead of leaving the user with a bare "cmd.exe exited with code N" (BL-16467). + // The original exception is preserved as InnerException for the log. + throw new ApplicationException( + DescribeFailedRabBuild(e.Message, buildOutput), + e + ); + } } - catch (ApplicationException e) + catch (Exception) { - // RAB exited with an error (RunProcess throws here on a non-zero exit code). - // Its own stdout/stderr (collected in buildOutput) usually explains why — e.g. a - // missing font — so surface those diagnostics alongside the exit-code summary - // instead of leaving the user with a bare "cmd.exe exited with code N" (BL-16467). - // The original exception is preserved as InnerException for the log. - throw new ApplicationException(DescribeFailedRabBuild(e.Message, buildOutput), e); + // The build didn't finish — the user cancelled, or RAB failed partway. Gradle may + // have left a partial or unsigned intermediate .apk under BuildRoot; delete those so + // a later status check or "Try on phone" cannot mistake one for a finished, signed + // app (FindLatestApkPath scans BuildRoot). This runs only on the abnormal path, so a + // successful build is untouched, and it never touches SafeApkRoot, so a failed + // rebuild still keeps the previous good APK. + DeleteIntermediateBuildApks(paths); + throw; } finally { @@ -924,17 +1034,41 @@ private void Install() { if (IsUpdateIncompatibleInstallFailure(installResult.Output)) { + // The first attempt failed only because a differently-signed copy is already + // installed, and that copy is still intact. If the user asked to cancel during + // that attempt, stop here — before the destructive uninstall — so their existing + // app is left untouched. + ThrowIfCancellationRequested(); + _progress.MessageWithoutLocalizing( $"A different signed copy of {appName} is already installed on {device.DisplayName}. Removing it and retrying...", ProgressKind.Warning ); - UninstallAppFromDevice(adbPath, device.Serial, packageName, paths.RabRoot); - installResult = InstallApkOnDevice( - adbPath, - device.Serial, - apkPath, - paths.RabRoot - ); + + // Uninstalling removes the phone's current copy, so once we start we must finish + // the reinstall even if the user cancels in between; otherwise the phone would be + // left with no app at all. Shield this pair of adb calls from cancellation, then + // honor any pending cancel once the app is safely back. + _protectCurrentProcessFromCancellation = true; + try + { + UninstallAppFromDevice(adbPath, device.Serial, packageName, paths.RabRoot); + installResult = InstallApkOnDevice( + adbPath, + device.Serial, + apkPath, + paths.RabRoot + ); + } + finally + { + _protectCurrentProcessFromCancellation = false; + } + + // The app is reinstalled; if the user cancelled during the protected recovery, + // honor it now. (A failed reinstall falls through to the error check below.) + if (installResult.ExitCode == 0) + ThrowIfCancellationRequested(); } if (installResult.ExitCode != 0) @@ -1227,6 +1361,13 @@ private List ExportBookInfos( for (var index = 0; index < booksToExport.Count; index++) { + // Creating a BloomPUB runs entirely in-process (no RAB subprocess whose exit lets + // RunProcess notice cancellation), so a user who clicks Cancel during a multi-book + // export would otherwise wait for every remaining book to finish. Check between + // books so cancellation takes effect promptly. + if (_cancelRequested) + throw new OperationCanceledException(); + var bookInfo = booksToExport[index]; var book = _collectionModel.GetBookFromBookInfo(bookInfo); var existing = @@ -2214,9 +2355,30 @@ internal virtual void RunProcess( if (!process.Start()) throw new ApplicationException($"Bloom could not start {fileName}."); - process.BeginOutputReadLine(); - process.BeginErrorReadLine(); - process.WaitForExit(); + // When protected, don't register the process for the cancellation kill so this call + // runs to completion even if the user cancels (see _protectCurrentProcessFromCancellation). + if (!_protectCurrentProcessFromCancellation) + { + lock (_currentProcessLock) + _currentProcess = process; + } + try + { + process.BeginOutputReadLine(); + process.BeginErrorReadLine(); + process.WaitForExit(); + } + finally + { + if (!_protectCurrentProcessFromCancellation) + { + lock (_currentProcessLock) + _currentProcess = null; + } + } + + if (!_protectCurrentProcessFromCancellation && _cancelRequested) + throw new OperationCanceledException(); if (process.ExitCode != 0) throw new ApplicationException( @@ -2311,9 +2473,30 @@ string workingDirectory if (!process.Start()) throw new ApplicationException($"Bloom could not start {fileName}."); - process.BeginOutputReadLine(); - process.BeginErrorReadLine(); - process.WaitForExit(); + // When protected, don't register the process for the cancellation kill so this call + // runs to completion even if the user cancels (see _protectCurrentProcessFromCancellation). + if (!_protectCurrentProcessFromCancellation) + { + lock (_currentProcessLock) + _currentProcess = process; + } + try + { + process.BeginOutputReadLine(); + process.BeginErrorReadLine(); + process.WaitForExit(); + } + finally + { + if (!_protectCurrentProcessFromCancellation) + { + lock (_currentProcessLock) + _currentProcess = null; + } + } + + if (!_protectCurrentProcessFromCancellation && _cancelRequested) + throw new OperationCanceledException(); return (process.ExitCode, string.Join(Environment.NewLine, outputLines)); } @@ -2460,34 +2643,91 @@ internal virtual void DownloadRabSetupInstallerFromUrl( Action reportProgress ) { + // Downloading the RAB installer can take many minutes on a slow connection and runs + // entirely in-process (no subprocess whose exit lets RunProcess notice cancellation). + // Thread the action's cancellation token through the HTTP calls so a Cancel click aborts + // even a stalled request/response immediately rather than waiting for the HttpClient + // timeout. When the user cancels, these calls throw an OperationCanceledException that + // RabPublishApi reports as a cancellation. + var cancellationToken = CurrentCancellationToken; using var httpClient = CreateRabInstallerHttpClient(); using var response = httpClient - .GetAsync(kRabSetupDownloadUrl, HttpCompletionOption.ResponseHeadersRead) + .GetAsync( + kRabSetupDownloadUrl, + HttpCompletionOption.ResponseHeadersRead, + cancellationToken + ) .GetAwaiter() .GetResult(); response.EnsureSuccessStatusCode(); - Directory.CreateDirectory(Path.GetDirectoryName(installerPath)); - using var responseStream = response - .Content.ReadAsStreamAsync() + .Content.ReadAsStreamAsync(cancellationToken) .GetAwaiter() .GetResult(); - using var fileStream = RobustFile.Create(installerPath); - CopyRabInstallerDownloadStream( + SaveDownloadStreamAtomically( + installerPath, responseStream, - fileStream, response.Content.Headers.ContentLength ?? -1, - reportProgress + reportProgress, + cancellationToken ); } + /// + /// Copies a download stream to by writing to a temporary + /// ".part" file next to it and moving it into place only after the whole download succeeds. + /// If the download is cancelled or fails, the partial file is deleted, so a half-written (or + /// zero-byte) file is never left at the real installer name — where would otherwise find it and Bloom would try to run a + /// truncated installer, a broken state that would not self-heal on the next attempt. + /// + internal void SaveDownloadStreamAtomically( + string installerPath, + Stream responseStream, + long totalBytes, + Action reportProgress, + CancellationToken cancellationToken + ) + { + Directory.CreateDirectory(Path.GetDirectoryName(installerPath)); + var partialPath = installerPath + ".part"; + try + { + using (var fileStream = RobustFile.Create(partialPath)) + { + CopyRabInstallerDownloadStream( + responseStream, + fileStream, + totalBytes, + reportProgress, + cancellationToken + ); + } + // The download finished; publish it under the real installer name. Delete any older + // installer first because RobustFile.Move does not overwrite. + RobustFile.Delete(installerPath); + RobustFile.Move(partialPath, installerPath); + } + catch (Exception) + { + // Cancelled or failed: don't leave a partial download behind. + try + { + RobustFile.Delete(partialPath); + } + catch (Exception) { } + throw; + } + } + internal virtual void CopyRabInstallerDownloadStream( Stream responseStream, Stream fileStream, long totalBytes, - Action reportProgress + Action reportProgress, + CancellationToken cancellationToken ) { var buffer = new byte[81920]; @@ -2495,11 +2735,24 @@ Action reportProgress while (true) { - var bytesRead = responseStream.Read(buffer, 0, buffer.Length); + // Reading the network stream can block indefinitely if the connection stalls, so pass + // the cancellation token into the read/write: a Cancel click then aborts the stalled + // read immediately. The _cancelRequested check is a cheap backstop that also stops the + // loop between chunks if this ever runs without an active-action token. + if (_cancelRequested) + throw new OperationCanceledException(); + + var bytesRead = responseStream + .ReadAsync(buffer, 0, buffer.Length, cancellationToken) + .GetAwaiter() + .GetResult(); if (bytesRead <= 0) break; - fileStream.Write(buffer, 0, bytesRead); + fileStream + .WriteAsync(buffer, 0, bytesRead, cancellationToken) + .GetAwaiter() + .GetResult(); transferredBytes += bytesRead; reportProgress?.Invoke(transferredBytes, totalBytes); } @@ -3682,6 +3935,40 @@ internal virtual string FindAppDefPath(RabWorkspacePaths paths) .FirstOrDefault(); } + /// + /// Deletes any intermediate .apk files Gradle may have written under the build folder. These + /// are never the deliverable — a finished build signs the app and writes it to SafeApkRoot — + /// so removing them after an interrupted or failed build keeps a partial or unsigned + /// intermediate from later being picked up by as if it were a + /// finished app. SafeApkRoot is intentionally left alone so a failed rebuild still keeps the + /// previous good APK. + /// + private static void DeleteIntermediateBuildApks(RabWorkspacePaths paths) + { + if (!Directory.Exists(paths.BuildRoot)) + return; + + foreach ( + var apkPath in Directory.GetFiles( + paths.BuildRoot, + "*.apk", + SearchOption.AllDirectories + ) + ) + { + try + { + RobustFile.Delete(apkPath); + } + catch (Exception) + { + // A leftover we can't delete (e.g. still locked by a lingering Gradle process) is + // not worth failing the cancellation/failure path over; it will be superseded by + // the next successful build's signed APK in SafeApkRoot. + } + } + } + internal virtual string FindLatestApkPath(RabWorkspacePaths paths) { var searchRoots = new[] diff --git a/src/BloomExe/Publish/Rab/RabPublishApi.cs b/src/BloomExe/Publish/Rab/RabPublishApi.cs index 90d34b9cc863..f172dbb3abbe 100644 --- a/src/BloomExe/Publish/Rab/RabPublishApi.cs +++ b/src/BloomExe/Publish/Rab/RabPublishApi.cs @@ -20,10 +20,25 @@ public class RabPublishApi public const string kWebSocketEventId_ActionComplete = "actionComplete"; private readonly RabProjectService _rabProjectService; + private readonly PublishView _publishView; - public RabPublishApi(RabProjectService rabProjectService) + public RabPublishApi(RabProjectService rabProjectService, PublishView publishView) { _rabProjectService = rabProjectService; + _publishView = publishView; + } + + /// + /// Enables or disables the main workspace tabs (Collections/Edit/Publish) for the duration + /// of a prepare/build/install action. While an action runs — i.e. while its Cancel button is + /// showing — the operation is modal: the user cannot navigate to another workspace tab until + /// it finishes or is cancelled. Mirrors how a BloomLibrary upload locks the tabs (see + /// LibraryPublishApi.SetParentControlsState). The publish-tool switcher on the Publish tab is + /// blocked separately on the React side (PublishTabPane). + /// + private void SetWorkspaceTabsEnabled(bool enable) + { + _publishView?.WorkspaceView?.SetTabsEnabled(enable); } /// @@ -121,6 +136,9 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) request.Failed("A prepare/build/install action is already running."); return; } + // Lock the workspace tabs for the duration so the action is modal (see + // SetWorkspaceTabsEnabled); re-enabled in the finally below. + SetWorkspaceTabsEnabled(false); _ = Task.Run(async () => { var succeeded = false; @@ -131,6 +149,11 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) await _rabProjectService.PrepareAsync(); succeeded = true; } + catch (OperationCanceledException) + { + // ReportCancellation logs before the UI tears down the subscription. + _rabProjectService.ReportCancellation("Prepare"); + } catch (Exception error) { // ReportFailure logs to the progress channel first so the error @@ -145,6 +168,7 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) // until the client is notified, preventing a status-poll in the gap // from incorrectly clearing the client's busyAction via recovery logic. _rabProjectService.ClearAction(); + SetWorkspaceTabsEnabled(true); _rabProjectService.SendActionCompleteEvent("prepare", succeeded); } }); @@ -162,6 +186,7 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) request.Failed("A prepare/build/install action is already running."); return; } + SetWorkspaceTabsEnabled(false); _ = Task.Run(async () => { var succeeded = false; @@ -172,6 +197,10 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) await _rabProjectService.BuildAsync(); succeeded = true; } + catch (OperationCanceledException) + { + _rabProjectService.ReportCancellation("Build"); + } catch (Exception error) { _rabProjectService.ReportFailure("Build", error); @@ -180,6 +209,7 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) finally { _rabProjectService.ClearAction(); + SetWorkspaceTabsEnabled(true); _rabProjectService.SendActionCompleteEvent("build", succeeded); } }); @@ -197,6 +227,7 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) request.Failed("A prepare/build/install action is already running."); return; } + SetWorkspaceTabsEnabled(false); _ = Task.Run(async () => { var succeeded = false; @@ -207,6 +238,10 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) await _rabProjectService.InstallAsync(); succeeded = true; } + catch (OperationCanceledException) + { + _rabProjectService.ReportCancellation("Try on phone"); + } catch (Exception error) { _rabProjectService.ReportFailure("Try on phone", error); @@ -215,6 +250,7 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) finally { _rabProjectService.ClearAction(); + SetWorkspaceTabsEnabled(true); _rabProjectService.SendActionCompleteEvent("install", succeeded); } }); @@ -223,6 +259,16 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) false, requiresSync: false ); + apiHandler.RegisterEndpointHandler( + kApiUrlPart + "cancel", + request => + { + _rabProjectService.RequestCancellation(); + request.PostSucceeded(); + }, + false, + requiresSync: false + ); } } } diff --git a/src/BloomTests/Publish/Rab/RabAppProjectTests.cs b/src/BloomTests/Publish/Rab/RabAppProjectTests.cs index 24d7aaee4b5d..f9b716e38d0c 100644 --- a/src/BloomTests/Publish/Rab/RabAppProjectTests.cs +++ b/src/BloomTests/Publish/Rab/RabAppProjectTests.cs @@ -7,6 +7,7 @@ using System.Runtime.CompilerServices; using System.Text; using System.Text.RegularExpressions; +using System.Threading; using System.Threading.Tasks; using System.Xml.Linq; using Bloom.Collection; @@ -974,6 +975,238 @@ await service.InstallAsync() Assert.That(service.InstallCommandCount, Is.EqualTo(1)); } + [Test] + public async Task InstallAsync_WhenCancelledBeforeRetry_DoesNotUninstallExistingApp() + { + using var tempFolder = new TemporaryFolder("RabAppProjectTests"); + var paths = new RabWorkspacePaths(tempFolder.Path); + var trackedBooks = new List + { + new RabBookPublishInfo + { + BookId = "book-1", + FolderPath = Path.Combine(tempFolder.Path, "book-1"), + Title = "Book One", + BloomPubPath = Path.Combine(paths.BloomPubRoot, "book-1.bloompub"), + }, + }; + Directory.CreateDirectory(trackedBooks[0].FolderPath); + + var service = new TestRabProjectService(paths, "Sample App", trackedBooks); + await service.PrepareAsync(); + await service.BuildAsync(); + + // The first install attempt reports the differently-signed-package failure that would + // normally trigger the uninstall-and-retry recovery. + service.InstallApkResults.Enqueue( + ( + 1, + "adb.exe: failed to install sample.apk: Failure [INSTALL_FAILED_UPDATE_INCOMPATIBLE: Existing package org.sil.en.stories signatures do not match newer version; ignoring!]" + ) + ); + + // The user cancels while that first attempt is running. + Assert.That(service.TryBeginAction("install"), Is.True); + service.RequestCancellation(); + + Assert.ThrowsAsync(async () => + await service.InstallAsync() + ); + + // We bailed before the destructive uninstall, so the phone's existing app is left + // untouched and no second install was attempted. + Assert.That(service.UninstallCommands, Is.Empty); + Assert.That(service.InstallCommandCount, Is.EqualTo(1)); + + service.ClearAction(); + } + + [Test] + public async Task InstallAsync_WhenCancelledDuringReplace_StillReinstallsBeforeHonoringCancel() + { + using var tempFolder = new TemporaryFolder("RabAppProjectTests"); + var paths = new RabWorkspacePaths(tempFolder.Path); + var trackedBooks = new List + { + new RabBookPublishInfo + { + BookId = "book-1", + FolderPath = Path.Combine(tempFolder.Path, "book-1"), + Title = "Book One", + BloomPubPath = Path.Combine(paths.BloomPubRoot, "book-1.bloompub"), + }, + }; + Directory.CreateDirectory(trackedBooks[0].FolderPath); + + var service = new TestRabProjectService(paths, "Sample App", trackedBooks); + await service.PrepareAsync(); + await service.BuildAsync(); + + service.InstallApkResults.Enqueue( + ( + 1, + "adb.exe: failed to install sample.apk: Failure [INSTALL_FAILED_UPDATE_INCOMPATIBLE: Existing package org.sil.en.stories signatures do not match newer version; ignoring!]" + ) + ); + service.InstallApkResults.Enqueue((0, "Performing Streamed Install")); + + Assert.That(service.TryBeginAction("install"), Is.True); + // Simulate the user cancelling at the moment the old app has just been uninstalled. + service.DuringUninstall = () => service.RequestCancellation(); + + Assert.ThrowsAsync(async () => + await service.InstallAsync() + ); + + // Even though the cancel landed mid-recovery, we finished the reinstall (one uninstall + // followed by a second install) before honoring the cancellation, so the phone is never + // left with no app. + Assert.That(service.UninstallCommands.Count, Is.EqualTo(1)); + Assert.That(service.InstallCommandCount, Is.EqualTo(2)); + + service.ClearAction(); + } + + [Test] + public async Task BuildAsync_WhenBuildFails_DeletesIntermediateApkButKeepsPreviousGoodApk() + { + using var tempFolder = new TemporaryFolder("RabAppProjectTests"); + var paths = new RabWorkspacePaths(tempFolder.Path); + var trackedBooks = new List + { + new RabBookPublishInfo + { + BookId = "book-1", + FolderPath = Path.Combine(tempFolder.Path, "book-1"), + Title = "Book One", + BloomPubPath = Path.Combine(paths.BloomPubRoot, "book-1.bloompub"), + }, + }; + Directory.CreateDirectory(trackedBooks[0].FolderPath); + + var service = new TestRabProjectService(paths, "Sample App", trackedBooks); + await service.PrepareAsync(); + await service.BuildAsync(); + + // Sanity check: a successful build produced a signed app in SafeApkRoot and left no + // stray .apk under the build folder. + var goodApk = service.FindLatestApkPath(paths); + Assert.That(goodApk, Is.Not.Null, "setup: the first build should produce an APK"); + Assert.That( + Path.GetDirectoryName(goodApk), + Is.EqualTo(paths.SafeApkRoot), + "setup: the finished APK should live in SafeApkRoot" + ); + Assert.That( + Directory.GetFiles(paths.BuildRoot, "*.apk", SearchOption.AllDirectories), + Is.Empty, + "setup: a successful build should not leave an APK under BuildRoot" + ); + + // A rebuild that fails after Gradle wrote an unsigned intermediate under BuildRoot. + service.FailNextBuild = true; + Assert.ThrowsAsync(async () => await service.BuildAsync()); + + // The intermediate under BuildRoot is deleted so it can't later be mistaken for a + // finished app by FindLatestApkPath... + Assert.That( + Directory.GetFiles(paths.BuildRoot, "*.apk", SearchOption.AllDirectories), + Is.Empty, + "a failed build should delete intermediate APKs under BuildRoot" + ); + // ...while the previously-built good APK in SafeApkRoot is left intact. + Assert.That( + File.Exists(goodApk), + Is.True, + "a failed rebuild should keep the previous good APK" + ); + Assert.That(service.FindLatestApkPath(paths), Is.EqualTo(goodApk)); + } + + [Test] + public void SaveDownloadStreamAtomically_WhenCancelledMidDownload_LeavesNoInstallerFile() + { + using var tempFolder = new TemporaryFolder("RabAppProjectTests"); + var paths = new RabWorkspacePaths(tempFolder.Path); + var service = new TestRabProjectService( + paths, + "Sample App", + new List() + ); + var installerPath = Path.Combine(tempFolder.Path, "Rab-Setup.exe"); + + // Simulate a download that writes some bytes and is then cancelled partway through. + service.CopyDownloadStreamOverride = fileStream => + { + var partialBytes = Encoding.UTF8.GetBytes("partial installer bytes"); + fileStream.Write(partialBytes, 0, partialBytes.Length); + throw new OperationCanceledException(); + }; + + using var responseStream = new MemoryStream(); + Assert.Throws(() => + service.SaveDownloadStreamAtomically( + installerPath, + responseStream, + -1, + (transferred, total) => { }, + CancellationToken.None + ) + ); + + // Neither the real installer name nor the temp file is left behind, so a later + // FindRabSetupInstallerPath cannot pick up (and try to run) a truncated installer. + Assert.That( + File.Exists(installerPath), + Is.False, + "a cancelled download must not leave a file at the installer name" + ); + Assert.That( + File.Exists(installerPath + ".part"), + Is.False, + "the temporary download file should be cleaned up on cancellation" + ); + } + + [Test] + public void SaveDownloadStreamAtomically_WhenDownloadCompletes_ReplacesAnyOlderInstaller() + { + using var tempFolder = new TemporaryFolder("RabAppProjectTests"); + var paths = new RabWorkspacePaths(tempFolder.Path); + var service = new TestRabProjectService( + paths, + "Sample App", + new List() + ); + var installerPath = Path.Combine(tempFolder.Path, "Rab-Setup.exe"); + // A stale installer from a previous run is present; a completed download must replace it + // (RobustFile.Move would otherwise fail because it does not overwrite). + RobustFile.WriteAllText(installerPath, "old installer"); + + service.CopyDownloadStreamOverride = fileStream => + { + var bytes = Encoding.UTF8.GetBytes("complete installer"); + fileStream.Write(bytes, 0, bytes.Length); + }; + + using var responseStream = new MemoryStream(); + service.SaveDownloadStreamAtomically( + installerPath, + responseStream, + -1, + (transferred, total) => { }, + CancellationToken.None + ); + + Assert.That(File.Exists(installerPath), Is.True); + Assert.That(File.ReadAllText(installerPath), Is.EqualTo("complete installer")); + Assert.That( + File.Exists(installerPath + ".part"), + Is.False, + "the temporary download file should be removed after a successful download" + ); + } + [Test] public async Task SetupAndBuildAsync_CreatesTrackedProjectState_AndValidApk() { @@ -2582,6 +2815,19 @@ public IReadOnlyDictionary DetachedEnvironmentVariables public List UninstallCommands { get; } = new List(); public List RunProcessCommands { get; } = new List(); public int InstallCommandCount { get; private set; } + + // When true, the next simulated build writes an unsigned intermediate .apk under + // BuildRoot (as Gradle would) and then fails, so tests can verify the interrupted-build + // cleanup removes it. + public bool FailNextBuild { get; set; } + + // Invoked from the simulated UninstallAppFromDevice so a test can act (e.g. request + // cancellation) at the exact moment the phone's existing app has just been removed. + public Action DuringUninstall { get; set; } + + // When set, replaces the real chunk-copy so a test can simulate a download that writes + // some bytes to the given (temp) file stream and then completes or throws. + public Action CopyDownloadStreamOverride { get; set; } public RabAdbConnectedDevice ConnectedDeviceToReturn { get; set; } = new RabAdbConnectedDevice { @@ -2715,6 +2961,24 @@ string workingDirectory if (tokens.Contains("-load") && tokens.Contains("-build")) { EmitSimulatedBuildOutput(string.Join(" ", rabArguments)); + if (FailNextBuild) + { + // Mimic Gradle having written an unsigned intermediate .apk under the build + // folder before the build failed; the interrupted-build cleanup should delete + // it while leaving any finished app in SafeApkRoot alone. + var intermediateApk = Path.Combine( + _paths.BuildRoot, + "app", + "build", + "outputs", + "apk", + "release", + "app-release-unsigned.apk" + ); + Directory.CreateDirectory(Path.GetDirectoryName(intermediateApk)); + RobustFile.WriteAllText(intermediateApk, "unsigned-intermediate"); + throw new ApplicationException("cmd.exe exited with code 1."); + } CreateApk(tokens); } } @@ -2876,6 +3140,30 @@ string workingDirectory ) { UninstallCommands.Add($"-s \"{deviceSerial}\" uninstall \"{packageName}\""); + DuringUninstall?.Invoke(); + } + + internal override void CopyRabInstallerDownloadStream( + Stream responseStream, + Stream fileStream, + long totalBytes, + Action reportProgress, + CancellationToken cancellationToken + ) + { + if (CopyDownloadStreamOverride != null) + { + CopyDownloadStreamOverride(fileStream); + return; + } + + base.CopyRabInstallerDownloadStream( + responseStream, + fileStream, + totalBytes, + reportProgress, + cancellationToken + ); } internal override void RunProcess( From 815a2c457dd0a3e31003a30bc03686a3c54531b6 Mon Sep 17 00:00:00 2001 From: Steve McConnel Date: Thu, 30 Jul 2026 15:35:07 -0600 Subject: [PATCH 2/3] Honor a pending cancel before launching the next RAB/adb subprocess (BL-16350) Devin review of PR #8133 caught that cancellation was only checked after a subprocess exited, never before launching one. So a Cancel clicked during the in-process work between two RAB runs (e.g. in Build() between the project-update run and the Gradle build) let the next multi-minute build launch and run to completion before the cancel was noticed -- and with navigation now locked, the user was stuck watching it finish. RunProcess/RunProcessCapturingOutput now throw OperationCanceledException before starting a process when a cancel is already pending, and also kill the process if a cancel landed in the narrow window between Start() and registering it as the current process. Both checks are skipped during the protected uninstall+reinstall recovery, which must still run to completion. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BloomExe/Publish/Rab/RabProjectService.cs | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/BloomExe/Publish/Rab/RabProjectService.cs b/src/BloomExe/Publish/Rab/RabProjectService.cs index ae5d4a418e00..bbba6eefa060 100644 --- a/src/BloomExe/Publish/Rab/RabProjectService.cs +++ b/src/BloomExe/Publish/Rab/RabProjectService.cs @@ -2352,6 +2352,13 @@ internal virtual void RunProcess( ReportProcessOutputLine(args.Data, ProgressKind.Warning); }; + // Don't launch another subprocess if the user has already asked to cancel: otherwise + // a long step (e.g. the Gradle build) would run to completion before the post-exit + // check below noticed it, leaving the user — with navigation locked — stuck watching + // it finish (BL-16350). The protected recovery is exempt (it must complete). + if (!_protectCurrentProcessFromCancellation && _cancelRequested) + throw new OperationCanceledException(); + if (!process.Start()) throw new ApplicationException($"Bloom could not start {fileName}."); @@ -2362,6 +2369,16 @@ internal virtual void RunProcess( lock (_currentProcessLock) _currentProcess = process; } + // Close the race between Start() above and registering _currentProcess: a cancel that + // arrived in that window found no process to kill, so kill it now. + if (!_protectCurrentProcessFromCancellation && _cancelRequested) + { + try + { + process.Kill(true); + } + catch (Exception) { } + } try { process.BeginOutputReadLine(); @@ -2470,6 +2487,13 @@ string workingDirectory ReportProcessOutputLine(args.Data, ProgressKind.Warning); }; + // Don't launch another subprocess if the user has already asked to cancel: otherwise + // a long step would run to completion before the post-exit check below noticed it, + // leaving the user — with navigation locked — stuck waiting (BL-16350). The protected + // recovery is exempt (it must complete). + if (!_protectCurrentProcessFromCancellation && _cancelRequested) + throw new OperationCanceledException(); + if (!process.Start()) throw new ApplicationException($"Bloom could not start {fileName}."); @@ -2480,6 +2504,16 @@ string workingDirectory lock (_currentProcessLock) _currentProcess = process; } + // Close the race between Start() above and registering _currentProcess: a cancel that + // arrived in that window found no process to kill, so kill it now. + if (!_protectCurrentProcessFromCancellation && _cancelRequested) + { + try + { + process.Kill(true); + } + catch (Exception) { } + } try { process.BeginOutputReadLine(); From bf4a6e0c7149b698192f3182463d1479efcc9d31 Mon Sep 17 00:00:00 2001 From: Steve McConnel Date: Thu, 30 Jul 2026 15:46:59 -0600 Subject: [PATCH 3/3] Don't report a download timeout (or other library cancellation) as a user cancel (BL-16350) Devin review of PR #8133 caught that the prepare/build/install handlers catch OperationCanceledException and report it as a user cancellation. But an HttpClient timeout during the RAB installer download throws TaskCanceledException, which also derives from OperationCanceledException -- so a stalled download that timed out was shown to the user as cancelled and never logged as the failure it is. Gate the catch with `when (_rabProjectService.IsCancellationRequested)`. Every user-initiated OperationCanceledException in this code is raised only while _cancelRequested is set, so a timeout (with _cancelRequested false) now falls through to the failure handler, which logs and surfaces the real error. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BloomExe/Publish/Rab/RabProjectService.cs | 9 +++++++++ src/BloomExe/Publish/Rab/RabPublishApi.cs | 9 ++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/BloomExe/Publish/Rab/RabProjectService.cs b/src/BloomExe/Publish/Rab/RabProjectService.cs index bbba6eefa060..0865d010e8d6 100644 --- a/src/BloomExe/Publish/Rab/RabProjectService.cs +++ b/src/BloomExe/Publish/Rab/RabProjectService.cs @@ -471,6 +471,15 @@ public RabAppSizeEstimates GetSizeEstimates() /// internal bool IsActionInProgress => _activeProgressAction != null; + /// + /// True when the user has actually requested cancellation of the current action. Lets the API + /// layer tell a user-initiated apart from one thrown + /// by library code (e.g. an HttpClient download timeout, whose TaskCanceledException also + /// derives from OperationCanceledException), so a genuine failure isn't misreported — and + /// unlogged — as a cancellation. + /// + internal bool IsCancellationRequested => _cancelRequested; + /// /// Atomically claims the action slot, setting to /// . Returns true if this call won the slot, false if another diff --git a/src/BloomExe/Publish/Rab/RabPublishApi.cs b/src/BloomExe/Publish/Rab/RabPublishApi.cs index f172dbb3abbe..852c3dd957eb 100644 --- a/src/BloomExe/Publish/Rab/RabPublishApi.cs +++ b/src/BloomExe/Publish/Rab/RabPublishApi.cs @@ -150,8 +150,13 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) succeeded = true; } catch (OperationCanceledException) + when (_rabProjectService.IsCancellationRequested) { - // ReportCancellation logs before the UI tears down the subscription. + // Only a real user cancel lands here; an OperationCanceledException + // from library code (e.g. a download timeout) falls through to the + // failure handler below so it's reported and logged, not silently + // shown as "cancelled". ReportCancellation logs before the UI tears + // down the subscription. _rabProjectService.ReportCancellation("Prepare"); } catch (Exception error) @@ -198,6 +203,7 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) succeeded = true; } catch (OperationCanceledException) + when (_rabProjectService.IsCancellationRequested) { _rabProjectService.ReportCancellation("Build"); } @@ -239,6 +245,7 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) succeeded = true; } catch (OperationCanceledException) + when (_rabProjectService.IsCancellationRequested) { _rabProjectService.ReportCancellation("Try on phone"); }