Update path: cancellation is non-functional, and three paths can hang the bootstrapper indefinitely

[joelc67]

Preface

This is a source review of the update path, not a reproduced bug report. I hit a failed update, went looking for the cause, and ended up reading MRB_Boostrap end to end. I have not been able to attach a debugger or reproduce these against a live patch server, and I did not build the project. Everything below is cited to specific lines at dd189d0 so you can dismiss anything I've misread quickly.

Findings are ordered by severity. Happy to split this into separate issues if you'd prefer one per defect — I've kept them together because finding 1 is the amplifier for findings 2 and 3.

(Edited to de-link finding references — GitHub was turning them into cross-links to unrelated issues. Apologies for any stray notifications.)

---

1. There is no CancellationTokenSource anywhere in the bootstrapper; the Cancel button is cosmetic

grep -rn "CancellationTokenSource" MRB_Boostrap/ returns zero hits.

• App.cs:29 calls ExecutePatchFlowAsync(entry) and relies on the = default parameter in IPatchFlowManager.cs:7, so the flow runs with CancellationToken.None.
• RestoreApp.cs:30 passes CancellationToken.None explicitly.
• ModernWindow.cs:122-126 — clicking Cancel sets _cancelEnabled = false, sets _currentStatus = "Cancelling update...", and repaints. Nothing is signalled.

Consequence: every cancellationToken.ThrowIfCancellationRequested() in PatchFlowManager, FileStager, HashValidator, BackupManager and FileDecompressor is unreachable, and every catch (OperationCanceledException) handler in the update path is dead code for its stated purpose.

The user-visible part is worse than the dead code: the window says "Cancelling update..." while the flow continues on into the destructive apply step at PatchFlowManager.cs:178.

---

2. If the user cancels the "save your build?" prompt, the bootstrapper waits forever

This one is trivially reproducible and I suspect it accounts for a share of "the updater just froze" reports.

• PatchFlowManager.cs:144-155 calls ProcessUtils.KillProcessAsync("MidsReborn"). UpdateCoordinator.cs:56 launches the bootstrapper without exiting Mids Reborn first, so this runs on every update.
• ProcessUtils.cs:27-31: if (!proc.CloseMainWindow()) proc.Kill();
• CloseMainWindow() returns true for successfully posting WM_CLOSE — it does not mean the window closed. So Kill() is skipped.
• MainWindow2.cs:1085-1107 (CloseCommand) shows a modal "Do you wish to save your build before closing?" with Yes/No/Cancel, and DialogResult.Cancel => true aborts the close.
• Back in ProcessUtils.cs:33, await proc.WaitForExitAsync(cancellationToken) has no timeout and no fallback to Kill(), and per finding 1 the token is None.

Repro: open Mids Reborn, modify a build without saving, trigger an update, and click Cancel on the save prompt. The bootstrapper sits on "Closing Mids Reborn..." permanently. Its own Cancel button does nothing, per finding 1.

Suggested fix: bound the wait (WaitForExitAsync with a timeout, then Kill()), and treat a still-running process after the grace period as the existing return -1 "Please close Mids Reborn and try again" path at :152-153 rather than blocking.

---

3. The download body loop has no timeout and no stall detection

FileDownloader.cs:

• :20 — the only timeout in the class is HttpClient.Timeout = TimeSpan.FromSeconds(15).
• :103 — GetAsync(url, HttpCompletionOption.ResponseHeadersRead, cancellationToken).
• :120-131 — the body is streamed with input.ReadAsync(buffer, 0, buffer.Length, cancellationToken) and nothing else.

With ResponseHeadersRead, the timeout CTS SendAsync creates is disposed once headers arrive, and SocketsHttpHandler applies no default read timeout to the response body. A server (or a proxy, or a flaky link) that returns headers promptly and then stalls mid-body leaves this loop blocked indefinitely. There is no byte-rate watchdog and no idle timeout.

Combined with finding 1, the hang is unbreakable from the UI — "Downloading patch..." with a frozen progress bar and a Cancel button that does nothing.

The HttpCompletionOption docs call this out directly and suggest a separate CTS for the content read: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpcompletionoption

---

4. Connect/header timeouts are logged as user cancellation and bypass the retry loop entirely

FileDownloader.cs:136-142:

catch (OperationCanceledException)
{
    _logger.LogWarning("Download canceled by user for {Url}", url);
    if (File.Exists(localPath))
        File.Delete(localPath);
    return false;
}

HttpClient throws TaskCanceledException (a subclass of OperationCanceledException) when Timeout elapses. This handler is declared before catch (Exception ex) at :143, so it wins — and it does not check cancellationToken.IsCancellationRequested.

Two consequences:

1. The retry loop is unreachable for this failure mode. The return false at :141 is inside while (attempt++ < maxRetries) (:92), so a 15-second DNS/TLS/header timeout aborts on attempt 1 and never reaches the backoff at :158-163. PatchFlowManager.cs:95 then throws "Failed to download patch." and the update dies.
2. The log line is unconditionally false. Given finding 1, the token is always None, so every exception this handler ever sees is a timeout — it can never actually be a user cancel. Anyone reading a user's log to diagnose a failed update is told the user cancelled it.

Standard fix is an exception filter:

catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
{
    // genuine user cancellation
}
catch (Exception ex)  // timeouts now land here and retry normally

15 seconds is also tight for a Cloudflare-fronted origin on a cold cache.

---

5. Manifest check: a 5-second timeout plus a mandatory HEAD preflight produces a misleading error

UpdateUtils.cs:

• :105 — Timeout = TimeSpan.FromSeconds(5)
• :120-123 — an unconditional HEAD before the GET
• :124-128 — if (!headResponse.IsSuccessful || headResponse.StatusCode == NotFound) → ShowMissingManifestWarning(...) → return new Manifest()

With ThrowOnAnyError = false (:102), a timeout surfaces as IsSuccessful == false and is indistinguishable from a real 404. The GET at :131-132 is never attempted. The user gets the modal at :161-171:

Could not locate the manifest for the {server} database. This may indicate a misconfiguration or an outdated or missing manifest. If this is a custom or community server, please reach out to the database administrator(s).

— which sends them to blame a database admin for what is a client-side timeout. This fires at startup (MainWindow2.cs:252/255) for updates.midsreborn.com and for every custom server manifest.

Also worth noting: some static and object hosts answer 405 to HEAD, which fails permanently here even though GET works fine. The Cloudflare UA workaround at :103-104 suggests you've already been fighting this layer.

Suggested: drop the HEAD preflight and let the GET's status code drive the branch, and raise the timeout.

---

6. Rollback deletes the install before restoring, and shares the failure mode it's meant to cover

PatchFlowManager.cs:207-210 correctly fires RollbackAsync when the apply fails partway (installing && backupMade are both set at :176/:167), so the common case is handled — that part of the design is sound.

The problem is the ordering in BackupManager.cs. :144-155 deletes the entire install tree (with File.Delete failures silently swallowed at :150-154) before the first restored byte is written at :170-184. There's no temp-swap or staged restore.

That matters most for the case rollback exists to handle. FileStager.cs:114-123 aborts the patch on the first MoveFileEx failure — typically a locked target (AV scanning a freshly written binary, a lingering handle). Rollback then hits the same lock: the delete fails silently, and File.WriteAllBytesAsync throws on that same file, returning false at :206-210 — after every other file in the install directory has already been deleted. The user ends up worse off than if the patch had simply aborted.

FileStager.cs:114 also gets one MoveFileEx attempt per file with no retry, which makes the transient-lock case more likely to be hit than it needs to be.

---

7. Smaller items

• FileDownloader.cs:107 — malformed message template: "HTTP error on attempt {Attempt}: {(Code)} {Reason}". (Code) isn't a valid property name, so the token is emitted literally and three arguments bind to two holes; the status code renders where the reason phrase should be. Every HTTP-error line in the downloader is wrong.
• FileDownloader.cs:105-109 — a non-success status code hits continue, which jumps past the Task.Delay(attempt * 1000) backoff at :158-163. Three immediate requests against a 503.
• PatchFlowManager.cs:105 — the hash URL is built from raw entry.SourceUri, while :47 normalizes it into baseUrl (guaranteed trailing /) and :87 uses that for the patch file. Currently masked because UpdateUtils.GetBaseUriFromFileUrl always returns a trailing slash, but it defeats the normalization written one block earlier and will bite any manifest that sets SourceUri differently.
• BackupManager.cs:36 and :147 — exclusions use rel.StartsWith(ex, ...) rather than path-segment matching, so any top-level file whose name merely begins with Data, Logs, Backup, Staging or Patches is silently omitted from the backup and unrecoverable on rollback.
• BackupManager.cs:27-30 — the application backup excludes MRBBootstrap.exe, Logs, Staging, Backup, Data, Patches but not Databases, while PatchCompressor.cs:167 excludes Databases from application patches. App-patch backups therefore capture ~100MB+ of database files the patch will never touch, buffer them again in a MemoryStream in FileCompressor.cs:21-43, and finding 6 then deletes and rewrites all of them on any app-patch rollback.
• App.cs:36-39 — StartMidsReborn() is inside the per-entry loop. With both an app and a database update queued (UpdateCoordinator.cs:60-89 can emit both), Mids Reborn is relaunched after the app patch, and the database patch's PatchFlowManager.cs:144-155 then kills the instance that was just started. Restart probably belongs after the loop.
• UpdateCoordinator.cs:93/112 — the temp manifest written to %TEMP% is never cleaned up by either process.

---

Notes

Findings 1, 2 and 4 are the ones I'd argue are worth fixing first — they're small, self-contained changes, and together they turn a class of silent hangs into actionable errors. Finding 2 in particular is a few lines.

Thanks for maintaining this — it's the backbone of build planning for the whole community, and none of the above is meant as a knock on the project.

[joelc67]

Correction: the version I'm running doesn't match any public branch

I need to correct the framing of the original post. I've since pulled my update log, and the updater I'm actually running is not the code I reviewed above.

I'm on 3.8.6.0 (installed from the 3.8.0.0 release here, then updated in-app since). Its update log references components that don't exist anywhere in this repository. I checked master, dev, and 4.0:

String from my update.log	Occurrences across master / dev / 4.0
PatchManager	0
PreInstallCleanup	0
UpdateStaging / UpdateBackup	0
Database name is required for db patch	0

The published bootstrapper's Step 9 pre-install cleanup is an empty stub (PatchFlowManager.cs:169-171); the one I'm running enumerates and deletes several thousand files. These are different implementations.

So: the findings in the original post stand as a review of MRB_Boostrap at dd189d0 on master, but they should not be read as an explanation of my failed update, and they may well describe code you've already replaced. Apologies for implying otherwise — I should have verified my build against the branch before writing it up. If the published updater is effectively dead code at this point, feel free to close this.

---

The actual failure, in case it's useful

Different bug, and it's in the unpublished updater. The in-app prompt offered Homecoming Database (2026.5.1337); the update fails every time, reproducibly, at the backup step:

[2026-07-31 10:51:41] [General] [PatchManager] CreateBackup failed: Database name is required for db patch.
[2026-07-31 10:51:41] [General] [PatchManager] Update failed: Failed to create backup snapshot.

Earlier in the same run:

[2026-07-31 10:51:30] [General] [PatchManager] installPath: ...\AppData\Roaming\LoadedCamel\MidsReborn\
[2026-07-31 10:51:30] [General] [PatchManager] patchType: Application
[2026-07-31 10:51:30] [General] [PatchManager] dbName:

dbName is empty, and the backup routine rejects a database patch without one — so the database name isn't being carried into the patch context for DB-type updates. It fails before anything is installed, so nothing is corrupted, but the database can never update.

One thing worth a look regardless of the above: the pre-install cleanup deleted the entire Assets\ and Images\ trees before the run failed —

[2026-07-31 10:51:32] [General] [PreInstallCleanup] Deleted: Assets\Archetypes\Class_Blaster.png
...
[2026-07-31 10:51:32] [General] [PreInstallCleanup] Deleted: Assets\Archetypes
[2026-07-31 10:51:32] [General] [PreInstallCleanup] Deleted: Assets

Destructive cleanup running before the backup step succeeds seems worth revisiting, since a failure after that point leaves the install stripped.

Happy to attach the full log (7,374 lines) if it helps.

[joelc67]

Follow-up: it's a deadlock, and a workaround for anyone stuck

Two things I only understood after repairing my own install.

The failure is self-reinforcing

My Databases\Homecoming\ contained exactly one file — Compare.mhd, 37 bytes. The real database is ~40MB across ~60 files. It had been gone for some time before the run I posted about; the cleanup log enumerates only those two entries, so there was nothing else there to begin with:

[PreInstallCleanup] Evaluating: Databases\Homecoming
[PreInstallCleanup] Evaluating: Databases\Homecoming\Compare.mhd

Which produces a loop with no exit from inside the app:

1. the database is missing, so the app prompts for the Homecoming update
2. the update fails because dbName is empty at the backup step
3. the database is still missing
4. back to 1 on next launch

Retrying never helps, and there's no in-app path to a working database. On my machine this persisted for months before I went looking at logs — I'd assumed the installer was just flaky.

Destructive cleanup runs before the guard that aborts the run

Same log, in order:

[10:51:32] [PreInstallCleanup] Deleted: Databases\Homecoming\Compare.mhd
[10:51:32] [PreInstallCleanup] Deleted: Databases\Homecoming
[10:51:32] [PreInstallCleanup] Deleted: Databases
...
[10:51:41] [PatchManager] CreateBackup failed: Database name is required for db patch.
[10:51:41] [PatchManager] Update failed: Failed to create backup snapshot.

The dbName validation is perfectly capable of rejecting this update — it just does so nine seconds after the tree has already been deleted. Validating the patch context before the cleanup step would turn a destructive failure into a clean no-op.

Workaround

The install ships a full bundle alongside the binaries — in my case mids3.8.6.0+db2026.5.1337.zip in %APPDATA%\LoadedCamel\MidsReborn\. It contains a complete Databases/ tree at the current version, so it can be used to restore without involving the updater at all:

1. close Mids Reborn
2. extract only Databases/* from that zip into %APPDATA%\LoadedCamel\MidsReborn\
3. relaunch

That restored 331 files / 110MB here (Homecoming 61 files / 39MB), all byte-size verified against the archive. It does not fix anything — the next database update will fail identically — but it gets a deadlocked install back to usable.